Sync HookState struct with OCI spec #1201

WeiZhang555 · 2016-11-28T03:28:58Z

* modify json name of ociVersion to version
* Add missing Status and Annotations field

Signed-off-by: Zhang Wei [email protected]

Edit:

HookState struct should follow definition of State in runtime-spec:

Remove redundant Rootfs field as rootfs can be retrived from
bundlePath/config.json

WeiZhang555 · 2016-11-28T03:32:46Z

I noticed that the json name of Version is changed from version to ociVersion in #724 , according to current spec( State ), this ociVersion is wrong, and I didn't find any occurrence of ociVersion in git history of runtime spec.

This modification is a breakdown to backward compatibility but we have to do it, and the earlier the better. We should follow the name of the spec, this is important.

cyphar · 2016-11-28T03:57:25Z

libcontainer/configs/config.go

+	Pid         int               `json:"pid"`
+	Root        string            `json:"root"`
+	BundlePath  string            `json:"bundlePath"`
+	Annotations map[string]string `json:"annotations"`


Since you've added fields, can you please update the code to actually fill them? And also add some tests.

Yep, I'm already doing this now, was sending this for a first glance in case someone is against this.

I was just thinking that the Status field is not very useful, because prestart,poststart,poststop happens at special time point and we don't need the status to identify container status, so I am also modifying the spec and trying to sync up HookState struct between spec and runc. :-)

WeiZhang555 · 2016-11-28T06:27:22Z

I‘ve sent another PR to runtime-spec: opencontainers/runtime-spec#633, for adding Root and removing Status. I'll update this once the spec change is merged. 😉

`HookState` struct should follow definition of `State` in runtime-spec: * modify json name of `version` to `ociVersion`. * Remove redundant `Rootfs` field as rootfs can be retrived from `bundlePath/config.json`. Signed-off-by: Zhang Wei <[email protected]>

WeiZhang555 · 2016-12-20T02:34:35Z

This is ready for review, Janky is also good, ping @cyphar @hqhq @mrunalp @crosbymichael

hqhq · 2016-12-22T03:29:52Z

LGTM

cyphar · 2016-12-22T09:03:42Z

libcontainer/integration/exec_test.go

 	config.Labels = append(config.Labels, fmt.Sprintf("bundle=%s", expectedBundlePath))
+
+	getRootfsFromBundle := func(bundle string) (string, error) {


Why not declare this as a function in utils_test.go. Seems a bit odd to define it like this.

Because this is a temporary function which is only used by TestHook, so I thought it's not worth a standalone function, and utils_test.go should contain common functions. :-)

Eh, okay. I don't really like it but it's just a test so w/e.

cyphar · 2016-12-22T22:31:53Z

LGTM

@cyphar

Closes #1201 LGTMs: @cyphar @hqhq

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 <[email protected]>

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 <[email protected]>

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 <[email protected]> Signed-off-by: Dongsu Park <[email protected]>

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 <[email protected]> Signed-off-by: Dongsu Park <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]>

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 <[email protected]>

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 <[email protected]>

cyphar reviewed Nov 28, 2016

View reviewed changes

WeiZhang555 changed the title ~~Sync HookState struct with OCI spec~~ [WIP] Sync HookState struct with OCI spec Nov 28, 2016

WeiZhang555 mentioned this pull request Dec 17, 2016

Bump runtime-spec to v1.0.0-rc3 #1233

Merged

WeiZhang555 force-pushed the syncup-hook-state branch 3 times, most recently from e1ddd3e to 0b43200 Compare December 19, 2016 15:56

WeiZhang555 force-pushed the syncup-hook-state branch from 0b43200 to a344b2d Compare December 19, 2016 16:00

WeiZhang555 changed the title ~~[WIP] Sync HookState struct with OCI spec~~ Sync HookState struct with OCI spec Dec 20, 2016

cyphar reviewed Dec 22, 2016

View reviewed changes

cyphar merged commit a344b2d into opencontainers:master Dec 22, 2016

cyphar added a commit that referenced this pull request Dec 22, 2016

merge branch 'pr-1201'

303f9a5

Closes #1201 LGTMs: @cyphar @hqhq

WeiZhang555 deleted the syncup-hook-state branch December 23, 2016 03:58

wking mentioned this pull request Feb 24, 2018

Get hook stage from stdin's 'status' value containers/oci-umount#35

Open

wking mentioned this pull request Feb 25, 2018

libcontainer: Set 'status' in hook stdin #1741

Merged

wking mentioned this pull request Mar 2, 2018

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync HookState struct with OCI spec #1201

Sync HookState struct with OCI spec #1201

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

cyphar Nov 28, 2016

WeiZhang555 Nov 28, 2016

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

WeiZhang555 commented Dec 20, 2016

hqhq commented Dec 22, 2016 •

edited by caniszczyk

Loading

cyphar Dec 22, 2016

WeiZhang555 Dec 22, 2016

cyphar Dec 22, 2016

cyphar commented Dec 22, 2016 •

edited by caniszczyk

Loading

		config.Labels = append(config.Labels, fmt.Sprintf("bundle=%s", expectedBundlePath))

		getRootfsFromBundle := func(bundle string) (string, error) {

Sync HookState struct with OCI spec #1201

Sync HookState struct with OCI spec #1201

Conversation

WeiZhang555 commented Nov 28, 2016 • edited Loading

WeiZhang555 commented Nov 28, 2016 • edited Loading

cyphar Nov 28, 2016

Choose a reason for hiding this comment

WeiZhang555 Nov 28, 2016

Choose a reason for hiding this comment

WeiZhang555 commented Nov 28, 2016 • edited Loading

WeiZhang555 commented Dec 20, 2016

hqhq commented Dec 22, 2016 • edited by caniszczyk Loading

cyphar Dec 22, 2016

Choose a reason for hiding this comment

WeiZhang555 Dec 22, 2016

Choose a reason for hiding this comment

cyphar Dec 22, 2016

Choose a reason for hiding this comment

cyphar commented Dec 22, 2016 • edited by caniszczyk Loading

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

WeiZhang555 commented Nov 28, 2016 •

edited

Loading

hqhq commented Dec 22, 2016 •

edited by caniszczyk

Loading

cyphar commented Dec 22, 2016 •

edited by caniszczyk

Loading