'prestart/poststart' hooks are done in the 'create' operation #1710

liangchenye · 2018-02-02T11:28:16Z

We are working on the lifecycle test and found 'runc' calls 'prestart/poststart' hooks in the 'create' phase.

It is not compliant with the spec:
https://github.com/opencontainers/runtime-spec/blob/master/config.md#prestart
https://github.com/opencontainers/runtime-spec/blob/master/config.md#poststart

There are similar OLD issues #1574 and #1135.

liangchenye · 2018-02-06T03:18:33Z

Another issue found in opencontainers/runtime-tools#569.
If the poststart hook fails, 'runc' fails in 'create a container', but it should just record a warning and continue to work according to the spec.

liangchenye · 2018-02-07T09:37:15Z

New issue found in opencontainers/runtime-tools#569.
If we have more than one hooks in poststart and one of them fails, the rest hooks should continue to run. But it is not now.

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess is unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone two. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us> Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. It depends on a pending PR opencontainers#1741. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us> Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. It depends on a pending PR opencontainers#1741. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us> Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. It depends on a pending PR opencontainers#1741. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us> Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. It depends on a pending PR opencontainers#1741. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. And add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 Signed-off-by: W. Trevor King <wking@tremily.us> Signed-off-by: Dongsu Park <dongsu@kinvolk.io> Signed-off-by: Aleksa Sarai <asarai@suse.de>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. It depends on a pending PR opencontainers#1741. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io> Signed-off-by: Aleksa Sarai <asarai@suse.de>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. I really think we should have len() guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Aleksa, however, is adamantly against them [3] citing a risk of sloppy copy/pastes causing the hook slice being len-guarded to diverge from the hook slice being iterated over within the guard. I think that ort of thing is very lo-risk, because: * We shouldn't be copy/pasting this, right? DRY for the win :). * There's only ever a few lines between the guard and the guarded loop. That makes broken copy/pastes easy to catch in review. * We should have test coverage for these. Guarding with the wrong slice is certainly not the only thing you can break with a sloppy copy/paste. But I'm not a maintainer ;). [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 [3]: opencontainers#1741 Signed-off-by: W. Trevor King <wking@tremily.us>

