Support import of VM/template for Managed block storage

[slavkap]

Changes introduced with this PR

• OVA import for virtual machines and templates is improved so imports work when the destination is managed block storage: the engine treats MBS like a first-class target for OVA extract/convert/import, disk handling, and validation, instead of only traditional storage domains.

This has been tested with MBS - StorPool and Ceph

Are you the owner of the code you are sending in, or do you have permission of the owner?

yes

[ryan-ronnander]

Hello, similar context again as on oVirt/vdsm#459 - testing on LINSTOR/DRBD.

While single-disk OVA import to an MBS storage domain worked on our cluster, multi-disk OVA imports always fail at extract time with 'No target disk path for OVA member'.

The pathToImageId -> UUID translation in extract_ova.py only lands for the single-disk case. The multi-disk processImages rebuild path never gets the translated mapping.

We took a swing at refactoring for reduced complexity and reduced LoC here: refactor branch (single commit on top of storpool-1138-ova-import).

With the refactor, we verified the imports with LINSTOR managed block storage by exporting then re-importing:

• VMs with 1, 2, and 3 disks on MBS
• a VM with mixed iSCSI + MBS disks
• a VM with iSCSI-only disks (no regressions on the non-MBS path)
• a template with an MBS disk

Happy to open a follow up PR with the branch above after this is merged. Or, if desired, pull the changes into this PR or let it inspire a similar refactoring?

Everything else works well, just a hiccup on multi-disk imports.

[peter-boden]

