Restore hook language removed by create/start split #532

wking · 2016-08-17T23:20:53Z

Details in the commit messages.

Instead of making these changes in a fresh commit off master:

o---o---o  master
         \
          o  tk/restore-hook-lifecycle

I've made them in two merges so git blame can tell that some lines come from historical commits, and that I'm just pulling them out of the history. The structure of this branch is:

a---c---o---o  master
 \ x         \
  b-d---------e  tk/restore-hook-lifecycle

where b is #384, c is #384 landing, and d and e are new for this PR.

wking · 2016-08-17T23:29:17Z

runtime.md

@@ -54,17 +56,28 @@ The lifecycle describes the timeline of events that happen from when a container
 3. Once the container is created additional actions MAY be performed based on the features the runtime chooses to support.
   However, some actions might only be available based on the current state of the container (e.g. only available while it is started).
 4. Runtime's `start` command is invoked with the unique identifier of the container.
-   The runtime MUST run the user-specified code, as specified by [`process`](config.md#process-configuration).
-5. The container's process is stopped.
+5. The [prestart hooks](config.md#prestart) MUST be invoked by the runtime.


On IRC, @Mrunal suggested switching steps 4 and 5, so the pre-start hooks fire at the end of create instead of the beginning of start. I think that would be surprising considering the naming (621bdb4 talks about a possible postcreate anchor in that position). But we already have the confusing poststop running after delete (621bdb4 talks about this too).

Personally, I'd rather drop hooks (#483). But if we keep them for backwards compat, I can see that updating consumers to go from runtime-executed hooks to orchestrator executed hooks would be a largish change. However, renaming config fields (prestart → postcreate) seems like a trivial change, and I'd much rather force pre-1.0 users to pay that cost now (they understood pre-1.0 was unstable) than have to explain to all new users why our hook names make no sense ;).

So to be clear on options for resolving this, we have:

a. Keep this line as it is, with prestart firing after start was called but before the meat of the start action (execing the user-specified code).
b. Shift prestart to fire at the end of create.
i. And still call it prestart.
ii. And rename it to postcreate.

I'm ok with (a) and (b.ii). I think (b.i) will have cause more work for users resolving confusion, while (b.ii) will cause a small amount of work for existing hook consumers who have to rename their config settings.

iamanush · 2016-08-19T02:45:59Z

@wking I'm curious if bringing back the hooks will then enable the VM-based runtime configuration details [1] as a hook?

[1] #405

wking · 2016-08-19T16:36:28Z

On Thu, Aug 18, 2016 at 07:46:01PM -0700, Anush Krishnamurthy wrote:

@wking I'm curious if bringing back the hooks will then enable the
VM-based runtime configuration details [1] as a hook?

#405 is currently (38cbfc9) adding vm.imagePath, vm.kernel.path,
vm.kernel.parameters, and vm.kernel.initrd. I don't see how you'd
address any of those via hooks; can you explain the connection you see
in more detail?

iamanush · 2016-08-24T02:00:31Z

@wking I was wondering if #405 can be enabled alternately via hooks.

wking · 2016-08-24T06:41:47Z

On Tue, Aug 23, 2016 at 07:00:32PM -0700, Anush Krishnamurthy wrote:

@wking I was wondering if #405 can be enabled alternately via hooks.

I still don't see how 1. Can you be more specific about how you see
it working? Without something like #405 landing, I think you'd have
to address the #405 use cases with annotations 2 or runtime options
[3](and not hooks).

iamanush · 2016-08-26T02:43:58Z

@wking: Sorry about the churn. Got it now. I confused myself.

mrunalp · 2017-01-06T00:01:21Z

Needs rebase.

wking · 2017-01-06T06:18:37Z

On Thu, Jan 05, 2017 at 04:01:22PM -0800, Mrunal Patel wrote: Needs rebase.

Recreated the origin/master merge with 8e8454c → 31d8883.

crosbymichael · 2017-02-02T22:09:04Z

runtime.md

@@ -13,6 +13,8 @@ The state of a container MUST include, at least, the following properties:
 * **`id`**: (string) is the container's ID.
 This MUST be unique across all containers on this host.
 There is no requirement that it be unique across hosts.
+The ID is provided in the state because hooks will be executed with the state as the payload.


we don't need to hijack the id's description to tell someone why its here because of another feature. It is enable to document that in the hooks description that the container's state is given to the hook as it's payload.

This line was just recovered from the graveyard. It sounds like you want it to stay dead, so I'll drop it when I rebase.

crosbymichael

You need to squash and fix the commits because they show up as merges of a remote tracking branch

I expect the lifecycle information was removed accidentally in be59415 (Split create and start, 2016-04-01, opencontainers#384), because for a time it seemed like that PR would also be removing hooks. Putting the lifecycle information back in, I made some tweaks to adjust to the new environment, for example: * Put the pre-start hooks after the 'start' call, but before the meat of the start call (the container-process exec trigger). Folks who want a post-create hook can add one with that name. I'd like to have renamed poststop to post-delete to avoid confusion like [1]. But the motivation for keeping hooks was backwards compatibility [2] so I've left the name alone. * Put each "...command is invoked..." lifecycle entry in its own list entry, to match the 'create' list entry. * Move the rules about what happens on hook failure into the lifecycle. This matches pre-split entries like: If any prestart hook fails, then the container MUST be stopped and the lifecycle continues at step 7. and avoids respecifying that information in a second location (config.md). * I added the warning section to try and follow post-split's generic "generates an error" approach while respecting the pre-split desire to see what failed (we had "then an error including the exit code and the stderr is returned to the caller" and "then an error is logged"). * I left the state 'id' context out, since Michael didn't want it [3]. [1]: opencontainers#395 Subject: Run post-stop hooks before the container sandbox is deleted. [2]: opencontainers#483 (comment) Subject: *: Remove hooks [3]: opencontainers#532 (comment) Subject: Restore hook language removed by create/start split Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-02-03T05:49:46Z

I've pushed 31d8883 → f0835e9, dropping the state ‘id’ context [1], removing the merges that enabled ‘git blame’ [2], picking up @jlbutler's suggested MUST additions [3], and tightening the lifecycle wording in the config.md entries. [1]: #532 (comment) [2]: #532 (review) [3]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2017-02-02.log.html#t2017-02-02T22:56:45

crosbymichael · 2017-02-28T21:58:18Z

@wking can you rebase this today?

I expect the lifecycle information was removed accidentally in be59415 (Split create and start, 2016-04-01, opencontainers#384), because for a time it seemed like that PR would also be removing hooks. Putting the lifecycle information back in, I made some tweaks to adjust to the new environment, for example: * Put the pre-start hooks after the 'start' call, but before the meat of the start call (the container-process exec trigger). Folks who want a post-create hook can add one with that name. I'd like to have renamed poststop to post-delete to avoid confusion like [1]. But the motivation for keeping hooks was backwards compatibility [2] so I've left the name alone. * Put each "...command is invoked..." lifecycle entry in its own list entry, to match the 'create' list entry. * Move the rules about what happens on hook failure into the lifecycle. This matches pre-split entries like: If any prestart hook fails, then the container MUST be stopped and the lifecycle continues at step 7. and avoids respecifying that information in a second location (config.md). * I added the warning section to try and follow post-split's generic "generates an error" approach while respecting the pre-split desire to see what failed (we had "then an error including the exit code and the stderr is returned to the caller" and "then an error is logged"). * I left the state 'id' context out, since Michael didn't want it [3]. [1]: opencontainers#395 Subject: Run post-stop hooks before the container sandbox is deleted. [2]: opencontainers#483 (comment) Subject: *: Remove hooks [3]: opencontainers#532 (comment) Subject: Restore hook language removed by create/start split Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-03-01T22:28:18Z

Rebased onto master with f0835e9 → 17165f8.

crosbymichael · 2017-03-03T17:37:47Z

LGTM

mrunalp · 2017-03-03T17:42:52Z

Needs rebase again :/

I expect the lifecycle information was removed accidentally in be59415 (Split create and start, 2016-04-01, opencontainers#384), because for a time it seemed like that PR would also be removing hooks. Putting the lifecycle information back in, I made some tweaks to adjust to the new environment, for example: * Put the pre-start hooks after the 'start' call, but before the meat of the start call (the container-process exec trigger). Folks who want a post-create hook can add one with that name. I'd like to have renamed poststop to post-delete to avoid confusion like [1]. But the motivation for keeping hooks was backwards compatibility [2] so I've left the name alone. * Put each "...command is invoked..." lifecycle entry in its own list entry, to match the 'create' list entry. * Move the rules about what happens on hook failure into the lifecycle. This matches pre-split entries like: If any prestart hook fails, then the container MUST be stopped and the lifecycle continues at step 7. and avoids respecifying that information in a second location (config.md). * I added the warning section to try and follow post-split's generic "generates an error" approach while respecting the pre-split desire to see what failed (we had "then an error including the exit code and the stderr is returned to the caller" and "then an error is logged"). * I left the state 'id' context out, since Michael didn't want it [3]. * Make runtime.md references to "generate an error" and "log a warning" links, so readers have an easier time finding more detail on that wording. Where I reference a section, I'm still using the auto-generated anchor for that header and not the anchors which were added in 41839d7 (Merge pull request opencontainers#707 from mrunalp/anchor_tags, 2017-03-03) and similar. Mrunal suggested that the manually-added anchors were mainly intended for the validation tooling [4]. [1]: opencontainers#395 Subject: Run post-stop hooks before the container sandbox is deleted. [2]: opencontainers#483 (comment) Subject: *: Remove hooks [3]: opencontainers#532 (comment) Subject: Restore hook language removed by create/start split [4]: http://ircbot.wl.linuxfoundation.org/eavesdrop/%23opencontainers/%23opencontainers.2017-03-03.log.html#t2017-03-03T18:02:12 Signed-off-by: W. Trevor King <wking@tremily.us>

wking · 2017-03-03T18:09:20Z

On Fri, Mar 03, 2017 at 09:42:53AM -0800, Mrunal Patel wrote: Needs rebase again :/

Rebased again (and made some “generate an error” instances links) with 17165f8 → f636808.

mrunalp · 2017-03-03T18:34:06Z

LGTM

crosbymichael · 2017-03-03T18:36:16Z

LGTM

wking reviewed Aug 17, 2016
View reviewed changes

wking mentioned this pull request Aug 23, 2016

validate: add network host-specific check opencontainers/runtime-tools#200

Merged

mrunalp added this to the 1.0.0 milestone Jan 4, 2017

wking force-pushed the tk/restore-hook-lifecycle branch 3 times, most recently from 58e0001 to 31d8883 Compare January 6, 2017 06:15

crosbymichael reviewed Feb 2, 2017

View reviewed changes

crosbymichael requested changes Feb 2, 2017

View reviewed changes

wking force-pushed the tk/restore-hook-lifecycle branch from 31d8883 to f0835e9 Compare February 3, 2017 05:43

wking force-pushed the tk/restore-hook-lifecycle branch from f0835e9 to 17165f8 Compare March 1, 2017 22:24

crosbymichael approved these changes Mar 3, 2017

View reviewed changes

wking force-pushed the tk/restore-hook-lifecycle branch from 17165f8 to f636808 Compare March 3, 2017 18:07

crosbymichael merged commit 7da699d into opencontainers:master Mar 3, 2017

wking deleted the tk/restore-hook-lifecycle branch March 3, 2017 19:12

vbatts mentioned this pull request Mar 6, 2017

Bump version #710

Merged

hqhq mentioned this pull request Aug 17, 2017

when should prestart hooks be called #909

Closed

wking mentioned this pull request Mar 26, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore hook language removed by create/start split #532

Restore hook language removed by create/start split #532

wking commented Aug 17, 2016

wking Aug 17, 2016

wking Aug 17, 2016 •

edited

Loading

iamanush commented Aug 19, 2016

wking commented Aug 19, 2016

iamanush commented Aug 24, 2016

wking commented Aug 24, 2016

iamanush commented Aug 26, 2016

mrunalp commented Jan 6, 2017

wking commented Jan 6, 2017 via email

crosbymichael Feb 2, 2017

wking Feb 2, 2017

crosbymichael left a comment

wking commented Feb 3, 2017 via email

crosbymichael commented Feb 28, 2017

wking commented Mar 1, 2017 via email

crosbymichael commented Mar 3, 2017 •

edited by caniszczyk

Loading

mrunalp commented Mar 3, 2017

wking commented Mar 3, 2017 via email

mrunalp commented Mar 3, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Mar 3, 2017 •

edited by caniszczyk

Loading

Restore hook language removed by create/start split #532

Restore hook language removed by create/start split #532

Conversation

wking commented Aug 17, 2016

wking Aug 17, 2016

Choose a reason for hiding this comment

wking Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

iamanush commented Aug 19, 2016

wking commented Aug 19, 2016

iamanush commented Aug 24, 2016

wking commented Aug 24, 2016

iamanush commented Aug 26, 2016

mrunalp commented Jan 6, 2017

wking commented Jan 6, 2017 via email

crosbymichael Feb 2, 2017

Choose a reason for hiding this comment

wking Feb 2, 2017

Choose a reason for hiding this comment

crosbymichael left a comment

Choose a reason for hiding this comment

wking commented Feb 3, 2017 via email

crosbymichael commented Feb 28, 2017

wking commented Mar 1, 2017 via email

crosbymichael commented Mar 3, 2017 • edited by caniszczyk Loading

mrunalp commented Mar 3, 2017

wking commented Mar 3, 2017 via email

mrunalp commented Mar 3, 2017 • edited by caniszczyk Loading

crosbymichael commented Mar 3, 2017 • edited by caniszczyk Loading

wking Aug 17, 2016 •

edited

Loading

crosbymichael commented Mar 3, 2017 •

edited by caniszczyk

Loading

mrunalp commented Mar 3, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Mar 3, 2017 •

edited by caniszczyk

Loading