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

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 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)?

Thanks
Laszlo