[V2V PATCH 0/5] Bring support for virtio-scsi back to Windows

[nbdkit PATCH 0/9] common: catch...

Checksums and other verification

Andrey Drobyshev

Wednesday, 22 February 2023 Wed, 22 Feb '23

12:20 p.m.

Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 ("Remove guestcaps_block_type Virtio_SCSI") support for installing virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for bringing this feature back have been out there for a while. E.g. I've found a corresponding issue which is still open [1]. The code in b28cd1dc, f0afc439 was removed due to removing the old in-place support. However, having the new in-place support present and bringing this same code (partially) back with several additions and improvements, I'm able to successfully convert and boot a Win guest with a virtio-scsi disk controller. So please consider the following implementation of this feature. [1] https://github.com/libguestfs/virt-v2v/issues/12 v2v: Andrey Drobyshev (2): Revert "Remove guestcaps_block_type Virtio_SCSI" convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block type convert/convert.ml | 2 +- convert/convert_linux.ml | 9 +++++++-- convert/convert_windows.ml | 1 + convert/target_bus_assignment.ml | 1 + lib/create_ovf.ml | 1 + lib/types.ml | 3 ++- lib/types.mli | 2 +- output/openstack_image_properties.ml | 7 +++++++ 9 files changed, 22 insertions(+), 6 deletions(-) common: Andrey Drobyshev (2): inject_virtio_win: add Virtio_SCSI to block_type inject_virtio_win: make virtio-scsi the default block driver Roman Kagan (1): inject_virtio_win: match only vendor/device mlcustomize/inject_virtio_win.ml | 25 ++++++++++++++++--------- mlcustomize/inject_virtio_win.mli | 2 +- 2 files changed, 17 insertions(+), 10 deletions(-) -- 2.31.1

Show replies by date

Andrey Drobyshev

Wednesday, 22 February Wed, 22 Feb

12:20 p.m.

New subject: [V2V PATCH 1/5] Revert "Remove guestcaps_block_type Virtio_SCSI"

