Add support for USB Disks #8159

acardace · 2022-07-22T10:34:07Z

What this PR does / why we need it:

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 for USB disks

acardace · 2022-07-25T07:22:06Z

/retest-required

rmohr · 2022-07-26T09:06:24Z

Hey @acardace I am curious, what are the use-cases for usb disks?

acardace · 2022-08-03T07:46:42Z

Hey @acardace I am curious, what are the use-cases for usb disks?

Hi @rmohr, it's pretty much for completeness since you can construct such a VM with plain libvirt or qemu.

Also, it can be useful for os-level debugging or test purposes.

acardace · 2022-08-03T07:46:59Z

/retest-required

rmohr

Looks pretty good. Two minor things.

tests/storage/storage.go

rmohr · 2022-08-03T07:58:49Z

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

+	//In ppc64le usb devices like mouse / keyboard are set by default,
+	//so we can't disable the controller otherwise we run into the following error:
+	//"unsupported configuration: USB is disabled for this domain, but USB devices are present in the domain XML"
+	if !isAMD64(c.Architecture) {


@zhlhahaha I wonder if we thought about this place also for arm.

Yes, that'd be interesting, I've left it as it was but this check could be specialized for ppc64le if there's no need for it in other arches different than x86_64.

Usb storage is not supported by the qemu-kvm binary on Arm64, we only support following storage devices.

Storage devices: name "scsi-block", bus SCSI, desc "SCSI block device passthrough" name "scsi-cd", bus SCSI, desc "virtual SCSI CD-ROM" name "scsi-disk", bus SCSI, desc "virtual SCSI disk or CD-ROM (legacy)" name "scsi-generic", bus SCSI, desc "pass through generic scsi device (/dev/sg*)" name "scsi-hd", bus SCSI, desc "virtual SCSI disk" name "vhost-user-fs-device", bus virtio-bus name "vhost-user-fs-pci", bus PCI name "virtio-blk-device", bus virtio-bus name "virtio-blk-pci", bus PCI, alias "virtio-blk" name "virtio-blk-pci-non-transitional", bus PCI name "virtio-blk-pci-transitional", bus PCI name "virtio-scsi-device", bus virtio-bus name "virtio-scsi-pci", bus PCI, alias "virtio-scsi" name "virtio-scsi-pci-non-transitional", bus PCI name "virtio-scsi-pci-transitional", bus PCI

So I think USB Disks may not work on Arm64.

There are so many limitation caused by qemu-kvm binary on Arm64, so I think if it is possible to build qemu-kvm in a normal way rather than removing so many devices. @rmohr

@zhlhahaha do we need to fail admission of VMIs with USB disks on Arm64 then?

@zhlhahaha do we need to fail admission of VMIs with USB disks on Arm64 then?

I think so.

Ah! I think this PR will do: #7007

Ah! I think this PR will do: #7007

Yes

rmohr · 2022-08-03T08:02:21Z

Hey @acardace I am curious, what are the use-cases for usb disks?

Hi @rmohr, it's pretty much for completeness since you can construct such a VM with plain libvirt or qemu.

Also, it can be useful for os-level debugging or test purposes.

Thanks for the details, I was hoping for some more concrete examples where we saw someone blocked or where adding an usb disk upfront helped debugging somehow. Do you know about some cases?

acardace · 2022-08-03T08:11:38Z

Hey @acardace I am curious, what are the use-cases for usb disks?

Hi @rmohr, it's pretty much for completeness since you can construct such a VM with plain libvirt or qemu.
Also, it can be useful for os-level debugging or test purposes.

Thanks for the details, I was hoping for some more concrete examples where we saw someone blocked or where adding an usb disk upfront helped debugging somehow. Do you know about some cases?

The best example I can think of is debugging a USB device kernel driver or the USB emulation code in qemu.

rmohr · 2022-08-03T09:13:19Z

/lgtm

lgtm for the code.

Not approving yet. Adding @davidvossel and @vladikr to hear their opinion about this, since we don't seem to have an immediate use-case for this.

rmohr · 2022-08-03T09:14:10Z

/cc @davidvossel
/cc @vladikr

acardace · 2022-08-03T09:34:52Z

/test pull-kubevirt-e2e-k8s-1.22-sig-compute-migrations

tests/migration_test.go

jean-edouard

Code LGTM, just a couple concerns about ARM and comment wording.
Also, functional tests are failing because of an obsolete helper function.

jean-edouard · 2022-08-15T13:54:07Z

tests/migration_test.go

+				)
+
+				By("Starting the VirtualMachineInstance")
+				vmi = runVMIAndExpectLaunch(vmi, 240)


@acardace this function doesn't exist anymore, which is why the tests fail

thanks, didn't rebase against master in my local workspace :)

jean-edouard · 2022-08-15T13:55:54Z

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

-
-	//usb controller is turned on, only when user specify input device with usb bus,
-	//otherwise it is turned off
-	//In ppc64le usb devices like mouse / keyboard are set by default,


This comment seems to assume that the only USB devices are input devices, which is not true anymore.
It's also hard to read, probably worth reworking...

I just removed the first 2 lines, thanks for noticing that.

Signed-off-by: Antonio Cardace <acardace@redhat.com>

jean-edouard · 2022-08-17T12:35:03Z

Legit failure: https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/8159/pull-kubevirt-e2e-k8s-1.23-sig-compute-migrations/1559846596550594560
The test needs to be adjusted for the new disk.

With this patch volumes can be attached to a VMI as USB mass storage devices. adjust existing Disk tests and add a new e2e one specific to USB disks. Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace · 2022-08-19T07:29:26Z

