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

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(a)redhat.com&gt; 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)?