SRIOV migration based hotplug #10185

AlonaKaplan · 2023-07-27T17:27:41Z

What this PR does / why we need it:
This PR adds support for migration based SRIOV hotplug.

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:

Release note:

Add support to migration based SRIOV hotplug.

kubevirt-bot · 2023-07-27T17:27:43Z

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

AlonaKaplan · 2023-07-27T17:28:23Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test

AlonaKaplan · 2023-07-27T18:10:13Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-unit-test

AlonaKaplan · 2023-07-27T19:00:52Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/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-kind-1.27-sriov

AlonaKaplan · 2023-07-27T19:18:32Z

/retest

AlonaKaplan · 2023-07-27T19:38:55Z

/retest

AlonaKaplan · 2023-07-27T20:22:11Z

/test pull-kubevirt-e2e-kind-1.27-sriov

AlonaKaplan · 2023-07-27T20:44:57Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/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-kind-1.27-sriov

AlonaKaplan · 2023-07-27T21:06:34Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/test pull-kubevirt-e2e-kind-1.27-sriov

AlonaKaplan · 2023-07-28T10:30:57Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/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-kind-1.27-sriov

AlonaKaplan · 2023-07-28T12:37:41Z

/test pull-kubevirt-e2e-k8s-1.26-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.26-sig-network
/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-kind-1.27-sriov

EdDev

Thank you!

I think we are still missing a test (see inline) and the commit messages still need fixing (subject line is too long).
But I will not block the change on those, feel free to remove the hold.

/hold

EdDev · 2023-08-02T15:27:14Z

pkg/virt-controller/watch/network_test.go

+			nil,
+			nil,
+			expectNoChange,
+		),


I was looking for "expectToChange" + new SR-IOV not added.
I do not think we have it.

EdDev · 2023-08-02T15:29:09Z

pkg/virt-controller/watch/vmi_test.go

 					vmi.Namespace, vmi.Spec.Domain.Devices.Interfaces, vmi.Spec.Networks, pod)).To(HaveOccurred())
 			})
 		})

-		Context("hotplug operation", func() {
+		Context("pod status update", func() {
+			const mac1 = "abc"


Not really sure how it is related to the code changes done here, but I will not dig into it further.

If an SRIOV interface is added to the spec of a running VM, don't update the pod multus annotation with this interface. To configure SRIOV on the pod, device plugin has to be invovled. Kubelet invokes the device plugin of pod startup only. Therefore, there is no point in adding the SRIOV interface to the multus annotation at this point. It should be added only on migration to the new target pod multus annotation. Signed-off-by: Alona Paz <alkaplan@redhat.com>

The vmi network and interfaces spec may contain interface that are not plugged to the pod yet and therefore not reported in the multus status annotation. Those interfaces should be ignored when building the "kubevirt.io/network-pci-map" annotation. Signed-off-by: Alona Paz <alkaplan@redhat.com>

If there are SRIOV interfaces in the spec that are not in the domain (plug pending)-> SRIOV hotplug is invoked. This commit changes the code to invoke the SRIOV hotplug only if those plug pending interfaces are reported in the multus annotation. If they are not reported by the multus annotation it means the pod cannot use the VF yet. Signed-off-by: Alona Paz <alkaplan@redhat.com>

A preparation step for hotplug_sriov test file to be added. Signed-off-by: Alona Paz <alkaplan@redhat.com>

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

ormergi · 2023-08-03T12:50:13Z

tests/network/hotplug_sriov.go

+	mac, err := GenerateRandomMac()
+	if err != nil {
+		return err
+	}


Since addSRIOVInterface used only in one place, also MAC address has no actual affect on the hotplug logic isn't it?

ormergi · 2023-08-03T13:08:33Z

pkg/virt-controller/watch/network.go

+		sriovIfaceNotPlugged := iface.SRIOV != nil && !ifaceInStatus
+		return !sriovIfaceNotPlugged


IMO the intermediate variable doesn't help understanding what will be the result, the reader still need to interpret what !sriovIfaceNotPlugged means.
How about using the direct condition with better descriptive name?

nonSriovDesiredIfaces := iface.SRIOV == nil || ifaceInStatus return nonSriovDesiredIfaces

We can address it later though.

ormergi · 2023-08-03T16:24:13Z

pkg/virt-controller/watch/network.go

@@ -24,23 +24,29 @@ import (
 	"kubevirt.io/kubevirt/pkg/network/vmispec"
 )

-func calculateDynamicInterfaces(vmi *v1.VirtualMachineInstance) ([]v1.Interface, []v1.Network, bool) {
-	vmiSpecIfaces := vmispec.FilterInterfacesSpec(vmi.Spec.Domain.Devices.Interfaces, func(iface v1.Interface) bool {
+func calculateInterfacesAndNetworksForMultusAnnotation(vmi *v1.VirtualMachineInstance) ([]v1.Interface, []v1.Network, bool) {


So its basicly set the desired interfaces in the VMI hence in the pod.
I would call it somthing like calcuclateDesiredInterfaces or calcuclateDesiredPodInterfaces.

We can address this later and improve the naming around that area.

AlonaKaplan · 2023-08-03T16:30:52Z

/approve

Increasing 2 lgtms to approve.

kubevirt-bot · 2023-08-03T16:31:08Z

[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

EdDev · 2023-08-06T08:13:00Z

/unhold

EdDev · 2023-08-06T10:26:36Z

/retest

kubevirt-commenter-bot · 2023-08-06T13:12: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-commenter-bot · 2023-08-06T20:12: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.

PR kubevirt#10185 introduced the SR-IOV migration-based hotplug with an E2E test that fails [1] in case the `HotplugNICs` feature gate is disabled. The CI system of kubevirt/kubevirtci, has an SR-IOV lane that deploys a cluster and runs the conformance test mechanism with a focus on SR-IOV tests[2] without enabling the `HotplugNICs` feature gate. This causes the pre-submit jobs in kubevirt/kubevirtci to fail[3], and requires a maintainer override in order to merge PRs. As an immediate fix, skip the SR-IOV migration based hotplug tests if the `HotplugNICs` feature gate is disabled. The kubevirt/kubevirt tests will not be affected by this change. A more permanent fix will follow. [1] https://github.com/kubevirt/kubevirt/blob/cb29e9f87d41deae0d9e1efa6b0cde53ecd8c28a/tests/network/hotplug_sriov.go#L52 [2] https://github.com/kubevirt/project-infra/blob/88e8dbd315a7bc908f637ebe3168f1fe7da97839/github/ci/prow-deploy/files/jobs/kubevirt/kubevirtci/kubevirtci-presubmits.yaml#L53 [3] https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirtci/1070/check-up-kind-1.27-sriov/1691706923780411392 Signed-off-by: Orel Misan <omisan@redhat.com>

kubevirt-bot requested review from 0xFelix, maiqueb and RamLavi July 27, 2023 17:27

AlonaKaplan force-pushed the sriov_hotplug branch 2 times, most recently from 22d2de4 to e227d20 Compare July 27, 2023 18:09

AlonaKaplan force-pushed the sriov_hotplug branch from e227d20 to 6dbd1fc Compare July 27, 2023 19:00

AlonaKaplan force-pushed the sriov_hotplug branch from 6dbd1fc to 1c752bc Compare July 27, 2023 21:05

AlonaKaplan force-pushed the sriov_hotplug branch from 1c752bc to a17920f Compare July 28, 2023 10:27

kubevirt-bot added size/XL and removed size/XXL labels Jul 28, 2023

AlonaKaplan force-pushed the sriov_hotplug branch from a17920f to 318e625 Compare July 28, 2023 12:36

kubevirt-bot added size/XXL and removed size/XL labels Jul 28, 2023

AlonaKaplan marked this pull request as ready for review July 28, 2023 15:15

EdDev approved these changes Aug 2, 2023

View reviewed changes

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 2, 2023

AlonaKaplan added 4 commits August 2, 2023 22:58

e2e: Rename hotplug test file to hotplug_bridge

38a3bd5

A preparation step for hotplug_sriov test file to be added. Signed-off-by: Alona Paz <alkaplan@redhat.com>

AlonaKaplan force-pushed the sriov_hotplug branch from 8eab2db to a21ebd4 Compare August 2, 2023 20:12

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2023

AlonaKaplan force-pushed the sriov_hotplug branch from a21ebd4 to 87993a4 Compare August 3, 2023 05:50

SRIOV hotplug, e2e: add SRIOV hotplug test

560e4cb

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

AlonaKaplan force-pushed the sriov_hotplug branch from 87993a4 to 560e4cb Compare August 3, 2023 06:42

EdDev approved these changes Aug 3, 2023

View reviewed changes

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

ormergi approved these changes Aug 3, 2023

View reviewed changes

kubevirt-bot assigned ormergi Aug 3, 2023

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

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 6, 2023

kubevirt-bot merged commit ca740d9 into kubevirt:main Aug 6, 2023
36 checks passed

orelmisan mentioned this pull request Aug 16, 2023

net, e2e: Skip SR-IOV hotplug if FG is disabled #10298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SRIOV migration based hotplug #10185

SRIOV migration based hotplug #10185

AlonaKaplan commented Jul 27, 2023

kubevirt-bot commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 28, 2023

AlonaKaplan commented Jul 28, 2023

EdDev left a comment

EdDev Aug 2, 2023

EdDev Aug 2, 2023

ormergi Aug 3, 2023

ormergi Aug 3, 2023

ormergi Aug 3, 2023

AlonaKaplan commented Aug 3, 2023

kubevirt-bot commented Aug 3, 2023

EdDev commented Aug 6, 2023

EdDev commented Aug 6, 2023

kubevirt-commenter-bot commented Aug 6, 2023

kubevirt-commenter-bot commented Aug 6, 2023

		sriovIfaceNotPlugged := iface.SRIOV != nil && !ifaceInStatus
		return !sriovIfaceNotPlugged

SRIOV migration based hotplug #10185

SRIOV migration based hotplug #10185

Conversation

AlonaKaplan commented Jul 27, 2023

kubevirt-bot commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 27, 2023

AlonaKaplan commented Jul 28, 2023

AlonaKaplan commented Jul 28, 2023

EdDev left a comment

Choose a reason for hiding this comment

EdDev Aug 2, 2023

Choose a reason for hiding this comment

EdDev Aug 2, 2023

Choose a reason for hiding this comment

ormergi Aug 3, 2023

Choose a reason for hiding this comment

ormergi Aug 3, 2023

Choose a reason for hiding this comment

ormergi Aug 3, 2023

Choose a reason for hiding this comment

AlonaKaplan commented Aug 3, 2023

kubevirt-bot commented Aug 3, 2023

EdDev commented Aug 6, 2023

EdDev commented Aug 6, 2023

kubevirt-commenter-bot commented Aug 6, 2023

kubevirt-commenter-bot commented Aug 6, 2023