add support for driver-specific options during container creation #24535

M1cha · 2024-11-11T21:46:10Z

This way has a huge disadvantage: The user will not see an error when he uses a non-existent option. Another disadvantage is, that if we add more options within podman, they might collide with the names chosen by plugins. Such issues might be hard to debug.
The advantage is that the usage is very nice:
--network bridge:opt1=val1,opt2=val2.

Alternatively, we could put this behind opt=, which is harder to use, but would solve all issues above:
--network bridge:opt=opt1=val1,opt=opt2=val2

Does this PR introduce a user-facing change?

A new `host_interface_name` option was added for bridge networks to specify the host side veth name, requires netavark >= 1.14.

Depends on: containers/common#2245
Required by: containers/netavark#1125

packit-as-a-service · 2024-11-11T21:59:41Z

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99 · 2024-11-12T16:40:24Z

FYI if you call go get github.com/containers/common@main && make vendor you can update c/common to the latest version so your code compiles and we can merge it. (Please do the vendor changes in a separate commit)

This will also need documentation on --network option to at least document to one new option you added.

I think putting this behind opt= may not be a bad idea to at least allow us to keep throwing errors for typos for the common options such as ip, mac, interface_name,... but I certainly could be convinced that we do not need that.

@mheon WDYT?

mheon · 2024-11-12T17:59:48Z

I want to say we should validate at create time (if necessary by calling netavark to do a dry run of setting up the container) but that is complex and will slow down container creation significantly... Which isn't worth it.

Current approach LGTM given that. I don't like it but the alternatives are worse.

Signed-off-by: Michael Zimmermann <[email protected]>

M1cha · 2024-11-12T19:23:48Z

Would a simple e2e test be the right choice for this change? Do those actually run netavark in the background? Otherwise such a test probably wouldn't be that useful.

Luap99 · 2024-11-13T11:33:00Z

yes in test/e2e there a bunch of tests all using netavark. The problem here is though that we use the distro version so we first need to release a netavark then update it here in our CI images and only then we could test the new option.

Having a test is important to ensure the option is fully working and correct send to netavark so we know it works end to end. You can add a Skip("") to the test so it is not run in CI yet but having a test that I can enable later when we update the CI images is always a good idea.

M1cha · 2024-11-13T13:42:49Z

The test TestParseNetworkFlag/bridge_mode_with_invalid_option fails with this change. Shall I remove it given your decision above?

Luap99 · 2024-11-13T13:56:40Z

Yes if we go with the current version that would need to be dropped. I guess you can update the test to ensure unknown keys end up in the options map.
I am still unsure if using the opt= syntax might be better to catch the top level typos at least for ip, mac, etc... But I agree the usability goes a bit down so maybe not worth it.

M1cha · 2024-11-13T16:42:48Z

I've added tests now. If you prefer the opt= variant, just tell me - it's actually very easy to do. I did not verify, that the code inside the !isRootless() guard works, since I currently don't have a setup where I can run these tests as root. Even running them rootless was a challenge already on my ostree system. If you want me test it, I'll set-up a VM and do so.

Can you help me with the test/e2e/search_test.go:380 failure? I can't reproduce that locally.

Luap99 · 2024-11-13T16:41:01Z

test/e2e/network_test.go

@@ -297,6 +298,23 @@ var _ = Describe("Podman network", func() {
 		Expect(rmAll).Should(ExitCleanly())
 	})

+	It("podman run container host interface name", func() {
+		Skip("We need to update netavark first.")


please say FIXME: We need netavark >= v1.14 for host interface support.

This is important for anyone looking at this to know when this can be dropped.

Luap99 · 2024-11-13T16:44:39Z

test/e2e/network_test.go

+		Skip("We need to update netavark first.")
+
+		ctrName := "testCtr"
+		vethName := "my_veth"


please do not use static names, that might cause conflicts on the host.
Append a random suffix, i.e. "my_veth" + stringid.GenerateRandomID()[:8]

Luap99 · 2024-11-13T16:50:00Z

test/e2e/network_test.go

+			veth, err := net.InterfaceByName(vethName)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(veth.Name).To(Equal(vethName))
+		}


diff --git a/test/e2e/network_test.go b/test/e2e/network_test.go index 018b7eb97..3d3f1d393 100644 --- a/test/e2e/network_test.go +++ b/test/e2e/network_test.go @@ -312,6 +312,11 @@ var _ = Describe("Podman network", func() { veth, err := net.InterfaceByName(vethName) Expect(err).ToNot(HaveOccurred()) Expect(veth.Name).To(Equal(vethName)) + } else { + session := podmanTest.Podman([]string{"unshare", "--rootless-netns", "ip", "link", "show", vethName}) + session.WaitWithDefaultTimeout() + Expect(session).Should(ExitCleanly()) + Expect(session.OutputToString()).To(ContainSubstring(vethName)) } }) we can test rootless as well

good idea, thanks.

Luap99 · 2024-11-13T16:52:23Z

pkg/specgen/namespaces_test.go

