config: change prestart hook spec to match reality #1169

corhere · 2022-11-02T21:53:41Z

runC originally implemented prestart hooks contrary to the spec. And it still implements them the same way today, as it would break a lot of projects which have come to rely on the existing behaviour. Any OCI runtime implementations which want to be compatible with projects that have come to rely on the existing runC behaviour must also implement them contrary to the spec. Furthermore, the Lifecycle section of the spec requires the existing runC behaviour for the prestart hook, directly contradicting the section of the spec which defines the prestart hook in config.md!

runtime-spec/runtime.md

Lines 52 to 77 in 494a5a6

    
           ## <a name="runtimeLifecycle" />Lifecycle 
        
           The lifecycle describes the timeline of events that happen from when a container is created to when it ceases to exist. 
        
           1. OCI compliant runtime's [`create`](runtime.md#create) command is invoked with a reference to the location of the bundle and a unique identifier. 
        
           2. The container's runtime environment MUST be created according to the configuration in [`config.json`](config.md). 
        
               If the runtime is unable to create the environment specified in the [`config.json`](config.md), it MUST [generate an error](#errors). 
        
               While the resources requested in the [`config.json`](config.md) MUST be created, the user-specified program (from [`process`](config.md#process)) MUST NOT be run at this time. 
        
               Any updates to [`config.json`](config.md) after this step MUST NOT affect the container. 
        
           3. The [`prestart` hooks](config.md#prestart) MUST be invoked by the runtime. 
        
               If any `prestart` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. 
        
           4. The [`createRuntime` hooks](config.md#createRuntime-hooks) MUST be invoked by the runtime. 
        
               If any `createRuntime` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. 
        
           5. The [`createContainer` hooks](config.md#createContainer-hooks) MUST be invoked by the runtime. 
        
               If any `createContainer` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. 
        
           6. Runtime's [`start`](runtime.md#start) command is invoked with the unique identifier of the container. 
        
           7. The [`startContainer` hooks](config.md#startContainer-hooks) MUST be invoked by the runtime. 
        
               If any `startContainer` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. 
        
           8. The runtime MUST run the user-specified program, as specified by [`process`](config.md#process). 
        
           9. The [`poststart` hooks](config.md#poststart) MUST be invoked by the runtime. 
        
               If any `poststart` hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded. 
        
           10. The container process exits. 
        
               This MAY happen due to erroring out, exiting, crashing or the runtime's [`kill`](runtime.md#kill) operation being invoked. 
        
           11. Runtime's [`delete`](runtime.md#delete) command is invoked with the unique identifier of the container. 
        
           12. The container MUST be destroyed by undoing the steps performed during create phase (step 2). 
        
           13. The [`poststop` hooks](config.md#poststop) MUST be invoked by the runtime. 
        
               If any `poststop` hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded.

runtime-spec/config.md

Line 445 in 494a5a6

    
           The `prestart` hooks MUST be called after the [`start`](runtime.md#start) operation is called but [before the user-specified program command is executed](runtime.md#lifecycle).

Given that existing implementations cannot be changed, the spec contradicts existing implementations, and the spec contradicts itself, amending the spec to align with the existing runC behaviour is the only viable way to resolve the contradiction.

Alternative to runtime: move prestart hook in lifecycle section #1167 for resolving the conflict within the spec

runC originally implemented prestart hooks contrary to the spec. And it still implements them the same way today, as it would break a lot of projects which have come to rely on the existing behaviour. Any OCI runtime implementations which want to be compatible with projects that have come to rely on the existing runC behaviour must also implement them contrary to the spec. Furthermore, the Lifecycle section of the spec requires the existing runC behaviour for the prestart hook, _directly contradicting the section of the spec which defines the prestart hook in config.md!_ Given that existing implementations cannot be changed, the spec contradicts existing implementations, and the spec contradicts _itself_, amending the spec to align with the existing runC behaviour is the only viable way to resolve the contradiction. Signed-off-by: Cory Snider <csnider@mirantis.com>

AkihiroSuda · 2022-11-02T23:50:47Z

Alternative to #1167 for resolving the conflict within the spec

Do you want which of #1167 and #1169 to be merged?

corhere · 2022-11-03T15:05:36Z

Do you want which of #1167 and #1169 to be merged?

I just want clarity on when in the container lifecycle hooks are expected to be executed. I have no strong preference on how.

h-vetinari · 2022-12-19T04:03:41Z

Do you want which of #1167 and #1169 to be merged?

I just want clarity on when in the container lifecycle hooks are expected to be executed. I have no strong preference on how.

I think this PR is clearly preferable to #1167, as it codifies existing behaviour (which was the hole point of the hooks saga), rather than break them through the spec (and have implementations not follow because it'd break their consumers).

AkihiroSuda · 2022-12-24T07:09:22Z

config.md

@@ -461,8 +462,6 @@ On Linux, for example, they are called after the container namespaces are create

 The definition of `createRuntime` hooks is currently underspecified and hooks authors, should only expect from the runtime that the mount namespace have been created and the mount operations performed. Other operations such as cgroups and SELinux/AppArmor labels might not have been performed by the runtime.

-Note: `runc` originally implemented `prestart` hooks contrary to the spec, namely as part of the `create` operation (instead of during the `start` operation). This incorrect implementation actually corresponds to `createRuntime` hooks. For runtimes that implement the deprecated `prestart` hooks as `createRuntime` hooks, `createRuntime` hooks MUST be called after the `prestart` hooks.


Maybe we can add a note that runc was contrary to the spec v1.0 but now conforms to this spec v1.1.

AkihiroSuda · 2023-01-04T23:14:20Z

code-review/pullapprove Pending

The PR is not mergable due to the pending pullapprove but the pullapprove link doesn't work.
Can we just remove it?
cc @caniszczyk

AkihiroSuda · 2023-01-05T00:34:11Z

cc @opencontainers/runc-maintainers to confirm that we are fine to amend the spec to match the reality of runc

AkihiroSuda · 2023-01-19T18:16:13Z

@opencontainers/runc-maintainers @opencontainers/runtime-spec-maintainers Ready to merge this?

giuseppe

LGTM

cyphar

LGTM.

corhere requested review from AkihiroSuda, crosbymichael, cyphar, dqminh, giuseppe, hqhq, kolyshkin, mrunalp, thaJeztah, tianon and vbatts as code owners November 2, 2022 21:53

AkihiroSuda reviewed Dec 24, 2022

View reviewed changes

AkihiroSuda approved these changes Dec 24, 2022

View reviewed changes

AkihiroSuda mentioned this pull request Dec 24, 2022

runtime: move prestart hook in lifecycle section #1167

Closed

tianon approved these changes Jan 4, 2023

View reviewed changes

hqhq approved these changes Jan 6, 2023

View reviewed changes

AkihiroSuda mentioned this pull request Jan 11, 2023

PRs are not mergeable due to the pending code-review/pullapprove #1174

Closed

giuseppe approved these changes Jan 20, 2023

View reviewed changes

cyphar approved these changes Jan 24, 2023

View reviewed changes

cyphar closed this in 5cfc4c3 Jan 24, 2023

cyphar merged commit 5cfc4c3 into opencontainers:main Jan 24, 2023

AkihiroSuda mentioned this pull request Jan 24, 2023

Release v1.1.0-rc.1 #1175

Merged

rumpl mentioned this pull request Jan 31, 2023

Move call of the prestart hooks over to the lib part containerd/runwasi#60

Merged

AkihiroSuda added this to the v1.1.0 milestone Feb 1, 2023

corhere deleted the align-prestart-with-reality branch June 3, 2023 20:04

AkihiroSuda mentioned this pull request Jun 26, 2023

Release v1.1.0 #1213

Merged

12 tasks

cyphar mentioned this pull request Jul 19, 2023

blog: Add "OCI Runtime Spec v1.1" opencontainers/opencontainers.org#133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: change prestart hook spec to match reality #1169

config: change prestart hook spec to match reality #1169

corhere commented Nov 2, 2022 •

edited

AkihiroSuda commented Nov 2, 2022

corhere commented Nov 3, 2022

h-vetinari commented Dec 19, 2022 •

edited

AkihiroSuda Dec 24, 2022

AkihiroSuda commented Jan 4, 2023

AkihiroSuda commented Jan 5, 2023

AkihiroSuda commented Jan 19, 2023

giuseppe left a comment

cyphar left a comment

	## <a name="runtimeLifecycle" />Lifecycle
	The lifecycle describes the timeline of events that happen from when a container is created to when it ceases to exist.

	1. OCI compliant runtime's [`create`](runtime.md#create) command is invoked with a reference to the location of the bundle and a unique identifier.
	2. The container's runtime environment MUST be created according to the configuration in [`config.json`](config.md).
	If the runtime is unable to create the environment specified in the [`config.json`](config.md), it MUST [generate an error](#errors).
	While the resources requested in the [`config.json`](config.md) MUST be created, the user-specified program (from [`process`](config.md#process)) MUST NOT be run at this time.
	Any updates to [`config.json`](config.md) after this step MUST NOT affect the container.
	3. The [`prestart` hooks](config.md#prestart) MUST be invoked by the runtime.
	If any `prestart` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12.
	4. The [`createRuntime` hooks](config.md#createRuntime-hooks) MUST be invoked by the runtime.
	If any `createRuntime` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12.
	5. The [`createContainer` hooks](config.md#createContainer-hooks) MUST be invoked by the runtime.
	If any `createContainer` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12.
	6. Runtime's [`start`](runtime.md#start) command is invoked with the unique identifier of the container.
	7. The [`startContainer` hooks](config.md#startContainer-hooks) MUST be invoked by the runtime.
	If any `startContainer` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12.
	8. The runtime MUST run the user-specified program, as specified by [`process`](config.md#process).
	9. The [`poststart` hooks](config.md#poststart) MUST be invoked by the runtime.
	If any `poststart` hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded.
	10. The container process exits.
	This MAY happen due to erroring out, exiting, crashing or the runtime's [`kill`](runtime.md#kill) operation being invoked.
	11. Runtime's [`delete`](runtime.md#delete) command is invoked with the unique identifier of the container.
	12. The container MUST be destroyed by undoing the steps performed during create phase (step 2).
	13. The [`poststop` hooks](config.md#poststop) MUST be invoked by the runtime.
	If any `poststop` hook fails, the runtime MUST [log a warning](#warnings), but the remaining hooks and lifecycle continue as if the hook had succeeded.

		@@ -461,8 +462,6 @@ On Linux, for example, they are called after the container namespaces are create

		The definition of `createRuntime` hooks is currently underspecified and hooks authors, should only expect from the runtime that the mount namespace have been created and the mount operations performed. Other operations such as cgroups and SELinux/AppArmor labels might not have been performed by the runtime.

		Note: `runc` originally implemented `prestart` hooks contrary to the spec, namely as part of the `create` operation (instead of during the `start` operation). This incorrect implementation actually corresponds to `createRuntime` hooks. For runtimes that implement the deprecated `prestart` hooks as `createRuntime` hooks, `createRuntime` hooks MUST be called after the `prestart` hooks.

config: change prestart hook spec to match reality #1169

config: change prestart hook spec to match reality #1169

Conversation

corhere commented Nov 2, 2022 • edited

AkihiroSuda commented Nov 2, 2022

corhere commented Nov 3, 2022

h-vetinari commented Dec 19, 2022 • edited

AkihiroSuda Dec 24, 2022

Choose a reason for hiding this comment

AkihiroSuda commented Jan 4, 2023

AkihiroSuda commented Jan 5, 2023

AkihiroSuda commented Jan 19, 2023

giuseppe left a comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

corhere commented Nov 2, 2022 •

edited

h-vetinari commented Dec 19, 2022 •

edited