libcontainer: Set 'status' in hook stdin #1741

wking · 2018-02-25T23:05:03Z

Finish off the work started in #1201 to bring hook stdin into compliance with runtime-spec.

Also:

Drop HookState, since there's no need for a local alias for specs.State.
~~Add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty.~~ I had to drop these to satisfy @cyphar here.
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, 'prestart/poststart' hooks are done in the 'create' operation #1710). 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 #568. I've left that alone too.

cyphar · 2018-02-26T07:41:44Z

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 #568.

That is almost definitely a bug, since the change you refer to was meant to move the hooks to be run earlier rather than duplicate running them. I'm a little confused why nobody has opened a bug about this (maybe pre-start just isn't used as often as post-start). /cc @mrunalp

Once the timing gets fixed we can drop the initProcessStartTime hacks which initProcess.start currently needs.

I'm not sure this is correct, but will take a proper look when I get a chance.

dongsupark · 2018-05-23T14:00:36Z

This PR looks good to me.
However, even with this PR, the runtime-tools validation/hooks_stdin.t test still fails with an empty status, as mentioned in opencontainers/runtime-tools#608.
Is this PR not sufficient to fix the issue?

wking · 2018-05-23T14:45:07Z

However, even with this PR, the runtime-tools validation/hooks_stdin.t test still fails with an empty status, as mentioned in opencontainers/runtime-tools#608.

Can you post the error you see after this patch? In the list in my topic post here I point out some issues I'm not addressing; maybe you're hitting #1710?

dongsupark · 2018-05-23T15:09:42Z

Basically the same as one from opencontainers/runtime-tools#608 (comment).

TAP version 13
not ok 1 - the state of the container MUST be passed to "prestart" hook over stdin
  ---
  {
    "error": "1 error occurred:\n\n* wrong status \"\" in the stdin of prestart hook, expected \"created\"",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-hooks"
  }
  ...
not ok 2 - the state of the container MUST be passed to "poststart" hook over stdin
  ---
  {
    "error": "1 error occurred:\n\n* wrong status \"\" in the stdin of poststart hook, expected \"running\"",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-hooks"
  }
  ...
not ok 3 - the state of the container MUST be passed to "poststop" hook over stdin
  ---
  {
    "error": "1 error occurred:\n\n* status in the stdin of poststop hook should not be empty",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-hooks"
  }
  ...
1..3

wking · 2018-05-23T15:14:15Z

* wrong status \"\" in the stdin of prestart hook, expected \"created\"

Huh. I'll have to dig deeper later today. Right now I don't see how this is possible.

dongsupark · 2018-05-24T13:42:37Z

@wking Sorry, ignore my comment above. There was a test setup issue.

Now with a correct test setup, I'm getting the following errors:

TAP version 13
not ok 1 - the state of the container MUST be passed to "prestart" hook over stdin
  ---
  {
    "error": "1 error occurred:\n\n* wrong status \"creating\" in the stdin of prestart hook, expected \"created\"",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-hooks"
  }
  ...
not ok 2 - the state of the container MUST be passed to "poststart" hook over stdin
  ---
  {
    "error": "1 error occurred:\n\n* wrong status \"created\" in the stdin of poststart hook, expected \"running\"",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#posix-platform-hooks"
  }
  ...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x4f5a02]

goroutine 1 [running]:
github.com/opencontainers/runtime-tools/vendor/github.com/hashicorp/go-multierror.(*Error).Error(0x0, 0x7dfc20, 0xc420224480)
	/home/dpark/go/src/github.com/opencontainers/runtime-tools/vendor/github.com/hashicorp/go-multierror/multierror.go:15 +0x22
main.main()
	/home/dpark/go/src/github.com/opencontainers/runtime-tools/validation/hooks_stdin.go:154 +0x11ba

The first two failures happen because of #1710, right?
The last panic is rather a runtime-tools issue. I'll create a PR to fix it in runtime-tools.

wking · 2018-05-29T22:42:12Z

The first two failures happen because of #1710, right?

Yeah.

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>

cyphar · 2018-11-13T23:49:11Z

So it looks like this has been merged into #1811. I'm going to close this and I'll get #1811 merged (hopefully for 1.0).

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>

cyphar · 2018-11-14T07:02:12Z

libcontainer/container_linux.go

