config: Clarify execution environment for hooks #953
Conversation
|
I updated my commitmsg with the DCO. |
|
We originally got language on this issue in #415, but it was accidentally dropped in #427. I'd rather address it with something closer to: $ git diff -U2
diff --git a/config.md b/config.md
index 06801f5..1949a2c 100644
--- a/config.md
+++ b/config.md
@@ -374,4 +374,5 @@ For POSIX platforms, the configuration structure supports `hooks` for configurin
* **`path`** (string, REQUIRED) with similar semantics to [IEEE Std 1003.1-2008 `execv`'s *path*][ieee-1003.1-2008-functions-exec].
This specification extends the IEEE standard in that **`path`** MUST be absolute.
+ Runtimes MUST resolve this value in the [runtime mount namespace](glossary.md#runtime-namespace).
* **`args`** (array of strings, OPTIONAL) with the same semantics as [IEEE Std 1003.1-2008 `execv`'s *argv*][ieee-1003.1-2008-functions-exec].
* **`env`** (array of strings, OPTIONAL) with the same semantics as [IEEE Std 1003.1-2008's `environ`][ieee-1003.1-2008-xbd-c8.1].
@@ -385,4 +386,5 @@ For POSIX platforms, the configuration structure supports `hooks` for configurin
Hooks allow users to specify programs to run before or after various lifecycle events.
Hooks MUST be called in the listed order.
+Hooks MUST be executed in the [runtime namespace](glossary.md#runtime-namespace).
The [state](runtime.md#state) of the container MUST be passed to hooks over stdin so that they may do work appropriate to the current state of the container.which leans on something closer to our existing namespace My suggestion doesn't address |
|
I believe we use the term
So maybe something like "hooks are executed in the runtime's environment" in your change instead of saying in the "container host". What do you think? |
|
I updated the patch with @wking's phrasing. I think it addresses @crosbymichael's concerns too. Interesting about opencontainers/runc#1738 - I didn't know about that Go semantics for exec.Cmd. Not sure it worth saying something in the spec since it's Go specific. But I would like to add a validation test in runtime-tools' g.AddPreStartHook. |
|
LGTM |
config.md
Outdated
| @@ -373,6 +373,7 @@ For POSIX platforms, the configuration structure supports `hooks` for configurin | |||
| Entries in the array contain the following properties: | |||
| * **`path`** (string, REQUIRED) with similar semantics to [IEEE Std 1003.1-2008 `execv`'s *path*][ieee-1003.1-2008-functions-exec]. | |||
| This specification extends the IEEE standard in that **`path`** MUST be absolute. | |||
| Runtimes MUST resolve this value in the [runtime mount namespace](glossary.md#runtime-namespace). | |||
There was a problem hiding this comment.
remove mount here to make it consistent with the link and the other usage in your changeset.
The spec didn't say whether the hooks are executed in the container environment or not (in Linux namespaces, with cgroups and rlimits applied). In rkt, hooks are executed in the container environment. In runc, hooks are executed outside of the container environment. The example with setting up the network namespace in "prestart" suggests that the command is generic and does not have to exist in the container rootfs. So I assume the spec was meant to say hooks are executed outside of the container environment. Signed-off-by: Alban Crequy <alban@kinvolk.io>
|
LGTM |
|
LGTM |
The spec didn't say whether the hooks are executed in the container
environment or not (in Linux namespaces, with cgroups and rlimits
applied).
In rkt, hooks are executed in the container environment. In runc, hooks
are executed outside of the container environment.
The example with setting up the network namespace in "prestart" suggests
that the command is generic and does not have to exist in the container
rootfs. So I assume the spec was meant to say hooks are executed outside
of the container environment.
Signed-off-by: Alban Crequy alban@kinvolk.io