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(a)redhat.com&gt; Fixes: a2a4f7a09996a5e66d027d0d9692e083eb0a8128 Reported-by: Nir Soffer <nsoffer(a)redhat.com&gt; Suggested-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- 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_Res... xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Vir... 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(a)redhat.com&gt; 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(a)redhat.com&gt; > Fixes: a2a4f7a09996a5e66d027d0d9692e083eb0a8128 > Reported-by: Nir Soffer <nsoffer(a)redhat.com&gt; > Suggested-by: Richard W.M. Jones <rjones(a)redhat.com&gt; > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > 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_Res... xmlns:vssd='http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_Vir... 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(a)redhat.com&gt; 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(a)redhat.com&gt;

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(a)redhat.com&gt; Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2034240 Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; + Message-Id: <20211220135640.12436-1-lersek(a)redhat.com&gt; + Tested-by: Nir Soffer <nsoffer(a)redhat.com&gt; + [lersek(a)redhat.com: drop parens around condition in "if"; thanks: Rich] + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; 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.