@@ -161,7 +161,14 @@ func TestParseNetworkFlag(t *testing.T) {
 			name:   "bridge mode with invalid option",
 			args:   []string{"bridge:abc=123"},
 			nsmode: Namespace{NSMode: Bridge},
-			err:    "unknown bridge network option: abc",
+			networks: map[string]types.PerNetworkOptions{


can you rename the test to not say invalid but rather unknown.

Also maybe add another case where add 2+ options to make sure it is working for multiple keys. I know it does as the code is simple enough but better to validate that

Luap99 · 2024-11-13T16:55:08Z

I run the test on my system and it worked there (having a netavark from main installed) + added a diff to make do something useful as rootless as well

Luap99 · 2024-11-13T16:55:57Z

test/e2e/network_test.go

+		Expect(container).Should(ExitCleanly())
+
+		if !isRootless() {
+			// make sure cni/netavark created bridge with expected name


oh and please drop cni here, we no longer support cni upstream and cni doesn't support this option

Luap99 · 2024-11-13T16:56:47Z

You can ignore the search failures. They are just random registry flakes. We rerun tests as needed.

This way has a huge disadvantage: The user will not see an error when he uses a non-existent option. Another disadvantage is, that if we add more options within podman, they might collide with the names chosen by plugins. Such issues might be hard to debug. The advantage is that the usage is very nice: --network bridge:opt1=val1,opt2=val2. Alternatively, we could put this behind `opt=`, which is harder to use, but would solve all issues above: --network bridge:opt=opt1=val1,opt=opt2=val2 Signed-off-by: Michael Zimmermann <[email protected]>

M1cha · 2024-11-13T18:27:02Z

Thanks for the review. I addressed all your comments in the most recent push.

mheon · 2024-11-13T23:37:52Z

Code LGTM

Luap99

/lgtm

openshift-ci · 2024-11-14T09:26:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, M1cha

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]

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

openshift-ci bot added the release-note-action-required label Nov 11, 2024

This was referenced Nov 11, 2024

PerNetworkOptions: add driver specific options containers/common#2245

Merged

Support changing host veth name containers/netavark#1125

Merged

M1cha force-pushed the network-driver-options branch from 6ab15aa to da9a0c3 Compare November 12, 2024 18:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

vendor: update containers/common

3e47e0b

Signed-off-by: Michael Zimmermann <[email protected]>

M1cha force-pushed the network-driver-options branch from da9a0c3 to 9c79590 Compare November 12, 2024 18:59

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

M1cha force-pushed the network-driver-options branch 2 times, most recently from 93b8978 to 828301f Compare November 13, 2024 15:31

Luap99 reviewed Nov 13, 2024

View reviewed changes

M1cha force-pushed the network-driver-options branch from 828301f to 315e741 Compare November 13, 2024 17:15

openshift-ci bot added release-note and removed release-note-action-required labels Nov 14, 2024

Luap99 approved these changes Nov 14, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Nov 14, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2024

openshift-merge-bot bot merged commit fa5e3b6 into containers:main Nov 14, 2024
86 checks passed

M1cha deleted the network-driver-options branch November 14, 2024 09:31

M1cha mentioned this pull request Nov 14, 2024

Predictable veth names #24523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for driver-specific options during container creation #24535

add support for driver-specific options during container creation #24535

M1cha commented Nov 11, 2024 •

edited by Luap99

Loading

packit-as-a-service bot commented Nov 11, 2024

Luap99 commented Nov 12, 2024

mheon commented Nov 12, 2024

M1cha commented Nov 12, 2024

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024

Luap99 Nov 13, 2024

Luap99 Nov 13, 2024

Luap99 Nov 13, 2024

M1cha Nov 13, 2024

Luap99 Nov 13, 2024

Luap99 commented Nov 13, 2024

Luap99 Nov 13, 2024

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024 •

edited

Loading

mheon commented Nov 13, 2024

Luap99 left a comment

openshift-ci bot commented Nov 14, 2024

add support for driver-specific options during container creation #24535

add support for driver-specific options during container creation #24535

Conversation

M1cha commented Nov 11, 2024 • edited by Luap99 Loading

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Nov 11, 2024

Luap99 commented Nov 12, 2024

mheon commented Nov 12, 2024

M1cha commented Nov 12, 2024

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024

Luap99 Nov 13, 2024

Choose a reason for hiding this comment

Luap99 Nov 13, 2024

Choose a reason for hiding this comment

Luap99 Nov 13, 2024

Choose a reason for hiding this comment

M1cha Nov 13, 2024

Choose a reason for hiding this comment

Luap99 Nov 13, 2024

Choose a reason for hiding this comment

Luap99 commented Nov 13, 2024

Luap99 Nov 13, 2024

Choose a reason for hiding this comment

Luap99 commented Nov 13, 2024

M1cha commented Nov 13, 2024 • edited Loading

mheon commented Nov 13, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 14, 2024

M1cha commented Nov 11, 2024 •

edited by Luap99

Loading

M1cha commented Nov 13, 2024 •

edited

Loading