This code is needed to check whether virtio-scsi driver was installed. This reverts commit f0afc439524853508938b2bfc758896f053462e3. --- convert/convert.ml | 2 +- convert/convert_linux.ml | 9 +++++++-- convert/target_bus_assignment.ml | 1 + lib/create_ovf.ml | 1 + lib/types.ml | 3 ++- lib/types.mli | 2 +- output/openstack_image_properties.ml | 7 +++++++ 7 files changed, 20 insertions(+), 5 deletions(-) diff --git a/convert/convert.ml b/convert/convert.ml index 0aa0e5cd..084619c8 100644 --- a/convert/convert.ml +++ b/convert/convert.ml @@ -252,7 +252,7 @@ and do_convert g source inspect i_firmware keep_serial_console interfaces = (* Did we manage to install virtio drivers? *) if not (quiet ()) then ( match guestcaps.gcaps_block_bus with - | Virtio_blk -> + | Virtio_blk | Virtio_SCSI -> info (f_"This guest has virtio drivers installed.") | IDE -> info (f_"This guest does not have virtio drivers installed.") diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml index d5c0f24d..dab4f36d 100644 --- a/convert/convert_linux.ml +++ b/convert/convert_linux.ml @@ -1068,8 +1068,12 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = (* Update 'alias scsi_hostadapter ...' *) let paths = augeas_modprobe ". =~ regexp('scsi_hostadapter.*')" in (match block_type with - | Virtio_blk -> - let block_module = "virtio_blk" in + | Virtio_blk | Virtio_SCSI -> + let block_module = + match block_type with + | Virtio_blk -> "virtio_blk" + | Virtio_SCSI -> "virtio_scsi" + | IDE -> assert false in if paths <> [] then ( (* There's only 1 scsi controller in the converted guest. @@ -1142,6 +1146,7 @@ let convert (g : G.guestfs) source inspect i_firmware keep_serial_console _ = let block_prefix_after_conversion = match block_type with | Virtio_blk -> "vd" + | Virtio_SCSI -> "sd" | IDE -> ide_block_prefix in let map = diff --git a/convert/target_bus_assignment.ml b/convert/target_bus_assignment.ml index 54c9516b..d13340c7 100644 --- a/convert/target_bus_assignment.ml +++ b/convert/target_bus_assignment.ml @@ -35,6 +35,7 @@ let rec target_bus_assignment source_disks source_removables guestcaps = let bus = match guestcaps.gcaps_block_bus with | Virtio_blk -> virtio_blk_bus + | Virtio_SCSI -> scsi_bus | IDE -> ide_bus in List.iteri ( fun i d -> diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index 5e444868..f0b32e01 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -920,6 +920,7 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-interface", (match guestcaps.gcaps_block_bus with | Virtio_blk -> "VirtIO" + | Virtio_SCSI -> "VirtIO_SCSI" | IDE -> "IDE"); "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; diff --git a/lib/types.ml b/lib/types.ml index e16da007..75c14fd4 100644 --- a/lib/types.ml +++ b/lib/types.ml @@ -400,12 +400,13 @@ type guestcaps = { gcaps_arch_min_version : int; gcaps_virtio_1_0 : bool; } -and guestcaps_block_type = Virtio_blk | IDE +and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_machine = I440FX | Q35 | Virt let string_of_block_type = function | Virtio_blk -> "virtio-blk" + | Virtio_SCSI -> "virtio-scsi" | IDE -> "ide" let string_of_net_type = function | Virtio_net -> "virtio-net" diff --git a/lib/types.mli b/lib/types.mli index 4a183dd3..65ef2e35 100644 --- a/lib/types.mli +++ b/lib/types.mli @@ -280,7 +280,7 @@ type guestcaps = { } (** Guest capabilities after conversion. eg. Was virtio found or installed? *) -and guestcaps_block_type = Virtio_blk | IDE +and guestcaps_block_type = Virtio_blk | Virtio_SCSI | IDE and guestcaps_net_type = Virtio_net | E1000 | RTL8139 and guestcaps_machine = I440FX | Q35 | Virt diff --git a/output/openstack_image_properties.ml b/output/openstack_image_properties.ml index c75d72fe..c76ad913 100644 --- a/output/openstack_image_properties.ml +++ b/output/openstack_image_properties.ml @@ -35,6 +35,7 @@ let create source inspect { target_buses; guestcaps; target_firmware } = "hw_disk_bus", (match guestcaps.gcaps_block_bus with | Virtio_blk -> "virtio" + | Virtio_SCSI -> "scsi" | IDE -> "ide"); "hw_vif_model", (match guestcaps.gcaps_net_bus with @@ -69,6 +70,12 @@ let create source inspect { target_buses; guestcaps; target_firmware } = List.push_back properties ("hw_cpu_threads", string_of_int threads); ); + (match guestcaps.gcaps_block_bus with + | Virtio_SCSI -> + List.push_back properties ("hw_scsi_model", "virtio-scsi") + | Virtio_blk | IDE -> () + ); + (match inspect.i_major_version, inspect.i_minor_version with | 0, 0 -> () | x, 0 -> List.push_back properties ("os_version", string_of_int x) -- 2.31.1

Andrey Drobyshev

12:20 p.m.

New subject: [COMMON PATCH 3/5] inject_virtio_win: match only vendor/device

From: Roman Kagan <rkagan(a)virtuozzo.com> Since different hypervisor vendors are allowed to use their own vendor-id as PCI subsystem-vendor-id for virtio devices, during v2v conversion it makes sense to only match the vendor/device and not the full device "path" in the Windows registry. This way the code will remain universal but will work for different hypervisor vendors. Signed-off-by: Roman Kagan <rkagan(a)virtuozzo.com> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com> --- mlcustomize/inject_virtio_win.ml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml index 5f4aab7..acb3a93 100644 --- a/mlcustomize/inject_virtio_win.ml +++ b/mlcustomize/inject_virtio_win.ml @@ -110,10 +110,10 @@ and get_inspection g root = virtio_win = ""; was_set = false } let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}" -let viostor_legacy_pciid = "VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00" -let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01" -let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00" -let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01" +let viostor_legacy_pciid = "VEN_1AF4&DEV_1001" +let viostor_modern_pciid = "VEN_1AF4&DEV_1042" +let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004" +let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048" let rec inject_virtio_win_drivers ({ g } as t) reg = (* Copy the virtio drivers to the guest. *) -- 2.31.1

Andrey Drobyshev

12:20 p.m.

New subject: [COMMON PATCH 4/5] inject_virtio_win: add Virtio_SCSI to block_type

Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com> --- mlcustomize/inject_virtio_win.ml | 2 +- mlcustomize/inject_virtio_win.mli | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml index acb3a93..62f7710 100644 --- a/mlcustomize/inject_virtio_win.ml +++ b/mlcustomize/inject_virtio_win.ml @@ -51,7 +51,7 @@ type t = { the user to select where they want to get drivers from. XXX *) } -type block_type = Virtio_blk | IDE +type block_type = Virtio_blk | Virtio_SCSI | IDE and net_type = Virtio_net | E1000 | RTL8139 and machine_type = I440FX | Q35 | Virt diff --git a/mlcustomize/inject_virtio_win.mli b/mlcustomize/inject_virtio_win.mli index 0ced02e..58169e1 100644 --- a/mlcustomize/inject_virtio_win.mli +++ b/mlcustomize/inject_virtio_win.mli @@ -20,7 +20,7 @@ type t (** Handle *) -type block_type = Virtio_blk | IDE +type block_type = Virtio_blk | Virtio_SCSI | IDE and net_type = Virtio_net | E1000 | RTL8139 and machine_type = I440FX | Q35 | Virt -- 2.31.1

Andrey Drobyshev

12:20 p.m.

New subject: [COMMON PATCH 5/5] inject_virtio_win: make virtio-scsi the default block driver

If virtio-scsi is present among the tools for this version of Windows, prefer it over virtio-blk. Originally-by: Roman Kagan <rkagan(a)virtuozzo.com> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com> --- mlcustomize/inject_virtio_win.ml | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml index 62f7710..751a9b0 100644 --- a/mlcustomize/inject_virtio_win.ml +++ b/mlcustomize/inject_virtio_win.ml @@ -176,7 +176,8 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = else ( (* Can we install the block driver? *) let block : block_type = - let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in + (* First list item is the default driver. *) + let filenames = ["vioscsi"; "virtio_blk"; "vrtioblk"; "viostor"] in let viostor_driver = try ( Some ( List.find ( @@ -200,10 +201,16 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = let target = sprintf "%s/system32/drivers/%s.sys" t.i_windows_systemroot driver_name in let target = g#case_sensitive_path target in + let legacy_pciid, modern_pciid = ( + if driver_name = "vioscsi" then + vioscsi_legacy_pciid, vioscsi_modern_pciid + else + viostor_legacy_pciid, viostor_modern_pciid + ) in g#cp source target; - add_guestor_to_registry t reg driver_name viostor_legacy_pciid; - add_guestor_to_registry t reg driver_name viostor_modern_pciid; - Virtio_blk in + add_guestor_to_registry t reg driver_name legacy_pciid; + add_guestor_to_registry t reg driver_name modern_pciid; + (if driver_name = "vioscsi" then Virtio_SCSI else Virtio_blk) in (* Can we install the virtio-net driver? *) let net : net_type = -- 2.31.1

Laszlo Ersek

Thursday, 23 February Thu, 23 Feb

4:43 a.m.

On 2/22/23 19:20, Andrey Drobyshev wrote:

...

(Preamble: I'm 100% deferring to Rich on this, so take my comments for what they are worth.) In my opinion, the argument made is weak. This cover letter does not say "why" -- it does not explain why virtio-blk is insufficient for *Virtuozzo*. Second, reference [1] -- issue #12 -- doesn't sound too convincing. It writes, "opinionated qemu-based VMs that exclusively use UEFI and only virtio devices". "Opinionated" is the key word there. They're entitled to an opinion, they're not entitled to others conforming to their opinion. I happen to be opinionated as well, and I hold the opposite view. (BTW even if they insist on UEFI + virtio, which I do sympathize with, requiring virtio-scsi exclusively is hard to sell. In particular, virtio-blk nowadays has support for trim/discard, so the main killer feature of virtio-scsi is no longer unique to virtio-scsi. Virtio-blk is also simpler code and arguably faster. Note: I don't want to convince anyone about what *they* support, just pointing out that virt-v2v outputting solely virtio-blk disks is entirely fine, as far as virt-v2v's mission is concerned -- "salvage 'pet' (not 'cattle') VMs from proprietary hypervisors, and make sure they boot". Virtio-blk is sufficient for booting, further tweaks are up to the admin (again, virt-v2v is not for mass/cattle conversions). The "Hetzner Cloud" is not a particular output module of virt-v2v, so I don't know why virt-v2v's mission should extend to making the converted VM bootable on "Hetzner Cloud".) Rich has recently added tools for working with the virtio devices in windows guests; maybe those can be employed as extra (manual) steps before or after the conversion. Third, the last patch in the series is overreaching IMO; it switches the default. That causes a behavior change for such conversions that have been working well, and have been thoroughly tested. It doesn't just add a new use case, it throws away an existent use case for the new one's sake, IIUC. I don't like that. Again -- fully deferring to Rich on the final verdict (and the review). Laszlo

...

v2v: Andrey Drobyshev (2): Revert "Remove guestcaps_block_type Virtio_SCSI" convert_windows: add Inject_virtio_win.Virtio_SCSI as a possible block type convert/convert.ml | 2 +- convert/convert_linux.ml | 9 +++++++-- convert/convert_windows.ml | 1 + convert/target_bus_assignment.ml | 1 + lib/create_ovf.ml | 1 + lib/types.ml | 3 ++- lib/types.mli | 2 +- output/openstack_image_properties.ml | 7 +++++++ 9 files changed, 22 insertions(+), 6 deletions(-) common: Andrey Drobyshev (2): inject_virtio_win: add Virtio_SCSI to block_type inject_virtio_win: make virtio-scsi the default block driver Roman Kagan (1): inject_virtio_win: match only vendor/device mlcustomize/inject_virtio_win.ml | 25 ++++++++++++++++--------- mlcustomize/inject_virtio_win.mli | 2 +- 2 files changed, 17 insertions(+), 10 deletions(-) -- 2.31.1

Richard W.M. Jones

4:56 a.m.

I'm probably not going to get around to looking at this before next week. But just wanted to say that while switching the default probably won't be acceptable, adding support for virtio-scsi may be OK. I'll look at the patches later! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

Denis V. Lunev

5:48 a.m.

On 2/23/23 11:43, Laszlo Ersek wrote:

...

On 2/22/23 19:20, Andrey Drobyshev wrote: > Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 > ("Remove guestcaps_block_type Virtio_SCSI") support for installing > virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for > bringing this feature back have been out there for a while. E.g. I've > found a corresponding issue which is still open [1]. > > The code in b28cd1dc, f0afc439 was removed due to removing the old in-place > support. However, having the new in-place support present and bringing > this same code (partially) back with several additions and improvements, > I'm able to successfully convert and boot a Win guest with a virtio-scsi > disk controller. So please consider the following implementation of > this feature. > > [1] https://github.com/libguestfs/virt-v2v/issues/12 (Preamble: I'm 100% deferring to Rich on this, so take my comments for what they are worth.) In my opinion, the argument made is weak. This cover letter does not say "why" -- it does not explain why virtio-blk is insufficient for *Virtuozzo*. Second, reference [1] -- issue #12 -- doesn't sound too convincing. It writes, "opinionated qemu-based VMs that exclusively use UEFI and only virtio devices". "Opinionated" is the key word there. They're entitled to an opinion, they're not entitled to others conforming to their opinion. I happen to be opinionated as well, and I hold the opposite view. (BTW even if they insist on UEFI + virtio, which I do sympathize with, requiring virtio-scsi exclusively is hard to sell. In particular, virtio-blk nowadays has support for trim/discard, so the main killer feature of virtio-scsi is no longer unique to virtio-scsi. Virtio-blk is also simpler code and arguably faster. Note: I don't want to convince anyone about what *they* support, just pointing out that virt-v2v outputting solely virtio-blk disks is entirely fine, as far as virt-v2v's mission is concerned -- "salvage 'pet' (not 'cattle') VMs from proprietary hypervisors, and make sure they boot". Virtio-blk is sufficient for booting, further tweaks are up to the admin (again, virt-v2v is not for mass/cattle conversions). The "Hetzner Cloud" is not a particular output module of virt-v2v, so I don't know why virt-v2v's mission should extend to making the converted VM bootable on "Hetzner Cloud".) Rich has recently added tools for working with the virtio devices in windows guests; maybe those can be employed as extra (manual) steps before or after the conversion. Third, the last patch in the series is overreaching IMO; it switches the default. That causes a behavior change for such conversions that have been working well, and have been thoroughly tested. It doesn't just add a new use case, it throws away an existent use case for the new one's sake, IIUC. I don't like that. Again -- fully deferring to Rich on the final verdict (and the review). Laszlo

OK. Let me clarify the situation a bit. These patches (for sure) originates from good old 2017 year, when VirtIO BLK was completely unacceptable to us due to missed discard feature which is now in. Thus you are completely right about the default and default changing (if that will happen) should be in the separate patch. Anyway, from the first glance it should not be needed. Normally, in inplace mode, which we are mostly worrying about v2v should bring the guest configuration in sync with what is written in domain.xml and that does not involve any defaults. VirtIO SCSI should be supported as users should have a freedom to choose between VirtIO SCSI and VirtIO BLK even after the guest installation. Does this sounds acceptable? Den

Laszlo Ersek

10:56 p.m.

On 2/23/23 12:48, Denis V. Lunev wrote:

...

On 2/23/23 11:43, Laszlo Ersek wrote: > On 2/22/23 19:20, Andrey Drobyshev wrote: >> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >> bringing this feature back have been out there for a while. E.g. I've >> found a corresponding issue which is still open [1]. >> >> The code in b28cd1dc, f0afc439 was removed due to removing the old >> in-place >> support. However, having the new in-place support present and bringing >> this same code (partially) back with several additions and improvements, >> I'm able to successfully convert and boot a Win guest with a virtio-scsi >> disk controller. So please consider the following implementation of >> this feature. >> >> [1] https://github.com/libguestfs/virt-v2v/issues/12 > (Preamble: I'm 100% deferring to Rich on this, so take my comments for > what they are worth.) > > In my opinion, the argument made is weak. This cover letter does not say > "why" -- it does not explain why virtio-blk is insufficient for > *Virtuozzo*. > > Second, reference [1] -- issue #12 -- doesn't sound too convincing. It > writes, "opinionated qemu-based VMs that exclusively use UEFI and only > virtio devices". "Opinionated" is the key word there. They're entitled > to an opinion, they're not entitled to others conforming to their > opinion. I happen to be opinionated as well, and I hold the opposite > view. > > (BTW even if they insist on UEFI + virtio, which I do sympathize with, > requiring virtio-scsi exclusively is hard to sell. In particular, > virtio-blk nowadays has support for trim/discard, so the main killer > feature of virtio-scsi is no longer unique to virtio-scsi. Virtio-blk is > also simpler code and arguably faster. Note: I don't want to convince > anyone about what *they* support, just pointing out that virt-v2v > outputting solely virtio-blk disks is entirely fine, as far as > virt-v2v's mission is concerned -- "salvage 'pet' (not 'cattle') VMs > from proprietary hypervisors, and make sure they boot". Virtio-blk is > sufficient for booting, further tweaks are up to the admin (again, > virt-v2v is not for mass/cattle conversions). The "Hetzner Cloud" is not > a particular output module of virt-v2v, so I don't know why virt-v2v's > mission should extend to making the converted VM bootable on "Hetzner > Cloud".) > > Rich has recently added tools for working with the virtio devices in > windows guests; maybe those can be employed as extra (manual) steps > before or after the conversion. > > Third, the last patch in the series is overreaching IMO; it switches the > default. That causes a behavior change for such conversions that have > been working well, and have been thoroughly tested. It doesn't just add > a new use case, it throws away an existent use case for the new one's > sake, IIUC. I don't like that. > > Again -- fully deferring to Rich on the final verdict (and the review). > > Laszlo OK. Let me clarify the situation a bit. These patches (for sure) originates from good old 2017 year, when VirtIO BLK was completely unacceptable to us due to missed discard feature which is now in. Thus you are completely right about the default and default changing (if that will happen) should be in the separate patch. Anyway, from the first glance it should not be needed. Normally, in inplace mode, which we are mostly worrying about v2v should bring the guest configuration in sync with what is written in domain.xml and that does not involve any defaults. VirtIO SCSI should be supported as users should have a freedom to choose between VirtIO SCSI and VirtIO BLK even after the guest installation. Does this sounds acceptable?

I've got zero experience with in-place conversions. I've skimmed <https://libguestfs.org/virt-v2v-in-place.1.html> now, but the use case continues to elude me. What is in-place conversion good for? If you already have a libvirt domain XML (i.e., one *not* output by virt-v2v as the result of a conversion from a foreign hypervisor), what do you need virt-v2v-in-place for? My understanding is that virt-v2v produces both an output disk (set) and a domain description (be it a QEMU cmdline, a libvirt domain XML, an OVF, ...), *and* that these two kinds of output belong together, there is not one without the other. What's the data flow with inplace conversion? Laszlo

Denis V. Lunev

Friday, 24 February Fri, 24 Feb

5:55 a.m.

On 2/24/23 05:56, Laszlo Ersek wrote:

...

On 2/23/23 12:48, Denis V. Lunev wrote: > On 2/23/23 11:43, Laszlo Ersek wrote: >> On 2/22/23 19:20, Andrey Drobyshev wrote: >>> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >>> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >>> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >>> bringing this feature back have been out there for a while. E.g. I've >>> found a corresponding issue which is still open [1]. >>> >>> The code in b28cd1dc, f0afc439 was removed due to removing the old >>> in-place >>> support. However, having the new in-place support present and bringing >>> this same code (partially) back with several additions and improvements, >>> I'm able to successfully convert and boot a Win guest with a virtio-scsi >>> disk controller. So please consider the following implementation of >>> this feature. >>> >>> [1] https://github.com/libguestfs/virt-v2v/issues/12 >> (Preamble: I'm 100% deferring to Rich on this, so take my comments for >> what they are worth.) >> >> In my opinion, the argument made is weak. This cover letter does not say >> "why" -- it does not explain why virtio-blk is insufficient for >> *Virtuozzo*. >> >> Second, reference [1] -- issue #12 -- doesn't sound too convincing. It >> writes, "opinionated qemu-based VMs that exclusively use UEFI and only >> virtio devices". "Opinionated" is the key word there. They're entitled >> to an opinion, they're not entitled to others conforming to their >> opinion. I happen to be opinionated as well, and I hold the opposite >> view. >> >> (BTW even if they insist on UEFI + virtio, which I do sympathize with, >> requiring virtio-scsi exclusively is hard to sell. In particular, >> virtio-blk nowadays has support for trim/discard, so the main killer >> feature of virtio-scsi is no longer unique to virtio-scsi. Virtio-blk is >> also simpler code and arguably faster. Note: I don't want to convince >> anyone about what *they* support, just pointing out that virt-v2v >> outputting solely virtio-blk disks is entirely fine, as far as >> virt-v2v's mission is concerned -- "salvage 'pet' (not 'cattle') VMs >> from proprietary hypervisors, and make sure they boot". Virtio-blk is >> sufficient for booting, further tweaks are up to the admin (again, >> virt-v2v is not for mass/cattle conversions). The "Hetzner Cloud" is not >> a particular output module of virt-v2v, so I don't know why virt-v2v's >> mission should extend to making the converted VM bootable on "Hetzner >> Cloud".) >> >> Rich has recently added tools for working with the virtio devices in >> windows guests; maybe those can be employed as extra (manual) steps >> before or after the conversion. >> >> Third, the last patch in the series is overreaching IMO; it switches the >> default. That causes a behavior change for such conversions that have >> been working well, and have been thoroughly tested. It doesn't just add >> a new use case, it throws away an existent use case for the new one's >> sake, IIUC. I don't like that. >> >> Again -- fully deferring to Rich on the final verdict (and the review). >> >> Laszlo > OK. Let me clarify the situation a bit. > > These patches (for sure) originates from good old 2017 year, when > VirtIO BLK was completely unacceptable to us due to missed > discard feature which is now in. > > Thus you are completely right about the default and default > changing (if that will happen) should be in the separate patch. > Anyway, from the first glance it should not be needed. > > Normally, in inplace mode, which we are mostly worrying > about v2v should bring the guest configuration in sync > with what is written in domain.xml and that does not involve > any defaults. > > VirtIO SCSI should be supported as users should have > a freedom to choose between VirtIO SCSI and VirtIO BLK > even after the guest installation. > > Does this sounds acceptable? I've got zero experience with in-place conversions. I've skimmed <https://libguestfs.org/virt-v2v-in-place.1.html> now, but the use case continues to elude me. What is in-place conversion good for? If you already have a libvirt domain XML (i.e., one *not* output by virt-v2v as the result of a conversion from a foreign hypervisor), what do you need virt-v2v-in-place for? My understanding is that virt-v2v produces both an output disk (set) and a domain description (be it a QEMU cmdline, a libvirt domain XML, an OVF, ...), *and* that these two kinds of output belong together, there is not one without the other. What's the data flow with inplace conversion? Laszlo

We use v2v as guest convertor engine and prepare VM configuration ourselves. This looks more appropriate for us as we have different constraints under different conditions. This makes sense outside of foreign hypervisor as we could change bus of the disk and then call v2v to teach the guest to boot from new location. This was revealed very useful to fix some strange issues on the customer's side. That is it. Den P.S. Resent (original mail was accidentally sent off-list)

Laszlo Ersek

6:02 a.m.

On 2/24/23 12:55, Denis V. Lunev wrote:

...

On 2/24/23 05:56, Laszlo Ersek wrote:

...

> I've got zero experience with in-place conversions. I've skimmed > <https://libguestfs.org/virt-v2v-in-place.1.html> now, but the use case > continues to elude me. > > What is in-place conversion good for? If you already have a libvirt > domain XML (i.e., one *not* output by virt-v2v as the result of a > conversion from a foreign hypervisor), what do you need > virt-v2v-in-place for? > > My understanding is that virt-v2v produces both an output disk (set) and > a domain description (be it a QEMU cmdline, a libvirt domain XML, an > OVF, ...), *and* that these two kinds of output belong together, there > is not one without the other. What's the data flow with inplace > conversion? > > Laszlo > We use v2v as guest convertor engine and prepare VM configuration ourselves. This looks more appropriate for us as we have different constraints under different conditions. This makes sense outside of foreign hypervisor as we could change bus of the disk and then call v2v to teach the guest to boot from new location. This was revealed very useful to fix some strange issues on the customer's side. That is it. Den P.S. Resent (original mail was accidentally sent off-list)

So the use case is more or less "-i libvirtxml", with the domain XML created from scratch (or liberally tweaked, starting from a "more authentic" original domain XML). The cover letter mentions the following commit: commit b28cd1dcfeb40e7002e8d0b0ce9dcc4ce86beb6c Author: Richard W.M. Jones <rjones(a)redhat.com> Date: Mon Nov 8 09:00:20 2021 +0000 Remove requested_guestcaps / rcaps This was part of the old in-place support. When we add new in-place support we'll do something else, but currently this is dead code so remove it completely. Note this removes the code for installing the virtio-scsi driver (only ever using virtio-blk). This was also dead code in the current implementation of virt-v2v, but worth remembering in case we want to resurrect this feature in a future version. Acked-by: Laszlo Ersek <lersek(a)redhat.com> So I guess that "something else" is what I'm missing here. Right now the proposed use case seems to be: - tweak the domain XML in some way (or generate it in some "bespoke" way from the outset) - perform an in-place conversion with virt-v2v, making sure that virt-v2v prefers virtio-scsi as first priority. This will end up injecting the virtio-scsi guest driver into the guest. This looks less than ideal to me. Even if we offer virtio-scsi, that should be driven by a specific knob. I initially thought that knob could be a new command line option. Then, upon reading we could change bus of the disk and then call v2v to teach the guest to boot from new location I thought that the contents of the tweaked domain XML would *steer* the guest driver selection. (This seemed plausible, because we used to have "source" domain properties, and I vaguely recalled that they once had been relevant for in-place conversion.) But these patches don't do any such steering, AFAICT. Why is the "rcaps" logic from before commit b28cd1dc not being brought back? Laszlo

Andrey Drobyshev

10:11 a.m.

On 2/24/23 14:02, Laszlo Ersek wrote:

...

On 2/24/23 12:55, Denis V. Lunev wrote: > On 2/24/23 05:56, Laszlo Ersek wrote: >> I've got zero experience with in-place conversions. I've skimmed >> <https://libguestfs.org/virt-v2v-in-place.1.html> now, but the use case >> continues to elude me. >> >> What is in-place conversion good for? If you already have a libvirt >> domain XML (i.e., one *not* output by virt-v2v as the result of a >> conversion from a foreign hypervisor), what do you need >> virt-v2v-in-place for? >> >> My understanding is that virt-v2v produces both an output disk (set) and >> a domain description (be it a QEMU cmdline, a libvirt domain XML, an >> OVF, ...), *and* that these two kinds of output belong together, there >> is not one without the other. What's the data flow with inplace >> conversion? >> >> Laszlo >> > We use v2v as guest convertor engine and prepare VM configuration > ourselves. This looks more appropriate for us as we have different > constraints under different conditions. > > This makes sense outside of foreign hypervisor as we could change > bus of the disk and then call v2v to teach the guest to boot from > new location. This was revealed very useful to fix some strange > issues on the customer's side. > > That is it. > > Den > > P.S. Resent (original mail was accidentally sent off-list) > So the use case is more or less "-i libvirtxml", with the domain XML created from scratch (or liberally tweaked, starting from a "more authentic" original domain XML). The cover letter mentions the following commit: commit b28cd1dcfeb40e7002e8d0b0ce9dcc4ce86beb6c Author: Richard W.M. Jones <rjones(a)redhat.com> Date: Mon Nov 8 09:00:20 2021 +0000 Remove requested_guestcaps / rcaps This was part of the old in-place support. When we add new in-place support we'll do something else, but currently this is dead code so remove it completely. Note this removes the code for installing the virtio-scsi driver (only ever using virtio-blk). This was also dead code in the current implementation of virt-v2v, but worth remembering in case we want to resurrect this feature in a future version. Acked-by: Laszlo Ersek <lersek(a)redhat.com> So I guess that "something else" is what I'm missing here. Right now the proposed use case seems to be: - tweak the domain XML in some way (or generate it in some "bespoke" way from the outset) - perform an in-place conversion with virt-v2v, making sure that virt-v2v prefers virtio-scsi as first priority. This will end up injecting the virtio-scsi guest driver into the guest. This looks less than ideal to me. Even if we offer virtio-scsi, that should be driven by a specific knob. I initially thought that knob could be a new command line option. Then, upon reading we could change bus of the disk and then call v2v to teach the guest to boot from new location I thought that the contents of the tweaked domain XML would *steer* the guest driver selection. (This seemed plausible, because we used to have "source" domain properties, and I vaguely recalled that they once had been relevant for in-place conversion.) But these patches don't do any such steering, AFAICT.

Yes, I think you got the scenario right -- we need to be able to provide the driver for the controller specified in the source domain XML. I agree that changing the default might not be the best solution here. On the other hand, AFAICT right now the way default is being chosen is rather strange: it's the 1st driver in the list of ["virtio_blk"; "vrtioblk"; "viostor"] which is present in the tools directory. What if it doesn't match the actual disk controller in the source VM? Doesn't make much sense to me.

...

Why is the "rcaps" logic from before commit b28cd1dc not being brought back?

My 5th patch actually includes bits of the b28cd1dc, namely adding vioscsi pciid to the registry. I'm not sure whether bringing back the entire rcaps is a good idea, let's probably see what Richard has to say about this. Maybe we can come up with a simpler way to "forward" the storage controller type when doing the initial inspection?

...

Laszlo

Laszlo Ersek

Sunday, 26 February Sun, 26 Feb

12:51 a.m.

On 2/24/23 17:11, Andrey Drobyshev wrote:

...

On 2/24/23 14:02, Laszlo Ersek wrote: > On 2/24/23 12:55, Denis V. Lunev wrote: >> On 2/24/23 05:56, Laszlo Ersek wrote: > >>> I've got zero experience with in-place conversions. I've skimmed >>> <https://libguestfs.org/virt-v2v-in-place.1.html> now, but the use case >>> continues to elude me. >>> >>> What is in-place conversion good for? If you already have a libvirt >>> domain XML (i.e., one *not* output by virt-v2v as the result of a >>> conversion from a foreign hypervisor), what do you need >>> virt-v2v-in-place for? >>> >>> My understanding is that virt-v2v produces both an output disk (set) and >>> a domain description (be it a QEMU cmdline, a libvirt domain XML, an >>> OVF, ...), *and* that these two kinds of output belong together, there >>> is not one without the other. What's the data flow with inplace >>> conversion? >>> >>> Laszlo >>> >> We use v2v as guest convertor engine and prepare VM configuration >> ourselves. This looks more appropriate for us as we have different >> constraints under different conditions. >> >> This makes sense outside of foreign hypervisor as we could change >> bus of the disk and then call v2v to teach the guest to boot from >> new location. This was revealed very useful to fix some strange >> issues on the customer's side. >> >> That is it. >> >> Den >> >> P.S. Resent (original mail was accidentally sent off-list) >> > > So the use case is more or less "-i libvirtxml", with the domain XML > created from scratch (or liberally tweaked, starting from a "more > authentic" original domain XML). > > The cover letter mentions the following commit: > > commit b28cd1dcfeb40e7002e8d0b0ce9dcc4ce86beb6c > Author: Richard W.M. Jones <rjones(a)redhat.com> > Date: Mon Nov 8 09:00:20 2021 +0000 > > Remove requested_guestcaps / rcaps > > This was part of the old in-place support. When we add new in-place > support we'll do something else, but currently this is dead code so > remove it completely. > > Note this removes the code for installing the virtio-scsi driver (only > ever using virtio-blk). This was also dead code in the current > implementation of virt-v2v, but worth remembering in case we want to > resurrect this feature in a future version. > > Acked-by: Laszlo Ersek <lersek(a)redhat.com> > > So I guess that "something else" is what I'm missing here. Right now the > proposed use case seems to be: > > - tweak the domain XML in some way (or generate it in some "bespoke" way > from the outset) > > - perform an in-place conversion with virt-v2v, making sure that > virt-v2v prefers virtio-scsi as first priority. This will end up > injecting the virtio-scsi guest driver into the guest. > > This looks less than ideal to me. Even if we offer virtio-scsi, that > should be driven by a specific knob. > > I initially thought that knob could be a new command line option. > > Then, upon reading > > we could change bus of the disk and then call v2v to teach the guest > to boot from new location > > I thought that the contents of the tweaked domain XML would *steer* the > guest driver selection. (This seemed plausible, because we used to have > "source" domain properties, and I vaguely recalled that they once had > been relevant for in-place conversion.) But these patches don't do any > such steering, AFAICT. Yes, I think you got the scenario right -- we need to be able to provide the driver for the controller specified in the source domain XML. I agree that changing the default might not be the best solution here. On the other hand, AFAICT right now the way default is being chosen is rather strange: it's the 1st driver in the list of ["virtio_blk"; "vrtioblk"; "viostor"] which is present in the tools directory. What if it doesn't match the actual disk controller in the source VM? Doesn't make much sense to me.

My understanding is that these are different driver names, but these drivers are all for the virtio-block device. IIUC the names had changed over time, for the same thing.

...

> > Why is the "rcaps" logic from before commit b28cd1dc not being brought > back? My 5th patch actually includes bits of the b28cd1dc, namely adding vioscsi pciid to the registry. I'm not sure whether bringing back the entire rcaps is a good idea, let's probably see what Richard has to say about this. Maybe we can come up with a simpler way to "forward" the storage controller type when doing the initial inspection?

Yes, let's hear what Rich thinks. Laszlo

Daniel P. Berrangé

Thursday, 23 February Thu, 23 Feb

6:20 a.m.

On Thu, Feb 23, 2023 at 11:43:38AM +0100, Laszlo Ersek wrote:

...

I think that issue shouldn't have used the word 'opionated' as it gives the wrong impression that the choice is somewhat arbitrary and interchangable. I think there are rational reasons why virtio-scsi is the better choice that they likely evaluated. The main tradeoffs for virtio-blk vs virtio-scsi are outlined by QEMU maintainers here: https://www.qemu.org/2021/01/19/virtio-blk-scsi-configuration/ TL;DR virtio-blk is preferred for maximum speed, while virtio-scsi is preferred if you want to be able to add lots of disks free of worrying about PCI slot availability. I can totally understand why public clouds would pick to only support virtio-scsi. The speed benefits of virtio-blk are likely not relevant / visible to their customers because when you're overcommiting hosts to serve many VM, the bottleneck is almost certainly somewhere other than the guest disk device choice. The ease of adding many disks is very interesting to public clouds though, especially if the VM has many NICs already taking up PCI slots. Getting into adding PCI bridges adds more complexity than using SCSI. OpenStack maintainers have also considered preferring virtio-scsi over virtio-blk for specifically this reason in the past and might not have even used virtio-blk at all, if it were not for the backcompat concerns. So I'd say it is a reasonable desire to want to (optionally) emit VMs that are setup for use of virtio-scsi instead of virtio-blk With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Richard W.M. Jones

Tuesday, 28 February Tue, 28 Feb

7:01 a.m.

On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote:

...

So you're right that we ended up removing virtio-scsi support because in-place conversion was temporarily removed, and it wasn't restored when the new virt-v2v-in-place tool was added. I looked at the patches, and I don't have any objection to 1-4. Patch 5 is the tricky one because it changes the default, but we (Red Hat) are likely to stick with virtio-blk for better or worse. Could we make this configurable? We've traditionally shied away from adding virt-v2v command line options which are purely for fine-tuning conversions. The reason for this is that when you're doing bulk conversions of hundreds of VMs from VMware, you want virt-v2v to do "the best it can", and it's not really feasible to hand configure every conversion. (There are some historical exceptions to this, like --root, but don't look at those!) I know the bulk conversion case doesn't apply to Virtuozzo, but it does matter for us. It could be argued that this isn't the same, because if we're bulk converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, but per target hypervisor. So a command line option would be OK in this case. BTW previously we "configured" this preference through the input libvirt XML. That was a terrible idea, so I'd prefer to avoid it this time around. Anyway if you can think of a better version of patch 5 then I think we can accept this. Thanks, Rich.

...

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

Laszlo Ersek

8:24 a.m.

On 2/28/23 14:01, Richard W.M. Jones wrote:

...

On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote: > Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 > ("Remove guestcaps_block_type Virtio_SCSI") support for installing > virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for > bringing this feature back have been out there for a while. E.g. I've > found a corresponding issue which is still open [1]. > > The code in b28cd1dc, f0afc439 was removed due to removing the old in-place > support. However, having the new in-place support present and bringing > this same code (partially) back with several additions and improvements, > I'm able to successfully convert and boot a Win guest with a virtio-scsi > disk controller. So please consider the following implementation of > this feature. > > [1] https://github.com/libguestfs/virt-v2v/issues/12 So you're right that we ended up removing virtio-scsi support because in-place conversion was temporarily removed, and it wasn't restored when the new virt-v2v-in-place tool was added. I looked at the patches, and I don't have any objection to 1-4. Patch 5 is the tricky one because it changes the default, but we (Red Hat) are likely to stick with virtio-blk for better or worse. Could we make this configurable? We've traditionally shied away from adding virt-v2v command line options which are purely for fine-tuning conversions. The reason for this is that when you're doing bulk conversions of hundreds of VMs from VMware, you want virt-v2v to do "the best it can", and it's not really feasible to hand configure every conversion. (There are some historical exceptions to this, like --root, but don't look at those!) I know the bulk conversion case doesn't apply to Virtuozzo, but it does matter for us. It could be argued that this isn't the same, because if we're bulk converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, but per target hypervisor. So a command line option would be OK in this case. BTW previously we "configured" this preference through the input libvirt XML. That was a terrible idea, so I'd prefer to avoid it this time around.

Ah, that's new information to me. I thought the "rcaps" (requested caps, derived from the input XML) was exactly what was missing here. Not because of the particular reason(s) that may have motivated rcaps in the past, but because in the use case at hand, the tweaking of the domain XML precedes the conversion (more like, the injection of the desired virtio drivers). Why was it a terrible idea?

...

Anyway if you can think of a better version of patch 5 then I think we can accept this.

ITYM "if you *cannot* think of a better version"... But anyway, if a command line option is acceptable to you, I'm perfectly fine with it as well. I'd like to avoid changing the default. I quite foresee tens of QE test runs breaking, resulting in as many bug reports, and then us eventually reverting the change-of-the-default downstream. I'd rather favor an experimental command line switch. Laszlo

Richard W.M. Jones

8:39 a.m.

On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote:

...

On 2/28/23 14:01, Richard W.M. Jones wrote: > On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote: >> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >> bringing this feature back have been out there for a while. E.g. I've >> found a corresponding issue which is still open [1]. >> >> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place >> support. However, having the new in-place support present and bringing >> this same code (partially) back with several additions and improvements, >> I'm able to successfully convert and boot a Win guest with a virtio-scsi >> disk controller. So please consider the following implementation of >> this feature. >> >> [1] https://github.com/libguestfs/virt-v2v/issues/12 > > So you're right that we ended up removing virtio-scsi support because > in-place conversion was temporarily removed, and it wasn't restored > when the new virt-v2v-in-place tool was added. > > I looked at the patches, and I don't have any objection to 1-4. > > Patch 5 is the tricky one because it changes the default, but we > (Red Hat) are likely to stick with virtio-blk for better or worse. > > Could we make this configurable? We've traditionally shied away from > adding virt-v2v command line options which are purely for fine-tuning > conversions. The reason for this is that when you're doing bulk > conversions of hundreds of VMs from VMware, you want virt-v2v to do > "the best it can", and it's not really feasible to hand configure > every conversion. (There are some historical exceptions to this, like > --root, but don't look at those!) I know the bulk conversion case > doesn't apply to Virtuozzo, but it does matter for us. > > It could be argued that this isn't the same, because if we're bulk > converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, > but per target hypervisor. So a command line option would be OK in > this case. > > BTW previously we "configured" this preference through the input > libvirt XML. That was a terrible idea, so I'd prefer to avoid it this > time around. Ah, that's new information to me. I thought the "rcaps" (requested caps, derived from the input XML) was exactly what was missing here. Not because of the particular reason(s) that may have motivated rcaps in the past, but because in the use case at hand, the tweaking of the domain XML precedes the conversion (more like, the injection of the desired virtio drivers). Why was it a terrible idea?

It was a hack. For copying (normal) conversions -i libvirtxml describes the input metadata. For in-place conversions "input" is a fuzzy concept, so it was overloaded to describe the conversion preferences. We can do better with a command line parameter.

...

> Anyway if you can think of a better version of patch 5 then I think we > can accept this. ITYM "if you *cannot* think of a better version"... But anyway, if a command line option is acceptable to you, I'm perfectly fine with it as well. I'd like to avoid changing the default. I quite foresee tens of QE test runs breaking, resulting in as many bug reports, and then us eventually reverting the change-of-the-default downstream. I'd rather favor an experimental command line switch.

Yes I'm sure we don't want to change the default; or at least we (meaning Red Hat, downstream) want to choose whether and when to change the default. BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora). Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

Andrey Drobyshev

Wednesday, 1 March Wed, 1 Mar

8:02 a.m.

On 2/28/23 16:39, Richard W.M. Jones wrote:

...

On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote: > On 2/28/23 14:01, Richard W.M. Jones wrote: >> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote: >>> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >>> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >>> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >>> bringing this feature back have been out there for a while. E.g. I've >>> found a corresponding issue which is still open [1]. >>> >>> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place >>> support. However, having the new in-place support present and bringing >>> this same code (partially) back with several additions and improvements, >>> I'm able to successfully convert and boot a Win guest with a virtio-scsi >>> disk controller. So please consider the following implementation of >>> this feature. >>> >>> [1] https://github.com/libguestfs/virt-v2v/issues/12 >> >> So you're right that we ended up removing virtio-scsi support because >> in-place conversion was temporarily removed, and it wasn't restored >> when the new virt-v2v-in-place tool was added. >> >> I looked at the patches, and I don't have any objection to 1-4. >> >> Patch 5 is the tricky one because it changes the default, but we >> (Red Hat) are likely to stick with virtio-blk for better or worse. >> >> Could we make this configurable? We've traditionally shied away from >> adding virt-v2v command line options which are purely for fine-tuning >> conversions. The reason for this is that when you're doing bulk >> conversions of hundreds of VMs from VMware, you want virt-v2v to do >> "the best it can", and it's not really feasible to hand configure >> every conversion. (There are some historical exceptions to this, like >> --root, but don't look at those!) I know the bulk conversion case >> doesn't apply to Virtuozzo, but it does matter for us. >> >> It could be argued that this isn't the same, because if we're bulk >> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, >> but per target hypervisor. So a command line option would be OK in >> this case. >> >> BTW previously we "configured" this preference through the input >> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this >> time around. > > Ah, that's new information to me. I thought the "rcaps" (requested caps, > derived from the input XML) was exactly what was missing here. Not > because of the particular reason(s) that may have motivated rcaps in the > past, but because in the use case at hand, the tweaking of the domain > XML precedes the conversion (more like, the injection of the desired > virtio drivers). > > Why was it a terrible idea? It was a hack. For copying (normal) conversions -i libvirtxml describes the input metadata. For in-place conversions "input" is a fuzzy concept, so it was overloaded to describe the conversion preferences. We can do better with a command line parameter.

Could you please elaborate on why is that a fuzzy concept? I understand that bringing back the entire requested capabilities (rcaps) framework might be undesirable, but the idea of choosing the block driver based on the source libvirt XML doesn't seem very wrong to me. Even if we perform some modifications on that source XML (which I'm not sure of) we end up with a VM config having a particular set of devices, and by that point all fuzziness should be eliminated. Am I missing something?

...

>> Anyway if you can think of a better version of patch 5 then I think we >> can accept this. > > ITYM "if you *cannot* think of a better version"... > > But anyway, if a command line option is acceptable to you, I'm perfectly > fine with it as well. I'd like to avoid changing the default. I quite > foresee tens of QE test runs breaking, resulting in as many bug reports, > and then us eventually reverting the change-of-the-default downstream. > I'd rather favor an experimental command line switch. Yes I'm sure we don't want to change the default; or at least we (meaning Red Hat, downstream) want to choose whether and when to change the default.

Speaking of the command line option: if we end up with such a solution, do you think it should be specific to and only applicable in the in-place mode?

...

BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora). Thanks, Rich.

Andrey Drobyshev

Thursday, 2 March Thu, 2 Mar

12:15 p.m.

On 2/28/23 16:39, Richard W.M. Jones wrote:

...

Speaking of the command line option: if we end up with such a solution, do you think it should be specific to and only applicable in the in-place mode?

...

BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora). Thanks, Rich.

P.S. Resent, the original mail ended up off the mailing list.

Richard W.M. Jones

12:36 p.m.

On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev wrote:

...

On 2/28/23 16:39, Richard W.M. Jones wrote: > On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote: >> On 2/28/23 14:01, Richard W.M. Jones wrote: >>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote: >>>> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >>>> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >>>> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >>>> bringing this feature back have been out there for a while. E.g. I've >>>> found a corresponding issue which is still open [1]. >>>> >>>> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place >>>> support. However, having the new in-place support present and bringing >>>> this same code (partially) back with several additions and improvements, >>>> I'm able to successfully convert and boot a Win guest with a virtio-scsi >>>> disk controller. So please consider the following implementation of >>>> this feature. >>>> >>>> [1] https://github.com/libguestfs/virt-v2v/issues/12 >>> >>> So you're right that we ended up removing virtio-scsi support because >>> in-place conversion was temporarily removed, and it wasn't restored >>> when the new virt-v2v-in-place tool was added. >>> >>> I looked at the patches, and I don't have any objection to 1-4. >>> >>> Patch 5 is the tricky one because it changes the default, but we >>> (Red Hat) are likely to stick with virtio-blk for better or worse. >>> >>> Could we make this configurable? We've traditionally shied away from >>> adding virt-v2v command line options which are purely for fine-tuning >>> conversions. The reason for this is that when you're doing bulk >>> conversions of hundreds of VMs from VMware, you want virt-v2v to do >>> "the best it can", and it's not really feasible to hand configure >>> every conversion. (There are some historical exceptions to this, like >>> --root, but don't look at those!) I know the bulk conversion case >>> doesn't apply to Virtuozzo, but it does matter for us. >>> >>> It could be argued that this isn't the same, because if we're bulk >>> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, >>> but per target hypervisor. So a command line option would be OK in >>> this case. >>> >>> BTW previously we "configured" this preference through the input >>> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this >>> time around. >> >> Ah, that's new information to me. I thought the "rcaps" (requested caps, >> derived from the input XML) was exactly what was missing here. Not >> because of the particular reason(s) that may have motivated rcaps in the >> past, but because in the use case at hand, the tweaking of the domain >> XML precedes the conversion (more like, the injection of the desired >> virtio drivers). >> >> Why was it a terrible idea? > > It was a hack. For copying (normal) conversions -i libvirtxml > describes the input metadata. For in-place conversions "input" is a > fuzzy concept, so it was overloaded to describe the conversion > preferences. We can do better with a command line parameter. Could you please elaborate on why is that a fuzzy concept? I understand that bringing back the entire requested capabilities (rcaps) framework might be undesirable, but the idea of choosing the block driver based on the source libvirt XML doesn't seem very wrong to me. Even if we perform some modifications on that source XML (which I'm not sure of) we end up with a VM config having a particular set of devices, and by that point all fuzziness should be eliminated. Am I missing something?

It's mixing up two concepts, the input metadata and the conversion parameters. Plus it's plain weird. And hard for end users to do. Command line options would be how you normally influence the behaviour of a command line tool.

...

> >>> Anyway if you can think of a better version of patch 5 then I think we >>> can accept this. >> >> ITYM "if you *cannot* think of a better version"... >> >> But anyway, if a command line option is acceptable to you, I'm perfectly >> fine with it as well. I'd like to avoid changing the default. I quite >> foresee tens of QE test runs breaking, resulting in as many bug reports, >> and then us eventually reverting the change-of-the-default downstream. >> I'd rather favor an experimental command line switch. > > Yes I'm sure we don't want to change the default; or at least we > (meaning Red Hat, downstream) want to choose whether and when to > change the default. Speaking of the command line option: if we end up with such a solution, do you think it should be specific to and only applicable in the in-place mode?

I think it would apply to all of the virt-v2v* tools, that seems sensible to me I think. When doing a copying conversion you might also want to prefer virtio-scsi. I can probably do an outline of a patch if you need it, but not today. Rich.

...

> > BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora). > > Thanks, > > Rich. > P.S. Resent, the original mail ended up off the mailing list.

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

Andrey Drobyshev

12:52 p.m.

On 3/2/23 20:36, Richard W.M. Jones wrote:

...

On Thu, Mar 02, 2023 at 08:15:37PM +0200, Andrey Drobyshev wrote: > On 2/28/23 16:39, Richard W.M. Jones wrote: >> On Tue, Feb 28, 2023 at 03:24:46PM +0100, Laszlo Ersek wrote: >>> On 2/28/23 14:01, Richard W.M. Jones wrote: >>>> On Wed, Feb 22, 2023 at 08:20:43PM +0200, Andrey Drobyshev wrote: >>>>> Since commits b28cd1dc ("Remove requested_guestcaps / rcaps"), f0afc439 >>>>> ("Remove guestcaps_block_type Virtio_SCSI") support for installing >>>>> virtio-scsi driver is missing in virt-v2v. AFAIU plans and demands for >>>>> bringing this feature back have been out there for a while. E.g. I've >>>>> found a corresponding issue which is still open [1]. >>>>> >>>>> The code in b28cd1dc, f0afc439 was removed due to removing the old in-place >>>>> support. However, having the new in-place support present and bringing >>>>> this same code (partially) back with several additions and improvements, >>>>> I'm able to successfully convert and boot a Win guest with a virtio-scsi >>>>> disk controller. So please consider the following implementation of >>>>> this feature. >>>>> >>>>> [1] https://github.com/libguestfs/virt-v2v/issues/12 >>>> >>>> So you're right that we ended up removing virtio-scsi support because >>>> in-place conversion was temporarily removed, and it wasn't restored >>>> when the new virt-v2v-in-place tool was added. >>>> >>>> I looked at the patches, and I don't have any objection to 1-4. >>>> >>>> Patch 5 is the tricky one because it changes the default, but we >>>> (Red Hat) are likely to stick with virtio-blk for better or worse. >>>> >>>> Could we make this configurable? We've traditionally shied away from >>>> adding virt-v2v command line options which are purely for fine-tuning >>>> conversions. The reason for this is that when you're doing bulk >>>> conversions of hundreds of VMs from VMware, you want virt-v2v to do >>>> "the best it can", and it's not really feasible to hand configure >>>> every conversion. (There are some historical exceptions to this, like >>>> --root, but don't look at those!) I know the bulk conversion case >>>> doesn't apply to Virtuozzo, but it does matter for us. >>>> >>>> It could be argued that this isn't the same, because if we're bulk >>>> converting, you're not fine-tuning virtio-scsi vs virtio-blk per VM, >>>> but per target hypervisor. So a command line option would be OK in >>>> this case. >>>> >>>> BTW previously we "configured" this preference through the input >>>> libvirt XML. That was a terrible idea, so I'd prefer to avoid it this >>>> time around. >>> >>> Ah, that's new information to me. I thought the "rcaps" (requested caps, >>> derived from the input XML) was exactly what was missing here. Not >>> because of the particular reason(s) that may have motivated rcaps in the >>> past, but because in the use case at hand, the tweaking of the domain >>> XML precedes the conversion (more like, the injection of the desired >>> virtio drivers). >>> >>> Why was it a terrible idea? >> >> It was a hack. For copying (normal) conversions -i libvirtxml >> describes the input metadata. For in-place conversions "input" is a >> fuzzy concept, so it was overloaded to describe the conversion >> preferences. We can do better with a command line parameter. > > > Could you please elaborate on why is that a fuzzy concept? I understand > that bringing back the entire requested capabilities (rcaps) framework > might be undesirable, but the idea of choosing the block driver based on > the source libvirt XML doesn't seem very wrong to me. Even if we > perform some modifications on that source XML (which I'm not sure of) we > end up with a VM config having a particular set of devices, and by that > point all fuzziness should be eliminated. Am I missing something? It's mixing up two concepts, the input metadata and the conversion parameters. Plus it's plain weird. And hard for end users to do. Command line options would be how you normally influence the behaviour of a command line tool. >> >>>> Anyway if you can think of a better version of patch 5 then I think we >>>> can accept this. >>> >>> ITYM "if you *cannot* think of a better version"... >>> >>> But anyway, if a command line option is acceptable to you, I'm perfectly >>> fine with it as well. I'd like to avoid changing the default. I quite >>> foresee tens of QE test runs breaking, resulting in as many bug reports, >>> and then us eventually reverting the change-of-the-default downstream. >>> I'd rather favor an experimental command line switch. >> >> Yes I'm sure we don't want to change the default; or at least we >> (meaning Red Hat, downstream) want to choose whether and when to >> change the default. > > Speaking of the command line option: if we end up with such a solution, > do you think it should be specific to and only applicable in the > in-place mode? I think it would apply to all of the virt-v2v* tools, that seems sensible to me I think. When doing a copying conversion you might also want to prefer virtio-scsi. I can probably do an outline of a patch if you need it, but not today.

Yes, sure. But speaking of the ways to influence the behaviour of a command line tool: there're also env variables, which are being used by v2v as well (for instance, for setting the tools ISO location). So what if we introduce an env variable here instead? I presume it would be much easier to implement. Smth like:

...

diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml index 62f7710..562f055 100644 --- a/mlcustomize/inject_virtio_win.ml +++ b/mlcustomize/inject_virtio_win.ml @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = (* Can we install the block driver? *) let block : block_type = let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in + let filenames = + match viostor_drv_env with + | None -> filenames + | Some str -> str :: filenames in let viostor_driver = try ( Some ( List.find (

As you can see here the default doesn't change if the variable isn't provided at all (or it is, but is set to a non-existing driver). What do you think of such a solution?

...

Rich. >> >> BTW/FYI we don't ship virt-v2v-in-place in RHEL (we do in Fedora). >> >> Thanks, >> >> Rich. >> > > P.S. Resent, the original mail ended up off the mailing list.

Richard W.M. Jones

2:59 p.m.

On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote:

...

On 3/2/23 20:36, Richard W.M. Jones wrote: > I can probably do an outline of a patch if you need it, but > not today. Yes, sure. But speaking of the ways to influence the behaviour of a command line tool: there're also env variables, which are being used by v2v as well (for instance, for setting the tools ISO location). So what if we introduce an env variable here instead? I presume it would be much easier to implement. Smth like:

The environment variable is a bit of a mistake too. It'd be better to specify that through the command line; and in fact that's what I went with for the new virt-customize --inject-virtio-win option: --inject-virtio-win METHOD Inject virtio-win drivers into a Windows guest. These drivers add virtio accelerated drivers suitable when running on top of a hypervisor that supports virtio (such as qemu/KVM). The operation also adjusts the Windows Registry so that the drivers are installed when the guest boots. The parameter can be one of: ISO The path to the ISO image containing the virtio-win drivers (eg. /usr/share/virtio-win/virtio-win.iso). [etc]

...

> diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml > index 62f7710..562f055 100644 > --- a/mlcustomize/inject_virtio_win.ml > +++ b/mlcustomize/inject_virtio_win.ml > @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = > (* Can we install the block driver? *) > let block : block_type = > let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in > + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in > + let filenames = > + match viostor_drv_env with > + | None -> filenames > + | Some str -> str :: filenames in > let viostor_driver = try ( > Some ( > List.find ( As you can see here the default doesn't change if the variable isn't provided at all (or it is, but is set to a non-existing driver). What do you think of such a solution?

I prefer the command line option even though it's a little bit more work. Offer above still stands if you can't work out the complexities of OCaml command line parsing. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

Andrey Drobyshev

3:31 p.m.

On 3/2/23 22:59, Richard W.M. Jones wrote:

...

On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote: > On 3/2/23 20:36, Richard W.M. Jones wrote: >> I can probably do an outline of a patch if you need it, but >> not today. > > Yes, sure. But speaking of the ways to influence the behaviour of a > command line tool: there're also env variables, which are being used by > v2v as well (for instance, for setting the tools ISO location). So what > if we introduce an env variable here instead? I presume it would be > much easier to implement. Smth like: The environment variable is a bit of a mistake too. It'd be better to specify that through the command line; and in fact that's what I went with for the new virt-customize --inject-virtio-win option: --inject-virtio-win METHOD Inject virtio-win drivers into a Windows guest. These drivers add virtio accelerated drivers suitable when running on top of a hypervisor that supports virtio (such as qemu/KVM). The operation also adjusts the Windows Registry so that the drivers are installed when the guest boots. The parameter can be one of: ISO The path to the ISO image containing the virtio-win drivers (eg. /usr/share/virtio-win/virtio-win.iso). [etc] >> diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml >> index 62f7710..562f055 100644 >> --- a/mlcustomize/inject_virtio_win.ml >> +++ b/mlcustomize/inject_virtio_win.ml >> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = >> (* Can we install the block driver? *) >> let block : block_type = >> let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in >> + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in >> + let filenames = >> + match viostor_drv_env with >> + | None -> filenames >> + | Some str -> str :: filenames in >> let viostor_driver = try ( >> Some ( >> List.find ( > > As you can see here the default doesn't change if the variable isn't > provided at all (or it is, but is set to a non-existing driver). > What do you think of such a solution? I prefer the command line option even though it's a little bit more work. Offer above still stands if you can't work out the complexities of OCaml command line parsing.

It's not about the command line parsing complexities, but rather about the proper way to pass that option from the top level of the call stack (convert ()) down to the inject_virtio_win_drivers (). That'd require adding one more extra argument to all the calls along this call stack, which might look a bit clumsy. Isn't there a more elegant way? So having a sketch in that regard would be indeed much appreciated.

...

Rich.

Richard W.M. Jones

Friday, 3 March Fri, 3 Mar

2:53 a.m.

On Thu, Mar 02, 2023 at 11:31:14PM +0200, Andrey Drobyshev wrote:

...

On 3/2/23 22:59, Richard W.M. Jones wrote: > On Thu, Mar 02, 2023 at 08:52:33PM +0200, Andrey Drobyshev wrote: >> On 3/2/23 20:36, Richard W.M. Jones wrote: >>> I can probably do an outline of a patch if you need it, but >>> not today. >> >> Yes, sure. But speaking of the ways to influence the behaviour of a >> command line tool: there're also env variables, which are being used by >> v2v as well (for instance, for setting the tools ISO location). So what >> if we introduce an env variable here instead? I presume it would be >> much easier to implement. Smth like: > > The environment variable is a bit of a mistake too. It'd be better to > specify that through the command line; and in fact that's what I went > with for the new virt-customize --inject-virtio-win option: > > --inject-virtio-win METHOD > Inject virtio-win drivers into a Windows guest. These drivers add > virtio accelerated drivers suitable when running on top of a > hypervisor that supports virtio (such as qemu/KVM). The operation > also adjusts the Windows Registry so that the drivers are installed > when the guest boots. > > The parameter can be one of: > > ISO The path to the ISO image containing the virtio-win drivers > (eg. /usr/share/virtio-win/virtio-win.iso). > [etc] > >>> diff --git a/mlcustomize/inject_virtio_win.ml b/mlcustomize/inject_virtio_win.ml >>> index 62f7710..562f055 100644 >>> --- a/mlcustomize/inject_virtio_win.ml >>> +++ b/mlcustomize/inject_virtio_win.ml >>> @@ -177,6 +177,11 @@ let rec inject_virtio_win_drivers ({ g } as t) reg = >>> (* Can we install the block driver? *) >>> let block : block_type = >>> let filenames = ["virtio_blk"; "vrtioblk"; "viostor"] in >>> + let viostor_drv_env = Sys.getenv_opt "VIOSTOR_DRIVER" in >>> + let filenames = >>> + match viostor_drv_env with >>> + | None -> filenames >>> + | Some str -> str :: filenames in >>> let viostor_driver = try ( >>> Some ( >>> List.find ( >> >> As you can see here the default doesn't change if the variable isn't >> provided at all (or it is, but is set to a non-existing driver). >> What do you think of such a solution? > > I prefer the command line option even though it's a little bit more > work. Offer above still stands if you can't work out the complexities > of OCaml command line parsing. It's not about the command line parsing complexities, but rather about the proper way to pass that option from the top level of the call stack (convert ()) down to the inject_virtio_win_drivers (). That'd require adding one more extra argument to all the calls along this call stack, which might look a bit clumsy. Isn't there a more elegant way? So having a sketch in that regard would be indeed much appreciated.

OK I'll come up with something, but may not be til Monday. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

444

days inactive

453

days old

guestfs@lists.libguestfs.org

Manage subscription

23 comments

5 participants

tags (0)

participants (5)

Andrey Drobyshev
Daniel P. Berrangé
Denis V. Lunev
Laszlo Ersek
Richard W.M. Jones

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[V2V PATCH 0/5] Bring support for virtio-scsi back to Windows