Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hooks order explanation in runtime.md #156

Closed
wants to merge 2 commits into from
Closed

Add hooks order explanation in runtime.md #156

wants to merge 2 commits into from

Conversation

liangchenye
Copy link
Member

I added a comment in:
#20

Since one hook may rely on another(s) in the hooks list,
add an explicit explanation will be helpful.

It will also be necessary if we convert the hooks to/from one prestart/poststop script.

The request is similar with wking's #142

If a hook returns a non-zero exit code, then an error including the exit code and the stderr is returned to the caller and the container is torn down.



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably leave these blank-line insertions out ;).

@wking
Copy link
Contributor

wking commented Sep 8, 2015

On Mon, Sep 07, 2015 at 06:44:53PM -0700, 梁辰晔 (Liang Chenye) wrote:

Since one hook may rely on another(s) in the hooks list, add an
explicit explanation will be helpful.

I left a few minor comments for cleaning up and signing-off on this
commit, but I think its a good change (just needs a bit more polishing
;).

Add an explicit explanation of the hooks order.

Signed-off-by: Liang Chenye <liangchenye@huawei.com>
@liangchenye liangchenye closed this Sep 8, 2015
@liangchenye liangchenye mentioned this pull request Sep 8, 2015
@wking
Copy link
Contributor

wking commented Sep 8, 2015

On Tue, Sep 08, 2015 at 12:20:31AM -0700, 梁辰晔 (Liang Chenye) wrote:

Closed #156.

Why close this?

@liangchenye
Copy link
Member Author

I opened a new one to make patch clearer to maintainers.

@wking
Copy link
Contributor

wking commented Sep 9, 2015

On Tue, Sep 08, 2015 at 05:52:40PM -0700, 梁辰晔 (Liang Chenye) wrote:

I opened a new one to make patch clearer to maintainers.

There's no need to open a new pull request. If you fix the commit
locally and force-push to the same branch in your GitHub remote,
GitHub will update any open PRs using that branch automatically. In
this case, #157 is already filed, but next time just force-push your
updated commit ;).

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 11, 2015
Extend [1,2,3] to avoid:

  hook 1: spawn ---------------> reaped
  hook 2:       spawn ----------------> reaped
  hook 3:             spawn -----> reaped

and explicitly require:

  hook 1: spawn --> reaped
  hook 2:                  spawn --> reaped
  hook 3:                                   spawn --> reaped

Folks who do want parallel execution are free to use a parallelizing
wrapper:

  hook 1: spawn ---------------------------> reaped
                child 1 -----> reaped
                  child 2 ---------> reaped
                    child 3 ---> reaped

Although that cuts both ways (with parallel hooks, folks could always
use a single hook with a serializing wrapper).  Still, I'd guess most
current implementations are already taking the serialized approach, so
it makes bundle-author life easier if we are explicit about that.

[1]: opencontainers#20 (comment)
[2]: opencontainers#156
[3]: opencontainers#157

Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants