7:09 a.m.
On Mon, Dec 20, 2021 at 12:04 PM Laszlo Ersek <lersek(a)redhat.com> wrote:
On 12/19/21 00:30, Nir Soffer wrote:
> Laszlo, can you explain why we need the number of allocated bytes
> after the disk was already converted to the target storage?
It's all described in commit a2a4f7a09996 ("lib/create_ovf: populate
"actual size" attributes again", 2021-12-10):
> commit a2a4f7a09996a5e66d027d0d9692e083eb0a8128
> Author: Laszlo Ersek <lersek(a)redhat.com>
> Date: Fri Dec 10 12:35:37 2021 +0100
>
> lib/create_ovf: populate "actual size" attributes again
>
> Commit 255722cbf39a ("v2v: Modular virt-v2v", 2021-09-07) removed the
> following attributes from the OVF output:
>
> - ovf:Envelope/References/File/@ovf:size
> -
ovf:Envelope/Section[@xsi:type='ovf:DiskSection_Type']/Disk/@ovf:actual_size
>
> Unfortunately, ovirt-engine considers the second one mandatory; without
> it, ovirt-engine refuses to import the OVF.
>
> Restore both attributes, using the utility functions added to the Utils
> module previously.
>
> (If we do not have the information necessary to fill in @ovf:actual_size,
> we still have to generate the attribute, only with empty contents.
> Ovirt-engine does cope with that.)
>
> Fixes: 255722cbf39afc0b012e2ac00d16fa6ba2f8c21f
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> Message-Id: <20211210113537.10907-4-lersek(a)redhat.com>
> Acked-by: Richard W.M. Jones <rjones(a)redhat.com>
Basically if we place an OVF file in the Export Storage Domain directory
of ovirt-engine such that the OVF lacks the above-mentioned
"actual_size" attribute, then ovirt-engine rejects the OVF file with a
parse error.
This behavior is actually a regression from ovirt-engine commit
1082d9dec289 ("core: undo recent generalization of ovf processing",
2017-08-09; however, we triggered this ovirt-engine bug first in
virt-v2v commit 255722cbf39a ("v2v: Modular virt-v2v", 2021-09-07).
Therefore restoring the previous virt-v2v behavior makes sense. We could
of course just provide an empty actual_size='' attribute (as explained
above), but that would still not restore the original virt-v2v behavior,
and I cannot tell what the consequences for ovirt-engine would be.
Yes, this commit makes sense, and providing empty value sounds like
the right approach to continue supporting legacy code.
Back to your email:
On 12/19/21 00:30, Nir Soffer wrote:
> 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
I made the exact same argument before my analysis / fix, namely in
bullet (1) of <https://bugzilla.redhat.com/show_bug.cgi?id=2027598#c16>;.
Please see Rich's answer to that at
<https://bugzilla.redhat.com/show_bug.cgi?id=2027598#c19>;. That was the
reason we continued working on the analysis / patch at all.
> 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.
>
> 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 code very much depends on being run during finalization -- please
search commit a2a4f7a09996 for the word "finalization".
The disconnection that you describe -- does that happen in the
"rhv-upload-finalize.py" script?
The finalize script finalize the image transfer. On the oVirt side this does:
1. Removing the authentication ticket that let imageio access the disk.
2. Verifying the upload disk contents using qemu-img info (format, size
backing file). If verification fails the disk is deleted and the
transfer will
fail.
3. Disconnecting the disk from the host. For example for block based storage
we deactivate the logical volume.
4. Marking the disk is legal, so the rest of the system can use it.
This is what we've got in
"output/output_rhv_upload.ml":
> (* Finalize all the transfers. *)
> let json_params =
> let ids = List.map (fun id -> JSON.String id) transfer_ids in
> let json_params = ("transfer_ids", JSON.List ids) :: json_params in
> let ids = List.map (fun uuid -> JSON.String uuid) disk_uuids in
> let json_params = ("disk_uuids", JSON.List ids) :: json_params in
> json_params in
> if Python_script.run_command finalize_script json_params [] <> 0 then
<------- is the output disk detached here?
Yes, it cannot be accessed at this point.
...
> (* Create the metadata. *)
> let ovf =
> Create_ovf.create_ovf source inspect target_meta disk_sizes
<------- output NBD block status collected here
Right, and the target is not accessible now.
...
I can do this *if*:
- you can please tell me *where exactly* I should collect the block
statuses inside the "rhv_upload_finalize" function
[output/output_rhv_upload.ml] -- that is, if you can confirm where the
disks get detached (because I need to collect the acutal sizes some
place before that)
It must happen before we call the python finalize script.
But for this we need first to add extents support:
https://listman.redhat.com/archives/libguestfs/2021-December/msg00197.html
- AND we can confirm if the "actual sizes" are actually
useful for *all*
output plugins different from the old rhv output. The whole thing is
being done for the old rhv output's sake -- if neither "rhv-upload"
nor "vdsm" (= the other two OVF-creating output plugins) need the
actual disk sizes, then the actual list construction should be unique
to "output/output_rhv_upload.ml".
Right, spending time on providing useful actual_size makes sense
only if actual_size is useful. I'm not sure why engine wants actual_size from
the OVF. oVirt storage code does not need this information after a disk was
copied to storage.
What the storage needs is the size to allocate on block storage. We calculate
this using qemu-img measure:
qemu-img measure -O qcow2 /path/to/disk
The "required" value should be passed to the initial_size argument when
creating
a disk. Without this it is not possible to import sparse disks to
block based storage
in oVirt.
So for rhv-upload case we need this info after converting the image,
and before we
create a disk in oVirt.
So the flow should be:
start the input
convert the image using the overlay
measure the *overlay* disk to get the required size
create disk in oVirt
create transfer in oVirt
start the output
run ndbcopy with input and output
stop the input
finalize the transfer in oVirt
create ovf
import ovf in engine
For "rhv" case, if I understand this correctly, it creates disks in an
export domain
which is always NFS (oVirt limitation). The actual_size in the OVF is
not needed.
oVirt will measure the disks when importing them to a data domain anyway.
For "vdsm" case - virt-v2v creates the disks in a data domain. The
actual disks are
created before running virt-v2v, so the actual_size is not needed.
Sorry about the regression, my only excuse is that the comment that
commit a2a4f7a09996 removed:
- (* virt-v2v 1.4x used to try to collect the actual size of the
- * sparse disk. It would be possible to get this information
- * accurately now by reading the extent map of the output disk
- * (this function is called during finalization), but we don't
- * yet do that. XXX
- *)
implied that fetching the extent map was safe during finalization. The
rhv-upload output plugin does not conform to that invariant however.
I think the issue is that we don't have a good way to test changes without
having a real oVirt setup.
Nir