rootless: Rearrange setup of rootless containers #3756

gabibeyer · 2019-08-07T18:40:51Z

This commit removes the previous cleanup in the stop functionality. It was originally added because the pipe wasn't closing on the kill to conmon, so the slirp4netns process wasn't stopping. This was fixed in this commit, and now the restart works with reentering the previous network namespace.

In order to run Podman with VM-based runtimes unprivileged, the
network must be set up prior to the container creation. Therefore
this commit modifies Podman to run rootless containers by:

create a network namespace
pass the netns persistent mount path to the slirp4netns
to create the tap inferface
pass the netns path to the OCI spec, so the runtime can
enter the netns

Closes #2897

Signed-off-by: Gabi Beyer gabrielle.n.beyer@intel.com

openshift-ci-robot · 2019-08-07T18:41:12Z

Hi @gabibeyer. Thanks for your PR.

I'm waiting for a containers or openshift 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.

mheon · 2019-08-07T18:41:23Z

/ok-to-test
/approve

openshift-ci-robot · 2019-08-07T18:41:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabibeyer, 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:

~~OWNERS~~ [mheon]

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

rh-atomic-bot · 2019-08-07T18:50:06Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

haircommander · 2019-08-07T18:50:44Z

bot, add author to whitelist

gabibeyer · 2019-08-08T17:59:44Z

/hold

gabibeyer · 2019-08-08T18:00:53Z

Should not be merged until slirp4netns 0.4.0 release

rh-atomic-bot · 2019-08-21T18:07:12Z

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

gabibeyer · 2019-08-23T18:23:44Z

/hold cancel

gabibeyer · 2019-08-23T19:24:11Z

@baude @mheon I'm having a hard time with the run_cleanup_test failure: error retrieving network namespace at /run/user/3347/netns/cni-4078184d-48c9-d5ed-872c-2bb1ebed8530: unknown FS magic on \"/run/user/3347/netns/cni-4078184d-48c9-d5ed-872c-2bb1ebed8530\": 1021994". I am unable to reproduce the failure locally, and after some searching I thought it may be related to the namespace persistent bind-mount not being mounted with shared permissions, so I added a commit to fix that, and it doesn't seem to be the issue. Do either of you have any ideas?

mheon · 2019-08-23T23:09:34Z

Hmm. Bad FS magic is strange... I'd more expect not finding the file at all, or a permission error, from user namespace issues...

@giuseppe Any ideas here?

rh-atomic-bot · 2019-09-06T11:50:28Z

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

rhatdan · 2019-09-06T17:53:28Z

Needs a rebase.

rh-atomic-bot · 2019-09-16T20:05:50Z

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

marcov · 2019-09-20T16:17:19Z

@gabibeyer can you rebase one more time?

@giuseppe @mheon : can you give an ack if the code looks good?

mheon · 2019-09-20T16:32:13Z

libpod/container_internal_linux.go

@@ -88,6 +88,10 @@ func (c *Container) prepare() (Err error) {
 				c.state.NetworkStatus = networkStatus
 			}
 		}
+
+		if c.config.NetMode == "slirp4netns" && !c.config.PostConfigureNetNS {


This should be conditional on a network namespace being present

Also, should only run this if createNetNSErr == nil

mheon · 2019-09-20T16:33:32Z

libpod/container_internal_linux.go

@@ -88,6 +88,10 @@ func (c *Container) prepare() (Err error) {
 				c.state.NetworkStatus = networkStatus
 			}
 		}
