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

[Laszlo Ersek]

On 12/20/21 11:17, 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)?

This flavor selection in the VDSM output plugin goes back to:

commit 0361ae81a16661e3b0bab052d28a9972b9514930
Author: Tomáš Golembiovský <tgolembi at redhat.com>
Date:   Thu Feb 22 11:41:08 2018 +0100

    v2v: vdsm: add --vdsm-fixed-ovf option
    
    Add option for -o vdsm that enables output of the modified OVF. oVirt
    engine should already be able to consume the OVF, but let's not take any
    chances and enable it only by command line argument. It can be made
    default later when it receives proper testing.
    
    Signed-off-by: Tomáš Golembiovský <tgolembi at redhat.com>

which tells me exactly nothing, unfortunately.

I think the only safe way to fix the issue is to *generally* go back to the OVF that virt-v2v used to generate before the modularization; that is, option (1).

Thanks,
Laszlo