Move pre-start hooks after container mounts by mrunalp · Pull Request #568 · opencontainers/runc

mrunalp · 2016-02-17T10:23:43Z

Today mounts in pre-start hooks get overriden by the default mounts.
Moving the pre-start hooks to after the container mounts and before
the pivot/move root gives better flexiblity in the hooks.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

mrunalp · 2016-02-17T10:26:46Z

@LK4D4 @crosbymichael PTAL
cc: @rhatdan

rhatdan · 2016-02-17T14:18:41Z

We want to mount /sys/fs/cgroups file systems using a prestart hook, but since /sys gets mounted after the hook, it mounts over the file system.

I think it makes more sense to have all of the file systems mounted up so that the prestart hook could manipulate content on them.

mrunalp · 2016-02-18T15:31:50Z

I tested this with docker and it works fine i.e. the networking setup works as expected.

crosbymichael · 2016-02-18T22:28:04Z

LGTM

mrunalp · 2016-02-20T04:46:41Z

@LK4D4 ping

hqhq · 2016-02-23T08:33:48Z

libcontainer/rootfs_linux.go

+	// manipulations.
+	if err := syncParentHooks(pipe); err != nil {
+		return err
+	}


The problem is setupRootfs will only be called when there is configs.NEWNS, prestart hooks should not depend on that.

Today mounts in pre-start hooks get overriden by the default mounts. Moving the pre-start hooks to after the container mounts and before the pivot/move root gives better flexiblity in the hooks. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2016-02-23T10:55:00Z

@hqhq Updated to handle the case when NEWNS isn't specified (though I expect this to be a very rare scenario as a container at minimum implies a new mount namespace if nothing else).

hqhq · 2016-02-23T11:23:31Z

LGTM

dqminh · 2016-02-24T01:56:27Z

LGTM

We also have post mount cmds, which is basically the same thing ? Do we want to deprecate those ?

mrunalp · 2016-02-24T04:34:31Z

@dqminh There is a difference. Those hooks run in the container context and not on the host (which is more powerful and can do everything that can be done in the container plus more). Also, those hooks aren't in the spec yet. I did mention on one of the issues to support those but it hasn't been done yet. Thanks!

mrunalp · 2016-02-24T04:37:27Z

Merging as I see 3 LGTMs and all comments are addressed :)

Move pre-start hooks after container mounts

…ent-comment specs-go/config: Drop "this field is platform dependent"

We dropped these in 4774080 (specs-go/config: Drop "this field is platform dependent", 2016-09-14, opencontainers#568) but f9e48e0 (Windows: User struct changes, 2016-09-14, opencontainers#565) was developed in parallel and brought in a new one. 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 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>

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>

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>

GordonTheTurtle added the status/0-triage label Feb 17, 2016

crosbymichael modified the milestone: 0.0.9 Feb 18, 2016

hqhq reviewed Feb 23, 2016
View reviewed changes

Move pre-start hooks after container mounts

2f27649

Today mounts in pre-start hooks get overriden by the default mounts. Moving the pre-start hooks to after the container mounts and before the pivot/move root gives better flexiblity in the hooks. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp force-pushed the move_hooks branch from 9a22362 to 2f27649 Compare February 23, 2016 10:52

mrunalp pushed a commit that referenced this pull request Feb 24, 2016

Merge pull request #568 from mrunalp/move_hooks

15b6b24

Move pre-start hooks after container mounts

mrunalp merged commit 15b6b24 into opencontainers:master Feb 24, 2016

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#568 from wking/drop-platform-depend…

66f4f35

…ent-comment specs-go/config: Drop "this field is platform dependent"

wking mentioned this pull request Feb 25, 2018

libcontainer: Set 'status' in hook stdin #1741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pre-start hooks after container mounts#568

Move pre-start hooks after container mounts#568
mrunalp merged 1 commit intoopencontainers:masterfrom
mrunalp:move_hooks

mrunalp commented Feb 17, 2016

Uh oh!

mrunalp commented Feb 17, 2016

Uh oh!

rhatdan commented Feb 17, 2016

Uh oh!

mrunalp commented Feb 18, 2016

Uh oh!

crosbymichael commented Feb 18, 2016

Uh oh!

mrunalp commented Feb 20, 2016

Uh oh!

hqhq Feb 23, 2016

Uh oh!

mrunalp commented Feb 23, 2016

Uh oh!

hqhq commented Feb 23, 2016

Uh oh!

dqminh commented Feb 24, 2016

Uh oh!

mrunalp commented Feb 24, 2016

Uh oh!

mrunalp commented Feb 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

mrunalp commented Feb 17, 2016

Uh oh!

mrunalp commented Feb 17, 2016

Uh oh!

rhatdan commented Feb 17, 2016

Uh oh!

mrunalp commented Feb 18, 2016

Uh oh!

crosbymichael commented Feb 18, 2016

Uh oh!

mrunalp commented Feb 20, 2016

Uh oh!

hqhq Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Feb 23, 2016

Uh oh!

hqhq commented Feb 23, 2016

Uh oh!

dqminh commented Feb 24, 2016

Uh oh!

mrunalp commented Feb 24, 2016

Uh oh!

mrunalp commented Feb 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants