Skip to content

rootless: automatically create a systemd scope#3959

Merged
openshift-merge-robot merged 3 commits into
containers:masterfrom
giuseppe:rootless-use-systemd-scope
Sep 12, 2019
Merged

rootless: automatically create a systemd scope#3959
openshift-merge-robot merged 3 commits into
containers:masterfrom
giuseppe:rootless-use-systemd-scope

Conversation

@giuseppe
Copy link
Copy Markdown
Member

@giuseppe giuseppe commented Sep 6, 2019

when running in rootless mode and using systemd as cgroup manager create automatically a systemd scope.

This solves a couple of issues:

on cgroup v2 it is necessary that a process before it can moved to a different cgroup tree must be in a directory owned by the unprivileged user. This is not always true, e.g. when creating a session with su -l.

Closes: #3937

Also, for running systemd in a container it was before necessary to specify "systemd-run --scope --user podman ...", now this is done automatically as part of this PR.

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

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Sep 6, 2019
Comment thread libpod/oci_internal_linux.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for spotting it :) got confused. Dropped the patch

Comment thread cmd/podman/main_local.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be unconditional? I feel like we only want it with systemd CGroups.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we also want this when using cgroupfs. It solves the problem of running systemd containers on cgroups v1 with: systemd-run --scope --user podman ...

If it fails though, we just print a debug message

@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch from 1021a9f to cb89afa Compare September 6, 2019 14:24
@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Sep 6, 2019

LGTM

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Sep 6, 2019

@msekletar PTAL

@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch from cb89afa to 460628a Compare September 9, 2019 09:12
@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch 3 times, most recently from 7ea517a to 8a55f3d Compare September 9, 2019 11:03
@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM assuming happy tests

@giuseppe
Copy link
Copy Markdown
Member Author

giuseppe commented Sep 9, 2019

tests are finally passing

Comment thread pkg/cgroups/cgroups_supported.go Outdated
Comment thread cmd/podman/main_local.go Outdated
@mheon
Copy link
Copy Markdown
Member

mheon commented Sep 9, 2019

I don't like increasing our dependency on systemd; I'd like to still be able to run in environments where it's not available. Still, this doesn't seem harmful... LGTM

@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch 2 times, most recently from 36f9ca5 to a224050 Compare September 10, 2019 05:54
@giuseppe
Copy link
Copy Markdown
Member Author

I've addressed the comments and pushed a new version.

I don't like increasing our dependency on systemd; I'd like to still be able to run in environments where it's not available. Still, this doesn't seem harmful... LGTM

I agree, Podman should work fine without systemd. If it fails to create the scope we only give a warning (if the cgroups manager is systemd) or a debug message (with cgroupfs).

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #3581) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Sep 11, 2019

@giuseppe needs a rebase.

@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch from a224050 to 61ab001 Compare September 11, 2019 15:05
when running as rootless, use the user session bus.  It is already
implemented in the pkg/cgroups so just re-use it.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
when running in rootless mode and using systemd as cgroup manager
create automatically a systemd scope when the user doesn't own the
current cgroup.

This solves a couple of issues:

on cgroup v2 it is necessary that a process before it can moved to a
different cgroup tree must be in a directory owned by the unprivileged
user.  This is not always true, e.g. when creating a session with su
-l.

Closes: containers#3937

Also, for running systemd in a container it was before necessary to
specify "systemd-run --scope --user podman ...", now this is done
automatically as part of this PR.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the rootless-use-systemd-scope branch from 61ab001 to 7e88bf7 Compare September 12, 2019 06:35
@giuseppe
Copy link
Copy Markdown
Member Author

rebased and tests are green

@mheon
Copy link
Copy Markdown
Member

mheon commented Sep 12, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2019
@openshift-merge-robot openshift-merge-robot merged commit 8c3349b into containers:master Sep 12, 2019
@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rawhide (crun): rootless(?): podman exec broken: OCI runtime error

7 participants