Laszlo Ersek
2021-Dec-20 10:17 UTC
[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 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
Laszlo Ersek
2021-Dec-20 10:27 UTC
[Libguestfs] [PATCH 2/5] v2v/lib/util.ml: Get disk allocation from input
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
Richard W.M. Jones
2021-Dec-20 10:46 UTC
[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 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