Re: [Libguestfs] [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(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)? 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?