-				Pid:         parent.pid(),
-				Bundle:      bundle,
-				Annotations: annotations,
+		if c.config.Hooks != nil && len(c.config.Hooks.Poststart) > 0 {


The len additions is really not necessary, and I think it should be removed from all the patches.

If you want, I can just combine this and #1811 in a new PR that I will rebase and fix so we can get 1.0 sooner.

The len additions is really not necessary...

From my commit message and topic post:

Add len guards to avoid computing the state when .Hooks is non-nil but the particular phase we're looking at is empty.

Are you saying that state computation is so cheap that it's not worth a len(c.config.Hooks.Poststart) > 0 guard?

Yeah, I don't think the state computation is expensive enough that it makes sense to have likely-to-be-incorrectly-copy-pasted checks.

... likely-to-be-incorrectly-copy-pasted checks.

Wouldn't we expect unit testing to catch this? If it's not covered now, I think it should be. The runtime-tools compliance suite should already cover it.

And building the status hits the filesystem for at least this, and probably more.

So they seem like a slam dunk to me, but if you're still unconvinced when I wake back up, I'll drop the guards ;).

/proc reading is quite cheap, and it should be noted that it will only be hit if config.Hooks != nil which IIRC would require having at least one hook defined... I mean, whatever. My main gripe with it is that it looks a bit ugly and I guarantee someone will screw it up in a copy-paste.

/proc reading is quite cheap...

Compared to, say hitting a disk. Certainly not compared to a len call.

My main gripe with it is that it looks a bit ugly and I guarantee someone will screw it up in a copy-paste.

