Allow LUN disks to be hotplugged #10529

alromeros · 2023-10-06T11:49:43Z

What this PR does / why we need it:

This PR aims to allow LUN disks to be hotplugged, instead of simply allowing regular Disk types.

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 # https://bugzilla.redhat.com/show_bug.cgi?id=2211266

Special notes for your reviewer:

Release note:

Allow LUN disks to be hotplugged

kubevirt-bot · 2023-10-06T11:49:45Z

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

alromeros · 2023-10-06T11:51:07Z

/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.26-sig-storage

alicefr · 2023-10-09T08:36:55Z

/cc

alromeros · 2023-10-10T16:09:40Z

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

alromeros · 2023-10-11T10:57:03Z

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

alromeros · 2023-10-11T13:57:12Z

/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.26-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-storage

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2023-10-20T16:44:18Z

Hey @alicefr, I'm waiting to see how tests behave but this should be ready for review. Thanks!

alromeros · 2023-10-23T06:57:30Z

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

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-update-admitter_test.go

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter.go

pkg/virt-launcher/virtwrap/converter/converter.go

0xFelix · 2023-10-23T07:47:48Z

pkg/virtctl/vm/add_volume.go

 )

 var (
-	serial string
-	cache  string
+	serial   string


Can you avoid global vars? (possibly in a follow up)

All virtctl commands use (and require) global variables to store flags, I think we should follow that standard.

That is not true and IMO using global variables is not a very good standard.

e.g. virtctl create {vm|instancetype|preference} do neither use nor require them.

Aw, you are right, I checked our other commands and assumed they were all the same 🤦 Anyway this command uses some common global variables that would require a considerable refactor in all vm commands, so I prefer to leave that for another PR.

pkg/virtctl/vm/add_volume.go

pkg/virtctl/vm/add_volume_test.go

tests/storage/hotplug.go

0xFelix · 2023-10-23T07:57:37Z

tests/storage/hotplug.go

+
+			vm, err = virtClient.VirtualMachine(testsuite.GetTestNamespace(vmi)).Create(context.Background(), tests.NewRandomVirtualMachine(vmi, true))
+			Expect(err).ToNot(HaveOccurred())
+			Eventually(func() bool {


You could make beReady() from vm_test.go public and use it here to:

Eventually(ThisVM(vm), 300*time.Second, 1*time.Second).Should(Not(beReady()))

Prefer to leave that for a possible follow-up, there are several instances of this code block between all tests and for consistency I think it'd be better to take out all the beCreated, beReady, beRestarted, etc. functions.

Agreed, do you want to create an issue for the follow-ups?

alromeros · 2023-10-24T07:50:20Z

/retest-required

0xFelix

Thanks

/lgtm

0xFelix · 2023-10-24T11:36:39Z

tests/storage/hotplug.go

+
+			vm, err = virtClient.VirtualMachine(testsuite.GetTestNamespace(vmi)).Create(context.Background(), tests.NewRandomVirtualMachine(vmi, true))
+			Expect(err).ToNot(HaveOccurred())
+			Eventually(func() bool {


Agreed, do you want to create an issue for the follow-ups?

alicefr · 2023-10-24T13:57:41Z

pkg/virtctl/vm/add_volume.go

 		},
 		VolumeSource: volumeSource,
 		DryRun:       *dryRunOption,
 	}
+	if diskType == "" || diskType == "disk" {


Nit: you could replace this with a switch case

alicefr · 2023-10-24T14:19:43Z

tests/storage/hotplug.go

+			nodeName = tests.NodeNameWithHandler()
+			address, device = tests.CreateSCSIDisk(nodeName, []string{})
+			By(fmt.Sprintf("Create PVC with SCSI disk %s", device))
+			pv, pvc, err = tests.CreatePVandPVCwithSCSIDisk(nodeName, device, util.NamespaceTestDefault, "scsi-disks", "scsipv", "scsipvc")


Do the namespaces always match for util.NamespaceTestDefault and the testsuite.GetTestNamespace? Could happen that you create the PVC and the VM in 2 different namespaces?

I guess it shouldn't be a problem but will only use the former for consistency.

alicefr · 2023-10-24T14:21:05Z

tests/storage/hotplug.go

+			removeVolumeVM(vm.Name, vm.Namespace, testNewVolume1, false)
+			removeVolumeVM(vm.Name, vm.Namespace, testNewVolume2, false)
+
+			verifyVolumeAndDiskVMRemoved(vm, testNewVolume1, testNewVolume2)


Can we move this into the AfterEach?

I think having a different number of volumes between tests overcomplicates this a little bit and would look less nice, prefer to leave it like this.

alicefr · 2023-10-24T14:22:42Z

I left a couple of questions but the code looks good!
/lgtm

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alicefr · 2023-10-25T05:54:27Z

@alromeros thanks for the work, looks good!

/approve
/lgtm

kubevirt-bot · 2023-10-25T05:54:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicefr

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

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

alromeros · 2023-10-25T10:33:46Z

/test pull-kubevirt-e2e-k8s-1.26-sig-storage

alromeros · 2023-10-25T13:33:06Z

/test pull-kubevirt-e2e-k8s-1.26-sig-storage

alromeros · 2023-12-20T14:24:29Z

/cherrypick release-1.1

kubevirt-bot · 2023-12-20T14:25:11Z

@alromeros: new pull request created: #10909

In response to this:

/cherrypick 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.

kubevirt-bot requested review from kbidarkar and marceloamaral October 6, 2023 11:50

kubevirt-bot requested a review from alicefr October 9, 2023 08:36

alromeros force-pushed the allow-lun-hotplug branch from 93f9d6e to 94d1e87 Compare October 10, 2023 16:08

alromeros force-pushed the allow-lun-hotplug branch from 94d1e87 to b6500d9 Compare October 11, 2023 07:46

alromeros force-pushed the allow-lun-hotplug branch from b6500d9 to e806320 Compare October 20, 2023 11:54

Allow LUN disks to be hotplugged

fbbf3f9

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the allow-lun-hotplug branch from e806320 to fbbf3f9 Compare October 20, 2023 12:02

alromeros marked this pull request as ready for review October 20, 2023 12:02

kubevirt-bot requested a review from 0xFelix October 20, 2023 12:02

Add unit test coverage for LUN hotplugs

a460a29

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the allow-lun-hotplug branch from a55e5f5 to a460a29 Compare October 20, 2023 15:25

alromeros changed the title ~~[WIP] Allow LUN disks to be hotplugged~~ Allow LUN disks to be hotplugged Oct 20, 2023

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 Oct 20, 2023

0xFelix reviewed Oct 23, 2023

View reviewed changes

alromeros force-pushed the allow-lun-hotplug branch 2 times, most recently from 7b0c064 to 79d6f76 Compare October 24, 2023 10:26

0xFelix approved these changes Oct 24, 2023

View reviewed changes

kubevirt-bot assigned 0xFelix Oct 24, 2023

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

alicefr reviewed Oct 24, 2023

View reviewed changes

kubevirt-bot assigned alicefr Oct 24, 2023

Address review comments

e5f5c78

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the allow-lun-hotplug branch from 79d6f76 to e5f5c78 Compare October 24, 2023 16:50

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

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

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

kubevirt-bot merged commit f3f020d into kubevirt:main Oct 26, 2023
37 checks passed

alromeros mentioned this pull request Dec 20, 2023

Update volume-hotplug documentation to cover LUN support kubevirt/user-guide#747

Merged

kubevirt-bot mentioned this pull request Dec 20, 2023

[release-1.1] Allow LUN disks to be hotplugged #10909

Closed

chenrui333 mentioned this pull request Mar 6, 2024

virtctl 1.2.0 Homebrew/homebrew-core#165266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow LUN disks to be hotplugged #10529

Allow LUN disks to be hotplugged #10529

alromeros commented Oct 6, 2023 •

edited

kubevirt-bot commented Oct 6, 2023

alromeros commented Oct 6, 2023

alicefr commented Oct 9, 2023

alromeros commented Oct 10, 2023

alromeros commented Oct 11, 2023

alromeros commented Oct 11, 2023

alromeros commented Oct 20, 2023

alromeros commented Oct 23, 2023

0xFelix Oct 23, 2023

alromeros Oct 23, 2023

0xFelix Oct 24, 2023

alromeros Oct 24, 2023

0xFelix Oct 23, 2023

alromeros Oct 24, 2023

0xFelix Oct 24, 2023

alromeros commented Oct 24, 2023

0xFelix left a comment

0xFelix Oct 24, 2023

alicefr Oct 24, 2023

alicefr Oct 24, 2023 •

edited

alromeros Oct 24, 2023 •

edited

alicefr Oct 24, 2023

alromeros Oct 24, 2023

alicefr commented Oct 24, 2023

alicefr commented Oct 25, 2023

kubevirt-bot commented Oct 25, 2023

alromeros commented Oct 25, 2023

alromeros commented Oct 25, 2023

alromeros commented Dec 20, 2023

kubevirt-bot commented Dec 20, 2023

Allow LUN disks to be hotplugged #10529

Allow LUN disks to be hotplugged #10529

Conversation

alromeros commented Oct 6, 2023 • edited

kubevirt-bot commented Oct 6, 2023

alromeros commented Oct 6, 2023

alicefr commented Oct 9, 2023

alromeros commented Oct 10, 2023

alromeros commented Oct 11, 2023

alromeros commented Oct 11, 2023

alromeros commented Oct 20, 2023

alromeros commented Oct 23, 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

alromeros commented Oct 24, 2023

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr Oct 24, 2023 • edited

Choose a reason for hiding this comment

alromeros Oct 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr commented Oct 24, 2023

alicefr commented Oct 25, 2023

kubevirt-bot commented Oct 25, 2023

alromeros commented Oct 25, 2023

alromeros commented Oct 25, 2023

alromeros commented Dec 20, 2023

kubevirt-bot commented Dec 20, 2023

alromeros commented Oct 6, 2023 •

edited

alicefr Oct 24, 2023 •

edited

alromeros Oct 24, 2023 •

edited