Finish off the work started in a344b2d (sync up `HookState` with OCI spec `State`, 2016-12-19, opencontainers#1201). And drop HookState, since there's no need for a local alias for specs.State. Also set c.initProcess in newInitProcess to support OCIState calls from within initProcess.start(). I think the cyclic references between linuxContainer and initProcess are unfortunate, but didn't want to address that here. I've also left the timing of the Prestart hooks alone, although the spec calls for them to happen before start (not as part of creation) [1,2]. Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs. I'm not sure why we trigger the prestart hooks in response to both procReady and procHooks. But we've had two prestart rounds in initProcess.start since 2f27649 (Move pre-start hooks after container mounts, 2016-02-17, opencontainers#568). I've left that alone too. I really think we should have len() guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty. Aleksa, however, is adamantly against them [3] citing a risk of sloppy copy/pastes causing the hook slice being len-guarded to diverge from the hook slice being iterated over within the guard. I think that ort of thing is very lo-risk, because: * We shouldn't be copy/pasting this, right? DRY for the win :). * There's only ever a few lines between the guard and the guarded loop. That makes broken copy/pastes easy to catch in review. * We should have test coverage for these. Guarding with the wrong slice is certainly not the only thing you can break with a sloppy copy/paste. But I'm not a maintainer ;). [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#prestart [2]: opencontainers#1710 [3]: opencontainers#1741 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

There's been a lot of discussion over in [1] about how to support the NVIDIA folks and others who want to be able to create devices (possibly after having loaded kernel modules) and bind userspace libraries into the container. Currently that's happening in the middle of runc's create-time mount handling before the container pivots to its new root directory with runc's incorrectly-timed prestart hook trigger [2]. With this commit, we extend hooks with a 'precreate' stage to allow trusted parties to manipulate the config JSON before calling the runtime's 'create'. I'm recycling the existing Hook schema from pkg/hooks for this, because we'll want Timeout for reliability and When to avoid the expense of fork/exec when a given hook does not need to make config changes [3]. [1]: opencontainers/runc#1811 [2]: opencontainers/runc#1710 [3]: containers#1828 (comment)

There's been a lot of discussion over in [1] about how to support the NVIDIA folks and others who want to be able to create devices (possibly after having loaded kernel modules) and bind userspace libraries into the container. Currently that's happening in the middle of runc's create-time mount handling before the container pivots to its new root directory with runc's incorrectly-timed prestart hook trigger [2]. With this commit, we extend hooks with a 'precreate' stage to allow trusted parties to manipulate the config JSON before calling the runtime's 'create'. I'm recycling the existing Hook schema from pkg/hooks for this, because we'll want Timeout for reliability and When to avoid the expense of fork/exec when a given hook does not need to make config changes [3]. [1]: opencontainers/runc#1811 [2]: opencontainers/runc#1710 [3]: containers#1828 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

So far Prestart, Poststart hooks have been called from the context of create, which does not satisfy the runtime spec. That's why the runtime-tools validation tests like `hooks_stdin.t` have failed. Let's move call sites of Prestart, Poststart to correct places. Unfortunately as for the Poststart hook, it's not possible to tell whether a specific call site is from create context or run context. That's why we needed to allow Create and Run methods to accept another parameter `action` (of type `CtAct`). Doing that, it's possible to set a variable `initProcess.IsCreate` that allows us distinguish Create from Run. See also opencontainers#1710. Signed-off-by: Dongsu Park <dongsu@kinvolk.io>

liangchenye mentioned this issue Feb 2, 2018

add lifecycle validation opencontainers/runtime-tools#558

Merged

cyphar mentioned this issue Feb 2, 2018

when should prestart hooks be called #1574

Closed

cyphar added this to the 1.0.0 milestone Feb 2, 2018

wking mentioned this issue Feb 25, 2018

libcontainer: Set 'status' in hook stdin #1741

Merged

alban mentioned this issue Mar 5, 2018

add hooks stdin test opencontainers/runtime-tools#589

Merged

cyphar mentioned this issue Mar 26, 2018

Why poststart and poststop hook are not consistent with the standard? #1765

Closed

wking mentioned this issue May 29, 2018

podman with bind mount leaving cgroup debris and prevents container restart containers/podman#730

Closed

dongsupark mentioned this issue May 30, 2018

libcontainer: call Prestart, Poststart hooks from correct places #1811

Closed

alban mentioned this issue Jul 20, 2018

Add runtime-tools' validation tests #1758

Closed

wking mentioned this issue Nov 19, 2018

Pre-create config manipulation? containers/podman#1828

Closed

wking mentioned this issue Nov 19, 2018

hooks: Add pre-create hooks for runtime-config manipulation containers/podman#1830

Merged

cyphar modified the milestones: 1.0.0-rc6, 1.0.0 Nov 21, 2018

h-vetinari mentioned this issue Oct 22, 2019

Add create-runtime, create-container, start-container hooks opencontainers/runtime-spec#1008

Merged

RenaudWasTaken mentioned this issue Jun 2, 2020

Add CreateRuntime, CreateContainer and StartContainer Hooks #2229

Merged

AkihiroSuda closed this as completed in #2229 Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

'prestart/poststart' hooks are done in the 'create' operation #1710

'prestart/poststart' hooks are done in the 'create' operation #1710

liangchenye commented Feb 2, 2018

liangchenye commented Feb 6, 2018

liangchenye commented Feb 7, 2018

'prestart/poststart' hooks are done in the 'create' operation #1710

'prestart/poststart' hooks are done in the 'create' operation #1710

Comments

liangchenye commented Feb 2, 2018

liangchenye commented Feb 6, 2018

liangchenye commented Feb 7, 2018