runtime-config: Require serial hook execution

[wking]

Extend #157 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.

[hqhq]

Wouldn't this cause confusion if "each hook process" includes the child processes it creates?

[wking]

On Thu, Dec 10, 2015 at 10:55:18PM -0800, Qiang Huang wrote:

-Hooks MUST be called in the listed order.
+Hooks MUST be called in the listed order, with each hook process being reaped before the next hook is executed.

Wouldn't this cause confusion if "each hook process" includes the
child processes it creates?

What sort of confusion? If the hook wants to explicitly launch a
daemon or some such, I'm fine with:
—
hook 1: spawn ------> reaped
daemon------------------------------------------->
hook 2: spawn --> reaped
hook 3: spawn --> reaped

If folks are worried about hook-children surviving an early hook exit:
—
hook 1: spawn ------(died)-> reaped
child------------------------------------------->
hook 2: spawn --> reaped
hook 3: spawn --> reaped

we can require the runtime to be more shell-like and make a new
process group to contain the hooks (but this will start getting pretty
complicated).

Without some sort of rule about reaping, I don't see much gain to the
“Hooks MUST be called in the listed order” requirement. If they're
all spawned one after the other, you don't have any guarantees that
earlier hooks will have accomplished their task before later hooks are
run.

It might be easier to just separate ‘create’ (most of the container
setup) from ‘exec’ (process launch and possible PID namespace
creation) and ‘destroy’ (remove anything left-over from ‘create’) and
get out of the hook business altogether [1,2,3].

[mrunalp]

I think we should take out reaped from the suggested change.

[hqhq]

I understand the scenario, it looks good, just wondering if the phrase "with each hook process being reaped" will make some users think the second hook needs to wait until all processes from the first hook are reaped. Because I do have this question when I first read this, maybe just me?

[wking]

On Mon, Dec 14, 2015 at 12:38:53AM -0800, Qiang Huang wrote:

I understand the scenario, it looks good, just wondering if the
phrase "with each hook process being reaped" will make some users
think the second hook needs to wait until all processes from the
first hook are reaped. Because I do have this question when I first
read this, maybe just me?

Probably not just you ;). Would some more polished version of:
“runtimes are just waiting on the hook process they launch, and don't
jump through hoops to track or kill any additional processes spawned
by the hook” help?

[hqhq]

Probably not just you ;). Would some more polished version of:
“runtimes are just waiting on the hook process they launch, and don't
jump through hoops to track or kill any additional processes spawned
by the hook” help?

I don't know, it looks complicated and still not clear enough, and I don't have a better suggestion TBH.
Maybe we can just keep it as it was, the original definition looks good to me, I don't think implementation authors would confuse about how to serialize hooks, as you said, they can be serialized or paralleled anyway. I think on this point, we can keep spec minimal and stable, and enable innovation and experimentation above and around it(quoted from OCI charter).

Let's also wait for other maintainers' options.

[wking]

On Tue, Dec 15, 2015 at 01:24:01AM -0800, Qiang Huang wrote:

Maybe we can just keep it as it was, the original definition looks
good to me, I don't think implementation authors would confuse about
how to serialize hooks, as you said, they can be serialized or
paralleled anyway.

My issue here is that config authors who need serialized execution
(e.g. for things like this 1) will be forced to use a shell script
or some such for all the steps. And folks who need parallelization
(maybe their hooks all talk together?) will be forced to use a shell
script or some such to set that up. So being vague makes life easier
for runtime authors (who get lots of choices) but hard for bundle
authors (who have to work around the ambiguity).

I think on this point, we can keep spec minimal and stable, and enable innovation and experimentation above and around it(quoted
from OCI charter).

If phrasing for serialization is too complicated (and we're
intentionally shooting for per-runtime choices around
serialization/parallelization), then I still think we should just drop
the current “Hooks MUST be called in the listed order” requirement
2. Without something along the lines of this PR, I don't see how
you'd test for compliance with that requirement.

[vbatts]

