-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 CreateRuntime, CreateContainer and StartContainer Hooks #2229
Add CreateRuntime, CreateContainer and StartContainer Hooks #2229
Conversation
Cool, thanks for tackling this! But - may I ask - why not add all the hooks from opencontainers/runtime-spec#1008 in one PR? |
@h-vetinari I'll be opening other PRs for these because they are less trivial 😛 |
db5e9a3
to
d56c58b
Compare
@RenaudWasTaken is there a blocker here? |
d56c58b
to
e18c6de
Compare
I just rebased my PR! Just getting a review for this specific piece of the 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.
couple comments...
e18c6de
to
31d8e57
Compare
Updated, thanks for the review @mikebrow ! |
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 non maintainer vote
runtime-spec |
@RenaudWasTaken Can you rebase this? @crosbymichael @mrunalp @dqminh @hqhq @cyphar @AkihiroSuda OTOH, if not all the hooks from opencontainers/runtime-spec#1008 are implemented for |
Apologies -- it did look like it was going to be the final RC at the time. And what an innocent time that was. 😉 I'll change the nicknames once we do an rc11.
I don't mind doing an rc11 now since it seems Kubernetes is asking for one. But whether or not we merge this (I haven't reviewed it yet) is a separate concern to rc11 -- if we need rc11 we can merge it without this because we'd need to implement the other hooks for the final rc before
Yes, there will have to be one RC which acts as the "feature freeze". Normally, RCs are all mean to be feature freezes but let's just say we've broken more than a couple of rules when it comes to how project releases should work. But as I've discussed here and on the OCI calls and mailing lists, we will change how runc does releases as soon as we've got |
Maybe dumb question, but can we consider shipping the next release as v11.0.0 to reflect the reality? |
Maybe jumping to OTOH, |
@RenaudWasTaken @cyphar What's current status? |
31d8e57
to
ebd384e
Compare
Hello! Sorry for the lag, the past few weeks have been complicated :) |
Small update, I've managed to take some to explore a bit more the code base and look into adding the other hooks. While doing so, I noticed a few things that need to be added to this PR:
Now that I'm a bit more familiar with the code base, it looks like the changes are less important than expected and might all go in this PR. More to come this weekend. |
ebd384e
to
7ab5854
Compare
Added the other hooks, I'll spend an hour or two to add some go tests during the weekend. |
Awesome to see this being picked back up, thanks @RenaudWasTaken! Should also adapt the PR title accordingly. |
7ab5854
to
8de63fb
Compare
Rebased :) ! @AkihiroSuda @cyphar Should we merge it and I'll create the followup two PRs:
|
a52db3e
to
5ccade3
Compare
Sorry I didn't realize the CI wasn't passing, pushed an update :) ! |
5ccade3
to
3cf3578
Compare
CentOS7 is failing because it doesn't use the Dockerfile and so
|
3cf3578
to
8f88f69
Compare
Hopefully fixed in this new one! |
Ah, you need to get |
8f88f69
to
b71b15e
Compare
Updated! |
No worries, I'm used to open source now and I sometime take a long time to review PRs :D ! |
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.
There are just a few minor nits left. I did notice you dropped the newSystemErrorWithCausef
wrappers for hooks but that debug information has never been useful and we need to migrate to pkg/errors
anyway so whatever.
Signed-off-by: Renaud Gaubert <[email protected]>
This patch adds a test based on real world usage of runc hooks (libnvidia-container). We verify that mounting a library inside a container and running ldconfig succeeds. Signed-off-by: Renaud Gaubert <[email protected]>
b71b15e
to
861afa7
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, thanks for slogging through this for so long.
I'll have the followup PRs by next week! |
Hello!
This PR implements the new
CreateRuntime
hook as defined in https://github.com/opencontainers/runtime-spec/blob/master/config.md#posix-platform-hooksThis is a fairly simple change,
CreateRuntime
are basicallyprestart
hooks with the requirement of being called after the now deprecatedprestart
hooks.I've done of refactoring to not have too much duplication of code by adding a function:
(*Hooks) RunHooks(name HookName, spec *specs.State) error
.This allows us to have a nicer statement in the code:
I'm looking into how to implement the
createContainer
andstartContainer
changes but am not super familiar with the code base and would happily take pointers.fixes #1710
cc @cyphar @mrunalp @vbatts