-
Notifications
You must be signed in to change notification settings - Fork 553
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 create-runtime, create-container, start-container hooks #1008
Conversation
cc @cyphar |
config.md
Outdated
The mount hooks MUST be called as part of the create operation at a time that would allow the hook to execute operations both inside and outside the container. | ||
For example, on linux this would happen before the `pivot_root` operation is executed but after the mount namespace was created and setup. | ||
|
||
For runtimes that implement the deprecated pre-start hooks as mount hooks, mount hooks MUST be called after the prestart hooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should specify the order. Since the definition of the prestart hook is not changed, this order makes it impossible to implement a container runtime with support for the two hooks following the spec. And for compatibility, hooks installed on both prestart and mount could both check if the mount task is already done, so the order should not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for compatibility, hooks installed on both prestart and mount could both check if the mount task is already done, so the order should not matter.
The goal here was that you can rely on ordering instead of checking for state you might have persisted in the mount or prestart hook.
this order makes it impossible to implement a container runtime with support for the two hooks following the spec
Do you mind detailing a bit more what you mean here :) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind detailing a bit more what you mean here :) ?
Sorry, I take it back.
So now, the spec mandates steps to happen in this order:
- the container namespaces are created
- Prestart hooks
- Mount hooks
- pivot_root
- Start hooks
- The user-specified process is executed
- Poststart hooks
I thought the spec was saying that the Prestart hooks must happen after the namespaces are fully configured (including the pivot_root), which would make things impossible. But the wording is more vague:
[The pre-start hooks] are called after the container namespaces are created
So I think it's ok :)
Thanks for your review @alban, will get to them tomorrow! |
config.md
Outdated
### <a name="configHooksMount" />Mount Hooks | ||
|
||
The mount hooks MUST be called as part of the create operation at a time that would allow the hook to execute operations both inside and outside the container. | ||
For example, on linux this would happen before the `pivot_root` operation is executed but after the mount namespace was created and setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linux -> Linux.
Comments addressed :) |
Any update on this? |
@cyphar can you look into this :) ? |
Ping @cyphar |
Can anyone provide an update/review here? Pings based on recent merges |
config.md
Outdated
@@ -384,6 +384,10 @@ For POSIX platforms, the configuration structure supports `hooks` for configurin | |||
* **`env`** (array of strings, OPTIONAL) with the same semantics as [IEEE Std 1003.1-2008's `environ`][ieee-1003.1-2008-xbd-c8.1]. | |||
* **`timeout`** (int, OPTIONAL) is the number of seconds before aborting the hook. | |||
If set, `timeout` MUST be greater than zero. | |||
* **`mount`** (array of objects, OPTIONAL) is an array of [mount hooks](#mount-hooks). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"mount" is a really bad naming selection. It is applicable to single vendor at the moment (NVidia) and from naming denotes other usages. Currently prestart hook is used by others not only to do mounts, but also to do various devices initialization. If we want to be explicit in defining execution timings of the hooks, let's define that, but please don't imply in the names single use case of the hook.
config.md
Outdated
@@ -375,7 +375,7 @@ For Windows based systems the user structure has the following fields: | |||
For POSIX platforms, the configuration structure supports `hooks` for configuring custom actions related to the [lifecycle](runtime.md#lifecycle) of the container. | |||
|
|||
* **`hooks`** (object, OPTIONAL) MAY contain any of the following properties: | |||
* **`prestart`** (array of objects, OPTIONAL) is an array of [pre-start hooks](#prestart). | |||
* **`prestart`** (array of objects, OPTIONAL, **DEPRECATED**) is an array of [pre-start hooks](#prestart). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd propose to not deprecate prestart hooks. They may be used for other purposes than mount and start hooks. For example our FPGA device plugin verifies and programs FPGA devices in a prestart hook.
Let's take a step back :) Pre-start hooks are not getting deprecated. In the spec, pre-start hooks are defined as follows:
After discussing this in opencontainers/runc#1811 we realized that this meant that this meant that pre-start hooks needed to be called after the After discussion, we decided to clarify the meaning of pre-start hooks and allow to have hooks both "before" and "after" Hence, we came up with the two new hooks:
So pre-start hooks aren't being deprecated "per-se", they are being renamed. The names were chosen based on LXC's current hooks but if you feel strongly about another name, I'll happily change the PR. |
@RenaudWasTaken the problem is that "mount" implies a lot of meaning and does more confusion than clarification in comparison with current I would rather do the names that will explicitly refer to environment where it is executed (before/after pivot_root, to be explicit). Something like: keep current |
That's unfortunately not an option, clarifying it either way is a breaking change.
I have no strong feelings on the naming, If we think there is more value in using a different name that is clearer than I'm happy to change this PR. |
config.md
Outdated
@@ -399,6 +403,20 @@ The [state](runtime.md#state) of the container MUST be passed to hooks over stdi | |||
The pre-start hooks MUST be called after the [`start`](runtime.md#start) operation is called but [before the user-specified program command is executed](runtime.md#lifecycle). | |||
On Linux, for example, they are called after the container namespaces are created, so they provide an opportunity to customize the container (e.g. the network namespace could be specified in this hook). | |||
|
|||
Note: Prestart hooks were deprecated in favor of mount and start hooks, which have a clearer definition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would explicitly mention that runc
incorrectly implemented them, to ensure users don't depend on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to use -host and -container or -inside and -outside suffixes to the hook names? It's still not clear to me if proposed hooks will be executed in the host or container namespace. "mount" looks quite vague to me in this regard. It requires some guesswork to understand its purpose. Would it be better to have prestart-host and prestart-container instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyphar @RenaudWasTaken Can you clarify the statement about "runc
incorrectly implemented them" ? From what I see in https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-hooks it says "Hooks MUST be executed in the runtime namespace.", and prestart is executed accordingly to this specification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pre-start hooks MUST be called after the start operation is called but before the user-specified program command is executed.
This points to having the hook after the pivot_root
is called :)
https://github.com/opencontainers/runtime-spec/blob/master/config.md#prestart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bart0sh, is the naming something that can be resolved by better detailling the hooks in the spec?
Today they are worded as follows:
mount hooks
The mount hooks MUST be called as part of the create operation at a time that would allow the hook to execute operations both inside and outside the container.
For example, on Linux this would happen before thepivot_root
operation is executed but after the mount namespace was created and setup.
start hooks
The start hooks MUST be called before the user-specified process is executed as part of the
start
operation.
This hook can be used to execute some operations in the container, for example running the ldconfig binary on linux before the container process is spawned.
Do you think we could reword the definition in a different way that would reduce the confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pre-start hooks MUST be called after the start operation is called but before the user-specified program command is executed.
This points to having the hook after the
pivot_root
is called :)
https://github.com/opencontainers/runtime-spec/blob/master/config.md#prestart
Well, not really. It specifies the timing. But where hook itself is called is specified by line that I quoted from spec above: "Hooks MUST be executed in the runtime namespace", and runtime namespace defined as https://github.com/opencontainers/runtime-spec/blob/master/glossary.md#runtime-namespace
"After pivot_root
" will be already not a runtime namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prestart hooks were deprecated in favor of mount and start hooks
mount and start hooks are executed in container namespace. Prestart hooks executed in runtime namespace. They can't be deprecated in favor of something that runs in a different namespace.
@kad runc will continue to support @RenaudWasTaken I think copying the LXC hooks is the only reasonable choice. I don't want to get caught up in bike-shedding over what hook names we should use. From my side, this LGTM -- as long as the NVIDIA folks are happy with it (sorry for the long time to review, I've been swamped with |
I've updated the PR :) |
/cc @opencontainers/runtime-spec-maintainers |
I like this idea. It keeps the original name "prestart", and clarifies how it has been split up.
Did you forget to push? ;-)
Glad to see this moving forward again in any case! I'm guessing you're thinking about solving this issue already for If my surmise about the situation is correct, how about releasing rc9 first and then solving the hooks under less time pressure? |
Thanks! Updated
The issue that I faced with using the It seemed to me that keeping existing terminology (i.e: used in LXC) was a lot less confusing since a |
8c0b611
to
3720456
Compare
In the latest update, the following changes were made:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some minor comments.
schema/defs.json
Outdated
@@ -108,6 +108,12 @@ | |||
"$ref": "#/definitions/Hook" | |||
} | |||
}, | |||
"ArrayOfContainerHooks": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this chunk be dropped now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed, thanks for noticing!
specs-go/config.go
Outdated
@@ -125,13 +125,26 @@ type Hook struct { | |||
Timeout *int `json:"timeout,omitempty"` | |||
} | |||
|
|||
// Hook specifies a command that is run in the container at a particular event in the lifecycle of a container |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here it should start with // Hooks ....
or leave an empty line after the comment.
Otherwise "make .golint" will complain about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed, thanks for noticing!
runtime.md
Outdated
3. The [`prestart` hooks](config.md#prestart) MUST be invoked by the runtime. | ||
If any `prestart` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. | ||
4. The [`create-runtime` hooks](config.md#create-runtime-hooks) MUST be invoked by the runtime. | ||
If any `create-runtime` hook fails, the runtime MUST [generate an error](#errors), stop the container, and continue the lifecycle at step 12. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should create-runtime
and create-container
be specified in camel case as well as in config.md
? At least the schema and the Go code seems to use the camel case version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed!
3720456
to
0351bd8
Compare
Signed-off-by: Renaud Gaubert <[email protected]>
0351bd8
to
c166268
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM from my side. |
I'm quite sure no-one wants to receive a ping from this thread ever again, but since some parts of the new hooks were left un(der)specified, I opened a follow-up issue to track things: #1039. |
In PR opencontainers#1008 3 new steps in the lifecycle were added and the referencing of the states to the corresponding lifecycle step was not adjusted. Corrected based on Tag v1.0.0-rc6.
In PR opencontainers#1008 3 new steps in the lifecycle were added and the referencing of the states to the corresponding lifecycle step was not adjusted. Corrected based on Tag v1.0.0-rc6. Signed-off-by: Pascal Theml <[email protected]>
Poststart hooks exist in runc since 2015 [1], and since that time until today, if a hook returned an error, runc kills the container. In 2020, commit c166268 (PR opencontainers#1008) added the following text (which became part of runtime-spec release v1.0.2): > 9. The `poststart` MUST be invoked by the runtime. If any > `poststart` hook fails, the runtime MUST log a warning, but the > remaining hooks and lifecycle continue as if the hook had succeeded. Now, this text conflicted with the pre-existing runtime (runc) behavior, and it still conflicts with the current runc behavior. At this point, we can either fix runtimes or the spec. To my mind, fixing the spec is a better approach, because: - initial implementation predates the spec wording by a few years; - the wording in the spec was never implemented (in runc); - returning an error (and stopping the container) seems like a more versatile approach, since a hook can usually choose whether to return an error or not. [1]: opencontainers/runc#392 Signed-off-by: Kir Kolyshkin <[email protected]>
Hello!
Since we discussed this during the last OCI weekly meeting, I wanted to continue the conversation on a more concrete change.
Related:
I've used the same names as LXC for the hooks. I'm happy to change them if we feel there are more appropriate names.
My reasoning while drafting this was to answer some of these questions:
mount
hook in a way that would clearly define the ability to mount "stuff" inside the containerHere it would be helpful to define the ordering such that the pre-start hook detects that it doesn't need to be executed in a setting where we specify both pre-start hook and mount hook for backwards compat reasons.
Questions still open:
What do you think about this first draft?
Signed-off-by: Renaud Gaubert [email protected]