extract OVA: join disks by tar member name; fix multi-disk imports - #1
Conversation
- enables multi-disk OVA imports to managed block storage domains - extract_ova.py joins disks by tar member name directly, removing the pathToImageId sub-field and image_mappings argv that bridged OVF and engine-fresh UUID universes - drops the now-orphaned imageMappings field on ConvertOvaParameters (the OVA path stashes the diskId-to-tar-name map on the parent ImportVmFromOvaParameters and resolves it once into ovaTarNamesByIndex before handing off to ConvertOva / ExtractOva) - tested on LINSTOR by exporting then re-importing: VMs with 1, 2, and 3 disks on managed block storage, mixed iSCSI + managed block storage, iSCSI-only, and a template Signed-off-by: Ryan Ronnander <rronnander@linbit.com>
| import sys | ||
| import time | ||
|
|
||
|
|
There was a problem hiding this comment.
Thank you @ryan-ronnander, pylint complain that this line is deleted
There was a problem hiding this comment.
Apologies, I'm just seeing these review comments on GitHub now. I'll restore the blank line to satisfy pylint and push a fix to the branch.
| if target is not None: | ||
| extract_disk(ova_path, ova_file.tell(), | ||
| target["path"], target["format"]) | ||
| # Skip past the entry's body, rounded up to the next 512-byte boundary. |
There was a problem hiding this comment.
pylint also complains here that the line is too long
There was a problem hiding this comment.
Good catch, this one is still over 79 chars. I'll wrap the comment and push the fix.
There was a problem hiding this comment.
hey @ryan-ronnander, thanks! I will fix it in my PR, don't worry :)
There was a problem hiding this comment.
Thanks! Got ahead of myself, then realized it was merged. Needed more coffee this morning 🙂
|
Apologies for for missing these earlier, @slavkap, I only just spotted the review on GitHub. Since you've already merged the commit as-is, please feel free to fix the blank line and the long comment directly in oVirt#1138 however suits you. Thanks for picking it up! |
2 participants
Made one small change compared to the other day, removed a tiny bit of dead code. Retested everything, everything looks good to me 👍
Co-contributor patch on top of
import-ova-mbsper our Matrix conversation. Cross-vendor tested on LINSTOR/DRBD; happy to fold this in however you prefer for the upstream PR (squashed into one of your commits, kept as a separate commit onimport-ova-mbs, or rebased + amended differently).What this changes
Replaces the pre-extract
pathToImageId/image_mappingsround-trip with a direct tar-member-name join key:extract_ova.pyjoins disks by tar member name (the OVF<File ovf:href>value) instead of a per-image fresh UUID table. The Python script now takes only(ova_path, disks_json)argv; the third positionalimage_mappingsargument is gone.ImportVmFromOvaCommandandImportVmTemplateFromOvaCommandcapture the OVF tar names beforesuper.processImages()mints fresh disk IDs, then build afreshDiskId → tarNamemap that's stashed on the parent parameters and resolved once intoovaTarNamesByIndexfor the childConvertOvaCommand/ExtractOvaCommand.ConvertOvaParametersloses the now-orphanimageMappingsfield/getter/setter (the OVA path uses the parent'simageMappingsfromImportVmFromOvaParameters).OvaImportManagedBlockSupport.pathToImageIdHelperis removed — same reason; nothing reads it after the join key changed.Net delta: -132 LOC, +111 LOC across 7 files.
The motivation is multi-disk OVA imports: pre-refactor, multi-disk imports fail at extract with
'No target disk path for OVA member'because the originalpathToImageIdtable only gets populated for the single-disk case. Single-disk continues to work because the original code had a single-disk fallback that the multi-disk path doesn't reach. With the tar-member-name join key, single, multi, mixed, and template paths all share one mechanism.Test results on LINSTOR
Each imported VM/template lands with all disks
status=okon the destination domain with the rightdisk_storage_type; for MBS destinations, a corresponding LINSTOR resource definition exists in the configured resource group with the DRBD layerUpToDate. Engine.log clean across all five runs.DCO
Signed-off-by: Ryan Ronnander <rronnander@linbit.com>is on the commit per oVirt's DCO requirement. Happy to rewrite the commit message to fitimport-ova-mbs's convention if you'd like.