Libguestfs - Dec 2021 - [PATCH 2/5] v2v/lib/util.ml: Get disk allocation from input

On Sun, Dec 19, 2021 at 12:40 AM Nir Soffer <nsoffer at redhat.com>
wrote:
>
> On Sat, Dec 18, 2021 at 11:33 PM Richard W.M. Jones <rjones at
redhat.com> wrote:
> >
> > On Sat, Dec 18, 2021 at 10:36:30PM +0200, Nir Soffer wrote:
> > > After finalizing the transfer, virt-v2v try to connect to the
output
> > > socket and query disk allocation. This may work for some outputs
> > > supporting block status, but for rhv_upload output this cannot
work for
> > > 2 reasons:
> > > - The rhv-upload-plugin does not support extents
> > > - The transfer was finalized before this call, so the plugin lost
access
> > >   to the image.
> > >
> > > Here is an example failure log:
> > >
> > > [  74.2] Creating output metadata
> > > python3 '/tmp/v2v.WMq8Tk/rhv-upload-finalize.py'
'/tmp/v2v.WMq8Tk/params6.json'
> > > finalizing transfer b03fe3ba-a4ff-4634-a0a0-10b3daba3cc2
> > > ...
> > > transfer b03fe3ba-a4ff-4634-a0a0-10b3daba3cc2 finalized in 2.118
seconds
> > > ...
> > > nbdkit: debug: accepted connection
> > > ...
> > > nbdkit: python[4]: debug: python: close
> > > virt-v2v: error: exception: NBD.Error("nbd_block_status:
request out of
> > > bounds: Invalid argument", 22)
> > >
> > > Fix by using the input socket.
> > > ---
> > >  lib/utils.ml | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/utils.ml b/lib/utils.ml
> > > index d6861d08..f6c85543 100644
> > > --- a/lib/utils.ml
> > > +++ b/lib/utils.ml
> > > @@ -171,21 +171,21 @@ let with_nbd_connect_unix ~socket
~meta_contexts ~f > > >      ~f:(fun () ->
> > >            List.iter (NBD.add_meta_context nbd) meta_contexts;
> > >            NBD.connect_unix nbd socket;
> > >            protect
> > >              ~f:(fun () -> f nbd)
> > >              ~finally:(fun () -> NBD.shutdown nbd)
> > >         )
> > >      ~finally:(fun () -> NBD.close nbd)
> > >
> > >  let get_disk_allocated ~dir ~disknr > > > -  let socket
= sprintf "%s/out%d" dir disknr
> > > +  let socket = sprintf "%s/in%d" dir disknr
> >
> > This patch is definitely wrong - we need to get the allocation size
> > from the output disk.  Options such as -oa preallocated, and just
> > general issues like block size, nbdcopy sparseness detection etc,
> > would affect this.
> >
> > It probably indicates a problem with rhv-upload-plugin (again) that
> > it's not really prepared to be an idempotent part of a disk image
> > pipeline.
>
> This patch also definitely works for rhv-upload, we create working
> vms.
>
> It can be fixed in another way, maybe skipping the check when using
> rhv-upload because it cannot provide the needed info.
>
> I'm not sure why this check is needed - RHV already has everything
> it needs at this point, and it does not care about disk allocation after
> the disk was uploaded. If RHV needs any info about the disk, it can
> get it from storage.
>
> Maybe you add stuff to the ovf that RHV does need?
I see that this code is not in rhel-9.0.0 branch, so this patch is not
needed for https://bugzilla.redhat.com/2032324

Laszlo, can you explain why we need the number of allocated bytes
after the disk was already converted to the target storage?

Looking at the bug:
https://bugzilla.redhat.com/2027598

This is a fix for the issue in the old rhv output, which is deprecated and
should not be used in new code, and it is not compatible with rhv-upload
which is the modern way to import into RHV.

Also this cannot work for RHV now, so we need a way to disable
this when importing to RHV.

We can expose the block status in RHV plugin, but to use this we must
call block status after the disk is converted, but before the output
is finalized, since when you finalize RHV disconnect the disk from
the host, and the RHV plugin cannot access it any more.

The flow for rhv-upload should be:

    run nbdcopy
    query block status
    finalize output
    create ovf

Nir

On Sun, Dec 19, 2021 at 1:30 AM Nir Soffer <nsoffer at redhat.com>
wrote:
>
> On Sun, Dec 19, 2021 at 12:40 AM Nir Soffer <nsoffer at redhat.com>
wrote:
> >
> > On Sat, Dec 18, 2021 at 11:33 PM Richard W.M. Jones <rjones at
redhat.com> wrote:
> > >
> > > On Sat, Dec 18, 2021 at 10:36:30PM +0200, Nir Soffer wrote:
> > > > After finalizing the transfer, virt-v2v try to connect to
the output
> > > > socket and query disk allocation. This may work for some
outputs
> > > > supporting block status, but for rhv_upload output this
cannot work for
> > > > 2 reasons:
> > > > - The rhv-upload-plugin does not support extents
> > > > - The transfer was finalized before this call, so the plugin
lost access
> > > >   to the image.
> > > >
> > > > Here is an example failure log:
> > > >
> > > > [  74.2] Creating output metadata
> > > > python3 '/tmp/v2v.WMq8Tk/rhv-upload-finalize.py'
'/tmp/v2v.WMq8Tk/params6.json'
> > > > finalizing transfer b03fe3ba-a4ff-4634-a0a0-10b3daba3cc2
> > > > ...
> > > > transfer b03fe3ba-a4ff-4634-a0a0-10b3daba3cc2 finalized in
2.118 seconds
> > > > ...
> > > > nbdkit: debug: accepted connection
> > > > ...
> > > > nbdkit: python[4]: debug: python: close
> > > > virt-v2v: error: exception:
NBD.Error("nbd_block_status: request out of
> > > > bounds: Invalid argument", 22)
> > > >
> > > > Fix by using the input socket.
> > > > ---
> > > >  lib/utils.ml | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/utils.ml b/lib/utils.ml
> > > > index d6861d08..f6c85543 100644
> > > > --- a/lib/utils.ml
> > > > +++ b/lib/utils.ml
> > > > @@ -171,21 +171,21 @@ let with_nbd_connect_unix ~socket
~meta_contexts ~f > > > >      ~f:(fun () ->
> > > >            List.iter (NBD.add_meta_context nbd)
meta_contexts;
> > > >            NBD.connect_unix nbd socket;
> > > >            protect
> > > >              ~f:(fun () -> f nbd)
> > > >              ~finally:(fun () -> NBD.shutdown nbd)
> > > >         )
> > > >      ~finally:(fun () -> NBD.close nbd)
> > > >
> > > >  let get_disk_allocated ~dir ~disknr > > > > - 
let socket = sprintf "%s/out%d" dir disknr
> > > > +  let socket = sprintf "%s/in%d" dir disknr
> > >
> > > This patch is definitely wrong - we need to get the allocation
size
> > > from the output disk.  Options such as -oa preallocated, and just
> > > general issues like block size, nbdcopy sparseness detection etc,
> > > would affect this.
> > >
> > > It probably indicates a problem with rhv-upload-plugin (again)
that
> > > it's not really prepared to be an idempotent part of a disk
image
> > > pipeline.
> >
> > This patch also definitely works for rhv-upload, we create working
> > vms.
> >
> > It can be fixed in another way, maybe skipping the check when using
> > rhv-upload because it cannot provide the needed info.
> >
> > I'm not sure why this check is needed - RHV already has everything
> > it needs at this point, and it does not care about disk allocation
after
> > the disk was uploaded. If RHV needs any info about the disk, it can
> > get it from storage.
> >
> > Maybe you add stuff to the ovf that RHV does need?
>
> I see that this code is not in rhel-9.0.0 branch, so this patch is not
> needed for https://bugzilla.redhat.com/2032324
>
> Laszlo, can you explain why we need the number of allocated bytes
> after the disk was already converted to the target storage?
>
> Looking at the bug:
> https://bugzilla.redhat.com/2027598
>
> This is a fix for the issue in the old rhv output, which is deprecated and
> should not be used in new code, and it is not compatible with rhv-upload
> which is the modern way to import into RHV.
>
> Also this cannot work for RHV now, so we need a way to disable
> this when importing to RHV.
>
> We can expose the block status in RHV plugin,
This patch adds extents to rhv-upload-plugin:
https://listman.redhat.com/archives/libguestfs/2021-December/msg00197.html

It depends on fixing the python plugin extents callback:
https://listman.redhat.com/archives/libguestfs/2021-December/msg00196.html


> but to use this we must
> call block status after the disk is converted, but before the output
> is finalized, since when you finalize RHV disconnect the disk from
> the host, and the RHV plugin cannot access it any more.
>
> The flow for rhv-upload should be:
>
>     run nbdcopy
>     query block status
>     finalize output
>     create ovf
>
> Nir

On Sun, Dec 19, 2021 at 01:30:28AM +0200, Nir Soffer
wrote:
> On Sun, Dec 19, 2021 at 12:40 AM Nir Soffer <nsoffer at redhat.com>
wrote:
> >
> > It can be fixed in another way, maybe skipping the check when using
> > rhv-upload because it cannot provide the needed info.
> >
> > I'm not sure why this check is needed - RHV already has everything
> > it needs at this point, and it does not care about disk allocation
after
> > the disk was uploaded. If RHV needs any info about the disk, it can
> > get it from storage.
> >
> > Maybe you add stuff to the ovf that RHV does need?
So looking at the commit (a2a4f7a09996) it seems as if it applies to
all OVF outputs (including -o rhv-upload), but I think we only
intended to add it to -o rhv output.

Normally we'd do that by checking if ovf_flavor = RHVExportStorageDomain.
So I think a fix is to make the actual_size stuff dependent on that test.
> I see that this code is not in rhel-9.0.0 branch, so this patch is not
> needed for https://bugzilla.redhat.com/2032324
It is in the latest rhel-9.0.0 branch.
> Laszlo, can you explain why we need the number of allocated bytes
> after the disk was already converted to the target storage?
> 
> Looking at the bug:
> https://bugzilla.redhat.com/2027598
> 
> This is a fix for the issue in the old rhv output, which is deprecated and
> should not be used in new code, and it is not compatible with rhv-upload
> which is the modern way to import into RHV.
> 
> Also this cannot work for RHV now, so we need a way to disable
> this when importing to RHV.
"RHV" here is confusing - I think you mean -o rhv-upload here?
(not -o rhv)
> We can expose the block status in RHV plugin, but to use this we must
> call block status after the disk is converted, but before the output
> is finalized, since when you finalize RHV disconnect the disk from
> the host, and the RHV plugin cannot access it any more.
> 
> The flow for rhv-upload should be:
> 
>     run nbdcopy
>     query block status
>     finalize output
>     create ovf
I suppose this won't be needed if we make the change above, but it was
Laszlo who investigated this issue in detail so let's see what he says.

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/19/21 00:30, Nir Soffer wrote:
> Laszlo, can you explain why we need the number of allocated bytes
> after the disk was already converted to the target storage?
It's all described in commit a2a4f7a09996 ("lib/create_ovf: populate
"actual size" attributes again", 2021-12-10):
> commit a2a4f7a09996a5e66d027d0d9692e083eb0a8128
> Author: Laszlo Ersek <lersek at redhat.com>
> Date:   Fri Dec 10 12:35:37 2021 +0100
>
>     lib/create_ovf: populate "actual size" attributes again
>
>     Commit 255722cbf39a ("v2v: Modular virt-v2v", 2021-09-07)
removed the
>     following attributes from the OVF output:
>
>     - ovf:Envelope/References/File/@ovf:size
>     -
ovf:Envelope/Section[@xsi:type='ovf:DiskSection_Type']/Disk/@ovf:actual_size
>
>     Unfortunately, ovirt-engine considers the second one mandatory; without
>     it, ovirt-engine refuses to import the OVF.
>
>     Restore both attributes, using the utility functions added to the Utils
>     module previously.
>
>     (If we do not have the information necessary to fill in
@ovf:actual_size,
>     we still have to generate the attribute, only with empty contents.
>     Ovirt-engine does cope with that.)
>
>     Fixes: 255722cbf39afc0b012e2ac00d16fa6ba2f8c21f
>     Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598
>     Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>     Message-Id: <20211210113537.10907-4-lersek at redhat.com>
>     Acked-by: Richard W.M. Jones <rjones at redhat.com>
Basically if we place an OVF file in the Export Storage Domain directory
of ovirt-engine such that the OVF lacks the above-mentioned
"actual_size" attribute, then ovirt-engine rejects the OVF file with a
parse error.

This behavior is actually a regression from ovirt-engine commit
1082d9dec289 ("core: undo recent generalization of ovf processing",
2017-08-09; however, we triggered this ovirt-engine bug first in
virt-v2v commit 255722cbf39a ("v2v: Modular virt-v2v", 2021-09-07).
Therefore restoring the previous virt-v2v behavior makes sense. We could
of course just provide an empty actual_size='' attribute (as explained
above), but that would still not restore the original virt-v2v behavior,
and I cannot tell what the consequences for ovirt-engine would be.

Back to your email:

On 12/19/21 00:30, Nir Soffer wrote:
> Looking at the bug:
> https://bugzilla.redhat.com/2027598
>
> This is a fix for the issue in the old rhv output, which is deprecated
> and
I made the exact same argument before my analysis / fix, namely in
bullet (1) of <https://bugzilla.redhat.com/show_bug.cgi?id=2027598#c16>.

Please see Rich's answer to that at
<https://bugzilla.redhat.com/show_bug.cgi?id=2027598#c19>. That was the
reason we continued working on the analysis / patch at all.
> should not be used in new code, and it is not compatible with
> rhv-upload which is the modern way to import into RHV.
>
> Also this cannot work for RHV now, so we need a way to disable this
> when importing to RHV.
>
> We can expose the block status in RHV plugin, but to use this we must
> call block status after the disk is converted, but before the output
> is finalized, since when you finalize RHV disconnect the disk from the
> host, and the RHV plugin cannot access it any more.
The code very much depends on being run during finalization -- please
search commit a2a4f7a09996 for the word "finalization".

The disconnection that you describe -- does that happen in the
"rhv-upload-finalize.py" script?

This is what we've got in "output/output_rhv_upload.ml":
>   (* Finalize all the transfers. *)
>   let json_params >     let ids = List.map (fun id -> JSON.String id)
transfer_ids in
>     let json_params = ("transfer_ids", JSON.List ids) ::
json_params in
>     let ids = List.map (fun uuid -> JSON.String uuid) disk_uuids in
>     let json_params = ("disk_uuids", JSON.List ids) ::
json_params in
>     json_params in
>   if Python_script.run_command finalize_script json_params [] <> 0
then         <------- is the output disk detached here?
>     error (f_"failed to finalize the transfers, see earlier
errors");
>
>   (* The storage domain UUID. *)
>   let sd_uuid >     match rhv_storagedomain_uuid with
>     | None -> assert false
>     | Some uuid -> uuid in
>
>   (* The volume and VM UUIDs are made up. *)
>   let vol_uuids = List.map (fun _ -> uuidgen ()) disk_sizes
>   and vm_uuid = uuidgen () in
>
>   (* Create the metadata. *)
>   let ovf >     Create_ovf.create_ovf source inspect target_meta
disk_sizes                 <------- output NBD block status collected here
>       Sparse output_format sd_uuid disk_uuids vol_uuids dir vm_uuid
>       OVirt in
>   let ovf = DOM.doc_to_string ovf in
Back to your email:

On 12/19/21 00:30, Nir Soffer wrote:
> The flow for rhv-upload should be:
>
>     run nbdcopy
>     query block status
>     finalize output
>     create ovf
Currently, commit a2a4f7a09996 passes the temp dir, where the NBD
sockets live, to "create_ovf". Then "create_ovf" collects
the block
status for each disk.

One of the other approaches I outlined earlier was this: let each output
plugin collect the list of block statuses (-> "int64 option list")
before calling "create_ovf". Then pass that *list* to
"create_ovf".
Please see here
<https://listman.redhat.com/archives/libguestfs/2021-December/msg00116.html>:
> Yet another idea was to collect the actual sizes for all the output
> disks in one go, separately, then pass in that list, to create_ovf. Then
> in create_ovf, I would replace:
>
>   (* Iterate over the disks, adding them to the OVF document. *)
>   List.iteri (
>     fun i (size, image_uuid, vol_uuid) ->
> ...
>   ) (List.combine3 sizes image_uuids vol_uuids)
>
> with "combine4", adding a fourth component (the actual size) to
the
> tuple parameter of the nameless iterator function.
This complication did not seem justified at that point (see the last
paragraph at
<https://listman.redhat.com/archives/libguestfs/2021-December/msg00119.html>).

I can do this *if*:

- you can please tell me *where exactly* I should collect the block
  statuses inside the "rhv_upload_finalize" function
  [output/output_rhv_upload.ml] -- that is, if you can confirm where the
  disks get detached (because I need to collect the acutal sizes some
  place before that)

- AND we can confirm if the "actual sizes" are actually useful for
*all*
  output plugins different from the old rhv output. The whole thing is
  being done for the old rhv output's sake -- if neither
"rhv-upload"
  nor "vdsm" (= the other two OVF-creating output plugins) need the
  actual disk sizes, then the actual list construction should be unique
  to "output/output_rhv_upload.ml".

Sorry about the regression, my only excuse is that the comment that
commit a2a4f7a09996 removed:

-      (* virt-v2v 1.4x used to try to collect the actual size of the
-       * sparse disk.  It would be possible to get this information
-       * accurately now by reading the extent map of the output disk
-       * (this function is called during finalization), but we don't
-       * yet do that. XXX
-       *)

implied that fetching the extent map was safe during finalization. The
rhv-upload output plugin does not conform to that invariant however.

Thanks,
Laszlo