-
Notifications
You must be signed in to change notification settings - Fork 236
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
feat(v2): Support cgroup.MoveTo in cgroupv2 manager #235
Conversation
Provides a method for the cgroup v2 manager to support moving processes to a specified cgroup. Fixes: containerd#234 Signed-off-by: yaoyinnan <[email protected]>
@estesp Please help me review this PR. kata-containers/kata-containers#4397 needs this PR to be merged. Thanks. |
Given there is no test, how have you validated this works properly on cgroups v2? I notice that it works differently than the v1 |
@estesp I tested the error in the cgroupv2 environment, the error message contains "no such process", the content is as follows:
This MoveTo() method exists in cgroup v1, but does not exist in cgroup v2's Manager. I need to be compatible with this method in order to make Kata compatible with cgroup v1 and cgroup v2 systems. In Kata I tested this function and it works fine. |
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
@AkihiroSuda Please help me review this PR. Thanks. |
@estesp Please help me contact @AkihiroSuda to help me review this PR, the kata-containers/kata-containers#4397 needs to wait for #235 to be merged. Thanks. |
Please consider adding a test |
Add unit test TestMoveTo() for manager.MoveTo(). Fixes: containerd#234 Signed-off-by: yaoyinnan <[email protected]>
@AkihiroSuda I have added the test. |
Provides a method for the cgroup v2 manager to support moving processes to a specified cgroup.
Fixes: #234
Signed-off-by: yaoyinnan [email protected]