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

rootless: don't bind mount /sys/fs/cgroup/systemd in systemd mode #1761

Merged
merged 2 commits into from Nov 7, 2018

Conversation

Projects
None yet
6 participants
@giuseppe
Member

giuseppe commented Nov 5, 2018

it is not writeable by non-root users so there is no point in having
access to it from a container.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@mheon

This comment has been minimized.

Collaborator

mheon commented Nov 5, 2018

/approve
LGTM

@openshift-ci-robot

This comment has been minimized.

Collaborator

openshift-ci-robot commented Nov 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan

This comment has been minimized.

Member

rhatdan commented Nov 5, 2018

LGTM

@giuseppe

This comment has been minimized.

Member

giuseppe commented Nov 5, 2018

when using a network namespace, if I bind mount /sys/fs/cgroup/systemd from the host, systemd gets a bit further (I also needed to patch systemd to not fail when sysctl is not available) but still doesn't work. Would it make sense to amend the change in this patch and bind mount /sys/fs/cgroup/systemd when using rootless?

@openshift-ci-robot openshift-ci-robot added size/M and removed size/S labels Nov 5, 2018

@@ -377,6 +377,15 @@ func (c *Container) setupSystemd(mounts []spec.Mount, g generate.Generator) erro
}
g.AddMount(systemdMnt)
}
if rootless.IsRootless() {

This comment has been minimized.

@rhatdan

rhatdan Nov 5, 2018

Member

How is this writable?

This comment has been minimized.

@mheon

mheon Nov 5, 2018

Collaborator

Is this safe? I know systemd really dislike having to share CGroups with anyone else without explicit units and delegation

This comment has been minimized.

@giuseppe

giuseppe Nov 5, 2018

Member

I think systemd detects in what user slice it is running and uses that. If I specify "ro", I get from systemd in the container:

Failed to create /user.slice/user-1000.slice/user@1000.service/gnome-terminal-server.service/7a24e4a13856680aebdb0f05657eea52ac353196e95bde23461066f665f5df98/init.scope control group: Read-only file system

This comment has been minimized.

@rhatdan

rhatdan Nov 5, 2018

Member

I think this is safe enough.
@giuseppe if you enter the container and go down the /sys/fs/cgroup/systemd/libpod-uuild...
Are the files owned by -1 or by the UserNS Root?

This comment has been minimized.

@giuseppe

giuseppe Nov 5, 2018

Member

they are owned by root in the user namespace:

[root@be714e845c11 /]# ls -l /sys/fs/cgroup/systemd/user.slice/user-1000.slice/user@1000.service/gnome-terminal-server.service/be714e845c116204fd737d52aa6b9989786bf37984dec99b74717d469ece5c80/init.scope
total 0
-rw-r--r--. 1 root root 0 Nov  5 23:38 cgroup.clone_children
-rw-r--r--. 1 root root 0 Nov  5 23:38 cgroup.procs
-rw-r--r--. 1 root root 0 Nov  5 23:38 notify_on_release
-rw-r--r--. 1 root root 0 Nov  5 23:38 tasks

This comment has been minimized.

@rhatdan

rhatdan Nov 6, 2018

Member

Awesome.

@giuseppe

This comment has been minimized.

Member

giuseppe commented Nov 5, 2018

I've opened a PR for systemd here: systemd/systemd#10646

With that patch it is possible to run systemd in a rootless container that has /proc mounted from the user namespace.

@giuseppe giuseppe force-pushed the giuseppe:rootless-systemd branch from ff2ca35 to 0d35072 Nov 6, 2018

@TomSweeneyRedHat

This comment has been minimized.

Collaborator

TomSweeneyRedHat commented Nov 6, 2018

@giuseppe tests aren't looking to be happy.

@giuseppe giuseppe force-pushed the giuseppe:rootless-systemd branch from 0d35072 to 59c9f7c Nov 6, 2018

if err != nil {
return err
}
sourcePath := filepath.Join("/sys/fs/cgroup/systemd", cgroupPath)

This comment has been minimized.

@TomSweeneyRedHat

TomSweeneyRedHat Nov 6, 2018

Collaborator

Not necessary for this PR, but we have "/sys/fs/cgroup/systemd" in a number of places, might be nice to make a constant for it in a common area.

@TomSweeneyRedHat

This comment has been minimized.

Collaborator

TomSweeneyRedHat commented Nov 6, 2018

LGTM assuming happy tests

@rhatdan

This comment has been minimized.

Member

rhatdan commented Nov 6, 2018

bot, retest this please

@mheon

This comment has been minimized.

Collaborator

mheon commented Nov 7, 2018

@giuseppe Please rebase to pick up CI fixes

giuseppe added some commits Nov 5, 2018

rootless: don't bind mount /sys/fs/cgroup/systemd in systemd mode
it is not writeable by non-root users so there is no point in having
access to it from a container.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
rootless: mount /sys/fs/cgroup/systemd from the host
systemd requires /sys/fs/cgroup/systemd to be writeable.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

@giuseppe giuseppe force-pushed the giuseppe:rootless-systemd branch from 59c9f7c to f813881 Nov 7, 2018

@mheon

This comment has been minimized.

Collaborator

mheon commented Nov 7, 2018

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 1e4e33b into containers:master Nov 7, 2018

9 checks passed

CAH 7-smoketested - Containerized (Podman in Docker) All tests passed.
Details
FAH28 - Containerized (Podman in Podman) All tests passed.
Details
Fedora 28 Cloud All tests passed.
Details
Fedora 29 Cloud All tests passed.
Details
ci/prow/images Job succeeded.
Details
ci/prow/lint Job succeeded.
Details
ci/prow/validate Job succeeded.
Details
required 2/2 PASSES
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment