Skip to content

Conversation

@hshiina
Copy link
Contributor

@hshiina hshiina commented Nov 16, 2021

Add another flag to see if path is specified with cgroup type in a
runtime spec for determining whether a new cgroup namespace is necessary
or not.

Signed-off-by: Hironori Shiina [email protected]

#if CLONE_NEWCGROUP
/* cgroup will be unshared later. Once the process is in the correct cgroup. */
init_status.all_namespaces &= ~CLONE_NEWCGROUP;
init_status.namespaces_to_unshare &= ~CLONE_NEWCGROUP;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does namespaces_to_unshare contains namespaces specified without path ? I am still a bit confused about this.

Copy link
Collaborator

@flouthoc flouthoc Nov 16, 2021

Choose a reason for hiding this comment

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

Tests are failing for: when uts namespace is specified by a path.

@giuseppe
Copy link
Member

thanks for opening a PR!

I am afraid this will break the existing semantic of unshare_flags.

What do you think about the following patch:

diff --git a/src/libcrun/linux.c b/src/libcrun/linux.c
index 4b5877c..4fab400 100644
--- a/src/libcrun/linux.c
+++ b/src/libcrun/linux.c
@@ -93,6 +93,10 @@ struct private_data_s
      namespaces are available.  */
   int unshare_flags;
 
+#if CLONE_NEWCGROUP
+  int unshare_cgroupns;
+#endif
+
   char *host_notify_socket_path;
   char *container_notify_socket_path;
   bool mount_dev_from_host;
@@ -2409,7 +2413,7 @@ int
 libcrun_container_enter_cgroup_ns (libcrun_container_t *container, libcrun_error_t *err)
 {
 #if CLONE_NEWCGROUP
-  if (get_private_data (container)->unshare_flags & CLONE_NEWCGROUP)
+  if (get_private_data (container)->unshare_cgroupns)
     {
       int ret = unshare (CLONE_NEWCGROUP);
       if (UNLIKELY (ret < 0))
@@ -3695,6 +3699,7 @@ libcrun_run_linux_container (libcrun_container_t *container, container_entrypoin
 #if CLONE_NEWCGROUP
   /* cgroup will be unshared later.  Once the process is in the correct cgroup.  */
   init_status.all_namespaces &= ~CLONE_NEWCGROUP;
+  get_private_data (container)->unshare_cgroupns = init_status.namespaces_to_unshare & CLONE_NEWCGROUP;
 #endif
 
   ret = socketpair (AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sync_socket);

If it works for you, feel free to amend it to your PR

@giuseppe
Copy link
Member

@hshiina had a chance to look at the previous comment? Does it work for you?

@hshiina
Copy link
Contributor Author

hshiina commented Nov 17, 2021

Thank you for the comments.
My fix is a breaking change and the suggested fix works. I will amend this.

@hshiina hshiina changed the title linux: Set unshared_flags correctly linux: Enter specified cgroup namespace Nov 17, 2021
Copy link
Member

@giuseppe giuseppe 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
Copy link
Collaborator

@hshiina
Copy link
Contributor Author

hshiina commented Nov 18, 2021

It seems that a fedora-minimal image with ID de6653dd060f doesn't have useradd while the current latest has.

In the test log (https://github.com/containers/crun/runs/4250222257?check_suite_focus=true#step:7:2375):

Caching quay.io/libpod/fedora-minimal:latest at /tmp/fedora-minimal-latest.tar...Running: /root/go/src/github.com/containers/podman/bin/podman --root /tmp/crio --runroot /tmp/crio-run --runtime /usr/bin/crun --conmon /usr/bin/conmon --cni-config-dir /etc/cni/net.d --cgroup-manager cgroupfs --tmpdir /tmp --events-backend file --storage-driver=vfs pull quay.io/libpod/fedora-minimal:latest
Trying to pull quay.io/libpod/fedora-minimal:latest...
Getting image source signatures
Copying blob sha256:c579b62c93930790f063b79c7e114c9b74fc430f57eb358bb8e6f044458af890
Copying blob sha256:c579b62c93930790f063b79c7e114c9b74fc430f57eb358bb8e6f044458af890
Copying config sha256:de6653dd060f52fcddff90a490c572251eb71f16d01f037336f863dc9dda2a06
Writing manifest to image destination
Storing signatures
de6653dd060f52fcddff90a490c572251eb71f16d01f037336f863dc9dda2a06

On my laptop:

$ podman images
REPOSITORY                       TAG                   IMAGE ID      CREATED       SIZE
quay.io/libpod/fedora-minimal    main                  de6653dd060f  2 weeks ago   92.5 MB
quay.io/libpod/fedora-minimal    latest                7b0f8c69a29c  2 weeks ago   119 MB
$ podman run quay.io/libpod/fedora-minimal:main useradd testuser
Error: executable file `useradd` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found
$ podman run quay.io/libpod/fedora-minimal:latest useradd testuser

@giuseppe
Copy link
Member

Could you please add another commit that updates podman to "main" instead of the current version we are using now?

@hshiina
Copy link
Contributor Author

hshiina commented Nov 18, 2021

I posted #784.

@giuseppe
Copy link
Member

CI must be fine now.

Could you please rebase this PR?

Add another flag to see if `path` is specified with cgroup `type` in a
runtime spec for determining whether a new cgroup namespace is necessary
or not.

Signed-off-by: Hironori Shiina <[email protected]>
@flouthoc flouthoc merged commit cabe432 into containers:main Nov 20, 2021
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.

3 participants