hooks: start with pre-start and post-stop hooks.

[mrunalp]

See #20
Starting with pre-start and post-stop and then will add more specific hooks tied to namespaces in Linux.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

[crosbymichael]

Should this be an array for the args?

[mrunalp]

As per the discussion in #20, have stuck with not using arguments but passing over a fd like stdin.

[LK4D4]

Now I think that args might be useful. You can have one binary for multiple hooks.

[mrunalp]

How about allowing arguments to be anything that the hooks want but still pass state over a fd?

[LK4D4]

Perfect

[mrunalp]

Updated.

[eyakubovich]

If containers are launched under some daemon that also handles coordination, I would rather let it take care of all the lifecycle related hooks. For example, if running under systemd, why not let it invoke the pre-start and post-exit events with its ExecStartPre and ExecStopPost. It just needs an ability to "nsenter" the container by its ID. This way we don't have to try to think of all the hooks that we might need. For example, ExecStartPost is also useful from systemd experience, do we add it? What about when we need to coordinate across multiple containers -- execute this action once these 3 containers have exited (e.g. shutdown load balancer once all the slaves have exited).

Granted, my examples might be "orchestration events" rather than container life-cycle events but is there a compelling reason to explicitly add hooks for just the container life-cycle. Finally, if these hooks are being declared in the container manifest, why can't the container just have a script such as this as the entry point:

#!/bin/sh
/path/to/pre-start-hook arg1 arg2
/path/to/executable
/path/to/post-stop

Just want to push back to make sure these are truly needed.

[mrunalp]

@eyakubovich perhaps the pre-start hook isn't named as it should. It is more of a pre-exec hook.
So, the intention is to call it after the init process is called, but before the user supplied command is exec'ed.

In case of user namespaces, this allows a hook to customize the network namespace (as an examplle) before the container process is exec'ed. The counter argument to this is that why can't one just create a network namespace outside and then setns to it. However, that doesn't quite work, as each namespace has an owning user namespace and only the root user in that user namespace has all the privileges to make modifications to that namespace. Also, setns itself isn't allowed to a namespace belonging to parent user namespace or a sibling user namespace.

So, such a pre-exec hook allows customization after the namespace fds are created, but before the process is exec'ed.

The flow is that container create process will launch an init process which clones into all the namespaces. The init process then waits on a pipe. The container create process then calls the pre-exec hook, that joins all the namespaces of init and make modifications to the namespaces. The create process then writes to the pipe to unblock the init process to continue on and exec the user supplied command.

One more thing of use that could be done during pre-exec is mounting the namespace fds on the host so these namespaces exist even after the container process exits (of course some caveats apply here). A good use-case (as mentioned by @vishh@rjnagal ) is keeping the mount namespace fd around and then examine the rootfs afterwards in a post-stop hook to debug if the container process exited abnormally.

@LK4D4 please correct or amend as necessary. I will add this to the document.
cc: @philips

[philips]

We should consider the lessons learned from the docker plugins and make this more abstract names instead of absolute paths: moby/moby#13514 (comment) and moby/moby#13859

[mrunalp]

