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

Fix how cgroup manager is created based on cgroups path #1288

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Oct 21, 2022

This aims to fix some issues with how cgroup manager is selected currently, and ultimately fix #1277 .

The issue with current implementation is that if we have to use systemd, i.e. when user namespace is set or running rootless, then we simply fallback to systemd manager, which expects the cgroup path to be either absent or of the form [slice]:[scope_prefix]:[name] .

However, the oci spec does not have any such restriction, and for cgroup path it simply states here that :

cgroupsPath (string, OPTIONAL) path to the cgroups. It can be used to either control the cgroups hierarchy for containers or to run a new process in an existing container.

The value of cgroupsPath MUST be either an absolute path or a relative path.

  • In the case of an absolute path (starting with /), the runtime MUST take the path to be relative to the cgroups mount point.
  • In the case of a relative path (not starting with /), the runtime MAY interpret the path relative to a runtime-determined location in the cgroups hierarchy.

Now I checked with runc and crun, with the same config as given to TestUserNamespace test, and as the path given is /testing/TestUserNamespaces-WritableRootFS , both of them create the cgroups at path /sys/fs/cgroup//testing/TestUserNamespaces-WritableRootFS , which is to say they use v2 amanger instead of systemd (?)

Thus, in this PR I have added check if the path is absolute and use v2 instead of systemd.

However, this is still not complete, as if the path given is relative, then this will still fail as systemd parsing will fail as before ; in which case the question is what is the correct thing to do in all cases?

Please take a look. Thanks.

Signed-off-by: Yashodhan Joshi yjdoc2@gmail.com

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Oct 21, 2022

The config given by test :

{ "ociVersion": "1.0.2-dev", "process": { "user": { "uid": 0, "gid": 0, "additionalGids": [ 1, 2, 3, 4, 6, 10, 11, 20, 26, 27 ] }, "args": [ "sh", "-c", "exit 7" ], "env": [ "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" ], "cwd": "/", "capabilities": { "bounding": [ "CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER", "CAP_MKNOD", "CAP_NET_RAW", "CAP_SETGID", "CAP_SETUID", "CAP_SETFCAP", "CAP_SETPCAP", "CAP_NET_BIND_SERVICE", "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE" ], "effective": [ "CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER", "CAP_MKNOD", "CAP_NET_RAW", "CAP_SETGID", "CAP_SETUID", "CAP_SETFCAP", "CAP_SETPCAP", "CAP_NET_BIND_SERVICE", "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE" ], "permitted": [ "CAP_CHOWN", "CAP_DAC_OVERRIDE", "CAP_FSETID", "CAP_FOWNER", "CAP_MKNOD", "CAP_NET_RAW", "CAP_SETGID", "CAP_SETUID", "CAP_SETFCAP", "CAP_SETPCAP", "CAP_NET_BIND_SERVICE", "CAP_SYS_CHROOT", "CAP_KILL", "CAP_AUDIT_WRITE" ] }, "rlimits": [ { "type": "RLIMIT_NOFILE", "hard": 1024, "soft": 1024 } ], "noNewPrivileges": true }, "root": { "path": "rootfs" }, "mounts": [ { "destination": "/proc", "type": "proc", "source": "proc", "options": [ "nosuid", "noexec", "nodev" ] }, { "destination": "/dev", "type": "tmpfs", "source": "tmpfs", "options": [ "nosuid", "strictatime", "mode=755", "size=65536k" ] }, { "destination": "/dev/pts", "type": "devpts", "source": "devpts", "options": [ "nosuid", "noexec", "newinstance", "ptmxmode=0666", "mode=0620", "gid=5" ] }, { "destination": "/dev/shm", "type": "tmpfs", "source": "shm", "options": [ "nosuid", "noexec", "nodev", "mode=1777", "size=65536k" ] }, { "destination": "/dev/mqueue", "type": "mqueue", "source": "mqueue", "options": [ "nosuid", "noexec", "nodev" ] }, { "destination": "/sys", "type": "sysfs", "source": "sysfs", "options": [ "nosuid", "noexec", "nodev", "ro" ] }, { "destination": "/run", "type": "tmpfs", "source": "tmpfs", "options": [ "nosuid", "strictatime", "mode=755", "size=65536k" ] } ], "linux": { "uidMappings": [ { "containerID": 0, "hostID": 1000, "size": 10000 } ], "gidMappings": [ { "containerID": 0, "hostID": 2000, "size": 10000 } ], "resources": { "devices": [ { "allow": false, "access": "rwm" } ] }, "cgroupsPath": "/testing/TestUserNamespaces-WritableRootFS", "namespaces": [ { "type": "pid" }, { "type": "ipc" }, { "type": "uts" }, { "type": "mount" }, { "type": "network" }, { "type": "user" } ], "maskedPaths": [ "/proc/acpi", "/proc/asound", "/proc/kcore", "/proc/keys", "/proc/latency_stats", "/proc/timer_list", "/proc/timer_stats", "/proc/sched_debug", "/sys/firmware", "/proc/scsi" ], "readonlyPaths": [ "/proc/bus", "/proc/fs", "/proc/irq", "/proc/sys", "/proc/sysrq-trigger" ] } }

@YJDoc2 YJDoc2 requested a review from utam0k October 21, 2022 10:28
@codecov-commenter
Copy link

Codecov Report

Merging #1288 (2d48e16) into main (497f2e6) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1288      +/-   ##
==========================================
+ Coverage   69.21%   69.22%   +0.01%     
==========================================
  Files         118      118              
  Lines       12514    12513       -1     
==========================================
+ Hits         8661     8662       +1     
+ Misses       3853     3851       -2     

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Good catch

@utam0k utam0k merged commit d3f3c5f into youki-dev:main Oct 21, 2022
@YJDoc2 YJDoc2 deleted the fix_cgroup_setup branch November 8, 2022 06:21
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.

Pass TestUserNamespaces of contaienrd
3 participants