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

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?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

Thanks!
Laszlo