Restore VMware-to-KVM migration changes from #13656 - #14256

Thanks, @DaanHoogland . I’m keeping these comments. They document a non-obvious cancellation/import interaction and explain why cleanup must preserve the imported VM’s disks. If anything in that explanation is technically inaccurate, please point it out; the deletion suggestions don’t identify an error.

Given that the earlier PR was reverted over concerns about independent testing, it is rather surprising to see review time spent removing explanations instead of helping close that testing gap. With prebuilt packages available, an independent CBT migration test would be considerably more useful than making the source four comment lines shorter.

If you have time to contribute further, could you help verify that workflow and report any functional issues? That would address the concern that actually held this work back.

Thanks, @DaanHoogland . I’m keeping these comments. They document a non-obvious cancellation/import interaction and explain why cleanup must preserve the imported VM’s disks. If anything in that explanation is technically inaccurate, please point it out; the deletion suggestions don’t identify an error.

Given that the earlier PR was reverted over concerns about independent testing, it is rather surprising to see review time spent removing explanations instead of helping close that testing gap. With prebuilt packages available, an independent CBT migration test would be considerably more useful than making the source four comment lines shorter.

If you have time to contribute further, could you help verify that workflow and report any functional issues? That would address the concern that actually held this work back.

The connect step is only added for the single-path case. addAllVolumes (L162–174, not part of this diff) runs getDiskFileInfo on each disk from listPhysicalDisks. For Linstor, disk.getPath() is /dev/drbd/by-res/cs-…/0, which only exists if the resource has a replica or diskless resource on this host. For every other volume qemu-img info fails, info == null, and the continue skips it without any error. In the UI, the import-data-disk list for a Linstor pool therefore shows only the volumes that happen to be on whichever host the management server sent the command to.

The listing is also expensive on large controllers. For each resource there are 2 API calls in getPhysicalDisk (volumeDefinitionList and viewResources), 1 in getVolumeInUseNode (resourceList), and 2 qemu-img info runs.

For Linstor you should probably, don't inspect devices when listing. The format is always RAW, size and in-use state come from the controller, and backing files and qcow2 encryption can't apply to a raw DRBD device. A single viewResources call (filtered to the pool's resource group) can provide name, size and in-use state for all resources at once. Connect and inspect only in the single-path case, as now.

Thanks, René — good catch. Addressed in 01058cd.

LINSTOR import listing now uses read-only controller metadata rather than local device inspection. Resources are filtered by the pool’s resource group, replicas are deduplicated, and format, size and in-use state no longer depend on a local DRBD device being present.

The implementation uses paginated resource-definition, resource-view and node-status queries, eliminating the per-volume API/qemu-img inspection loop. Busy resources and resources whose usage cannot be established reliably are marked as locked.

Regression tests cover remote-only replicas, resource-group filtering, replica deduplication, pagination and unavailable/unknown usage states.

LinstorStorageAdaptor.connectPhysicalDisk returns true whenever resourceMakeAvailableOnNode succeeds, including when the resource was already on this node. linstorConnected therefore means "the call succeeded", not "we created the local resource". The disconnect then goes through tryDisconnectLinstor, which:

• deletes the local resource if it is diskless and not a tiebreaker, even if it existed before this check (e.g. placed by an admin, or left over from another operation);
• calls removeTwoPrimariesProps if the resource is in use on another node. If that volume is being live-migrated, this strips allow-two-primaries and protocol in the middle of the migration.

I suggest: before connecting, check whether a resource for this name already exists on the local node (one viewResources filtered to the local node and this resource), and only disconnect if this check created it. If the in-use check reports another node, skip the disconnect completely so the allow-two-primaries properties are left alone. Same pattern applies to LibvirtGetVolumesOnStorageCommandWrapper.java L72–82 (see comment there).

Thanks — agreed that a successful connect does not establish ownership of the local resource. Addressed in both wrappers in 01058cd.

I took a slightly different approach from tracking temporary-resource ownership: single-volume LINSTOR inspection now also uses read-only controller metadata, so neither inspection path calls connectPhysicalDisk or disconnectPhysicalDisk.

This removes the inspection-side risk of deleting an existing diskless resource or changing allow-two-primaries/protocol during live migration. Actual device connection remains the responsibility of the normal attach/start path; this metadata check does not claim to verify local device accessibility.

Regression tests verify that inspection does not call connect/disconnect or invoke qemu-img. The generic connection/disconnection implementation is unchanged.

Same issue as in LibvirtCheckVolumeCommandWrapper L89: this disconnect can remove a local diskless resource that existed before the call, and can strip allow-two-primaries from a resource that is in use elsewhere. It should only disconnect if this call created the local resource.

Please see comment/response above ^^^

This check keeps the listing to resources in the pool's own resource group. But importVolume path=… (GetVolumesOnStorage with a volume path) and importVm diskpath=… (CheckVolume) go straight to getPhysicalDisk(name), which accepts any cs-* resource on the controller. The duplicate checks on the management server only look within the target pool:

• VolumeImportUnmanageManagerImpl.java:370: volumeDao.findByPoolIdAndPath(pool.getId(), volumePath)
• UnmanagedVMsManagerImpl.java:3237: findByPoolIdAndPath(poolId, diskPath)

Resource names are unique per LINSTOR controller, and several CloudStack pools on one controller (different resource groups, e.g. SSD and HDD) is a common setup. Scenario:

1. Pool A (resource group rg-ssd) has volume X, i.e. resource cs-X, attached to a stopped VM.
2. An admin runs importVolume with storageid= (resource group rg-hdd, same controller) and path=X.
3. All checks pass. The in-use check doesn't help because nothing is running.
4. Two CloudStack volumes now point at the same DRBD resource. Deleting either one calls deleteResourceDefinition(cs-X), and the other volume's data is gone.

Suggested fix: on the management server, before sending CheckVolume or GetVolumesOnStorage for a Linstor pool, look up cs-<path> on the controller (the driver already builds the API client via LinstorUtil.getLinstorAPI(pool.getHostAddress(), …)) and reject the import if ResourceDefinition.getResourceGroupName() doesn't match the pool's resource group. That uses the controller's own data, covers resources this CloudStack database doesn't know about, and avoids comparing controller addresses across pools. It applies to both importVolume and importVm importsource=shared.

Thanks for the concrete cross-pool example — the stopped-VM case makes it clear why an in-use check alone is insufficient. Addressed in 01058cd.

Both DATA-volume import and shared-storage VM/ROOT import now validate the resource definition’s group against the selected pool’s group on the management server, before agent inspection and allocation. The VM import path also revalidates against the actual deployment pool before CheckVolume. Agent-side LINSTOR inspection independently enforces the same group check.

A mismatch, missing resource or failed ownership lookup rejects the import rather than allowing it to proceed. Regression tests cover cross-group rejection in both management paths and agent-side validation.

Across these fixes, the targeted local Maven run passed 195 unit tests with Checkstyle enabled.

Package rebuild and live LINSTOR/E2E validation are still pending.