I mainly have structural suggestions/remarks. The current implementation weaves MBS-specific branches throughout existing classes using ifs etc. (e.g. if (isManagedBlockStorage()) checks. This makes each class harder to reason about,
you always have to mentally filter out the "other" path. My suggestion is to keep each class focused on one storage type and put MBS logic in dedicated places. A patch illustrating all of the below is attached: mbs-refactor-suggestion.patch

(mind you; generated with help of AI, not tested nor compiled, so just use it as a general guide 😅 )

Also, you don't HAVE to blindly agree with me, I'm open for discussion 👼

StorageDomainValidator, one predicate instead of six scattered checks (minor)

Both Cinder and MBS are externally managed backends where oVirt has no reliable free-space data. Right now each space-check method has
its own isCinderDomain() || isManagedBlockStorage() bypass, and three methods are already missing the MBS bypass (inconsistent, intentional?). Add a
single protected method:

protected boolean isExternallyManagedStorage() {                                                                                        
    return storageDomain.getStorageType().isCinderDomain()                                                                              
        || storageDomain.getStorageType().isManagedBlockStorage();                                                                      
}                                                                                                                                       

Replace all six bypass conditions with it.

ExtractOvaCommand + new MbsExtractOvaCommand, subclass instead of branching

In this PR ExtractOvaCommand imports ManagedBlockStorageCommandUtil, DisksFilter, ManagedBlockStorageDisk, and
DeviceInfoReturn, none of which belong in a general extraction command. The four things that actually differ between IRS and MBS are:
pre-image setup, how a disk path is resolved, how a disk is torn down, and how the image-mapping YAML is built. Extract those as
protected template methods with IRS defaults in the base class, and put all MBS-specific overrides in a new MbsExtractOvaCommand. The
parent import commands pick which action type to dispatch via a single overridable extractOvaActionType() method.

The same can be looked at for the other commands in bll/exportimport

DiskProfileHelper, the MBS check becomes redundant with the subclass approach

The PR adds an isManagedBlockStorageDomain() check inside DiskProfileHelper to skip profile validation for MBS destinations. This
works, but it means a general-purpose helper now needs to know about storage domain types, and it queries storageDomainDao once per
disk to do so.

With the MbsImportVmFromOvaCommand subclass approach this becomes unnecessary. Each MBS command simply overrides
setAndValidateDiskProfiles() to return true:

// MbsImportVmFromOvaCommand                                                                                                            
@Override                                                                                                                               
protected boolean setAndValidateDiskProfiles() {                                                                                        
    return true;  // MBS domains have no disk profiles                                                                                  
}                                                                                                                                       

DiskProfileHelper is then never called from an MBS command path in the first place, making the bypass inside it a no-op that can be
removed. The helper goes back to knowing nothing about storage domain types, and the StorageDomainDao injection added by this PR can be
dropped along with it.

The same override could again be added other Mbs...Commands where needed.

AddDiskCommand, fix the type mismatch at source instead of patching around it

isDiskImageTargetingManagedBlockStorageDomain() exists because during OVA import, disk objects arrive at AddDisk typed as IMAGE
even though they are going to an MBS domain. The command then has to inspect the storage domain in four separate places to route
correctly. The fix is to correct the type earlier: in adjustDisk() inside the import command, convert the DiskImage to a
ManagedBlockStorageDisk when the destination is MBS (add a static ManagedBlockStorageDisk.copyFrom(DiskImage)). AddDiskCommand then
sees MANAGED_BLOCK_STORAGE and the existing routing handles it — isDiskImageTargetingManagedBlockStorageDomain() and all four call
sites can be deleted.

The relevant changes:

• ManagedBlockStorageDisk.java, the new copyFrom() static factory
• MbsImportVmFromOvaCommand.java, the adjustDisk() override that calls copyFrom()
• AddDiskCommand.java, the four removed call sites and the deleted isDiskImageTargetingManagedBlockStorageDomain() method

[slavkap]

Thanks for the review, @peter-boden — I've addressed the structural points:

• Subclass split — MBS OVA extract/convert/import live in dedicated commands and action types. Base OVA commands keep the shared flow, with protected extension points for IRS defaults (prepareImagePath, teardownImage, extractOvaActionType, convertOvaActionType, etc.) that the MBS subclasses override only where needed.

• AddDisk — ManagedBlockStorageDisk.copyFrom() in import adjustDisk(); removed MBS workarounds from AddDiskCommand.

• Disk profiles — MBS import commands skip validation via override; DiskProfileHelper stays storage-agnostic.

• Space checks — Single isVendorManagedBlock() bypass in StorageDomainValidator for Cinder + MBS.

Ryan's multi-disk OVA extract fix is included too. Please take another look when you can.

[peter-boden]

I found one more issue that I missed in the previous review, and also a new issue that was introduced due to the refactor (kind-off my fault 🙈 )

[peter-boden]

This method pairs ovaTarNamesByIndex with diskPaths/diskList by index. These two lists come from different places and are not guaranteed to share the same order:

• ovaTarNamesByIndex is built in the parent (ImportVmFromOvaCommand.buildOvaTarNamesByIndex, ImportVmTemplateFromOvaCommand equivalent) from imageMappings, a Map<diskId, tarName> that is already keyed by disk id.
• diskList in the child is reloaded via updateDisksFromDb() -> diskDao.getAllForVm -> GetDisksVmGuid, which has no ORDER BY and joins a multi-table view. Order is not guaranteed.

When the two orders diverge, disk i's tar name gets paired with disk j's path, so the wrong OVA member is extracted into the wrong volume. Silent, no error. Single-disk imports can't hit this (index 0 to index 0 always matches),
which is likely why the 1/2/3-disk LINSTOR test passed but real-world multi-disk still risks corruption depending on plan/heap order.

Compounding issue: extract_ova.py has no coverage check. If a tar name the engine hands over never matches a member in the OVA, that disk is silently skipped and the playbook returns rc 0 with an empty volume.

Suggested fix: stop pairing by index.

1. On ConvertOvaParameters, replace List<String> ovaTarNamesByIndex with Map<Guid, String> diskIdToTarName. The parent already has the disk-id keyed map (imageMappings), just pass it through instead of flattening to a list.
2. In prepareDisksJson, look up the tar name via diskIdToTarName.get(diskList.get(i).getId()) instead of tarNames.get(i). Throw if a lookup misses or a tar name is duplicated, don't silently drop a disk.
3. In extract_ova.py, track a remaining = set(disks) and discard on match, sys.exit(1) if anything is left after the loop.

This removes the ordering dependency entirely, no ORDER BY/sort needed anywhere, and adds loud-failure behavior for a mismatch instead of a silent bad import.

[slavkap]

Thanks for catching both issues @peter-boden! For extract, we now pass diskIdToTarName (Map<Guid, String>) instead of pairing by index, fail on miss/duplicate in prepareDisksJson, and extract_ova.py exits 1 if any tar name is unmatched. For mixed template import, MbsImportVmTemplateFromOvaCommand no longer overrides setAndValidateDiskProfiles() with return true; it supplies a per-disk skip predicate so only MBS-destined disks skip profiles, using destination-based lookup in validate() (before adjustDisk/copyFrom). Destination resolution goes through getDestinationDomainIdForDisk() keyed by original OVF disk id so non-MBS disks in a mixed template still get profiles and the correct storage domain. Tested with mixed MBS (with StorPool) + NFS template OVA import.

[peter-boden]

Correction first: my earlier "subclass per storage type" suggestion works for VMs but does not
work for templates, and that assumption is why this gap exists.

VM import targets one storage domain for the whole import (getStorage().getSelectedItem()
on the frontend), so all disks are MBS or none. A blanket setAndValidateDiskProfiles() { return true; }
in MbsImportVmFromOvaCommand is correct there.

Template import is different. Per-disk destination is an established feature (predates this PR):
ImportDiskData carries its own selectedStorageDomain per disk, and buildImportTemplateFromOvaParameters
writes it to imageToDestinationDomainMap keyed per disk. So a single template can have
disk1 -> MBS and disk2 -> iSCSI, and a batch can mix an all-iSCSI template with an MBS template.

importsToManagedBlockStorage returns true if any disk of any template in the batch targets MBS,
so a mixed template or mixed batch runs the whole batch through MbsImportVmTemplateFromOvaCommand.
Its setAndValidateDiskProfiles() returns true unconditionally, so it skips disk-profile assignment
and the isDiskProfilePermitted check for every disk, including non-MBS disks. Before this refactor
the template command did not override setAndValidateDiskProfiles (it used the base per-disk
validation), so this is a regression. Non-MBS disks in a mixed template or batch then:

• get no default disk profile set when getDiskProfileId() == null, which is required on non-MBS domains
• skip the profile permission check, so a user without ATTACH_DISK_PROFILE rights on the
  destination domain can import a disk there unvalidated

On the fix, the per-disk MBS decision is a destination-domain property, not a command property,
and OvaImportManagedBlockSupport.diskTargetsManagedBlockStorage already computes it per disk.
Have DiskProfileHelper accept a skip predicate (keeps it storage-type-agnostic), the base
ImportVmTemplateCommandBase.setAndValidateDiskProfiles pass "skip nothing", and
MbsImportVmTemplateFromOvaCommand supply diskTargetsManagedBlockStorage as the predicate
instead of return true. That covers the mixed-batch and single-mixed-template cases and keeps
the discrimination out of the command body.

One lifecycle note: setAndValidateDiskProfiles runs in validate(), before adjustDisk/copyFrom
(which runs in executeCommand()/processImages). So at validation time MBS-destined disks are
still DiskImage typed as IMAGE, and DiskProfileHelper's existing != IMAGE skip does not catch
them. The discrimination has to be destination-based at this phase, not type-based. removeVmImages
runs post-adjustDisk/persist and already partitions correctly by getDiskStorageType() inside
removeDisksAfterFailedOvaImport, so that path does not need the same change.

[peter-boden]

Changes look good to me now, props 👍

I'm however not able to test these changes, so if e.g. @ryan-ronnander you could take another look? The changes are quite substantial. Especially a test with multiple disk could be interesting I think.

[ryan-ronnander]

Sure thing, just tested the recent changes against LINSTOR/DRBD MBS backend. All operations checked with sha256 against the original disk contents.

Looks good from our side 👍 , thanks @slavkap.

---

Test results table:

Test	Result
Multi-disk OVA round trip (2-disk MBS VM, export → import onto a second MBS domain)	PASS - both disks bit-for-bit, bootable/non-bootable pairing preserved
Mixed VM export/import (one iSCSI disk + one MBS disk → import onto MBS)	PASS - both transient-snapshot paths run in one export, both disks bit-for-bit
Template chain (MBS VM → template → OVA export → externaltemplateimports onto MBS → VM from imported template)	PASS - bit-for-bit through the whole chain (MbsImportVmTemplateFromOva)
Re-import of the same OVA (second import, new name)	PASS - fresh disk ids each import, no collision
Import of MBS origin OVA into non-MBS iSCSI domain	PASS - content bit-for-bit

[slavkap]

@peter-boden, @ryan-ronnander, thank you for your time to review and test this!