Macvtap binding plugin #10800

AlonaKaplan · 2023-11-28T13:28:28Z

What this PR does / why we need it:
This PR allows using macvtap binding as a plugin.

Those are the extra steps that need to be done to run a VM with a macvtap plugin binding interface (the rest is similar to the old API usage).
Enable network binding plugin feature-gate in the KV CR

kubectl patch kubevirts -n kubevirt kubevirt --type=json -p='[{"op": "add", "path": "/spec/configuration/developerConfiguration/featureGates/-",   "value": "NetworkBindingPlugins"}]'

Register the macvtap binding plugin in the KV CR

note: use the name of the network-attachment-definition defined earlier.

kubectl patch kubevirts -n kubevirt kubevirt --type=json -p='[{"op": "add", "path": "/spec/configuration/network",   "value": {
            "binding": {
                "macvtap": {
                    domainAttachmentType: tap,
                }
            }
        }}]'

Create (and apply) a VM with a macvtap binding interface

---
apiVersion: kubevirt.io/v1
kind: VirtualMachine
metadata:
  labels:
    kubevirt.io/vm: vm-net-binding-macvtap
  name: vm-net-binding-macvtap
spec:
  running: true
  template:
    metadata:
      labels:
        kubevirt.io/vm: vm-net-binding-macvtap
    spec:
      domain:
        devices:
          disks:
          - disk:
              bus: virtio
            name: containerdisk
          interfaces:
          - name: macvtapnet
            binding:
              name: macvtap
            ports:
            - name: http
              port: 80
              protocol: TCP
          rng: {}
        resources:
          requests:
            memory: 1024M
      networks:
      - name: macvtapnet
        mutlus:
          networkName: macvtapNad  
      terminationGracePeriodSeconds: 0
      volumes:
      - containerDisk:
          image: registry:5000/kubevirt/fedora-with-test-tooling-container-disk:devel
        name: containerdisk

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

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:

Support macvtap as a binding plugin

In case the binding CR doesn't define an annotation, an empty one shouldn't be added to the multus annotation of the pod. Signed-off-by: Alona Paz <alkaplan@redhat.com>

Signed-off-by: Alona Paz <alkaplan@redhat.com>

kubevirt-bot · 2023-11-28T13:28:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ormergi · 2023-11-28T13:57:14Z

Regarding the example in PR the description, isn't it necessary to create NAD for mavtap binding?

AlonaKaplan · 2023-11-28T15:14:59Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test
/test pull-kubevirt-e2e-k8s-1.27-sig-network

AlonaKaplan · 2023-11-28T15:18:18Z

Regarding the example in PR the description, isn't it necessary to create NAD for mavtap binding?

Edited the comment to emphasize those are just the extra steps.

ormergi · 2023-11-28T14:04:12Z

pkg/virt-controller/services/multus_annotations.go

