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

[Richard W.M. Jones]

On Mon, Dec 20, 2021 at 12:28:01PM +0100, Laszlo Ersek wrote:
> On 12/20/21 11:46, Richard W.M. Jones wrote:
> > On Mon, Dec 20, 2021 at 11:17:47AM +0100, Laszlo Ersek wrote:
> >> On 12/19/21 11:34, Richard W.M. Jones wrote:
> >>> 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.
> >>
> >> We have a common utility function (create_ovf) that no longer works for
> >> all call sites.
> >>
> >> (1) One way to fix it is to let each call site compute the list of disk
> >> sizes ("int64 option list"), pass that in to "create_ovf", and let
> >> "create_ovf" merge (combine) that list with the other lists it already
> >> merges. The callers that don't need this functionality can just pass in
> >> a list of "None"s. I outlined this in my previous email.
> >>
> >> (2) The other way to fix it is to restrict the logic inside
> >> "create_ovf", based on some other (simpler) parameter that tells apart
> >> the callers. If you tell me that "ovf_flavor = RHVExportStorageDomain"
> >> is a condition I can rely on, in "create_ovf", I'm happy to use that --
> >> it's a lot simpler than the other option! Note that this would not
> >> generally restore the pre-modularization OVF output of virt-v2v, but
> >> maybe it does not matter for vdsm and rhv-upload?
> >>
> >> - "output_rhv.ml": passes in RHVExportStorageDomain as flavor
> >>
> >> - "output_rhv_upload.ml": passes OVirt as flavor
> >>
> >> - "output_vdsm.ml": well, this is tricky; this output plugin defaults to
> >> RHVExportStorageDomain, but can be flipped to OVirt via the "-oo
> >> vdsm-ovf-flavour" option! Does that match what we want to do in (2)?
> > 
> > It's a bit of a hack isn't it, but it would solve the problem
> > for now.
> > 
> > The fundamental problem here (again) is that OVF isn't a real format.
> > It's a collection of formats which share some common XML elements.  In
> > this case it turns out we now have 3 formats: RHVExportStorageDomain,
> > OVirt, and (for -o vdsm) DataDomain (since -o vdsm writes directly
> > into the oVirt Data Domain).
> > 
> > So ... I don't know ...  How about a ?(need_actual_sizes = false)
> > optional param on create_ovf?  Ugly but effective?
> 
> Can I perhaps change the "dir:string" parameter of create_ovf to
> "dir:string option"?

Overloading the meaning ... hmm.  I think a separate named parameter
would be clearer?  We won't forget in a year's time why it was this way.

Rich.

> Thanks!
> Laszlo

-- 
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/