-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Podman6: Remove CNI #27827
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
base: main
Are you sure you want to change the base?
Podman6: Remove CNI #27827
Conversation
Remove test cases that specifically test CNI functionality: - Remove "podman --cni-config-dir backwards compat" test - Remove "podman inspect container single/two CNI networks" tests - Remove "podman CNI network create with internal should not have dnsname" test - Remove "podman run in custom CNI network with --static-ip" test - Remove "podman rootless cni adds /usr/sbin to PATH" test Update CNI-specific references to be network-backend agnostic: - Update skip reasons from "Requires root CNI networking" to "Requires root networking" - Change --rootless-cni flag usage to --rootless-netns - Update comments from "CNI network" to "network" - Update test assertions to remove CNI-specific messaging Remove CNI-related test documentation and comments: - Remove commented-out CNI error messages from Python API tests - Remove CNI network namespace error documentation from upgrade tests - Remove CNI-related comments from BATS tests Remove unused import of github.com/containernetworking/plugins/pkg/ns from test/e2e/run_networking_test.go (test-only usage). Signed-off-by: Lokesh Mandvekar <[email protected]>
Remove test framework code that supported dual network backends: Test utilities: - Remove NetworkBackend enum type and constants (CNI, Netavark) - Remove NetworkBackend.ToString() method - Remove NetworkBackend field from PodmanTest struct Test infrastructure: - Remove SkipIfCNI() helper function - Remove SkipIfNetavark() helper function - Remove network backend selection logic based on NETWORK_BACKEND env var - Remove CNI-specific network config directory setup - Hardcode "netavark" in podman command line construction (flag will be removed in later commit) Simplify test helpers: - Simplify generateNetworkConfig() to only generate Netavark configs - Remove conditional CNI vs Netavark network ID logic - Update IP allocation comment to remove CNI-specific behavior description Remove SkipIfCNI() calls from tests: - Remove skip guards from Netavark-only feature tests - These tests now run universally since Netavark is the only backend Documentation: - Remove NETWORK_BACKEND environment variable from test/README.md All tests that were previously skipped with SkipIfCNI (Netavark-only features) will now run for all users since Netavark is the only supported network backend. Signed-off-by: Lokesh Mandvekar <[email protected]>
Remove user-facing CLI options for CNI network backend: CLI flags: - Remove --network-backend global flag - Remove flag registration and shell completion for network backend Shell completions: - Remove AutocompleteNetworkBackend() function - Remove references to CNI and Netavark type constants Backward compatibility: - Remove --rootless-cni flag alias for podman unshare - Remove SetNormalizeFunc that mapped rootless-cni to rootless-netns - Update --rootless-netns flag description to mention only netavark The --network-backend flag was already hidden, and is now completely removed. Users can no longer specify CNI as a network backend option. The --rootless-cni alias is also removed; users must use --rootless-netns. Signed-off-by: Lokesh Mandvekar <[email protected]>
Remove runtime configuration options for CNI network backend: Runtime options: - Remove WithNetworkBackend() runtime option function - Function allowed setting network backend programmatically Flag handling: - Remove --network-backend flag change detection - Remove call to WithNetworkBackend() when flag changed - Remove TODO comment about CNI plugins directory flag The network backend configuration is now handled entirely by the vendored common/libnetwork code, which will default to Netavark. There is no longer any way to configure CNI as the network backend through Podman's runtime initialization. Note: libpod/info.go keeps existing NetworkBackend reporting logic which will automatically report "netavark" as the only backend since configuration defaults to netavark and cannot be changed to CNI. Signed-off-by: Lokesh Mandvekar <[email protected]>
Remove CNI-specific conditional logic and update comments throughout the libpod networking code: - Simplified DNS configuration logic in container_internal_common.go to always use netavark behavior (removed backend checks) - Removed CNI-specific iptables chain error regex pattern - Updated all comments referencing 'CNI' to use 'netavark' or 'network backend' - Renamed variable 'cniNet' to 'netInfo' for clarity - Updated field and type documentation to remove CNI references All networking code now assumes netavark as the sole backend. Signed-off-by: Lokesh Mandvekar <[email protected]>
Removed all CNI-specific documentation from man pages: - podman.1.md: Simplified --network-config-dir to only mention netavark directories - podman-network.1.md: Removed dual backend description, now states netavark is the only backend - podman-network-create.1.md: Removed CNI-specific notes about DNS and DHCP socket configuration - podman-network-connect.1.md: Removed CNI limitation note about network aliases - options/network-alias.md: Removed CNI limitation note about network aliases - podman-info.1.md: Updated example output to show netavark backend information instead of CNI All man pages now reflect netavark as the sole network backend. Signed-off-by: Lokesh Mandvekar <[email protected]>
Remove CNI migration instructions and backend selection guidance from the basic networking tutorial. Simplify DHCP configuration section to only document netavark setup, removing CNI-specific instructions. Signed-off-by: Lokesh Mandvekar <[email protected]>
|
/packit test |
|
@containers/podman-maintainers PTAL |
| // not exist so we should not log this specific error as error. This would confuse users otherwise. | ||
| // Match both CNI and netavark chain errors for iptables-legacy, iptables-nft, and nftables. | ||
| // Note: We keep the CNI pattern for backwards compatibility during migration. | ||
| errMsg := err.Error() | ||
| isExpectedError := strings.Contains(errMsg, "Couldn't load target") || | ||
| strings.Contains(errMsg, "does not exist") || | ||
| strings.Contains(errMsg, "No such file or directory") || | ||
| strings.Contains(errMsg, "table inet netavark") || | ||
| strings.Contains(errMsg, "NETAVARK-") || | ||
| strings.Contains(errMsg, "CNI-") | ||
|
|
||
| if isExpectedError { | ||
| logrus.Info(err) | ||
| } else { | ||
| logrus.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed, first we dropped iptables from netavark as well and if we drop CNI it makes no sense to keep the CNI- match
But with nftables I think we never error since we only delete rules we know exists so this should not result ever in visable errors. So in short I would recommend to remove this completly and keep the log level error.
If we then still see errors we should address that one the netavark side.
| return newPort, errors.New("must provide a non-empty container host IP to publish") | ||
| } else if *hostIP != "0.0.0.0" { | ||
| // If hostIP is 0.0.0.0, leave it unset - CNI treats | ||
| // If hostIP is 0.0.0.0, leave it unset - netavark treats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't treat it differently in nv. We should likely chnage this here though I would recommend we keep that as is for this PR as changing this will liekly cause more work.
| "--cgroup-manager", config.Engine.CgroupManager, | ||
| "--tmpdir", config.Engine.TmpDir, | ||
| "--network-config-dir", config.Network.NetworkConfigDir, | ||
| "--network-backend", config.Network.NetworkBackend, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that is right but since you removed the flag from the parser it means any container started with 5.X has this set as cleanup command so when they update to 6.0 on a live system without reboot all cleanup command will fail during flag parsing as this option is no longer there.
I am not sure how important that is but maybe it is easier to keep network-backend as hidden flag defined for now. In the cli code I mean, the removal here is correct.
| } | ||
| }) | ||
|
|
||
| It("podman inspect container two CNI networks (container not running)", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we still want these tests, just remove the CNI from the names here
| ) | ||
|
|
||
| var _ = Describe("Podman run networking", func() { | ||
| BeforeEach(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the empty BeforeEach then as well
| Expect(session).To(ExitWithError(125, "faccessat /run/netns/xxy: no such file or directory")) | ||
| }) | ||
|
|
||
| It("podman run in custom CNI network with --static-ip", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well I think we must keep that to test the static ip option
| } | ||
|
|
||
| # bats test_tags=ci:parallel | ||
| @test "podman rootless cni adds /usr/sbin to PATH" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this must be kept, on debian rootless has no sbin and we still have to call nftables from there
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?