And get that screw-up through CI and review? I'm sceptical ;). But whatever, I'm not a maintainer. Rebased onto master (because I'm touching it anyway) and dropped the len guards with 6ff5804 -> e238686.

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>

cyphar · 2018-11-14T14:55:50Z

This is one of two final PRs for 1.0. Thanks!

/cc @opencontainers/runc-maintainers

cyphar · 2018-11-14T14:56:05Z

/cc @mrunalp

You said you wanted to test this.

cyphar · 2018-11-14T14:56:37Z

LGTM LGTM LGTM LGTM.

(Yoohoo! PullApprove! Time to wake up!)

crosbymichael · 2018-11-14T21:27:59Z

LGTM

cyphar · 2018-11-14T23:40:35Z

I'll wait until @mrunalp checks this before merging.

cyphar · 2018-11-18T23:22:10Z

/cc @RenaudWasTaken @flx42

Does this break you folks as well? This one is pretty critical for OCI compliance.

RenaudWasTaken · 2018-11-19T05:00:04Z

Does this break you folks as well? This one is pretty critical for OCI compliance.

Thanks for asking!
My understanding of this PR is that you are adding the status field in the struct / json a hook can read from stdin.
In this case, this is a non breaking change in the "hook API" and it won't affect us, as we only rely on getting the Bundle to read the spec.

cyphar · 2018-11-19T19:39:00Z

@RenaudWasTaken Yeah, that's right.

Merging since the change is won't break existing users.

@cyphar

libcontainer: Set 'status' in hook stdin LGTMs: @cyphar @crosbymichael Closes #1741

Finish off the work started in 398a409 (sync up `HookState` with OCI spec `State`, 2016-12-19, #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, #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/runc#1710 [3]: opencontainers/runc#1741 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

Finish off the work started in bbf0c7b (sync up `HookState` with OCI spec `State`, 2016-12-19, #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, #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/runc#1710 [3]: opencontainers/runc#1741 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

Finish off the work started in 7e84bab (sync up `HookState` with OCI spec `State`, 2016-12-19, #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, #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/runc#1710 [3]: opencontainers/runc#1741 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

wking force-pushed the oci-hook-state branch 2 times, most recently from d1eaede to eb53815 Compare February 25, 2018 23:06

wking mentioned this pull request Feb 25, 2018

server/container_create: Drop 'stage' environment variable from hooks cri-o/cri-o#1360

Closed

wking force-pushed the oci-hook-state branch 2 times, most recently from 4ddee81 to 49c7f84 Compare February 26, 2018 05:21

wking force-pushed the oci-hook-state branch from 49c7f84 to 6ff5804 Compare February 27, 2018 17:24

wking mentioned this pull request Mar 2, 2018

runc does not pass correct OCI state values to hooks #1057

Open

alban mentioned this pull request Mar 5, 2018

add hooks stdin test opencontainers/runtime-tools#589

Merged

wking mentioned this pull request May 18, 2018

check the status of the state passed to hooks over stdin opencontainers/runtime-tools#608

Merged

wking mentioned this pull request May 29, 2018

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

Closed

dongsupark mentioned this pull request May 30, 2018

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

Closed

alban mentioned this pull request Jul 20, 2018

Add runtime-tools' validation tests #1758

Closed

cyphar closed this Nov 13, 2018

cyphar reopened this Nov 14, 2018

cyphar reviewed Nov 14, 2018

View reviewed changes

cyphar added this to the 1.0.0 milestone Nov 14, 2018

wking force-pushed the oci-hook-state branch from 6ff5804 to d804eab Compare November 14, 2018 14:48

wking force-pushed the oci-hook-state branch from d804eab to e238686 Compare November 14, 2018 14:50

cyphar merged commit e238686 into opencontainers:master Nov 19, 2018

cyphar added a commit that referenced this pull request Nov 19, 2018

merge branch 'pr-1741'

ceefc3f

libcontainer: Set 'status' in hook stdin LGTMs: @cyphar @crosbymichael Closes #1741

Ace-Tang mentioned this pull request Nov 30, 2018

ci: update runc version to 1.0.0-rc6-1 AliyunContainerService/pouch#2516

Merged

adrianreber mentioned this pull request Dec 3, 2018

cr: get pid from criu notify when restore #1944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libcontainer: Set 'status' in hook stdin #1741

libcontainer: Set 'status' in hook stdin #1741

wking commented Feb 25, 2018 •

edited

Loading

cyphar commented Feb 26, 2018

dongsupark commented May 23, 2018

wking commented May 23, 2018

dongsupark commented May 23, 2018

wking commented May 23, 2018

dongsupark commented May 24, 2018

wking commented May 29, 2018

cyphar commented Nov 13, 2018

cyphar Nov 14, 2018

wking Nov 14, 2018

cyphar Nov 14, 2018

wking Nov 14, 2018

wking Nov 14, 2018

wking Nov 14, 2018

cyphar Nov 14, 2018 •

edited

Loading

wking Nov 14, 2018

cyphar commented Nov 14, 2018

cyphar commented Nov 14, 2018

cyphar commented Nov 14, 2018 •

edited by caniszczyk

Loading

crosbymichael commented Nov 14, 2018 •

edited by caniszczyk

Loading

cyphar commented Nov 14, 2018

cyphar commented Nov 18, 2018

RenaudWasTaken commented Nov 19, 2018

cyphar commented Nov 19, 2018

libcontainer: Set 'status' in hook stdin #1741

libcontainer: Set 'status' in hook stdin #1741

Conversation

wking commented Feb 25, 2018 • edited Loading

cyphar commented Feb 26, 2018

dongsupark commented May 23, 2018

wking commented May 23, 2018

dongsupark commented May 23, 2018

wking commented May 23, 2018

dongsupark commented May 24, 2018

wking commented May 29, 2018

cyphar commented Nov 13, 2018

cyphar Nov 14, 2018

Choose a reason for hiding this comment

wking Nov 14, 2018

Choose a reason for hiding this comment

cyphar Nov 14, 2018

Choose a reason for hiding this comment

wking Nov 14, 2018

Choose a reason for hiding this comment

wking Nov 14, 2018

Choose a reason for hiding this comment

wking Nov 14, 2018

Choose a reason for hiding this comment

cyphar Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

wking Nov 14, 2018

Choose a reason for hiding this comment

cyphar commented Nov 14, 2018

cyphar commented Nov 14, 2018

cyphar commented Nov 14, 2018 • edited by caniszczyk Loading

crosbymichael commented Nov 14, 2018 • edited by caniszczyk Loading

cyphar commented Nov 14, 2018

cyphar commented Nov 18, 2018

RenaudWasTaken commented Nov 19, 2018

cyphar commented Nov 19, 2018

wking commented Feb 25, 2018 •

edited

Loading

cyphar Nov 14, 2018 •

edited

Loading

cyphar commented Nov 14, 2018 •

edited by caniszczyk

Loading

crosbymichael commented Nov 14, 2018 •

edited by caniszczyk

Loading