cgroup: rmdir the entire systemd scope #1146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flouthoc
reviewed
Feb 20, 2023
src/libcrun/cgroup-systemd.c
Outdated
| path_to_scope = xstrdup (cgroup_status->path); | ||
| tmp = strstr (path_to_scope, cgroup_status->scope); | ||
| if (tmp) | ||
| tmp[strlen(cgroup_status->scope)] = '\0'; |
There was a problem hiding this comment.
Suggested change
| tmp[strlen(cgroup_status->scope)] = '\0'; | |
| tmp[strlen (cgroup_status->scope)] = '\0'; |
flouthoc
reviewed
Feb 20, 2023
src/libcrun/container.c
Outdated
| @@ -2485,7 +2485,14 @@ libcrun_container_run_internal (libcrun_container_t *container, libcrun_context_ | |||
| return ret; | |||
|
|
|||
| fail: | |||
| return cleanup_watch (context, def, cgroup_status, pid, sync_socket, terminal_fd, err); | |||
| ret = cleanup_watch (context, def, cgroup_status, pid, sync_socket, terminal_fd, err); | |||
There was a problem hiding this comment.
Suggested change
| ret = cleanup_watch (context, def, cgroup_status, pid, sync_socket, terminal_fd, err); | |
| ret = cleanup_watch (context, def, cgroup_status, pid, sync_socket, terminal_fd, err); |
flouthoc
requested changes
Feb 20, 2023
giuseppe
force-pushed
the
fix-cgroupv1-regression
branch
from
91533f3 to
51d3ff6
Compare
commit 7ea7617 caused a regression on cgroup v1, and some directories that are created manually are not cleaned up on container termination causing a cgroup leak. Fix it by deleting the entire systemd scope directory instead of deleting only the final cgroup. Closes: containers#1144 Signed-off-by: Giuseppe Scrivano <[email protected]>
make sure the cgroup is destroyed on errors. Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
fix-cgroupv1-regression
branch
from
51d3ff6 to
6cdf51c
Compare
|
LGTM |
rst0git
reviewed
Oct 25, 2024
| return destroy_cgroup_path (cgroup_status->path, mode, err); | ||
| path_to_scope = get_cgroup_scope_path (cgroup_status->path, cgroup_status->scope); | ||
|
|
||
| return destroy_cgroup_path (path_to_scope, mode, err); |
There was a problem hiding this comment.
@giuseppe This change breaks crun restore with cgroups v1:
$ podman pull ubi8
$ podman run -d ubi8 sleep 300
$ podman container checkpoint -l
$ podman container restore -l
Error: OCI runtime error: crun: CRIU restoring failed -52. Please check CRIU logfile `/var/lib/containers/storage/overlay-containers/b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9/userdata/restore.log`
$ tail /var/lib/containers/storage/overlay-containers/b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9/userdata/restore.log
(00.000882) cg: Making controller dir .criu.cgyard.8cbQy1/freezer (freezer), type cgroup
(00.000900) cg: Created cgroup dir freezer/machine.slice/libpod-b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9.scope/container
(00.000904) cg: Restore special props
(00.000905) cg: Making controller dir .criu.cgyard.8cbQy1/cpuset (cpuset), type cgroup
(00.000929) cg: Created cgroup dir cpuset/machine.slice/libpod-b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9.scope/container
(00.000932) cg: Restore special props
(00.000933) cg: Restoring cgroup property value [0-15] to [cpuset/machine.slice/libpod-b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9.scope/container/cpuset.cpus]
(00.000939) Error (criu/cgroup.c:1490): cg: Failed writing 0-15 to cpuset/machine.slice/libpod-b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9.scope/container/cpuset.cpus: Permission denied
(00.000944) Error (criu/cgroup.c:1756): cg: Restoring cpuset.cpus special property failed
(00.000944) Error (criu/cgroup.c:1815): cg: Restoring special cpuset props failed!
The Permission denied error appears when writing to cpuset/machine.slice/libpod-b10e1136e6cefcf97759deef7377036b6cefc0eacf9591976671712f4e3c4ac9.scope/container/cpuset.cpus because the parent cpuset.cpus is empty.
There was a problem hiding this comment.
this change was made 2 years ago. How did it work until now? Was it always broken?
There was a problem hiding this comment.
How did it work until now? Was it always broken?
crun restore with cgroups v1 has been broken since version 1.8.1, but it works with cgroups v2. We have seen this error reported in checkpoint-restore/criu#2000, checkpoint-restore/criu#2091 and https://issues.redhat.com/browse/RHEL-32180
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
commit 7ea7617 caused a regression on cgroup v1, and some directories that are created manually are not cleaned up on container termination causing a cgroup leak.
Fix it by deleting the entire systemd scope directory instead of deleting only the final cgroup.
Closes: #1144
Signed-off-by: Giuseppe Scrivano [email protected]