Honestly, serial is the best guarantee without introducing a syntax like
that of systemd.unit files where services (
http://www.freedesktop.org/software/systemd/man/systemd.service.html) have
a Requires= and After= notation to enforce order, otherwise it is
parallelizable.

On Tue, Dec 15, 2015 at 11:10 AM, W. Trevor King notifications@github.com
wrote:

On Tue, Dec 15, 2015 at 01:24:01AM -0800, Qiang Huang wrote:

Maybe we can just keep it as it was, the original definition looks
good to me, I don't think implementation authors would confuse about
how to serialize hooks, as you said, they can be serialized or
paralleled anyway.

My issue here is that config authors who need serialized execution
(e.g. for things like this 1) will be forced to use a shell script
or some such for all the steps. And folks who need parallelization
(maybe their hooks all talk together?) will be forced to use a shell
script or some such to set that up. So being vague makes life easier
for runtime authors (who get lots of choices) but hard for bundle
authors (who have to work around the ambiguity).

I think on this point, we can keep spec minimal and stable, and enable innovation and experimentation above and around it(quoted
from OCI charter).

If phrasing for serialization is too complicated (and we're
intentionally shooting for per-runtime choices around
serialization/parallelization), then I still think we should just drop
the current “Hooks MUST be called in the listed order” requirement
2. Without something along the lines of this PR, I don't see how
you'd test for compliance with that requirement.

—
Reply to this email directly or view it on GitHub
#265 (comment).

[wking]

On Tue, Dec 15, 2015 at 08:28:55AM -0800, Vincent Batts wrote:

Honestly, serial is the best guarantee without introducing a syntax
like that of systemd.unit files…

That works for me, and it is where I'm going with the changes
suggested by this PR (as of 330223b). I think we're on the same
page then (Qiang liked it here 1), but we need to figure out if/how
we want to clarify Qiang's concern about hook-child reaping 1, since
my suggested approach to that 2 seems too confusing 3. Re-reading
3, it's possible that Qiang's “keep it as it was” was “the current
PR looks good to me” and not “the pre-330223b31 spec is fine as it
stands” which was how I'd read his comment when I wrote 4.

[philips]

I am in favor of serial execution of plugins. I am confused about the "reaped" language though. It seems like saying "Hooks will be executed in order and the next hook won't be executed until the previous has exited".

[wking]

On Wed, Dec 16, 2015 at 08:46:16AM -0800, Brandon Philips wrote:

I am confused about the "reaped" language though. It seems like
saying "Hooks will be executed in order and the next hook won't be
executed until the previous has exited".

Reaped is one step past process-exit, when the zombie's entry is
removed from the process table 1. As far as the spec goes, the
distinction is that after hook-process-exit, there is an exit code,
but until the host-process reaps the hook-process, the host-process
won't know what that exit code is (barring something complicated like
a waitid call with WNOWAIT).

[vbatts]

that is the fully implication of the current sentence. The act of waiting on a pid is an implementation detail.

[crosbymichael]

There is no need to tell people that they need to reap processes in the specification.

[wking]

On Wed, Jan 13, 2016 at 01:50:49PM -0800, Vincent Batts wrote:

that is the fully implication of the current sentence. The act of
waiting on a pid is an implementation detail.

I don't mind if the runtime decides to not collect the PID, but I
don't see how the current “Hooks MUST be called in the listed order”
requires the runtime to wait for a hook to exit before launching the
next one.

Anyhow, I'm fine waiting on compliance testing to bring this back
around, assuming a split create/exec hasn't obsoleted hooks 1 by the
time we get compliance testing for hooks.

[wking]

Now that we've decided to keep hooks and closed #483, I think we want
to re-open this one. I think testing it is going to be tricky, but
I'm pretty sure we don't want parallel hook execution (and
@crosbymichael seemed to agree 1). If we want to leave the PR
closed until ocitools validates hook functionality, I'm happy to float
a Most Egregious Accurate Interpretation of the current spec wording
;) 2.

 Subject: Reviewing the specifications from a compliance perspective and a thought to certification
 Date: Tue, 9 Aug 2016 12:38:33 -0700 (PDT)
 Message-Id: <d3fda8a4-e87a-4560-9d3f-6041b9098b91@opencontainers.org>