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

Return empty runtime directory if we're not rootless #4657

Merged

Conversation

@jdieter
Copy link
Contributor

jdieter commented Dec 7, 2019

Currently, we return a runtime directory of the form
/run/user/<uid>, even when running as root. Depending on configuration,
that directory may be deleted when the user logs out, which is quite
awkward when the container is started as a systemd service and then
someone logs in and out as root.

This patch fixes the problem by returning an empty runtime directory if the
container is being started by root. The runtime should automatically use
the default runtime directory (/run/crun when crun is used), which should
be accessible to root.

Tested in Fedora 31 by running containers under both root and a regular
user. State for root containers is stored in /run/crun, while state for
rootless containers is in /run/user/<uid>/crun.

Note that this change will have some odd effects if podman containers are running when podman is updated with this fix. It may make sense for a %post script to move state information from /run/user/0/crun to /run/crun.

Currently, we return a runtime directory of the form
`/run/user/<uid>`, even when running as root.  Depending on configuration,
that directory may be deleted when the user logs out, which is quite
awkward when the container is started as a systemd service and then
someone logs in and out as root.

This patch fixes the problem by returning an empty runtime directory if the
container is being started by root.  The runtime should automatically use
the default runtime directory (`/run/crun` when crun is used), which should
be accessible to root.

Tested in Fedora 31 by running containers under both root and a regular
user.  State for root containers is stored in `/run/crun`, while state for
rootless containers is in `/run/user/<uid>/crun`.

Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
@openshift-ci-robot

This comment has been minimized.

Copy link
Collaborator

openshift-ci-robot commented Dec 7, 2019

Hi @jdieter. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@giuseppe

This comment has been minimized.

Copy link
Member

giuseppe commented Dec 7, 2019

/approve
/ok-to-test

@openshift-ci-robot

This comment has been minimized.

Copy link
Collaborator

openshift-ci-robot commented Dec 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, jdieter

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

@giuseppe

This comment has been minimized.

Copy link
Member

giuseppe commented Dec 7, 2019

thanks a lot for looking into it and preparing a patch.

LGTM

@mheon

This comment has been minimized.

Copy link
Collaborator

mheon commented Dec 7, 2019

@cbs228

This comment has been minimized.

Copy link

cbs228 commented Dec 7, 2019

I also have this problem with systemd-managed containers on FC31. Until this patch is released, I believe that adding

Environment=XDG_RUNTIME_DIR=/run

to your .service files will move the runtime directory away from /run/user/0 and into /run.

Edit: You may also need to set this environment variable when running podman commands on these services from the console, i.e.

sudo env XDG_RUNTIME_DIR=/run podman ps
@mheon

This comment has been minimized.

Copy link
Collaborator

mheon commented Dec 7, 2019

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 7287f69 into containers:master Dec 7, 2019
21 of 22 checks passed
21 of 22 checks passed
tide Not mergeable.
Details
build_each_commit Task Summary
Details
build_without_cgo Task Summary
Details
ci/prow/images Job succeeded.
Details
gating Task Summary
Details
meta Task Summary
Details
special_testing_cgroupv2 TEST_REMOTE_CLIENT:false Task Summary
Details
special_testing_cgroupv2 TEST_REMOTE_CLIENT:true Task Summary
Details
special_testing_cross CROSS_PLATFORM:darwin Task Summary
Details
special_testing_cross CROSS_PLATFORM:windows Task Summary
Details
special_testing_endpoint Task Summary
Details
special_testing_in_podman Task Summary
Details
special_testing_rootless TEST_REMOTE_CLIENT:false Task Summary
Details
special_testing_rootless TEST_REMOTE_CLIENT:true Task Summary
Details
success Task Summary
Details
test_building_snap Task Summary
Details
testing TEST_REMOTE_CLIENT:false image:fedora-30-libpod-5642998972416000 Task Summary
Details
testing TEST_REMOTE_CLIENT:false image:ubuntu-18-libpod-5642998972416000 Task Summary
Details
testing TEST_REMOTE_CLIENT:true image:fedora-30-libpod-5642998972416000 Task Summary
Details
testing TEST_REMOTE_CLIENT:true image:ubuntu-18-libpod-5642998972416000 Task Summary
Details
varlink_api Task Summary
Details
vendor Task Summary
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.