+
+		if c.config.NetMode == "slirp4netns" && !c.config.PostConfigureNetNS {
+			rootlessSetupErr = c.runtime.setupRootlessNetNS(c)


Error handling flow is broken for this one - if this fails we need to report it much earlier than that return below to ensure proper cleanup of resources on error. I think it'd be better to reuse createNetNSErr here - it already has the right logic here.

mheon · 2019-09-20T16:40:28Z

libpod/networking_linux.go


-	cmd := exec.Command(path, cmdArgs...)
+	cmdArgs = append(cmdArgs, "-c", "-e", "3", "-r", "4")


Can we get a comment explaining what the flags we're adding here do?

mheon · 2019-09-20T16:47:29Z

Other than the error handling in prepare() I think this is basically ready.

In order to run Podman with VM-based runtimes unprivileged, the network must be set up prior to the container creation. Therefore this commit modifies Podman to run rootless containers by: 1. create a network namespace 2. pass the netns persistent mount path to the slirp4netns to create the tap inferface 3. pass the netns path to the OCI spec, so the runtime can enter the netns Closes containers#2897 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

Update documentation to show Kata Containers support is no longer a limitation with merging of commit 486a5b9 Signed-off-by: gabi beyer <gabrielle.n.beyer@intel.com>

To 'avoid unknown FS magic on "/run/user/1000/netns/...": 1021994' make the network namespace bind-mount recursively shared, so the mount is back-propogated to the host. Signed-off-by: gabi beyer <gabrielle.n.beyer@intel.com>

Add two unit tests to determine whether mounts are being listed correctly. One tests that a created container is not listed until mounted. The second checks that running containers are mounted, and then no longer listed as mounted when they stop running. The final test creates three containers, mounts two, and checks that mount correctly only lists the two mounted. Signed-off-by: gabi beyer <gabrielle.n.beyer@intel.com>

marcov · 2019-09-24T09:14:58Z

Thank you @gabibeyer @mheon

Error handling in prepare() should have been improved now.

mheon · 2019-09-24T13:29:55Z

LGTM
@vrothberg @baude @rhatdan @TomSweeneyRedHat PTAL

TomSweeneyRedHat · 2019-09-24T16:30:03Z

LGTM

mheon · 2019-09-24T16:33:01Z

/lgtm

Merging away

openshift-ci-robot requested review from baude and giuseppe August 7, 2019 18:40

openshift-ci-robot added the size/L label Aug 7, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 7, 2019

openshift-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019

gabibeyer force-pushed the rootlessOrdering branch 2 times, most recently from 4b7b6ef to c971002 Compare August 7, 2019 19:00

gabibeyer changed the title ~~rootless: Rearrange setup of rootless containers~~ rootless: Rearrange setup of rootless containers ***CIRRUS: TEST IMAGES*** Aug 8, 2019

gabibeyer force-pushed the rootlessOrdering branch from c971002 to 7b0e4d8 Compare August 8, 2019 02:14

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2019

gabibeyer force-pushed the rootlessOrdering branch 2 times, most recently from d183fb4 to 7b8be8b Compare August 19, 2019 21:23

gabibeyer changed the title rootless: Rearrange setup of rootless containers ***CIRRUS: TEST IMAGES*** rootless: Rearrange setup of rootless containers Aug 22, 2019

gabibeyer force-pushed the rootlessOrdering branch 2 times, most recently from 1198de9 to 486a5b9 Compare August 22, 2019 22:02

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2019

gabibeyer force-pushed the rootlessOrdering branch from 20f7e70 to 5dd9d4f Compare August 27, 2019 18:21

gabibeyer force-pushed the rootlessOrdering branch 3 times, most recently from b684a77 to 012dbb1 Compare September 12, 2019 21:04

gabibeyer force-pushed the rootlessOrdering branch from 012dbb1 to 84e3213 Compare September 19, 2019 22:27

mheon reviewed Sep 20, 2019

View reviewed changes

gabibeyer force-pushed the rootlessOrdering branch 2 times, most recently from f2013d2 to aceb557 Compare September 23, 2019 21:33

Gabi Beyer added 4 commits September 24, 2019 11:01

Add Kata Containers support

f50ba20

Update documentation to show Kata Containers support is no longer a limitation with merging of commit 486a5b9 Signed-off-by: gabi beyer <gabrielle.n.beyer@intel.com>

Make netns bind mount shared

0115be1

To 'avoid unknown FS magic on "/run/user/1000/netns/...": 1021994' make the network namespace bind-mount recursively shared, so the mount is back-propogated to the host. Signed-off-by: gabi beyer <gabrielle.n.beyer@intel.com>

marcov force-pushed the rootlessOrdering branch from aceb557 to 65d5a98 Compare September 24, 2019 09:14

openshift-ci-robot assigned mheon Sep 24, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2019

openshift-merge-robot merged commit b300b98 into containers:master Sep 24, 2019

rh-atomic-bot mentioned this pull request Sep 24, 2019

[wip] podman: add support for specifying MAC #4058

Closed

giuseppe mentioned this pull request Sep 29, 2019

podman run SIGSEGV on git master in rootless mode #4113

Closed

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 bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rootless: Rearrange setup of rootless containers #3756

rootless: Rearrange setup of rootless containers #3756

gabibeyer commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

mheon commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

rh-atomic-bot commented Aug 7, 2019

haircommander commented Aug 7, 2019

gabibeyer commented Aug 8, 2019

gabibeyer commented Aug 8, 2019

rh-atomic-bot commented Aug 21, 2019

gabibeyer commented Aug 23, 2019

gabibeyer commented Aug 23, 2019

mheon commented Aug 23, 2019

rh-atomic-bot commented Sep 6, 2019

rhatdan commented Sep 6, 2019

rh-atomic-bot commented Sep 16, 2019

marcov commented Sep 20, 2019

mheon Sep 20, 2019

mheon Sep 20, 2019

mheon Sep 20, 2019

mheon Sep 20, 2019

mheon commented Sep 20, 2019

marcov commented Sep 24, 2019

mheon commented Sep 24, 2019

TomSweeneyRedHat commented Sep 24, 2019

mheon commented Sep 24, 2019


		cmd := exec.Command(path, cmdArgs...)
		cmdArgs = append(cmdArgs, "-c", "-e", "3", "-r", "4")

rootless: Rearrange setup of rootless containers #3756

rootless: Rearrange setup of rootless containers #3756

Conversation

gabibeyer commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

mheon commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

rh-atomic-bot commented Aug 7, 2019

haircommander commented Aug 7, 2019

gabibeyer commented Aug 8, 2019

gabibeyer commented Aug 8, 2019

rh-atomic-bot commented Aug 21, 2019

gabibeyer commented Aug 23, 2019

gabibeyer commented Aug 23, 2019

mheon commented Aug 23, 2019

rh-atomic-bot commented Sep 6, 2019

rhatdan commented Sep 6, 2019

rh-atomic-bot commented Sep 16, 2019

marcov commented Sep 20, 2019

mheon Sep 20, 2019

Choose a reason for hiding this comment

mheon Sep 20, 2019

Choose a reason for hiding this comment

mheon Sep 20, 2019

Choose a reason for hiding this comment

mheon Sep 20, 2019

Choose a reason for hiding this comment

mheon commented Sep 20, 2019

marcov commented Sep 24, 2019

mheon commented Sep 24, 2019

TomSweeneyRedHat commented Sep 24, 2019

mheon commented Sep 24, 2019