Skip to content
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

cgroup2: exec: join the cgroup of the init process on EBUSY #2416

Merged
merged 1 commit into from
May 31, 2020

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented May 19, 2020

Fix #2356.

Contains #2418


Note: this behavior is different from crun v0.13.
crun joins "crun-exec` group rather than the init group: containers/crun@af1080b
The next version of crun will switch to the init cgroup: containers/crun#365

@AkihiroSuda AkihiroSuda force-pushed the exec-join-init-cgroup branch 4 times, most recently from e93631c to dc133ec Compare May 19, 2020 15:35
@AkihiroSuda AkihiroSuda force-pushed the exec-join-init-cgroup branch 2 times, most recently from 4ece281 to 9643a87 Compare May 19, 2020 23:43
@AkihiroSuda AkihiroSuda marked this pull request as ready for review May 19, 2020 23:45
@AkihiroSuda
Copy link
Member Author

@kolyshkin @cyphar @giuseppe PTAL

// On cgroup v2 + nesting + domain controllers, EnterPid may fail with EBUSY.
// https://github.com/opencontainers/runc/issues/2356#issuecomment-621277643
// Try to join the cgroup of InitProcessPid.
if cgroups.IsCgroup2UnifiedMode() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it not be done only with err == EBUSY?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is worth trying to join pid1 even on other errors

@AkihiroSuda
Copy link
Member Author

@kolyshkin @mrunalp PTAL

@mrunalp
Copy link
Contributor

mrunalp commented May 27, 2020

Added one comment about removing the warning otherwise looks ready to merge. Thanks!

@AkihiroSuda AkihiroSuda force-pushed the exec-join-init-cgroup branch from 9643a87 to 5099176 Compare May 27, 2020 23:39
@AkihiroSuda
Copy link
Member Author

updated

@mrunalp
Copy link
Contributor

mrunalp commented May 27, 2020

Thanks, will tag once tests pass again.

@mrunalp
Copy link
Contributor

mrunalp commented May 27, 2020

LGTM

Approved with PullApprove

@mrunalp
Copy link
Contributor

mrunalp commented May 28, 2020

@kolyshkin @cyphar ptal

@AkihiroSuda AkihiroSuda added this to the 1.0.0-rc90 (tentative) milestone May 28, 2020
@AkihiroSuda AkihiroSuda force-pushed the exec-join-init-cgroup branch from 5099176 to b502a4b Compare May 30, 2020 04:12
mrunalp
mrunalp previously approved these changes May 30, 2020
Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kolyshkin
kolyshkin previously approved these changes May 30, 2020
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a nit with whitespace, but LGTM overall

@AkihiroSuda AkihiroSuda dismissed stale reviews from kolyshkin and mrunalp via c91fe9a May 31, 2020 04:09
@AkihiroSuda AkihiroSuda force-pushed the exec-join-init-cgroup branch from b502a4b to c91fe9a Compare May 31, 2020 04:09
@AkihiroSuda
Copy link
Member Author

fixed

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mrunalp mrunalp merged commit 0f7ffbe into opencontainers:master May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgroup2: how can we support nested containers with domain controllers?
4 participants