-
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
runc create/run: warn on rootless + shared pidns + no cgroup #4398
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,7 +580,18 @@ func (p *initProcess) start() (retErr error) { | |
// cgroup. We don't need to worry about not doing this and not being root | ||
// because we'd be using the rootless cgroup manager in that case. | ||
if err := p.manager.Apply(p.pid()); err != nil { | ||
return fmt.Errorf("unable to apply cgroup configuration: %w", err) | ||
if errors.Is(err, cgroups.ErrRootless) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the purpose of warning, I think this PR LGTM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was thinking about it, too, but let's do improvement at a time, shall we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, we are trying to cut 1.2.0 for some time now, and this PR is a result of a recent development in the area (a regression described in #4394 (and fixed by #4395). As I noted in #4394 (comment), I'm OK with the fix in #4395, but it would be nice to also introduce a warning; this is what this PR does. I think we can introduce |
||
// ErrRootless is to be ignored except when | ||
// the container doesn't have private pidns. | ||
if !p.config.Config.Namespaces.IsPrivate(configs.NEWPID) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to not put this condition in the previous if? Do you think it is more readable like this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did that initially, but rolled it back later since changing the code like this complicates the review (the whole block changes instead of just one line). I will add a separate commit that does it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad; I mixed this up with something else. Updated; PTAL @rata There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ughm, I just broke everything. This is two
No way to do that with a single |
||
// TODO: make this an error in runc 1.3. | ||
logrus.Warn("Creating a rootless container with no cgroup and no private pid namespace. " + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about s/no cgroup/no devices cgroup/ ? |
||
"Such configuration is strongly discouraged (as it is impossible to properly kill all container's processes) " + | ||
"and will result in an error in a future runc version.") | ||
} | ||
} else { | ||
return fmt.Errorf("unable to apply cgroup configuration: %w", err) | ||
} | ||
} | ||
if p.intelRdtManager != nil { | ||
if err := p.intelRdtManager.Apply(p.pid()); err != nil { | ||
|
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 think this is too strict? Maybe we should add a condition here: