[v2v PATCH] output_rhv: restrict block status collection to the old RHV output
Nir reports that, despite the comment we removed in commit a2a4f7a09996, we generally cannot access the output NBD servers in the finalization stage. In particular, in the "rhv_upload_finalize" function [output/output_rhv_upload.ml], the output disks are disconnected before "create_ovf" is called. Consequently, the "get_disk_allocated" call in the "create_ovf" -> "add_disks" -> "get_disk_allocated" chain fails. Rich suggests that we explicitly restrict the "get_disk_allocated" call with a new optional boolean parameter to the one output plugin that really needs it, namely the old RHV one. Accordingly, revert the VDSM test case to its state at (a2a4f7a09996^). Cc: Nir Soffer <nsoffer@redhat.com> Fixes: a2a4f7a09996a5e66d027d0d9692e083eb0a8128 Reported-by: Nir Soffer <nsoffer@redhat.com> Suggested-by: Richard W.M. Jones <rjones@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/create_ovf.mli | 3 +- lib/create_ovf.ml | 35 +++++++++++++--------- output/output_rhv.ml | 4 +-- tests/test-v2v-o-vdsm-options.ovf.expected | 4 +-- 4 files changed, 27 insertions(+), 19 deletions(-) diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli index 0d1cc5a9311a..d6d4e62eeb86 100644 --- a/lib/create_ovf.mli +++ b/lib/create_ovf.mli @@ -46,7 +46,8 @@ val ovf_flavour_to_string : ovf_flavour -> string val create_ovf : Types.source -> Types.inspect -> Types.target_meta -> int64 list -> Types.output_allocation -> string -> string -> string list -> - string list -> string -> string -> ovf_flavour -> DOM.doc + string list -> ?need_actual_sizes:bool -> string -> string -> + ovf_flavour -> DOM.doc (** Create the OVF file. Actually a {!DOM} document is created, not a file. It can be written diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index dbac3405989b..678d29942abe 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -531,7 +531,8 @@ let rec create_ovf source inspect { output_name; guestcaps; target_firmware; target_nics } sizes output_alloc output_format - sd_uuid image_uuids vol_uuids dir vm_uuid ovf_flavour = + sd_uuid image_uuids vol_uuids ?(need_actual_sizes = false) dir + vm_uuid ovf_flavour = assert (List.length sizes = List.length vol_uuids); let memsize_mb = source.s_memory /^ 1024L /^ 1024L in @@ -745,7 +746,7 @@ let rec create_ovf source inspect (* Add disks to the OVF XML. *) add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf; + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf; (* Old virt-v2v ignored removable media. XXX *) @@ -791,7 +792,7 @@ and get_flavoured_section ovf ovirt_path rhv_path rhv_path_attr = function (* This modifies the OVF DOM, adding a section for each disk. *) and add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf = + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf = let references = let nodes = path_to_nodes ovf ["ovf:Envelope"; "References"] in match nodes with @@ -839,7 +840,12 @@ and add_disks sizes guestcaps output_alloc output_format b /^ 1073741824L in let size_gb = bytes_to_gb size in - let actual_size = get_disk_allocated ~dir ~disknr:i in + let actual_size = + if need_actual_sizes then + get_disk_allocated ~dir ~disknr:i + else + None + in let format_for_rhv = match output_format with @@ -891,16 +897,17 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; ] in - (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. If - * we don't know the actual size, we must create the attribute with - * empty contents. - *) - List.push_back attrs - ("ovf:actual_size", - match actual_size with - | None -> "" - | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) - ); + if (need_actual_sizes) then + (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) + ); e "Disk" !attrs [] in append_child disk disk_section; diff --git a/output/output_rhv.ml b/output/output_rhv.ml index b902a7ee4619..6a67b7aa152b 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -183,8 +183,8 @@ and rhv_finalize dir source inspect target_meta (* Create the metadata. *) let ovf = Create_ovf.create_ovf source inspect target_meta sizes - output_alloc output_format esd_uuid image_uuids vol_uuids dir vm_uuid - Create_ovf.RHVExportStorageDomain in + output_alloc output_format esd_uuid image_uuids vol_uuids + ~need_actual_sizes:true dir vm_uuid Create_ovf.RHVExportStorageDomain in (* Write it to the metadata file. *) let dir = esd_mp // esd_uuid // "master" // "vms" // vm_uuid in diff --git a/tests/test-v2v-o-vdsm-options.ovf.expected b/tests/test-v2v-o-vdsm-options.ovf.expected index bd5b5e7d38ec..23ca180f4c2f 100644 --- a/tests/test-v2v-o-vdsm-options.ovf.expected +++ b/tests/test-v2v-o-vdsm-options.ovf.expected @@ -2,7 +2,7 @@ <ovf:Envelope xmlns:rasd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSett...' xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingDa...' xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:ovf='http://schemas.dmtf.org/ovf/envelope/1/' xmlns:ovirt='http://www.ovirt.org/ovf' ovf:version='0.9'> <!-- generated by virt-v2v --> <References> - <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v' ovf:size='#SIZE#'/> + <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v'/> </References> <NetworkSection> <Info>List of networks</Info> @@ -10,7 +10,7 @@ </NetworkSection> <DiskSection> <Info>List of Virtual Disks</Info> - <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True' ovf:actual_size='1'/> + <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True'/> </DiskSection> <VirtualSystem ovf:id='VM'> <Name>windows</Name> -- 2.19.1.3.g30247aa5d201
On Mon, Dec 20, 2021 at 3:56 PM Laszlo Ersek <lersek@redhat.com> wrote:
Nir reports that, despite the comment we removed in commit a2a4f7a09996, we generally cannot access the output NBD servers in the finalization stage. In particular, in the "rhv_upload_finalize" function [output/output_rhv_upload.ml], the output disks are disconnected before "create_ovf" is called.
Consequently, the "get_disk_allocated" call in the "create_ovf" -> "add_disks" -> "get_disk_allocated" chain fails.
Rich suggests that we explicitly restrict the "get_disk_allocated" call with a new optional boolean parameter to the one output plugin that really needs it, namely the old RHV one.
Accordingly, revert the VDSM test case to its state at (a2a4f7a09996^).
Cc: Nir Soffer <nsoffer@redhat.com> Fixes: a2a4f7a09996a5e66d027d0d9692e083eb0a8128 Reported-by: Nir Soffer <nsoffer@redhat.com> Suggested-by: Richard W.M. Jones <rjones@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/create_ovf.mli | 3 +- lib/create_ovf.ml | 35 +++++++++++++--------- output/output_rhv.ml | 4 +-- tests/test-v2v-o-vdsm-options.ovf.expected | 4 +-- 4 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli index 0d1cc5a9311a..d6d4e62eeb86 100644 --- a/lib/create_ovf.mli +++ b/lib/create_ovf.mli @@ -46,7 +46,8 @@ val ovf_flavour_to_string : ovf_flavour -> string val create_ovf : Types.source -> Types.inspect -> Types.target_meta -> int64 list -> Types.output_allocation -> string -> string -> string list -> - string list -> string -> string -> ovf_flavour -> DOM.doc + string list -> ?need_actual_sizes:bool -> string -> string -> + ovf_flavour -> DOM.doc (** Create the OVF file.
Actually a {!DOM} document is created, not a file. It can be written diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index dbac3405989b..678d29942abe 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -531,7 +531,8 @@ let rec create_ovf source inspect { output_name; guestcaps; target_firmware; target_nics } sizes output_alloc output_format - sd_uuid image_uuids vol_uuids dir vm_uuid ovf_flavour = + sd_uuid image_uuids vol_uuids ?(need_actual_sizes = false) dir + vm_uuid ovf_flavour = assert (List.length sizes = List.length vol_uuids);
let memsize_mb = source.s_memory /^ 1024L /^ 1024L in @@ -745,7 +746,7 @@ let rec create_ovf source inspect
(* Add disks to the OVF XML. *) add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf; + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf;
(* Old virt-v2v ignored removable media. XXX *)
@@ -791,7 +792,7 @@ and get_flavoured_section ovf ovirt_path rhv_path rhv_path_attr = function
(* This modifies the OVF DOM, adding a section for each disk. *) and add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf = + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf = let references = let nodes = path_to_nodes ovf ["ovf:Envelope"; "References"] in match nodes with @@ -839,7 +840,12 @@ and add_disks sizes guestcaps output_alloc output_format b /^ 1073741824L in let size_gb = bytes_to_gb size in - let actual_size = get_disk_allocated ~dir ~disknr:i in + let actual_size = + if need_actual_sizes then + get_disk_allocated ~dir ~disknr:i + else + None + in
let format_for_rhv = match output_format with @@ -891,16 +897,17 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; ] in - (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. If - * we don't know the actual size, we must create the attribute with - * empty contents. - *) - List.push_back attrs - ("ovf:actual_size", - match actual_size with - | None -> "" - | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) - ); + if (need_actual_sizes) then + (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) + ); e "Disk" !attrs [] in append_child disk disk_section;
diff --git a/output/output_rhv.ml b/output/output_rhv.ml index b902a7ee4619..6a67b7aa152b 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -183,8 +183,8 @@ and rhv_finalize dir source inspect target_meta (* Create the metadata. *) let ovf = Create_ovf.create_ovf source inspect target_meta sizes - output_alloc output_format esd_uuid image_uuids vol_uuids dir vm_uuid - Create_ovf.RHVExportStorageDomain in + output_alloc output_format esd_uuid image_uuids vol_uuids + ~need_actual_sizes:true dir vm_uuid Create_ovf.RHVExportStorageDomain in
(* Write it to the metadata file. *) let dir = esd_mp // esd_uuid // "master" // "vms" // vm_uuid in diff --git a/tests/test-v2v-o-vdsm-options.ovf.expected b/tests/test-v2v-o-vdsm-options.ovf.expected index bd5b5e7d38ec..23ca180f4c2f 100644 --- a/tests/test-v2v-o-vdsm-options.ovf.expected +++ b/tests/test-v2v-o-vdsm-options.ovf.expected @@ -2,7 +2,7 @@ <ovf:Envelope xmlns:rasd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSett...' xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingDa...' xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:ovf='http://schemas.dmtf.org/ovf/envelope/1/' xmlns:ovirt='http://www.ovirt.org/ovf' ovf:version='0.9'> <!-- generated by virt-v2v --> <References> - <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v' ovf:size='#SIZE#'/> + <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v'/> </References> <NetworkSection> <Info>List of networks</Info> @@ -10,7 +10,7 @@ </NetworkSection> <DiskSection> <Info>List of Virtual Disks</Info> - <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True' ovf:actual_size='1'/> + <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True'/> </DiskSection> <VirtualSystem ovf:id='VM'> <Name>windows</Name> -- 2.19.1.3.g30247aa5d201
I don't fully understand the ocaml part, but the intent looks good, and the patch works for me. I tested it on rhel 8.6 host with upstream nbdkit, libnbd, and ovirt, virt-v2v master with your patch applied, and https://listman.redhat.com/archives/libguestfs/2021-December/msg00187.html Without https://listman.redhat.com/archives/libguestfs/2021-December/msg00189.html Tested using: $ cat v2v/v2v.sh #!/bin/sh vm_name=${1:-v2v} shift ./run virt-v2v \ -i disk /var/tmp/fedora-32.raw \ -o rhv-upload \ -oc https://engine-dev/ovirt-engine/api \ -op ~/.config/ovirt/engine-dev/password \ -on $vm_name \ -os alpine-nfs-00 \ -of qcow2 \ -oo rhv-cafile=/etc/pki/vdsm/certs/cacert.pem \ -oo rhv-cluster=el8 \ -oo rhv-direct=true \ "$@" $ ~/v2v/v2v.sh [ 12.7] Opening the source [ 16.6] Inspecting the source [ 40.9] Checking for sufficient free disk space in the guest [ 40.9] Converting Fedora 32 (Thirty Two) to run on KVM virt-v2v: warning: /files/boot/grub2/device.map/hd0 references unknown device "vda". You may have to fix this entry manually after conversion. virt-v2v: This guest has virtio drivers installed. [ 63.6] Mapping filesystem data to avoid copying unused and blank areas [ 64.6] Closing the overlay [ 64.7] Assigning disks to buses [ 64.7] Checking if the guest needs BIOS or UEFI to boot [ 64.7] Copying disk 1/1 █ 100% [****************************************] [ 68.7] Creating output metadata [ 79.0] Finishing off You can add Tested-by: Nir Soffer <nsoffer@redhat.com> Nir
On Mon, Dec 20, 2021 at 6:20 PM Nir Soffer <nsoffer@redhat.com> wrote: Attached also output of another run using -v
On 12/20/21 17:20, Nir Soffer wrote:
On Mon, Dec 20, 2021 at 3:56 PM Laszlo Ersek <lersek@redhat.com> wrote:
Nir reports that, despite the comment we removed in commit a2a4f7a09996, we generally cannot access the output NBD servers in the finalization stage. In particular, in the "rhv_upload_finalize" function [output/output_rhv_upload.ml], the output disks are disconnected before "create_ovf" is called.
Consequently, the "get_disk_allocated" call in the "create_ovf" -> "add_disks" -> "get_disk_allocated" chain fails.
Rich suggests that we explicitly restrict the "get_disk_allocated" call with a new optional boolean parameter to the one output plugin that really needs it, namely the old RHV one.
Accordingly, revert the VDSM test case to its state at (a2a4f7a09996^).
Cc: Nir Soffer <nsoffer@redhat.com> Fixes: a2a4f7a09996a5e66d027d0d9692e083eb0a8128 Reported-by: Nir Soffer <nsoffer@redhat.com> Suggested-by: Richard W.M. Jones <rjones@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- lib/create_ovf.mli | 3 +- lib/create_ovf.ml | 35 +++++++++++++--------- output/output_rhv.ml | 4 +-- tests/test-v2v-o-vdsm-options.ovf.expected | 4 +-- 4 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli index 0d1cc5a9311a..d6d4e62eeb86 100644 --- a/lib/create_ovf.mli +++ b/lib/create_ovf.mli @@ -46,7 +46,8 @@ val ovf_flavour_to_string : ovf_flavour -> string val create_ovf : Types.source -> Types.inspect -> Types.target_meta -> int64 list -> Types.output_allocation -> string -> string -> string list -> - string list -> string -> string -> ovf_flavour -> DOM.doc + string list -> ?need_actual_sizes:bool -> string -> string -> + ovf_flavour -> DOM.doc (** Create the OVF file.
Actually a {!DOM} document is created, not a file. It can be written diff --git a/lib/create_ovf.ml b/lib/create_ovf.ml index dbac3405989b..678d29942abe 100644 --- a/lib/create_ovf.ml +++ b/lib/create_ovf.ml @@ -531,7 +531,8 @@ let rec create_ovf source inspect { output_name; guestcaps; target_firmware; target_nics } sizes output_alloc output_format - sd_uuid image_uuids vol_uuids dir vm_uuid ovf_flavour = + sd_uuid image_uuids vol_uuids ?(need_actual_sizes = false) dir + vm_uuid ovf_flavour = assert (List.length sizes = List.length vol_uuids);
let memsize_mb = source.s_memory /^ 1024L /^ 1024L in @@ -745,7 +746,7 @@ let rec create_ovf source inspect
(* Add disks to the OVF XML. *) add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf; + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf;
(* Old virt-v2v ignored removable media. XXX *)
@@ -791,7 +792,7 @@ and get_flavoured_section ovf ovirt_path rhv_path rhv_path_attr = function
(* This modifies the OVF DOM, adding a section for each disk. *) and add_disks sizes guestcaps output_alloc output_format - sd_uuid image_uuids vol_uuids dir ovf_flavour ovf = + sd_uuid image_uuids vol_uuids need_actual_sizes dir ovf_flavour ovf = let references = let nodes = path_to_nodes ovf ["ovf:Envelope"; "References"] in match nodes with @@ -839,7 +840,12 @@ and add_disks sizes guestcaps output_alloc output_format b /^ 1073741824L in let size_gb = bytes_to_gb size in - let actual_size = get_disk_allocated ~dir ~disknr:i in + let actual_size = + if need_actual_sizes then + get_disk_allocated ~dir ~disknr:i + else + None + in
let format_for_rhv = match output_format with @@ -891,16 +897,17 @@ and add_disks sizes guestcaps output_alloc output_format "ovf:disk-type", "System"; (* RHBZ#744538 *) "ovf:boot", if is_bootable_drive then "True" else "False"; ] in - (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. If - * we don't know the actual size, we must create the attribute with - * empty contents. - *) - List.push_back attrs - ("ovf:actual_size", - match actual_size with - | None -> "" - | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) - ); + if (need_actual_sizes) then + (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) + ); e "Disk" !attrs [] in append_child disk disk_section;
diff --git a/output/output_rhv.ml b/output/output_rhv.ml index b902a7ee4619..6a67b7aa152b 100644 --- a/output/output_rhv.ml +++ b/output/output_rhv.ml @@ -183,8 +183,8 @@ and rhv_finalize dir source inspect target_meta (* Create the metadata. *) let ovf = Create_ovf.create_ovf source inspect target_meta sizes - output_alloc output_format esd_uuid image_uuids vol_uuids dir vm_uuid - Create_ovf.RHVExportStorageDomain in + output_alloc output_format esd_uuid image_uuids vol_uuids + ~need_actual_sizes:true dir vm_uuid Create_ovf.RHVExportStorageDomain in
(* Write it to the metadata file. *) let dir = esd_mp // esd_uuid // "master" // "vms" // vm_uuid in diff --git a/tests/test-v2v-o-vdsm-options.ovf.expected b/tests/test-v2v-o-vdsm-options.ovf.expected index bd5b5e7d38ec..23ca180f4c2f 100644 --- a/tests/test-v2v-o-vdsm-options.ovf.expected +++ b/tests/test-v2v-o-vdsm-options.ovf.expected @@ -2,7 +2,7 @@ <ovf:Envelope xmlns:rasd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSett...' xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingDa...' xmlns:xsi='http://www.w3.org/2001/XMLSchema-instance' xmlns:ovf='http://schemas.dmtf.org/ovf/envelope/1/' xmlns:ovirt='http://www.ovirt.org/ovf' ovf:version='0.9'> <!-- generated by virt-v2v --> <References> - <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v' ovf:size='#SIZE#'/> + <File ovf:href='VOL' ovf:id='VOL' ovf:description='generated by virt-v2v'/> </References> <NetworkSection> <Info>List of networks</Info> @@ -10,7 +10,7 @@ </NetworkSection> <DiskSection> <Info>List of Virtual Disks</Info> - <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True' ovf:actual_size='1'/> + <Disk ovf:diskId='IMAGE' ovf:size='1' ovf:capacity='536870912' ovf:fileRef='VOL' ovf:parentRef='' ovf:vm_snapshot_id='#UUID#' ovf:volume-format='COW' ovf:volume-type='Sparse' ovf:format='http://en.wikipedia.org/wiki/Byte' ovf:disk-interface='VirtIO' ovf:disk-type='System' ovf:boot='True'/> </DiskSection> <VirtualSystem ovf:id='VM'> <Name>windows</Name> -- 2.19.1.3.g30247aa5d201
I don't fully understand the ocaml part, but the intent looks good, and the patch works for me.
I tested it on rhel 8.6 host with upstream nbdkit, libnbd, and ovirt,
virt-v2v master with your patch applied, and https://listman.redhat.com/archives/libguestfs/2021-December/msg00187.html
Without https://listman.redhat.com/archives/libguestfs/2021-December/msg00189.html
Thank you Nir, this is a good and careful test then -- I first thought you applied the wrong patch from that series as well, but then realized you backed that one out. So I'm happy to take your Tested-by, thanks! Now -- I'm unsure if I should just go ahead and push this. Because I'm still new in the v2v team, I prefer to wait for an ACK from Rich. But Rich is on PTO, and we do honor that. How urgent is the fix? If it's urgent, I'll push it. Thanks! Laszlo
Tested using:
$ cat v2v/v2v.sh #!/bin/sh
vm_name=${1:-v2v} shift
./run virt-v2v \ -i disk /var/tmp/fedora-32.raw \ -o rhv-upload \ -oc https://engine-dev/ovirt-engine/api \ -op ~/.config/ovirt/engine-dev/password \ -on $vm_name \ -os alpine-nfs-00 \ -of qcow2 \ -oo rhv-cafile=/etc/pki/vdsm/certs/cacert.pem \ -oo rhv-cluster=el8 \ -oo rhv-direct=true \ "$@"
$ ~/v2v/v2v.sh [ 12.7] Opening the source [ 16.6] Inspecting the source [ 40.9] Checking for sufficient free disk space in the guest [ 40.9] Converting Fedora 32 (Thirty Two) to run on KVM virt-v2v: warning: /files/boot/grub2/device.map/hd0 references unknown device "vda". You may have to fix this entry manually after conversion. virt-v2v: This guest has virtio drivers installed. [ 63.6] Mapping filesystem data to avoid copying unused and blank areas [ 64.6] Closing the overlay [ 64.7] Assigning disks to buses [ 64.7] Checking if the guest needs BIOS or UEFI to boot [ 64.7] Copying disk 1/1 █ 100% [****************************************] [ 68.7] Creating output metadata [ 79.0] Finishing off
You can add
Tested-by: Nir Soffer <nsoffer@redhat.com>
Nir
On Mon, Dec 20, 2021 at 02:56:40PM +0100, Laszlo Ersek wrote:
+ if (need_actual_sizes) then
You don't need (and shouldn't use) the parentheses here.
+ (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size)
FYI I was going to suggest (untested!): actual_size |> Option.map (bytes_to_gb |> Int64.to_string) |> Option.default "" but actually once I wrote it out like this it's pretty clunky and obscure. If the match was less complex it could make sense to consider this, but I don't think in this case. Rest of the patch is all good, thanks, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> 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
On 12/21/21 14:47, Richard W.M. Jones wrote:
On Mon, Dec 20, 2021 at 02:56:40PM +0100, Laszlo Ersek wrote:
+ if (need_actual_sizes) then
You don't need (and shouldn't use) the parentheses here.
Ah, you are totally right -- "muscle memory" from C. I've been getting better at not using parens with "if"s in OCaml, but I still fail to catch myself sometimes. Funnily enough, in this single patch, I wrote *both* "if need_actual_sizes then" *and* "if (need_actual_sizes) then".
+ (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents. + *) + List.push_back attrs + ("ovf:actual_size", + match actual_size with + | None -> "" + | Some actual_size -> Int64.to_string (bytes_to_gb actual_size)
FYI I was going to suggest (untested!):
actual_size |> Option.map (bytes_to_gb |> Int64.to_string) |> Option.default ""
Interesting, thanks for teaching me this! ... after consulting <https://ocaml.org/api/Option.html>, is the last function "Option.value", rather than "Option.default"?
but actually once I wrote it out like this it's pretty clunky and obscure. If the match was less complex it could make sense to consider this, but I don't think in this case.
Rest of the patch is all good, thanks, so:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
I've removed the superfluous parens, and have re-run "make check". $ git range-diff --color \ master \ restrict-actual-size-to-rhv-upload-rhbz-2034240-v1 \ apply
1: 9695e6f2394f ! 1: 07b12fe99fb9 output_rhv: restrict block status collection to the old RHV output @@ -23,6 +23,10 @@ Suggested-by: Richard W.M. Jones <rjones@redhat.com> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek@redhat.com> + Message-Id: <20211220135640.12436-1-lersek@redhat.com> + Tested-by: Nir Soffer <nsoffer@redhat.com> + [lersek@redhat.com: drop parens around condition in "if"; thanks: Rich] + Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
diff --git a/lib/create_ovf.mli b/lib/create_ovf.mli --- a/lib/create_ovf.mli @@ -97,7 +101,7 @@ - | None -> "" - | Some actual_size -> Int64.to_string (bytes_to_gb actual_size) - ); -+ if (need_actual_sizes) then ++ if need_actual_sizes then + (* Ovirt-engine considers the "ovf:actual_size" attribute mandatory. + * If we don't know the actual size, we must create the attribute + * with empty contents.
Commit 07b12fe99fb9. Thanks! Laszlo
On Wed, Dec 22, 2021 at 10:34:25AM +0100, Laszlo Ersek wrote:
... after consulting <https://ocaml.org/api/Option.html>, is the last function "Option.value", rather than "Option.default"?
Actually we have our own Option module (mltools/std_utils.ml*). We have to wait until the oldest OCaml compiler we support (currently 4.05) rises to the first version which introduced this (4.08) and then we can switch over.
Commit 07b12fe99fb9.
Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (3)
-
Laszlo Ersek -
Nir Soffer -
Richard W.M. Jones