@philips I don't mind doing that. We would be prescribing standard locations to load the hooks from then, right? Also, that doesn't gel quite well with allowing arbitrary arguments, IMO (which isn't necessarily bad but we need to define useful arguments).

[LK4D4]

@mrunalp Yeah, so pre-exec is after container creation, but before process start - for initial container setup, for linux it will be network creation for example.

[mrunalp]

Pushed update to specify directories for hooks. Still need to resolve whether we need any arguments at all? The case that @LK4D4 mentioned could be solved by symlinks or copying the hooks with different names.

[crosbymichael]

This should be an array so that you can specify multiple binaries for a single hook.

"pre-start": [
["ps", "aux"],
["ip", "a"]
]

[mrunalp]

👍 Updating

[crosbymichael]

Do we need a specific directory to store the hooks in? Why can a hook not be any file on the filesystem?

[mrunalp]

@crosbymichael That is up for debate. I am leaning towards any file. @philips do you have any arguments against?

[mrunalp]

Any more thoughts for these hooks?

[wking]

On Wed, Jul 01, 2015 at 01:55:54PM -0700, Mrunal Patel wrote:

Pushed update to specify directories for hooks…

I'd rather not specify well-known directories for hooks, since that
seems like it may be tricky to maintain. Who installs the hooks?
Most users will be running some high-level tool and not runc directly.
Do they put their hooks somewhere else, and have the high-level tool
copy them into the right spot (this sounds wasteful)? Do they push
the hooks directly to the runc locations (now you have to explain to
the user that you're using runc under the hood)?

To make it easy to distribute hooks, I'd rather have relative hook
paths be based on the config's directory (like root.path). But I'd be
ok with “check through a set of well-known directories for hooks
matching the relative path, and make the config's directort the last
well-known directory in that list”.

In any case, I feel like allowing absolute-paths for hooks makes sense
and is independent of how we decide to handle relative paths.

[jonboulle]

On Wed, Jul 8, 2015 at 4:14 AM, W. Trevor King notifications@github.com
wrote:

I'd rather not specify well-known directories for hooks, since that

+1, location of hooks should be at implementation's discretion

[mrunalp]

Updated to take out the hooks directory paragraph.

[wking]

On Wed, Jul 08, 2015 at 09:47:38AM -0700, Mrunal Patel wrote:

Updated to take out the hooks directory paragraph.

Hmm. Will runc take an argument or a RUNC_HOOK_PATH environment
variable? Will it just search the PATH? Are we looking for the hooks
in the host filesystem? In the container's filesystem?

Since this all seems complicated, I went back and reread
@eyakubovich's request for motivation 1 and your response 2. If
the pre-start and post-stop hooks are executed in the same namespace
as the main process, I still don't see why users who need them can't
bundle them into a wrapper script. With something like:

$ cat rootfs/bin/wrap.sh
#!/bin/sh
fix-mounts arg1 arg2 &&
setup-network eth0 &&
"$@"
cleanup -f --exit-code "$?"

You could:

runc wrap.sh docker --daemon

or some such to get full control over your setup/teardown without any
spec-side complication. I'm not sure how a pre-exec hook that has
joined all the init namespaces would be mounting namespace fds on the
host anyway. So:

• We should decide if the hook-like functionality is executed inside
  or outside the container.
• If it's inside, I suggest we just drop hooks from the spec in favor
  of wrapper scripts.
• If it's outside, I suggest we interpret hooks like any other
  host-side executable (absolute paths are absolute, relative paths
  are relative to $PATH), with an additional convenience fallback that
  we expand them from the config directory if we can't find a match in
  PATH. That's like appending ‘.’ to the PATH when launching the hook
  processes, but keeping the original PATH for any children of the
  hook processes.

[mrunalp]

@wking The hooks won't be run in the container's namespaces. They will be run from the host and they would get the state data including the container pid, so they could join the namespaces that they want to.
The hooks will also be run from the host's filesystem. If we require the path to the hooks to be absolute then there is no need for setting a special PATH variable. We could also just use the default PATH to search for the hooks when it is not absolute.

I will clarify this in the doc if there is no objection.
Thanks!

[mrunalp]

Pushed update with clarification about where the hooks will be executed from.

[mrunalp]

Fixed.

[mrunalp]

For post-stop I think it makes sense to log errors and continue executing the remaining hooks. Updated to reflect that in the document and moved the previous generic section to pre-start section.

[wking]

On Wed, Jul 08, 2015 at 11:45:09AM -0700, Mrunal Patel wrote:

I will clarify this in the doc…

It looks better to me now :).

My main concern with host-PATH lookups that don't include the root
container-bundle directory (where the config file lives) is that
you're injecting additional unversioned dependencies. On the one
hand, using host-installed software makes it easier for the host to
trust the hooks to not abuse their outside-the-container privileges.
On the other hand, it means you need to be able to count on the weak
API-compatibility link between the argument syntax that container
author wrote for and the version of a tool that the host sysadmin has
actually installed (assuming they've installed it at all :p). Since
you're exposing your whole filesystem to arbitrary args anyway, folks
will already be able to do things like:

"hooks" : {
"pre-start": [
["rm", "-rf", "/"]
]
}

I think it makes more sense to include the root container-bundle
directory in the lookup chain, which will give the container author a
say in which tool is run. But for folks like me that are only running
trusted containers (and not offering container-running-as-a-service),
I guess:

$ PATH=".:$PATH" runc

is easy enough ;).

Another thing to consider here is whether the hook-block should be
platform-specific or not. I expect it should be a single Hooks type
that's used as a field in the per-platform types (e.g. Linux.Hooks,
Windows.Hooks, …). And then handle inconsistencies within the
platform in the hook-executables themselves?

[LK4D4]

@crosbymichael Maybe I missing something. So, I need to run /usr/bin/docker with os.Args[0] == "setup-namespace", how can I do this with array of args?

[crosbymichael]

@LK4D4 if argv0 is an abs path then just set it as the path.

[LK4D4]

@crosbymichael Okay, but I still think that Args+Path more understandable, then "if first element is abs path, then argv[0] is actually argv[1]"

[mrunalp]

@crosbymichael WDYT? Need to decide one way or the other :)

[crosbymichael]

@mrunalp I think @LK4D4 is right, sorry for the back and forth

[mrunalp]

@crosbymichael No problem ;) I think it makes sense to add Path, too. Updated.
@LK4D4

[LK4D4]

LGTM

[crosbymichael]

I would change this to after the container's namespaces are created and not say started because it's confusing with the hooks name as prestart

[mrunalp]

How about

The pre-start hooks are called after the container process is spawned, but before the user supplied command is executed.
They are called after the container namespaces are created on Linux, so they provide an opportunity to customize the container.
In Linux, for e.g., the network namespace could be configured in this hook.

[mrunalp]

@crosbymichael updated

[crosbymichael]

LGTM