passt, sidecar: Look for optional link #12354

qinqon · 2024-07-15T14:35:30Z

What this PR does

Before this PR:
Passt uses the source device "eth0" for domain xml

After this PR:
At some enviroments that should be "ovn-udn1" this PR try this first, if it do not exist do a fallback to eth0.

ovn-kubernetes place where the interface is not eth0:

https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/cni/udn/primary_network.go#L30

Why we need it and why it was done in this way

The following tradeoffs were made:
Using harcoded value is not flexible, but we plan to make it configurable in the future.

The following alternatives were considered:
Try to find the interface that has the default gw, problem is that at multus you can configure de default network at other interfaces.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

Use optional interface at passt binding sidecar

cmd/sidecars/network-passt-binding/domain/configurator.go

EdDev · 2024-07-15T17:23:37Z

cmd/sidecars/network-passt-binding/domain/configurator.go

 	"kubevirt.io/kubevirt/pkg/network/namescheme"
 	"kubevirt.io/kubevirt/pkg/network/vmispec"
 )

+type NetworkHandler interface {


I see no reason for it to be exposed.

It should describe the behavior, so consider a name like linkFinder or linkGetter. (please adjust the member name as well please, per your choice)

EdDev · 2024-07-15T17:44:23Z

cmd/sidecars/network-passt-binding/domain/configurator.go

 type NetworkConfiguratorOptions struct {
 	IstioProxyInjectionEnabled bool
 	UseVirtioTransitional      bool
+	NetworkHandler             NetworkHandler


Ahh this is a bit ugly.
It is not a configuration option, so it looks like an attempt to ride on some existing struct.

The simple option: Pass it to the NewPasstNetworkConfigurator directly.
The complex option: Use a builder pattern to avoid adding that nil all over the places that do not need it.

I am good with both options at this stage.

EdDev · 2024-07-15T17:45:07Z

cmd/sidecars/network-passt-binding/domain/configurator.go

+	// link do not exist
+	// FIXME: This will be configurable in the future.
+	const (
+		optionalLinkName = "ovn-udn1"


Can you add a link that shows from where this is coming from?
Not a must if it is linked in the commit message.

EdDev · 2024-07-15T17:46:10Z

cmd/sidecars/network-passt-binding/domain/configurator.go

+	_, err := p.handler.LinkByName(optionalLinkName)
+	if err != nil {


nit: Can be inlined (i.e. if _, err := p...; err != nil {).

EdDev · 2024-07-15T17:46:34Z

cmd/sidecars/network-passt-binding/domain/configurator.go

+		_, notFound := err.(vishnetlink.LinkNotFoundError)
+		if notFound {


EdDev · 2024-07-15T17:57:16Z

cmd/sidecars/network-passt-binding/domain/configurator_test.go

+			fakeNetlink := fakenetlink.New()
+			fakeNetlink.LinkAdd(&vishnetlink.Veth{
+				LinkAttrs: vishnetlink.LinkAttrs{
+					Name: "ovn-udn1",
+				},
+			})


I do not think we need to use the fake for this simple test.

Creating a stub should do the job:

type netlinkStub struct{} func (nl netLinkStub) LinkByName(name string) (vishnetlink.Link, error) { return &vishnetlink.Veth{LinkAttrs: vishnetlink.LinkAttrs{Name: "ovn-udn1"}} }

We do not really need to mimic any real logic of netlink here.

EdDev · 2024-07-15T18:03:53Z

cmd/sidecars/network-passt-binding/domain/configurator.go

@@ -65,9 +73,13 @@ func NewPasstNetworkConfigurator(ifaces []vmschema.Interface, networks []vmschem
 	if iface.Binding == nil || iface.Binding != nil && iface.Binding.Name != PasstPluginName {
 		return nil, fmt.Errorf("interface %q is not set with Passt network binding plugin", network.Name)
 	}
+	if opts.NetworkHandler == nil {
+		opts.NetworkHandler = &netlink.NetLink{}


If I understand correctly, the unit tests that do not use a stub/fake netlink, will in practice try to call netlink and look for the interface. We do not really want to mix the OS into this, so this is a good reasoning for not making the link getter optional. But it can be solved by making sure the tests always pass in a stub that returns the error.

fabiand · 2024-07-15T21:21:34Z

Could a CNI not just set en ENV var with the DEV name to use?

EdDev · 2024-07-15T21:58:43Z

Could a CNI not just set en ENV var with the DEV name to use?

The CNI is executed in the pod network namespace only, it cannot touch the container env variable.

fabiand · 2024-07-15T22:16:00Z

The SPEC does at least not permit it. I still wonder if it could be configured - as we know the pod. DPs can only do this.

qinqon · 2024-07-16T05:39:33Z

The SPEC does at least not permit it. I still wonder if it could be configured - as we know the pod. DPs can only do this.

One idea as a follow up PR after we unblock the kubevirt binding for ovn-k primary networks is to add a "intefaceName" option at the Kubevirt CR binding option ? then kubevirt can pass it to the sidecard and also to the NetworkSelectionElement virt-launcher annotation that already has a "interface" field:

k8s.v1.cni.cncf.io/networks: '[{"name":"netbindingpasst","namespace":"passt-poc","interface":"ovn-udn1"}

For the Kubevirt CR it would be something like

kubectl patch kubevirts -n kubevirt kubevirt --type=json -p='[{"op": "add", "path": "/spec/configuration/network",   "value": {
            "binding": {
                "ovn-passt": {
                    "networkAttachmentDefinition": "default/netbindingpasst",
                    "sidecarImage": "quay.io/kubevirt/network-passt-binding:20231205_29a16d5c9",
                    "interface": "ovn-udn1",
                    "migration": {
                        "method": "link-refresh"
                    },
                    "computeResourceOverhead": {
                      "requests": {
                        "memory": "500Mi",
                      }
                    }
                }
            }
        }}]'

ormergi · 2024-07-16T11:46:45Z

@qinqon Maybe the binding plugin's downard API can be utilized to avoid passing the interface name (low level detail, depend on env), in a way it"ll mount the network-status annotation so the sidecar can take the interface name form there.
Please see the downward API design [1] and vDPA example [2].

The sidecar could realize the interface-name from the device-info.
In case device-info doesnt provide the right info, the downward API could be extended to provide the interface-name, in other words the network-status bits with the network-intreface name instead of the device-info part.

[1] https://github.com/kubevirt/community/blob/main/design-proposals/network-binding-plugin/network-binding-plugin.md#reacting-to-cni-results-multus-networksstatus-annotation
[2] https://github.com/kubevirt/community/blob/main/design-proposals/network-binding-plugin/network-binding-plugin.md#appendix-f-concrete-example-for-vdpa-binding

ormergi · 2024-07-16T11:49:57Z

/assign

ormergi · 2024-07-16T11:52:57Z

cmd/sidecars/network-passt-binding/domain/configurator.go

+
+	sourceLinkName, err := p.discoverSourceLinkName()
+	if err != nil {
+		return nil, err
+	}


nit: I would move the link discovery to top of this function, to make if fail faster.

ormergi · 2024-07-16T11:57:02Z

cmd/sidecars/network-passt-binding/server/server.go

@@ -88,7 +88,7 @@ func (s V1alpha3Server) OnDefineDomain(_ context.Context, params *hooksV1alpha3.
 		IstioProxyInjectionEnabled: istioProxyInjectionEnabled,
 	}

-	passtConfigurator, err := domain.NewPasstNetworkConfigurator(vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, opts)
+	passtConfigurator, err := domain.NewPasstNetworkConfigurator(vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, opts, nil)


Shouldn't it have the concrete link discoverer?

On nill it fallback to netlink, so everything is fine.

Oh, I missed that part.

nit: I would pass the concrete link-discoverer here &netlink.NetLink{}, instead of having NewPasstNetworkConfigurator account for it.
IMO it would also make the code cleaner and more predictable.

At some systems the passt binding has to use ovn-udn1 as source link for the domain xml. This change add this as an optional link name and do a fallback to eth0 if the optional link is not found. Signed-off-by: Enrique Llorente <ellorent@redhat.com>

fabiand · 2024-07-16T20:48:30Z

My point about providing the ifname via CNI is to make it almost self-contained. I'd not put it into the KubeVirt cr as somebody has to configure it.

ormergi

Overall looks good

I left one suggestion, please consider it.

ormergi · 2024-07-17T08:46:38Z

cmd/sidecars/network-passt-binding/server/server.go

@@ -88,7 +88,7 @@ func (s V1alpha3Server) OnDefineDomain(_ context.Context, params *hooksV1alpha3.
 		IstioProxyInjectionEnabled: istioProxyInjectionEnabled,
 	}

-	passtConfigurator, err := domain.NewPasstNetworkConfigurator(vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, opts)
+	passtConfigurator, err := domain.NewPasstNetworkConfigurator(vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, opts, nil)


Oh, I missed that part.

nit: I would pass the concrete link-discoverer here &netlink.NetLink{}, instead of having NewPasstNetworkConfigurator account for it.
IMO it would also make the code cleaner and more predictable.

EdDev

Thank you.

The inline comments can be treated later if there is no time at the moment, nothing urgent there.

The usage of the hard-coded optional alternative link name is not that nice, but there is a challenge to pass this information through other means at the moment.

Passing information from the CNI step to the sidecar is not trivial.
Adding an API field for this is too noisy.
Using an admission webhook is possible (adding an env variable for the sidecar given the NAD information) and this could be considered for graduation.

/approve

EdDev · 2024-07-18T04:25:22Z

cmd/sidecars/network-passt-binding/domain/configurator_test.go

+type defaultNetLinkStub struct{}
+
+func (nl defaultNetLinkStub) LinkByName(name string) (vishnetlink.Link, error) {
+	return nil, vishnetlink.LinkNotFoundError{}
+}
+
+type netLinkStub struct{}
+
+func (nl netLinkStub) LinkByName(name string) (vishnetlink.Link, error) {
+	return &vishnetlink.Veth{LinkAttrs: vishnetlink.LinkAttrs{Name: name}}, nil
+}


I usually prefer these stubs at the tail of the file. But we can adjust it as a follow up.

EdDev · 2024-07-18T04:29:10Z

cmd/sidecars/network-passt-binding/domain/configurator.go

@@ -65,10 +72,14 @@ func NewPasstNetworkConfigurator(ifaces []vmschema.Interface, networks []vmschem
 	if iface.Binding == nil || iface.Binding != nil && iface.Binding.Name != PasstPluginName {
 		return nil, fmt.Errorf("interface %q is not set with Passt network binding plugin", network.Name)
 	}
+	if linkFinder == nil {


I would prefer not to have this asked here, as it opens up the option in the tests to use the concrete unintentionally.

We can fix it later though.

EdDev · 2024-07-18T04:33:08Z

cmd/sidecars/network-passt-binding/domain/configurator.go

+	)
+
+	if _, err := p.linkFinder.LinkByName(optionalLinkName); err != nil {
+		if _, notFound := err.(vishnetlink.LinkNotFoundError); notFound {


nit: errors.As is also an alternative.

That's not working since LinkNotFoundError do not do a proper error implementation, it just wrap an error
https://github.com/vishvananda/netlink/blob/d13535d71ed3e430e26c7538cd6266d026b26f54/link.go#L1412

This is how is tested at their own tests
https://github.com/vishvananda/netlink/blob/main/link_test.go#L2919

EdDev · 2024-07-18T04:36:02Z

cmd/sidecars/network-passt-binding/domain/configurator_test.go

@@ -309,5 +324,29 @@ var _ = Describe("pod network configurator", func() {

 			Expect(testMutator.Mutate(mutatedDomSpec)).To(Equal(mutatedDomSpec))
 		})
+		It("should set domain interface source link to the optional one if exists", func() {


Newline is missing (splitting between tests).

EdDev · 2024-07-18T04:36:58Z

cmd/sidecars/network-passt-binding/domain/configurator_test.go

+
+			Expect(testMutator.Mutate(mutatedDomSpec)).To(Equal(mutatedDomSpec))
+		})
+


redundant newline

EdDev · 2024-07-18T04:49:11Z

cmd/sidecars/network-passt-binding/domain/configurator_test.go

 	vmschema "kubevirt.io/api/core/v1"

 	"kubevirt.io/kubevirt/cmd/sidecars/network-passt-binding/domain"

 	domainschema "kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/api"
 )

+type defaultNetLinkStub struct{}


nit: You do not really need two stub types.

type netLinkStub struct{ link vishnetlink.Link err error } func (nl netLinkStub) LinkByName(name string) (vishnetlink.Link, error) { return link, err } netlinkStubWithNoLink = &netLinkStub{err: vishnetlink.LinkNotFoundError{}}

And for the single place where you need the link, you just fill it up there:
&netLinkStub{link: &vishnetlink.Veth{LinkAttrs: vishnetlink.LinkAttrs{Name: name}}}

But this is not really a must.

kubevirt-bot · 2024-07-18T05:00:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EdDev

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:

~~cmd/sidecars/network-passt-binding/OWNERS~~ [EdDev]

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

kubevirt-commenter-bot · 2024-07-18T05:00:47Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

qinqon · 2024-07-18T06:36:46Z

Thank you.

The inline comments can be treated later if there is no time at the moment, nothing urgent there.

The usage of the hard-coded optional alternative link name is not that nice, but there is a challenge to pass this information through other means at the moment.

Passing information from the CNI step to the sidecar is not trivial.

Adding an API field for this is too noisy.

Using an admission webhook is possible (adding an env variable for the sidecar given the NAD information) and this could be considered for graduation.

/approve

Tnaks so much!

We will prepare a jira to make kubevirt ovn-kubernetes primary networks aware and address these comments at follow up PR.

qinqon · 2024-07-18T06:37:15Z

/test pull-kubevirt-e2e-k8s-1.28-sig-compute

qinqon · 2024-07-19T06:49:38Z

/cherry-pick release-1.3

kubevirt-bot · 2024-07-19T06:50:17Z

@qinqon: new pull request created: #12409

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 15, 2024

kubevirt-bot requested review from EdDev and RamLavi July 15, 2024 14:35

kubevirt-bot added size/M sig/network labels Jul 15, 2024

maiqueb approved these changes Jul 15, 2024

View reviewed changes

kubevirt-bot assigned maiqueb Jul 15, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024

EdDev suggested changes Jul 15, 2024

View reviewed changes

cmd/sidecars/network-passt-binding/domain/configurator.go Outdated Show resolved Hide resolved

cmd/sidecars/network-passt-binding/domain/configurator.go Outdated Show resolved Hide resolved

kubevirt-bot assigned EdDev Jul 15, 2024

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2024

qinqon force-pushed the passt-add-extra-interface branch from 7638063 to 861dcfa Compare July 15, 2024 16:30

kubevirt-bot added the sig/buildsystem Denotes an issue or PR that relates to changes in the build system. label Jul 15, 2024

qinqon force-pushed the passt-add-extra-interface branch from 861dcfa to 047220a Compare July 15, 2024 17:15

qinqon requested a review from EdDev July 15, 2024 17:15

EdDev reviewed Jul 15, 2024

View reviewed changes

qinqon force-pushed the passt-add-extra-interface branch from 047220a to 9c6f579 Compare July 16, 2024 05:32

qinqon force-pushed the passt-add-extra-interface branch 2 times, most recently from 61d71ba to efde4a6 Compare July 16, 2024 06:46

qinqon requested a review from EdDev July 16, 2024 07:21

kubevirt-bot requested a review from ormergi July 16, 2024 11:49

kubevirt-bot assigned ormergi Jul 16, 2024

ormergi reviewed Jul 16, 2024

View reviewed changes

qinqon force-pushed the passt-add-extra-interface branch from efde4a6 to 873c23d Compare July 16, 2024 15:54

qinqon requested a review from ormergi July 16, 2024 15:54

ormergi approved these changes Jul 17, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2024

EdDev approved these changes Jul 18, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2024

qinqon mentioned this pull request Jul 18, 2024

Passt add extra interface v2 #12392

Open

kubevirt-bot merged commit 7a7ed78 into kubevirt:main Jul 18, 2024
41 checks passed

kubevirt-bot mentioned this pull request Jul 19, 2024

[release-1.3] passt, sidecar: Look for optional link #12409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

passt, sidecar: Look for optional link #12354

passt, sidecar: Look for optional link #12354

qinqon commented Jul 15, 2024 •

edited

Loading

EdDev Jul 15, 2024

EdDev Jul 15, 2024

EdDev Jul 15, 2024

EdDev Jul 15, 2024

EdDev Jul 15, 2024

EdDev Jul 15, 2024

EdDev Jul 15, 2024

fabiand commented Jul 15, 2024 via email

EdDev commented Jul 15, 2024

fabiand commented Jul 15, 2024 via email

qinqon commented Jul 16, 2024 •

edited

Loading

ormergi commented Jul 16, 2024 •

edited

Loading

ormergi commented Jul 16, 2024

ormergi Jul 16, 2024

ormergi Jul 16, 2024

qinqon Jul 16, 2024

ormergi Jul 17, 2024 •

edited

Loading

fabiand commented Jul 16, 2024 via email

ormergi left a comment

ormergi Jul 17, 2024 •

edited

Loading

EdDev left a comment

EdDev Jul 18, 2024

EdDev Jul 18, 2024

EdDev Jul 18, 2024

qinqon Jul 18, 2024 •

edited

Loading

EdDev Jul 18, 2024

EdDev Jul 18, 2024

EdDev Jul 18, 2024

kubevirt-bot commented Jul 18, 2024

kubevirt-commenter-bot commented Jul 18, 2024

qinqon commented Jul 18, 2024 •

edited

Loading

qinqon commented Jul 18, 2024

qinqon commented Jul 19, 2024

kubevirt-bot commented Jul 19, 2024

		_, err := p.handler.LinkByName(optionalLinkName)
		if err != nil {

		_, notFound := err.(vishnetlink.LinkNotFoundError)
		if notFound {


		Expect(testMutator.Mutate(mutatedDomSpec)).To(Equal(mutatedDomSpec))
		})

passt, sidecar: Look for optional link #12354

passt, sidecar: Look for optional link #12354

Conversation

qinqon commented Jul 15, 2024 • edited Loading

What this PR does

Why we need it and why it was done in this way

Checklist

Release note

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

Choose a reason for hiding this comment

fabiand commented Jul 15, 2024 via email

EdDev commented Jul 15, 2024

fabiand commented Jul 15, 2024 via email

qinqon commented Jul 16, 2024 • edited Loading

ormergi commented Jul 16, 2024 • edited Loading

ormergi commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

fabiand commented Jul 16, 2024 via email

ormergi left a comment

Choose a reason for hiding this comment

ormergi Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

EdDev left a 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

qinqon Jul 18, 2024 • edited Loading

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

kubevirt-bot commented Jul 18, 2024

kubevirt-commenter-bot commented Jul 18, 2024

qinqon commented Jul 18, 2024 • edited Loading

qinqon commented Jul 18, 2024

qinqon commented Jul 19, 2024

kubevirt-bot commented Jul 19, 2024

qinqon commented Jul 15, 2024 •

edited

Loading

qinqon commented Jul 16, 2024 •

edited

Loading

ormergi commented Jul 16, 2024 •

edited

Loading

ormergi Jul 17, 2024 •

edited

Loading

ormergi Jul 17, 2024 •

edited

Loading

qinqon Jul 18, 2024 •

edited

Loading

qinqon commented Jul 18, 2024 •

edited

Loading