/retest

jean-edouard

/approve

kubevirt-bot · 2022-08-22T13:07:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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~~ [jean-edouard]

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

acardace · 2022-08-22T13:54:03Z

@rmohr can you take another look? I just had to rebase pretty much.

rmohr · 2022-08-22T15:17:10Z

Seems like no objections to adding usb disk support.

/lgtm

rmohr · 2022-08-22T15:18:05Z

@rmohr can you take another look? I just had to rebase pretty much.

Will probably compete with #7007. Let's see who wins the race :)

acardace · 2022-08-22T20:39:10Z

/retest

kubevirt-bot · 2022-08-22T20:47:15Z

@acardace: 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-fossa	`2579ebd`	link	false	`/test pull-kubevirt-fossa`

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 · 2022-08-23T00:19:53Z

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

acardace · 2022-08-23T08:59:27Z

@rmohr can you take another look? I just had to rebase pretty much.

Will probably compete with #7007. Let's see who wins the race :)

Thank you for taking a look, btw won :)

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. size/L labels Jul 22, 2022

kubevirt-bot requested review from alicefr and kbidarkar July 22, 2022 11:32

rmohr reviewed Aug 3, 2022

View reviewed changes

acardace force-pushed the usb-disks branch from 253f399 to 25ab37c Compare August 3, 2022 09:09

rmohr added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2022

kubevirt-bot requested review from davidvossel and vladikr August 3, 2022 09:14

vladikr reviewed Aug 3, 2022

View reviewed changes

tests/migration_test.go Outdated Show resolved Hide resolved

acardace force-pushed the usb-disks branch from 25ab37c to 310242a Compare August 5, 2022 08:21

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

jean-edouard reviewed Aug 15, 2022

View reviewed changes

libvmi: add WithEmptyDisk Option

ecf5979

Signed-off-by: Antonio Cardace <acardace@redhat.com>

acardace force-pushed the usb-disks branch from 310242a to 9198aed Compare August 17, 2022 10:16

acardace force-pushed the usb-disks branch from 9198aed to 2579ebd Compare August 18, 2022 12:22

Add support for USB Disks

2579ebd

With this patch volumes can be attached to a VMI as USB mass storage devices. adjust existing Disk tests and add a new e2e one specific to USB disks. Signed-off-by: Antonio Cardace <acardace@redhat.com>

jean-edouard reviewed Aug 22, 2022

View reviewed changes

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

acardace requested review from rmohr and removed request for kbidarkar, davidvossel and alicefr August 22, 2022 13:53

kubevirt-bot assigned rmohr Aug 22, 2022

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

kubevirt-bot merged commit 3fe3b04 into kubevirt:main Aug 23, 2022

kubevirt-bot mentioned this pull request Sep 14, 2022

Macvtap binding mode for pod network #7648

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for USB Disks #8159

Add support for USB Disks #8159

acardace commented Jul 22, 2022

acardace commented Jul 25, 2022

rmohr commented Jul 26, 2022

acardace commented Aug 3, 2022

acardace commented Aug 3, 2022

rmohr left a comment

rmohr Aug 3, 2022

acardace Aug 3, 2022

zhlhahaha Aug 3, 2022 •

edited

zhlhahaha Aug 3, 2022

jean-edouard Aug 15, 2022

zhlhahaha Aug 15, 2022

jean-edouard Aug 16, 2022

zhlhahaha Aug 17, 2022

rmohr commented Aug 3, 2022 •

edited

acardace commented Aug 3, 2022

rmohr commented Aug 3, 2022 •

edited

rmohr commented Aug 3, 2022

acardace commented Aug 3, 2022

jean-edouard left a comment

jean-edouard Aug 15, 2022

acardace Aug 17, 2022

jean-edouard Aug 15, 2022

acardace Aug 17, 2022

jean-edouard commented Aug 17, 2022

acardace commented Aug 19, 2022

jean-edouard left a comment

kubevirt-bot commented Aug 22, 2022

acardace commented Aug 22, 2022

rmohr commented Aug 22, 2022

rmohr commented Aug 22, 2022

acardace commented Aug 22, 2022

kubevirt-bot commented Aug 22, 2022 •

edited

kubevirt-commenter-bot commented Aug 23, 2022

acardace commented Aug 23, 2022

Add support for USB Disks #8159

Add support for USB Disks #8159

Conversation

acardace commented Jul 22, 2022

acardace commented Jul 25, 2022

rmohr commented Jul 26, 2022

acardace commented Aug 3, 2022

acardace commented Aug 3, 2022

rmohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhlhahaha Aug 3, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohr commented Aug 3, 2022 • edited

acardace commented Aug 3, 2022

rmohr commented Aug 3, 2022 • edited

rmohr commented Aug 3, 2022

acardace commented Aug 3, 2022

jean-edouard 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

Choose a reason for hiding this comment

jean-edouard commented Aug 17, 2022

acardace commented Aug 19, 2022

jean-edouard left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Aug 22, 2022

acardace commented Aug 22, 2022

rmohr commented Aug 22, 2022

rmohr commented Aug 22, 2022

acardace commented Aug 22, 2022

kubevirt-bot commented Aug 22, 2022 • edited

kubevirt-commenter-bot commented Aug 23, 2022

acardace commented Aug 23, 2022

zhlhahaha Aug 3, 2022 •

edited

rmohr commented Aug 3, 2022 •

edited

rmohr commented Aug 3, 2022 •

edited

kubevirt-bot commented Aug 22, 2022 •

edited