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

cgroup: make target cgroup threaded if needed #931

Merged

Conversation

giuseppe
Copy link
Member

if moving a process fails with EOPNOTSUPP, then change the target
cgroup type to threaded and attempt the migration again.

Closes: #923

Signed-off-by: Giuseppe Scrivano [email protected]

Copy link

@PavelSosin-320 PavelSosin-320 left a comment

Choose a reason for hiding this comment

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

Maybe, it is true also in the Gnome workstation scenario where resources are also managed at the upper level, i.e. rootless session level using systemd.

if moving a process fails with EOPNOTSUPP, then change the target
cgroup type to threaded and attempt the migration again.

Closes: containers#923

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the make-target-cgroup-threaded-if-needed branch from 1ce198f to 17d1c16 Compare May 26, 2022 19:09
@skepticoitusInteruptus
Copy link

Thanks for sparing the time to look into this fellahs (@giuseppe, @flouthoc, @PavelSosin-320).

I'm not qualified to comment on any implementation details (I'm a Java dude; learning Go is on my TODO list).

But just as a general observation @giuseppe , have you considered writing some kind of "unit test"?

From the Done! outcome you listed, I'm not 100% certain whether or not my expectations are on the same page as what this PR is implementing(?)

Do you think that some kind of test(s) that setup the preconditions and expectations/assertions I described in #923 would increase confidence that the original issue, as described, is being addressed?

@flouthoc
Copy link
Collaborator

@giuseppe Could we mere this or are we still waiting for discussion here #923 following PR also contains commit c44937b which we also need to unblock CI

@giuseppe
Copy link
Member Author

let's merge, we can improve it later

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc flouthoc merged commit 85e79fa into containers:main May 31, 2022
Spindel pushed a commit to Spindel/base-image that referenced this pull request Oct 23, 2022
crun is still broken when host is running cgroups v2, waiting for
containers/crun#931 to land in a release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Correct crun's behavior when it runs inside a cgroup v2 container
4 participants