@@ -77,7 +77,9 @@ func GenerateMultusCNIAnnotationFromNameScheme(namespace string, interfaces []v1
 				if err != nil {
 					return "", err
 				}
-				multusNetworkAnnotationPool.add(*bindingPluginAnnotationData)
+				if bindingPluginAnnotationData != nil {
+					multusNetworkAnnotationPool.add(*bindingPluginAnnotationData)


nit: It seems the root cause for the additional empty element in Multus annotation is passing bindingPluginAnnotationData by value instead of pointer to add.
We can avoid yet another if in this function by passing pointer and performing nil checking inside add.

But then the nil check will be in the "add" method. So we anyway need to add a nil check.

ormergi · 2023-11-28T14:27:27Z

pkg/virt-handler/non-root.go

+			if errors.Is(err, os.ErrNotExist) {
+				return nil
+			}
+			return nil


Why is that error ignored?
It seems it will be ignored whether its ErrNoExist or not.

It means that is is ok if the device is just not there, in this case we don't need to change the ownership - there is no device.

This looks wrong or I miss something.
Of the device is missing or not, you always return nil.

you"re right. It is a bug. Done.

ormergi · 2023-11-28T14:31:52Z

pkg/virt-handler/non-root.go

 	for _, inf := range vmi.Spec.Domain.Devices.Interfaces {
-		if inf.Macvtap != nil {
-			macvtap[inf.Name] = struct{}{}
+		if domainAttachmentByInterfaceName[inf.Name] == string(v1.Tap) && inf.Masquerade == nil && inf.Bridge == nil {


What will happen when Mavtap interface API is used, will it work?

We will soon see in the tests are passing:) But generally, mactvap old API should return true here (since it is in the map).

ormergi · 2023-11-28T15:39:35Z

tests/network/bindingplugin.go

+			macvtapLowerDevice = "eth0"
+			macvtapNetworkName = "net1"


Aren't these can set as consts?

ormergi · 2023-11-28T15:41:17Z

tests/network/bindingplugin.go

+		}
+
+		BeforeEach(func() {
+			virtClient = kubevirt.Client()


I think kubevirt.Client() can be called directly, its implemented as singleton and its safe to call it more then once.

ormergi · 2023-11-28T15:44:43Z

tests/network/bindingplugin.go

+			var nodeList *k8sv1.NodeList
+			var nodeName string


These seem redundant because they only used in the BeforeEach

AlonaKaplan · 2023-11-28T18:09:22Z

/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test
/test pull-kubevirt-e2e-k8s-1.27-sig-network

EdDev · 2023-11-29T11:58:03Z

pkg/virt-handler/non-root.go

 	for _, inf := range vmi.Spec.Domain.Devices.Interfaces {
-		if inf.Macvtap != nil {
-			macvtap[inf.Name] = struct{}{}
+		if domainAttachmentByInterfaceName[inf.Name] == string(v1.Tap) && inf.Masquerade == nil && inf.Bridge == nil {


masquerade and bridge
are exluded since we know in advance there is no tap device on those
bindings

We indeed know, but why is that a good reasoning to keep conditioning on it?
I thought we can generalize it to not have to ask. As a side effect, it will assure we are able to handle it for other future bindings that do use a simple tap without a device behind it.

Bottom line, it will be nice to specify here all the users of tap & tap-device.

We can generalize it and everything will perfectly work if inf.Masquerade == nil && inf.Bridge == nil will be removed from the condition.
The reason I prefer to exclude it is because the next step is to step into the virt-laucnher's network namespace and looking for the device. It is a bit expensive, so if we can avoid it. Why not?
Another bindings don't have this privilege (that we know in advance if the tap device is there or not), so they we have to step into the network namespace to check.

EdDev · 2023-11-29T12:56:51Z

pkg/virt-handler/non-root.go

+			if errors.Is(err, os.ErrNotExist) {
+				return nil
+			}
+			return nil


This looks wrong or I miss something.
Of the device is missing or not, you always return nil.

EdDev · 2023-11-29T13:03:11Z

pkg/virt-handler/non-root.go

-		_, isMacvtapNetwork := macvtap[net.Name]
-		if podInterfaceName, exists := networkNameScheme[net.Name]; isMacvtapNetwork && exists {
+		_, isTapNetwork := tapNetworks[net.Name]
+		if podInterfaceName, exists := networkNameScheme[net.Name]; isTapNetwork && exists {
 			tapDevices[net.Name] = podInterfaceName


This is odd.

There is a hidden assumption that if there is a device behind a tap domainAttachement, it is always with the pod interface name.
Why is that correct? Maybe it is a device used with the tap* naming.

Well, you could always define this as a "rule", but it feels ugly.
Can this be solved in the future or will we be stuck with this forever? (I mean, assuming we let this slide in, can this be improved without breaking things).

yes, this is the rule. I should use the pod interface naming.
If we will improve it in the future we will have to somehow pass the "name" of the tap device.
I don't think it will break anything, we can say that the default is the pod interface name.

EdDev · 2023-11-29T13:12:08Z

tests/network/bindingplugin.go

+			macvtapNetworkConfNAD = `{"apiVersion":"k8s.cni.cncf.io/v1","kind":"NetworkAttachmentDefinition","metadata":{"name":"%s","namespace":"%s", "annotations": {"k8s.v1.cni.cncf.io/resourceName": "macvtap.network.kubevirt.io/%s"}},"spec":{"config":"{ \"cniVersion\": \"0.3.1\", \"name\": \"%s\", \"type\": \"macvtap\"}"}}`
+			macvtapBindingName    = "macvtap"
+			macvtapLowerDevice    = "eth0"
+			macvtapNetworkName    = "net1"
+		)
+
+		createMacvtapNetworkAttachmentDefinition := func(namespace, networkName, macvtapLowerDevice string) error {
+			macvtapNad := fmt.Sprintf(macvtapNetworkConfNAD, networkName, namespace, macvtapLowerDevice, networkName)
+			return createNetworkAttachmentDefinition(kubevirt.Client(), networkName, namespace, macvtapNad)
+		}


Can you please reuse or do something similar to createBasicNetworkAttachmentDefinition, this one is too noisy. You know I do not like these closure functions :).

removed it.

EdDev · 2023-11-29T13:17:48Z

tests/network/bindingplugin.go

+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		Context("can run a virtual machine with one macvtap interface", func() {


I do not see a need for another indented context here. We are in the "macvtap" context, and if there is a need for some pre-setup, it can be done at that level.

But I also think it is wrong to keep the current separation between the fixture and test body.
The test itself is also about defining the VM correctly.

EdDev · 2023-11-29T13:19:41Z

tests/network/bindingplugin.go

+				nodeList = libnode.GetAllSchedulableNodes(kubevirt.Client())
+				Expect(nodeList.Items).NotTo(BeEmpty(), "schedulable kubernetes nodes must be present")
+				nodeName := nodeList.Items[0].Name


I do not understand why this is needed.
We do not check this for other tests, what is special about this one?

If there are no scheduled nodes, it should fail on timing out to create the VM.
I do not need to know about these details unless you tell me it is a must.

It is nit needed, removed.

EdDev · 2023-11-29T13:21:03Z

tests/network/bindingplugin.go

+				Expect(nodeList.Items).NotTo(BeEmpty(), "schedulable kubernetes nodes must be present")
+				nodeName := nodeList.Items[0].Name
+
+				chosenMACHW, err := GenerateRandomMac()


Where is this GenerateRandomMac coming from? I guess it should be moved to libnet.

kubevirt/tests/network/mac.go

Line 8 in ea0a699

func GenerateRandomMac() (net.HardwareAddr, error) {

A separate PR can move it.

EdDev · 2023-11-29T13:22:21Z

tests/network/bindingplugin.go

+					libvmi.WithInterface(
+						*libvmi.InterfaceWithMac(
+							&macvtapIface, chosenMAC)),


nit: Can these appear on the same line?

EdDev · 2023-11-29T13:26:36Z

tests/network/bindingplugin.go

+					libvmi.WithInterface(
+						*libvmi.InterfaceWithMac(
+							&macvtapIface, chosenMAC)),
+					libvmi.WithNetwork(libvmi.MultusNetwork(libvmi.DefaultInterfaceName, macvtapNetworkName)))


I;m having hard time understanding how macvtap can be the primary network.

It is not the primary. Changed the name so it will be clearer.

The old code changed the ownership on the macvtap pod interface. To allow using macvtap binding as a plugin the code is changed to support any binding that is using tap domain attachment. If a device exists, the ownership will be changed (masquerade and bridge are exluded since we know in advance there is no tap device on those bindings). Signed-off-by: Alona Paz <alkaplan@redhat.com>

Signed-off-by: Alona Paz <alkaplan@redhat.com>

EdDev

Thank you!

EdDev · 2023-11-29T16:45:42Z

tests/network/bindingplugin.go

+			var vmi *v1.VirtualMachineInstance
+			var chosenMAC string


nit: You do not need these now.

EdDev · 2023-11-29T16:46:17Z

tests/network/bindingplugin.go

+			Expect(err).ToNot(HaveOccurred())
+			chosenMAC = chosenMACHW.String()
+
+			ifaceName := "macvtapIface"


nit: I would call it networkName and set it as a const.

When we use iface, it has too many possible interpretations (logical iface/network name, name on the OS kernel).

AlonaKaplan · 2023-11-30T07:33:12Z

Raising to approve.

/approve

kubevirt-bot · 2023-11-30T07:33:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

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~~ [AlonaKaplan]

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 · 2023-11-30T13:22:25Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2023-11-30T20:23:24Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2023-11-30T23:23:25Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2023-12-01T07:23:25Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kvaps · 2023-12-01T10:58:27Z

Hi @AlonaKaplan, wouldn't you like to add an opportunity from #7648 to make macvtap method working also for default podNetworking and other CNIs?

I can investigate into it to make this working.

kubevirt-commenter-bot · 2023-12-01T14:24:19Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot · 2023-12-01T17:00:09Z

@AlonaKaplan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-check-tests-for-flakes	`e378ba2`	link	false	`/test pull-kubevirt-check-tests-for-flakes`

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/test-infra repository. I understand the commands that are listed here.

kubevirt-commenter-bot · 2023-12-01T23:24:20Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

AlonaKaplan · 2023-12-04T09:19:23Z

/cherry-pick release-1.1

kubevirt-bot · 2023-12-04T09:20:02Z

@AlonaKaplan: new pull request created: #10831

In response to this:

/cherry-pick release-1.1

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/test-infra repository.

AlonaKaplan added 2 commits November 28, 2023 10:44

binding plugin, cni: Avoid empty annotation

02f6dc5

In case the binding CR doesn't define an annotation, an empty one shouldn't be added to the multus annotation of the pod. Signed-off-by: Alona Paz <alkaplan@redhat.com>

virt-handler: remove redundant nil check

e9be2d1

Signed-off-by: Alona Paz <alkaplan@redhat.com>

kubevirt-bot requested review from jean-edouard and ormergi November 28, 2023 13:28

ormergi reviewed Nov 28, 2023

View reviewed changes

AlonaKaplan force-pushed the macvtap_plugin branch from cafadd2 to d9f4752 Compare November 28, 2023 18:09

AlonaKaplan marked this pull request as ready for review November 29, 2023 06:45

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

kubevirt-bot requested review from lyarwood and RamLavi November 29, 2023 06:45

EdDev reviewed Nov 29, 2023

View reviewed changes

AlonaKaplan force-pushed the macvtap_plugin branch from d9f4752 to f596e06 Compare November 29, 2023 14:35

kubevirt-bot added size/M and removed size/L labels Nov 29, 2023

AlonaKaplan added 2 commits November 29, 2023 16:51

macvtap binding plugin: e2e test

e378ba2

Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the macvtap_plugin branch from f596e06 to e378ba2 Compare November 29, 2023 14:53

EdDev approved these changes Nov 29, 2023

View reviewed changes

kubevirt-bot assigned EdDev Nov 29, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023

kubevirt-bot merged commit 6017b2a into kubevirt:main Dec 2, 2023
36 of 37 checks passed

kubevirt-bot mentioned this pull request Dec 4, 2023

[release-1.1] Fix: Macvtap binding plugin #10831

Merged

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Macvtap binding plugin #10800

Macvtap binding plugin #10800

Conversation

AlonaKaplan commented Nov 28, 2023 • edited

kubevirt-bot commented Nov 28, 2023

ormergi commented Nov 28, 2023

AlonaKaplan commented Nov 28, 2023

AlonaKaplan commented Nov 28, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormergi Nov 28, 2023 • edited

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

AlonaKaplan commented Nov 28, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

AlonaKaplan commented Nov 30, 2023

kubevirt-bot commented Nov 30, 2023

kubevirt-commenter-bot commented Nov 30, 2023

kubevirt-commenter-bot commented Nov 30, 2023

kubevirt-commenter-bot commented Nov 30, 2023

kubevirt-commenter-bot commented Dec 1, 2023

kvaps commented Dec 1, 2023

kubevirt-commenter-bot commented Dec 1, 2023

kubevirt-bot commented Dec 1, 2023 • edited

kubevirt-commenter-bot commented Dec 1, 2023

AlonaKaplan commented Dec 4, 2023

kubevirt-bot commented Dec 4, 2023

AlonaKaplan commented Nov 28, 2023 •

edited

ormergi Nov 28, 2023 •

edited

kubevirt-bot commented Dec 1, 2023 •

edited