Conversation
|
Packit jobs failed. @containers/packit-build please check. |
lsm5
force-pushed
the
podman6-no-cni
branch
4 times, most recently
from
00a5fe1 to
f5654b3
Compare
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
mostly looks good, commit structure is good and helped for the reviews, thanks.
| } | ||
|
|
||
| func defaultNetworkBackend(store storage.Store, conf *config.Config) (backend types.NetworkBackend, err error) { | ||
| func defaultNetworkBackend(store storage.Store, _ *config.Config) (backend types.NetworkBackend, err error) { |
There was a problem hiding this comment.
we should be able to just delete this, no need to mess with the state file at all
basically in the main function if backend == "" || backend == netavark then return netavark anything else return hard error, no need to have anything more if we do not need it
| TmpDir: opts.Config.Engine.TmpDir, | ||
| ChildIP: childIP, | ||
| ContainerID: opts.ContainerID, | ||
| RootlessCNI: netStatus != nil, |
There was a problem hiding this comment.
I think this is still needed at least as long slirp4netns support is still in the code base, this bool is for the rootless bridge networking, it just comes from a time where we never renamed it.
we need this to remap the source ip of rootlessport via the extrac socket that rootlessport exposes in the podman repo.
This can go away once we fully get rid of slirp and rootlessport but not as part of CNI removal.
common/pkg/netns/netns_linux.go
Outdated
| func getCurrentThreadNetNSPath() string { | ||
| // /proc/self/ns/net returns the namespace of the main thread, not | ||
| // of whatever thread this goroutine is running on. Make sure we | ||
| // use the thread's net namespace since the thread is switching around | ||
| return fmt.Sprintf("/proc/%d/task/%d/ns/net", os.Getpid(), unix.Gettid()) | ||
| } |
There was a problem hiding this comment.
see 4914fe7 we should not reimport that one
overall I think inlining this logic here makes sense
|
@Luap99 comments addressed. @containers/container-libs-maintainers PTAL |
|
LGTM |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
I have no opinion on whether we want to do this but the implementation of that decision looks good.
- Also
common/.golangci.ymlstill contains a CNI flag - A commit message says “Systems that previously used CNI are automatically migrated to netavark via the defaultNetworkBackend sentinel file.”, but all of that code was removed.
- (I don’t have much of an opinion on the ordering of code in
netns_linux.gobut I spent quite a lot of time determining the correspondence between the original and the copy. A separate commit copying ~unmodified code, and then reordering, would have been easier to review.)
|
Addressed comments, but I'll take a bit to vendor the latest into buildah and podman before removing draft status. You're welcome to review this though. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
LGTM, please merge whenever appropriate.
Drop the build-amd64-cni target and cni-tagged unit tests from the Makefile as part of removing CNI support in favor of netavark-only networking for Podman 6. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Remove the CNI-tagged interface files and simplify NetworkBackend() to only support netavark. Any other backend value results in an error. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Remove the NetworkBackend type and CNI constants from rootlessnetns, drop the CNI /var/lib/cni mount logic, and update comments throughout. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Remove the CNI network backend constant, CNIPluginDirs config field, DefaultCNIPluginDirs variable, and cni_plugin_dirs from config files and tests. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Delete the entire libnetwork/cni/ directory including all source files, tests, and test fixtures. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Copy the NetNS interface, netNS struct, GetNS(), WithNetNSPath(), GetCurrentNS(), and related types from the vendored containernetworking/plugins/pkg/ns package into common/pkg/netns. Update all consumers to import from common/pkg/netns instead. This removes the runtime dependency on containernetworking/plugins for network namespace management. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Move the NetNS interface, netNS struct, and related types and methods above the existing netns functions so that type definitions precede their usage. No functional changes. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Remove github.com/containernetworking/cni and github.com/containernetworking/plugins from go.mod and vendor. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Update containers.conf.5.md to reflect netavark-only networking and remove cni_plugin_dirs documentation. Clean up remaining CNI references in netavark test file comments. Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
|
Linked buildah and podman PRs have passing tests apart from the likely unrelated compose and machine test failures. I think I've addressed all comments on this PR too. Removing draft status from here. PTAL @containers/container-libs-maintainers |
4 participants
See individual commits.
This is vendored into containers/buildah#6453 and containers/podman#28202