Allow creation of a rootless netns backed by Pasta #1846

mheon · 2024-02-06T19:55:52Z

This makes the code for setting up rootless network namespaces dependent on what the default rootless network provider is, and allows Pasta to be used for traffic forwarding on the rootless netns.

Luap99 · 2024-02-07T10:08:25Z

libnetwork/internal/rootlessnetns/netns_linux.go

-	// rootlessNetNsSilrp4netnsPidFile is the name of the rootless netns slirp4netns pid file
-	rootlessNetNsSilrp4netnsPidFile = "rootless-netns-slirp4netns.pid"
+	// rootlessNetNsConnPidFile is the name of the rootless netns slirp4netns/pasta pid file
+	rootlessNetNsConnPidFile = "rootless-netns-slirp4netns.pid"


might as well rename the file, this code was only add during 5.0-dev so it is fine to break this here

Luap99 · 2024-02-07T10:10:27Z

libnetwork/internal/rootlessnetns/netns_linux.go

@@ -113,7 +114,14 @@ func (n *Netns) getOrCreateNetns() (ns.NetNS, bool, error) {
 	if err != nil {
 		return nil, false, wrapError("create netns", err)
 	}
-	err = n.setupSlirp4netns(nsPath)
+	switch strings.ToLower(n.config.Network.DefaultRootlessNetworkCmd) {
+	case "slirp4netns":


use constant, slirp4netns.BinaryName
also match "" as well because we allow this in podman,buildah atm

Luap99 · 2024-02-07T10:10:37Z

libnetwork/internal/rootlessnetns/netns_linux.go

+	switch strings.ToLower(n.config.Network.DefaultRootlessNetworkCmd) {
+	case "slirp4netns":
+		err = n.setupSlirp4netns(nsPath)
+	case "pasta":


use pasta.BinaryName

Luap99 · 2024-02-07T10:12:42Z

libnetwork/internal/rootlessnetns/netns_linux.go

-		multiErr = multierror.Append(multiErr, wrapError("kill slirp4netns", err))
+	// Pasta does not require teardown, it will tear itself down so long as
+	// we get rid of the netns.
+	if n.config.Network.DefaultRootlessNetworkCmd == "slirp4netns" {


use the constant here as well

Luap99 · 2024-02-07T10:13:57Z

libnetwork/internal/rootlessnetns/netns_linux.go

+	pastaOpts := pasta.SetupOptions{
+		Config:       n.config,
+		Netns:        nsPath,
+		ExtraOptions: []string{"-p", rootlessNetNsConnPidFile},


doesn't this create the file in the cwd? It must be wrapped in n.getPath() to get a proper absolute path.

Luap99 · 2024-02-07T10:14:03Z

libnetwork/internal/rootlessnetns/netns_linux.go

+
+	if systemd.RunsOnSystemd() {
+		// Treat these as fatal - if pasta failed to write a PID file something is probably wrong.
+		pidfile, err := os.ReadFile(rootlessNetNsConnPidFile)


mheon · 2024-02-07T18:14:57Z

Comments addressed

rhatdan · 2024-02-07T20:38:42Z

libnetwork/internal/rootlessnetns/netns_linux.go

@@ -113,7 +114,14 @@ func (n *Netns) getOrCreateNetns() (ns.NetNS, bool, error) {
 	if err != nil {
 		return nil, false, wrapError("create netns", err)
 	}
-	err = n.setupSlirp4netns(nsPath)
+	switch strings.ToLower(n.config.Network.DefaultRootlessNetworkCmd) {
+	case "", slirp4netns.BinaryName:


Should this be switched to default to pasta?

I don't think it matters, it should always be explicitly set in containers.conf

Some distributions do not ship a containers.conf, or was the default modified in common/pkg/config/default.go?

Looks like not yet.

DefaultRootlessNetworkCmd: "slirp4netns",

Luap99

Before merging this needs to be vendored into podman and tested there, I guess you can either switch the default directly in this PR to test or write a containers.conf in podman CI.
Either way I expect failing tests in podman as some likely depend on certain slirp behaviours.

Luap99 · 2024-02-08T11:33:37Z

libnetwork/internal/rootlessnetns/netns_linux.go

+	pastaOpts := pasta.SetupOptions{
+		Config:       n.config,
+		Netns:        nsPath,
+		ExtraOptions: []string{"-p", pidPath},


-p is actually not the pid file, -P is, but I recommend to use --pid

Luap99 · 2024-02-08T11:35:34Z

libnetwork/internal/rootlessnetns/netns_linux.go

+		}
+		pid, err := strconv.Atoi(string(pidfile))
+		if err != nil {
+			return fmt.Errorf("unable to decode pasta PID (got %q): %w", string(pidfile), err)


no need to include the pidfile data in the error as the Atoi error already includes this so you would stutter the error.

rhatdan · 2024-02-09T15:33:40Z

LGTM
@Luap99 PTAL
@mheon has the tests passed for Podman?

mheon · 2024-02-09T15:35:03Z

Tests are here: containers/podman#21563

Mostly passing now (minor issue in APIv2) but the system tests are timing out. Potentially could be Pasta being slower to start than slirp4netns?

Luap99 · 2024-02-09T15:51:33Z

If you change the default here in the same commit then this needs docs changes as well for it.

Anyhow I would think we can merge this without making pasta default here in this PR. And figure out the remaining test problems next week as the timeouts are very concerning. If pasta is actually so much slower than I am not sure if we want to continue to make it the default given it adds at least 10 mins to the system tests.

sbrivio-rh · 2024-02-16T07:43:55Z

Potentially could be Pasta being slower to start than slirp4netns?

$ time for i in `seq 1 100`; do podman run --net=pasta --rm -it alpine true; done
real	0m15.358s
user	0m4.360s
sys	0m2.823s
$ time for i in `seq 1 100`; do podman run --net=slirp4netns --rm -it alpine true; done
real	0m14.399s
user	0m4.285s
sys	0m1.640s

I'd call that "the same". Is there any particular configuration or sequence I should look into?

Luap99 · 2024-02-16T09:32:50Z

Potentially could be Pasta being slower to start than slirp4netns?
$ time for i in `seq 1 100`; do podman run --net=pasta --rm -it alpine true; done
real	0m15.358s
user	0m4.360s
sys	0m2.823s
$ time for i in `seq 1 100`; do podman run --net=slirp4netns --rm -it alpine true; done
real	0m14.399s
user	0m4.285s
sys	0m1.640s
I'd call that "the same". Is there any particular configuration or sequence I should look into?

No looks good to me as well. I wouldn't worry about that, the problem in podman PR clearly shows the problem is leaking pasta processes not speed.

sbrivio-rh · 2024-02-16T09:56:53Z

No looks good to me as well. I wouldn't worry about that, the problem in podman PR clearly shows the problem is leaking pasta processes not speed.

Okay, let me prepare a release with the fix then. We don't know if it fixes that issue or not, but I think it's a critical fix anyway, as the inotify watch setup can semi-silently fail.

Luap99

just so it is not forgotten, you have to update the docs if you change the default (i.e. see #1854)

mheon · 2024-02-22T16:23:17Z

Ack, will do

Luap99

One final, nit. Rest looks good here and podman tests seem to be happy as well.

One last ask would be to make sure this vendors into buildah as well without breaking tests there. Might need to users newer VM images there first (not sure if they got updated for the latest pasta)

Luap99 · 2024-02-27T12:21:02Z

libnetwork/internal/rootlessnetns/netns_linux.go

+	if err := n.cleanupRootlessNetns(); err != nil {
 		multiErr = multierror.Append(multiErr, wrapError("kill slirp4netns", err))


the wrapError message needs to change as well, maybe to kill networking process?

This makes the code for setting up rootless network namespaces dependent on what the default rootless network provider is, and allows Pasta to be used for traffic forwarding on the rootless netns. This also switches the default rootless network provider to Pasta Signed-off-by: Matt Heon <mheon@redhat.com>

mheon · 2024-02-27T13:17:29Z

Fixed & force-pushed. Let's get this landed.

Luap99

LGTM

/hold
Please do not merge this yet, c/common seems to be blocked in podman (containers/podman#21828), lets not add more changes on top of that require large changes in podman. Once the latest common is merged in podman we can merged this and do another vendor with all your test fixes in the PR.
Also please do a buildah test run I suspect you may need some test changes there as well.

mheon · 2024-02-27T13:41:42Z

Buildah run is a good call, will do that now.

mheon · 2024-02-27T13:59:24Z

containers/buildah#5359 for Buildah

mheon · 2024-02-28T19:02:32Z

@Luap99 I think we decided we can merge this as-is now, yes?

Luap99 · 2024-02-28T19:26:05Z

If we vendor the latest c/common already in podman then yes, however looks like there are failing test in containers/podman#21774 which sounds like c/storage regressions to me but haven't really looked closely. I can do so tomorrow morning.

Luap99

LGTM

/lgtm
/hold
containers/podman#21774 is about to merge,
remove the hold when you are ready to rebase your podman PR against c/common@main

openshift-ci · 2024-02-29T16:41:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, 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~~ [Luap99,mheon]

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

mheon · 2024-02-29T16:51:28Z

/hold cancel

openshift-ci bot added the approved label Feb 6, 2024

mheon force-pushed the allow_rootless_netns_pasta branch 3 times, most recently from e06bf83 to a202476 Compare February 6, 2024 20:46

Luap99 reviewed Feb 7, 2024

View reviewed changes

rhatdan added the 5.0 label Feb 7, 2024

mheon force-pushed the allow_rootless_netns_pasta branch from a202476 to 3a37c0b Compare February 7, 2024 18:14

rhatdan reviewed Feb 7, 2024

View reviewed changes

Luap99 mentioned this pull request Feb 8, 2024

Switch default from slirp4netns to pasta #1854

Closed

Luap99 requested changes Feb 8, 2024

View reviewed changes

mheon force-pushed the allow_rootless_netns_pasta branch 7 times, most recently from af15ea1 to a9fe405 Compare February 9, 2024 14:59

mheon force-pushed the allow_rootless_netns_pasta branch 9 times, most recently from e9c35ad to 1a6727c Compare February 14, 2024 19:28

mheon force-pushed the allow_rootless_netns_pasta branch from 1a6727c to dd99f6c Compare February 15, 2024 13:35

mheon force-pushed the allow_rootless_netns_pasta branch from 4965bb8 to 328022e Compare February 21, 2024 13:18

Luap99 requested changes Feb 22, 2024

View reviewed changes

mheon force-pushed the allow_rootless_netns_pasta branch 2 times, most recently from 45f4aee to dd24d75 Compare February 26, 2024 14:30

Luap99 reviewed Feb 27, 2024

View reviewed changes

mheon force-pushed the allow_rootless_netns_pasta branch from dd24d75 to fdf91cd Compare February 27, 2024 13:17

Luap99 approved these changes Feb 27, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Feb 27, 2024

Luap99 approved these changes Feb 29, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Feb 29, 2024

openshift-ci bot added the lgtm label Feb 29, 2024

openshift-ci bot removed the do-not-merge/hold label Feb 29, 2024

openshift-merge-bot bot merged commit 2c4ea34 into containers:main Feb 29, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creation of a rootless netns backed by Pasta #1846

Allow creation of a rootless netns backed by Pasta #1846

mheon commented Feb 6, 2024

Luap99 Feb 7, 2024

Luap99 Feb 7, 2024

Luap99 Feb 7, 2024

Luap99 Feb 7, 2024

Luap99 Feb 7, 2024

Luap99 Feb 7, 2024

mheon commented Feb 7, 2024

rhatdan Feb 7, 2024

mheon Feb 7, 2024

rhatdan Feb 8, 2024

rhatdan Feb 8, 2024

Luap99 left a comment

Luap99 Feb 8, 2024

Luap99 Feb 8, 2024

rhatdan commented Feb 9, 2024

mheon commented Feb 9, 2024

Luap99 commented Feb 9, 2024

sbrivio-rh commented Feb 16, 2024

Luap99 commented Feb 16, 2024

sbrivio-rh commented Feb 16, 2024

Luap99 left a comment

mheon commented Feb 22, 2024

Luap99 left a comment

Luap99 Feb 27, 2024

mheon commented Feb 27, 2024

Luap99 left a comment

mheon commented Feb 27, 2024

mheon commented Feb 27, 2024

mheon commented Feb 28, 2024

Luap99 commented Feb 28, 2024

Luap99 left a comment

openshift-ci bot commented Feb 29, 2024

mheon commented Feb 29, 2024

		if err := n.cleanupRootlessNetns(); err != nil {
		multiErr = multierror.Append(multiErr, wrapError("kill slirp4netns", err))

Allow creation of a rootless netns backed by Pasta #1846

Allow creation of a rootless netns backed by Pasta #1846

Conversation

mheon commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 9, 2024

mheon commented Feb 9, 2024

Luap99 commented Feb 9, 2024

sbrivio-rh commented Feb 16, 2024

Luap99 commented Feb 16, 2024

sbrivio-rh commented Feb 16, 2024

Luap99 left a comment

Choose a reason for hiding this comment

mheon commented Feb 22, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Feb 27, 2024

Luap99 left a comment

Choose a reason for hiding this comment

mheon commented Feb 27, 2024

mheon commented Feb 27, 2024

mheon commented Feb 28, 2024

Luap99 commented Feb 28, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 29, 2024

mheon commented Feb 29, 2024