[PATCH] v2v: add support for virtio-scsi
Virtio-SCSI offers a number of advantages over virtio-blk, in particular, it supports SCSI UNMAP (aka trim) which is crucial for keeping the virtual drive from wasting host disk space. This patch adds support for virtio-scsi as the virtual disk connection type both on input and on output of v2v. Virtio-blk remains the default, so for now virtio-scsi-based guests can only be produced in --in-place mode. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- v2v/OVF.ml | 4 +++- v2v/convert_linux.ml | 15 +++++++++------ v2v/input_libvirtxml.ml | 32 ++++++++++++++++++++------------ v2v/output_glance.ml | 1 + v2v/test-v2v-print-source.sh | 2 +- v2v/types.ml | 9 ++++++--- v2v/types.mli | 14 +++++++------- v2v/v2v.ml | 13 ++++++++----- v2v/windows_virtio.ml | 30 ++++++++++++++++++++++++------ 9 files changed, 79 insertions(+), 41 deletions(-) diff --git a/v2v/OVF.ml b/v2v/OVF.ml index f8afc8c..dc1e971 100644 --- a/v2v/OVF.ml +++ b/v2v/OVF.ml @@ -454,7 +454,9 @@ and add_disks targets guestcaps output_alloc sd_uuid image_uuids vol_uuids ovf = "ovf:format", "http://en.wikipedia.org/wiki/Byte"; (* wtf? *) "ovf:disk-interface", (match guestcaps.gcaps_block_bus with - | Virtio_blk -> "VirtIO" | IDE -> "IDE"); + | Virtio_blk -> "VirtIO" + | Virtio_SCSI -> "SCSI" + | IDE -> "IDE"); "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_boot_drive then "True" else "False"; ] in diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml index 0a2e439..90355fb 100644 --- a/v2v/convert_linux.ml +++ b/v2v/convert_linux.ml @@ -803,8 +803,9 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = * major number of vdX block devices. If we change it, RHEL 3 * KVM guests won't boot. *) - [ "virtio"; "virtio_ring"; "virtio_blk"; "virtio_net"; - "virtio_pci" ] + List.filter (fun m -> List.mem m kernel.ki_modules) + [ "virtio"; "virtio_ring"; "virtio_blk"; + "virtio_scsi"; "virtio_net"; "virtio_pci" ] else [ "sym53c8xx" (* XXX why not "ide"? *) ] in @@ -1134,7 +1135,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = (* Update 'alias scsi_hostadapter ...' *) let paths = augeas_modprobe ". =~ regexp('scsi_hostadapter.*')" in match block_type with - | Virtio_blk -> + | Virtio_blk | Virtio_SCSI -> if paths <> [] then ( (* There's only 1 scsi controller in the converted guest. * Convert only the first scsi_hostadapter entry to virtio @@ -1148,14 +1149,15 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = List.iter (fun path -> ignore (g#aug_rm path)) (List.rev paths_to_delete); - g#aug_set (path ^ "/modulename") "virtio_blk" + g#aug_set (path ^ "/modulename") + (string_of_block_type block_type) ) else ( (* We have to add a scsi_hostadapter. *) let modpath = discover_modpath () in g#aug_set (sprintf "/files%s/alias[last()+1]" modpath) "scsi_hostadapter"; g#aug_set (sprintf "/files%s/alias[last()]/modulename" modpath) - "virtio_blk" + (string_of_block_type block_type) ) | IDE -> (* There is no scsi controller in an IDE guest. *) @@ -1247,6 +1249,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = let block_prefix_after_conversion = match block_type with | Virtio_blk -> "vd" + | Virtio_SCSI -> "sd" | IDE -> ide_block_prefix in let map = @@ -1255,7 +1258,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = let block_prefix_before_conversion = match disk.s_controller with | Some Source_IDE -> ide_block_prefix - | Some Source_SCSI -> "sd" + | Some Source_virtio_SCSI | Some Source_SCSI -> "sd" | Some Source_virtio_blk -> "vd" | None -> (* This is basically a guess. It assumes the source used IDE. *) diff --git a/v2v/input_libvirtxml.ml b/v2v/input_libvirtxml.ml index 9d8963d..6f325e6 100644 --- a/v2v/input_libvirtxml.ml +++ b/v2v/input_libvirtxml.ml @@ -181,6 +181,12 @@ let parse_libvirt_xml ?conn xml = None ) in + (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in + (Xml.xpathobj_nr_nodes obj) > 0 in + (* Non-removable disk devices. *) let disks = let get_disks, add_disk = @@ -208,12 +214,13 @@ let parse_libvirt_xml ?conn xml = let controller = let target_bus = xpath_string "target/@bus" in - match target_bus with - | None -> None - | Some "ide" -> Some Source_IDE - | Some "scsi" -> Some Source_SCSI - | Some "virtio" -> Some Source_virtio_blk - | Some _ -> None in + match target_bus, has_virtio_scsi with + | None, _ -> None + | Some "ide", _ -> Some Source_IDE + | Some "scsi", true -> Some Source_virtio_SCSI + | Some "scsi", false -> Some Source_SCSI + | Some "virtio", _ -> Some Source_virtio_blk + | Some _, _ -> None in let format = match xpath_string "driver/@type" with @@ -297,12 +304,13 @@ let parse_libvirt_xml ?conn xml = let controller = let target_bus = xpath_string "target/@bus" in - match target_bus with - | None -> None - | Some "ide" -> Some Source_IDE - | Some "scsi" -> Some Source_SCSI - | Some "virtio" -> Some Source_virtio_blk - | Some _ -> None in + match target_bus, has_virtio_scsi with + | None, _ -> None + | Some "ide", _ -> Some Source_IDE + | Some "scsi", true -> Some Source_virtio_SCSI + | Some "scsi", false -> Some Source_SCSI + | Some "virtio", _ -> Some Source_virtio_blk + | Some _, _ -> None in let slot = let target_dev = xpath_string "target/@dev" in diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml index 9487f0b..2572de6 100644 --- a/v2v/output_glance.ml +++ b/v2v/output_glance.ml @@ -86,6 +86,7 @@ object "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 diff --git a/v2v/test-v2v-print-source.sh b/v2v/test-v2v-print-source.sh index 8af6104..789555d 100755 --- a/v2v/test-v2v-print-source.sh +++ b/v2v/test-v2v-print-source.sh @@ -63,7 +63,7 @@ hypervisor type: test video: qxl sound: disks: - /windows.img (raw) [virtio] + /windows.img (raw) [virtio-blk] removable media: NICs: Network \"default\" mac: 00:11:22:33:44:55 [virtio]" ]; then diff --git a/v2v/types.ml b/v2v/types.ml index 821b7ec..9a23b4a 100644 --- a/v2v/types.ml +++ b/v2v/types.ml @@ -55,7 +55,8 @@ and source_disk = { s_format : string option; s_controller : s_controller option; } -and s_controller = Source_IDE | Source_SCSI | Source_virtio_blk +and s_controller = Source_IDE | Source_SCSI | + Source_virtio_blk | Source_virtio_SCSI and source_removable = { s_removable_type : s_removable_type; s_removable_controller : s_controller option; @@ -187,7 +188,8 @@ and string_of_source_disk { s_qemu_uri = qemu_uri; s_format = format; and string_of_controller = function | Source_IDE -> "ide" | Source_SCSI -> "scsi" - | Source_virtio_blk -> "virtio" + | Source_virtio_blk -> "virtio-blk" + | Source_virtio_SCSI -> "virtio-scsi" and string_of_source_removable { s_removable_type = typ; s_removable_controller = controller; @@ -366,13 +368,14 @@ and requested_guestcaps = { rcaps_net_bus : guestcaps_net_type option; rcaps_video : guestcaps_video_type option; } -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_video_type = QXL | Cirrus let string_of_block_type block_type = (match block_type with | Virtio_blk -> "virtio-blk" + | Virtio_SCSI -> "virtio-scsi" | IDE -> "ide") let string_of_net_type net_type = (match net_type with diff --git a/v2v/types.mli b/v2v/types.mli index 4595a1f..71cfa31 100644 --- a/v2v/types.mli +++ b/v2v/types.mli @@ -65,12 +65,9 @@ and source_disk = { } (** A source disk. *) -and s_controller = Source_IDE | Source_SCSI | Source_virtio_blk -(** Source disk controller. - - For the purposes of this field, we can treat virtio-scsi as - [SCSI]. However we don't support conversions from virtio in any - case so virtio is here only to make it work for testing. *) +and s_controller = Source_IDE | Source_SCSI | + Source_virtio_blk | Source_virtio_SCSI +(** Source disk controller. *) and source_removable = { s_removable_type : s_removable_type; (** Type. *) @@ -218,10 +215,13 @@ and requested_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_video_type = QXL | Cirrus +val string_of_block_type : guestcaps_block_type -> string +val string_of_net_type : guestcaps_net_type -> string +val string_of_video : guestcaps_video_type -> string val string_of_guestcaps : guestcaps -> string val string_of_requested_guestcaps : requested_guestcaps -> string diff --git a/v2v/v2v.ml b/v2v/v2v.ml index 4d0d525..647ae11 100644 --- a/v2v/v2v.ml +++ b/v2v/v2v.ml @@ -731,10 +731,11 @@ and do_convert g inspect source keep_serial_console rcaps = (* Did we manage to install virtio drivers? *) if not (quiet ()) then ( - if guestcaps.gcaps_block_bus = Virtio_blk then - info (f_"This guest has virtio drivers installed.") - else - info (f_"This guest does not have virtio drivers installed."); + match guestcaps.gcaps_block_bus with + | Virtio_blk | Virtio_SCSI -> + info (f_"This guest has virtio drivers installed.") + | _ -> + info (f_"This guest does not have virtio drivers installed.") ); guestcaps @@ -939,6 +940,7 @@ and target_bus_assignment source targets guestcaps = let t = BusSlotTarget t in match guestcaps.gcaps_block_bus with | Virtio_blk -> insert virtio_blk_bus i t + | Virtio_SCSI -> insert scsi_bus i t | IDE -> insert ide_bus i t ) targets; @@ -952,7 +954,7 @@ and target_bus_assignment source targets guestcaps = | None -> ide_bus (* Wild guess, but should be safe. *) | Some Source_virtio_blk -> virtio_blk_bus | Some Source_IDE -> ide_bus - | Some Source_SCSI -> scsi_bus in + | Some Source_virtio_SCSI | Some Source_SCSI -> scsi_bus in match r.s_removable_slot with | None -> ignore (insert_after bus 0 (BusSlotRemovable r)) | Some desired_slot_nr -> @@ -992,6 +994,7 @@ and rcaps_from_source source = let block_type = match source_block_type with | Some Source_virtio_blk -> Some Virtio_blk + | Some Source_virtio_SCSI -> Some Virtio_SCSI | Some Source_IDE -> Some IDE | Some t -> error (f_"source has unsupported hard disk type '%s'") (string_of_controller t) diff --git a/v2v/windows_virtio.ml b/v2v/windows_virtio.ml index 7ca4068..a54dbca 100644 --- a/v2v/windows_virtio.ml +++ b/v2v/windows_virtio.ml @@ -35,6 +35,7 @@ let virtio_win = let scsi_class_guid = "{4D36E97B-E325-11CE-BFC1-08002BE10318}" let viostor_pciid = "VEN_1AF4&DEV_1001&SUBSYS_00021AF4&REV_00" +let vioscsi_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00" let rec install_drivers g inspect systemroot root current_cs rcaps = (* Copy the virtio drivers to the guest. *) @@ -43,7 +44,7 @@ let rec install_drivers g inspect systemroot root current_cs rcaps = if not (copy_drivers g inspect driverdir) then ( match rcaps with - | { rcaps_block_bus = Some Virtio_blk } + | { rcaps_block_bus = (Some Virtio_blk | Some Virtio_SCSI) } | { rcaps_net_bus = Some Virtio_net } | { rcaps_video = Some QXL } -> error (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s") @@ -66,19 +67,25 @@ let rec install_drivers g inspect systemroot root current_cs rcaps = (* Can we install the block driver? *) let block : guestcaps_block_type = let has_viostor = g#exists (driverdir // "viostor.inf") in - match rcaps.rcaps_block_bus, has_viostor with - | Some Virtio_blk, false -> + let has_vioscsi = g#exists (driverdir // "vioscsi.inf") in + match rcaps.rcaps_block_bus, has_viostor, has_vioscsi with + | Some Virtio_blk, false, _ -> error (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") inspect.i_major_version inspect.i_minor_version inspect.i_arch virtio_win - | None, false -> + | Some Virtio_SCSI, _, false -> + error (f_"there is no vioscsi (virtio SCSI passthrough) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") + inspect.i_major_version inspect.i_minor_version + inspect.i_arch virtio_win + + | None, false, _ -> warning (f_"there is no viostor (virtio block device) driver for this version of Windows (%d.%d %s). virt-v2v looks for this driver in %s\n\nThe guest will be configured to use a slower emulated device.") inspect.i_major_version inspect.i_minor_version inspect.i_arch virtio_win; IDE - | (Some Virtio_blk | None), true -> + | (Some Virtio_blk | None), true, _ -> (* Block driver needs tweaks to allow booting; the rest is set up by PnP * manager *) let source = driverdir // "viostor.sys" in @@ -89,7 +96,18 @@ let rec install_drivers g inspect systemroot root current_cs rcaps = viostor_pciid; Virtio_blk - | Some IDE, _ -> + | Some Virtio_SCSI, _, true -> + (* Block driver needs tweaks to allow booting; the rest is set up by PnP + * manager *) + let source = driverdir // "vioscsi.sys" in + let target = sprintf "%s/system32/drivers/vioscsi.sys" systemroot in + let target = g#case_sensitive_path target in + g#cp source target; + add_guestor_to_registry g root current_cs "vioscsi" + vioscsi_pciid; + Virtio_SCSI + + | Some IDE, _, _ -> IDE in (* Can we install the virtio-net driver? *) -- 2.5.5
On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
(match guestcaps.gcaps_block_bus with - | Virtio_blk -> "VirtIO" | IDE -> "IDE"); + | Virtio_blk -> "VirtIO" + | Virtio_SCSI -> "SCSI"
I believe this should be "VirtIO_SCSI", although it's difficult to get a straight answer from the massive and convoluted Java sources of ovirt-engine ...
@@ -1148,14 +1149,15 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = List.iter (fun path -> ignore (g#aug_rm path)) (List.rev paths_to_delete);
- g#aug_set (path ^ "/modulename") "virtio_blk" + g#aug_set (path ^ "/modulename") + (string_of_block_type block_type)
Here ...
) else ( (* We have to add a scsi_hostadapter. *) let modpath = discover_modpath () in g#aug_set (sprintf "/files%s/alias[last()+1]" modpath) "scsi_hostadapter"; g#aug_set (sprintf "/files%s/alias[last()]/modulename" modpath) - "virtio_blk" + (string_of_block_type block_type) )
... and here don't seem right. Firstly, string_of_block_type returns "virtio-blk" (not "virtio_blk"). Maybe for this configuration file it doesn't matter. But, the real problem is that string_of_block_type is essentially an internal debugging function. What we should really have is a new function (eg. "linux_module_of_block_type") which converts the block type to a Linux module name.
+ (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK. A true test would involve checking the <target bus="scsi"> on each disk and matching it back to the controller. In other words, a huge pain! Maybe you can add an "XXX" comment in the source about this.
+ (Xml.xpathobj_nr_nodes obj) > 0 in ^ ^ You don't need these parentheses. In functional languages, function application always binds tightest of all, eg:
f x + 1 is the same as: (f x) + 1
diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml index 9487f0b..2572de6 100644 --- a/v2v/output_glance.ml +++ b/v2v/output_glance.ml @@ -86,6 +86,7 @@ object "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
This is wrong, or at least, incomplete. Apparently you need: hw_disk_bus=scsi (as above) hw_scsi_model=virtio-scsi (additional property) See: http://www.sebastien-han.fr/blog/2015/02/02/openstack-and-ceph-rbd-discard/
if not (copy_drivers g inspect driverdir) then ( match rcaps with - | { rcaps_block_bus = Some Virtio_blk } + | { rcaps_block_bus = (Some Virtio_blk | Some Virtio_SCSI) }
I don't think you need parens there. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote:
On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
(match guestcaps.gcaps_block_bus with - | Virtio_blk -> "VirtIO" | IDE -> "IDE"); + | Virtio_blk -> "VirtIO" + | Virtio_SCSI -> "SCSI"
I believe this should be "VirtIO_SCSI", although it's difficult to get a straight answer from the massive and convoluted Java sources of ovirt-engine ...
@@ -1148,14 +1149,15 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = List.iter (fun path -> ignore (g#aug_rm path)) (List.rev paths_to_delete);
- g#aug_set (path ^ "/modulename") "virtio_blk" + g#aug_set (path ^ "/modulename") + (string_of_block_type block_type)
Here ...
) else ( (* We have to add a scsi_hostadapter. *) let modpath = discover_modpath () in g#aug_set (sprintf "/files%s/alias[last()+1]" modpath) "scsi_hostadapter"; g#aug_set (sprintf "/files%s/alias[last()]/modulename" modpath) - "virtio_blk" + (string_of_block_type block_type) )
... and here don't seem right.
Firstly, string_of_block_type returns "virtio-blk" (not "virtio_blk"). Maybe for this configuration file it doesn't matter.
But, the real problem is that string_of_block_type is essentially an internal debugging function.
What we should really have is a new function (eg. "linux_module_of_block_type") which converts the block type to a Linux module name.
+ (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK. A true test would involve checking the <target bus="scsi"> on each disk and matching it back to the controller. In other words, a huge pain! Maybe you can add an "XXX" comment in the source about this.
+ (Xml.xpathobj_nr_nodes obj) > 0 in ^ ^ You don't need these parentheses. In functional languages, function application always binds tightest of all, eg:
f x + 1
is the same as:
(f x) + 1
diff --git a/v2v/output_glance.ml b/v2v/output_glance.ml index 9487f0b..2572de6 100644 --- a/v2v/output_glance.ml +++ b/v2v/output_glance.ml @@ -86,6 +86,7 @@ object "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
This is wrong, or at least, incomplete.
Apparently you need:
hw_disk_bus=scsi (as above) hw_scsi_model=virtio-scsi (additional property)
See: http://www.sebastien-han.fr/blog/2015/02/02/openstack-and-ceph-rbd-discard/
if not (copy_drivers g inspect driverdir) then ( match rcaps with - | { rcaps_block_bus = Some Virtio_blk } + | { rcaps_block_bus = (Some Virtio_blk | Some Virtio_SCSI) }
I don't think you need parens there.
Thanks a lot for the review, I'll fix the comments and respin. Roman.
On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote:
On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
+ (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK. A true test would involve checking the <target bus="scsi"> on each disk and matching it back to the controller. In other words, a huge pain! Maybe you can add an "XXX" comment in the source about this.
On a second look, I'm not sure I got your comment right. AFAICS there's no way in libvirt xml to define multiple scsi buses, so all the matching I can do is just to notice the presence of virtio-scsi controller, and attribute all SCSI drives to it. This is exactly what happens in the patch. Am I missing something? Thanks, Roman.
On Wed, Apr 13, 2016 at 07:45:42PM +0300, Roman Kagan wrote:
On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote:
On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
+ (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK. A true test would involve checking the <target bus="scsi"> on each disk and matching it back to the controller. In other words, a huge pain! Maybe you can add an "XXX" comment in the source about this.
On a second look, I'm not sure I got your comment right.
AFAICS there's no way in libvirt xml to define multiple scsi buses,
I think this is possible, although massively unlikely in practice. I think the different controllers would have different index="0"/index="1"/... attributes.
so all the matching I can do is just to notice the presence of virtio-scsi controller, and attribute all SCSI drives to it. This is exactly what happens in the patch.
Am I missing something?
So first off I think this part of the patch and your assumption is fine, there's no need to change the code. However there is a possibility that the source guest might have multiple mixed virtio-blk + virtio-scsi disks. In that case the disks would look like this: <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file=...> <target dev='vda' bus='virtio'/> </disk> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file=...> <target dev='sda' bus='scsi'/> </disk> <controller type="scsi" index="0" model="virtio-scsi"/> It's not something that we should worry about, and a simple comment with "XXX" in it is fine, if you agree with my analysis. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On Wed, Apr 13, 2016 at 06:39:55PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 13, 2016 at 07:45:42PM +0300, Roman Kagan wrote:
On Wed, Apr 13, 2016 at 10:11:54AM +0100, Richard W.M. Jones wrote:
On Tue, Apr 12, 2016 at 06:46:31PM +0300, Roman Kagan wrote:
+ (* Presence of virtio-scsi controller. *) + let has_virtio_scsi = + let obj = Xml.xpath_eval_expression xpathctx + "/domain/devices/controller[@model='virtio-scsi']" in
I guess this short cut is OK. A true test would involve checking the <target bus="scsi"> on each disk and matching it back to the controller. In other words, a huge pain! Maybe you can add an "XXX" comment in the source about this.
On a second look, I'm not sure I got your comment right.
AFAICS there's no way in libvirt xml to define multiple scsi buses,
I think this is possible, although massively unlikely in practice. I think the different controllers would have different index="0"/index="1"/... attributes.
How the binding of drives to controllers is done, then? Is it specified in drive's "address"? And indeed we probably don't have to worry about this case.
so all the matching I can do is just to notice the presence of virtio-scsi controller, and attribute all SCSI drives to it. This is exactly what happens in the patch.
Am I missing something?
So first off I think this part of the patch and your assumption is fine, there's no need to change the code.
However there is a possibility that the source guest might have multiple mixed virtio-blk + virtio-scsi disks. In that case the disks would look like this:
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file=...> <target dev='vda' bus='virtio'/> </disk> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file=...> <target dev='sda' bus='scsi'/> </disk> <controller type="scsi" index="0" model="virtio-scsi"/>
It's not something that we should worry about, and a simple comment with "XXX" in it is fine, if you agree with my analysis.
This case is handled fine in the patch, isn't it? has_virtio_scsi is a local variable which is only consulted when populating per-drive s_controller field: for drives with bus='scsi' it differentiates between Source_virtio_SCSI and Source_SCSI. The place where we make up a single disk type is v2v.ml:rcaps_from_source, but there I explicitly error out if the guest has multiple disk types (which may be too harsh but simplifies things for now). Roman.
On Wed, Apr 13, 2016 at 09:04:54PM +0300, Roman Kagan wrote:
On Wed, Apr 13, 2016 at 06:39:55PM +0100, Richard W.M. Jones wrote:
I think this is possible, although massively unlikely in practice. I think the different controllers would have different index="0"/index="1"/... attributes.
How the binding of drives to controllers is done, then? Is it specified in drive's "address"?
Yes, apparently using <address/> under <disk/>.
This case is handled fine in the patch, isn't it? has_virtio_scsi is a local variable which is only consulted when populating per-drive s_controller field: for drives with bus='scsi' it differentiates between Source_virtio_SCSI and Source_SCSI.
Oh I see, target_bus is consulted for each drive. So this is right (except for the scsi/virtio-scsi case, but who knows about that).
The place where we make up a single disk type is v2v.ml:rcaps_from_source, but there I explicitly error out if the guest has multiple disk types (which may be too harsh but simplifies things for now).
Yup. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (2)
-
Richard W.M. Jones -
Roman Kagan