[RFC] libpod: remove PostConfigureNetNS option

[Luap99]

We currently have two different code paths for configuring the network namespace.
Normally we create the netns and configure it and then pass the path down to the oci runtime. However this does not work when a custom userns is used for the container as the netns must be owned by the container userns. In this case we let the oci runtime create the container with an empty netns and then afterwards do our setup.

This difference is configured via the field PostConfigureNetNS in the container config.

Over the year I fixed many bug in the PostConfigureNetNS code path because it is not as tested as the normal one, the latest one is #17963.
We need special handling for hosts and resolv.conf file generation and the network setup (CNI, netavark, slip4netns, pasta) and this very annoying to maintain because of this complexity.

I wonder why we need the PostConfigureNetNS option at all, couldn't we always use the PostConfigureNetNS code path and let the oci runtime create the netns for us? This should work even when there is no extra userns for the container. We would only need to maintain the single code path and not worry about the corner cases.

@mheon @giuseppe PTAL, is there anything I am overlooking?

[mheon]

Could be a performance hit for startup time - the usual, non user namespace path allows parallel network configuration and storage mounting, while user namespaces force those two to run separately. I have no idea how big of an impact this is though.

…

On Tue, Mar 28, 2023 at 11:56 Paul Holzinger ***@***.***> wrote: We currently have two different code paths for configuring the network namespace. Normally we create the netns and configure it and then pass the path down to the oci runtime. However this does not work when a custom userns is used for the container as the netns must be owned by the container userns. In this case we let the oci runtime create the container with an empty netns and then afterwards do our setup. This difference is configured via the field PostConfigureNetNS in the container config. Over the year I fixed many bug in the PostConfigureNetNS code path because it is not as tested as the normal one, the latest one is #17963 <#17963>. We need special handling for hosts and resolv.conf file generation and the network setup (CNI, netavark, slip4netns, pasta) and this very annoying to maintain because of this complexity. I wonder why we need the PostConfigureNetNS option at all, couldn't we always use the PostConfigureNetNS code path and let the oci runtime create the netns for us? This should work even when there is no extra userns for the container. We would only need to maintain the single code path and not worry about the corner cases. @mheon <https://github.com/mheon> @giuseppe <https://github.com/giuseppe> PTAL, is there anything I am overlooking? — Reply to this email directly, view it on GitHub <#17965>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCHL6E7GWI5PB3LTF2DW6MC4FANCNFSM6AAAAAAWKYVPRM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[Luap99]

It is easy enough to measure by using this patch:

diff --git a/pkg/specgen/generate/namespaces.go b/pkg/specgen/generate/namespaces.go
index ff5154cec..7b7dbfc23 100644
--- a/pkg/specgen/generate/namespaces.go
+++ b/pkg/specgen/generate/namespaces.go
@@ -268,7 +268,7 @@ func namespaceOptions(s *specgen.SpecGenerator, rt *libpod.Runtime, pod *libpod.
                toReturn = append(toReturn, libpod.WithCgroupsMode(s.CgroupsMode))
        }
 
-       postConfigureNetNS := !s.UserNS.IsHost()
+       postConfigureNetNS := true
        // when we are rootless we default to slirp4netns
        if rootless.IsRootless() && (s.NetNS.IsPrivate() || s.NetNS.IsDefault()) {
                s.NetNS.NSMode = specgen.Slirp

Using the benchmark from hack/perf I see this result:

Benchmark 1: ../../bin/podman run --name=123 docker.io/library/alpine:latest true
  Time (mean ± σ):     324.6 ms ±  24.0 ms    [User: 58.3 ms, System: 58.4 ms]
  Range (min … max):   289.7 ms … 403.8 ms    100 runs
 
Benchmark 2: ../../bin/podman-post-netns run --name=123 docker.io/library/alpine:latest true
  Time (mean ± σ):     328.4 ms ±  21.0 ms    [User: 58.3 ms, System: 59.3 ms]
  Range (min … max):   297.3 ms … 411.8 ms    100 runs
 
Summary
  '../../bin/podman run --name=123 docker.io/library/alpine:latest true' ran
    1.01 ± 0.10 times faster than '../../bin/podman-post-netns run --name=123 docker.io/library/alpine:latest true'

So on my laptop It seems to be 1ms slower so I would call it insignificant. Also this is a very trivial patch, when we implement this properly we could remove some of the redundant code calls the the PostConfigureNetNS code path.

[giuseppe]

sorry for missing the notification here.

I think that is a good idea, I cannot think of any case where we need the !PostConfigureNetNS case now.

This was also suggested by @mheon when the bool was originally added (as part of adding support for userns): 6e0491b#r185135552

I'd not expect anything to break at this point, the user namespace code path is quite well tested, my suggestion is to go forward with it

[Luap99]

Thanks @giuseppe, I will put it on my TODO list.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[rhatdan]

Any movement on this?