vmware: restore legacy VMDK data disk lifecycle (attachVolume HTTP 500 on VMware 8) - #13857
Open
H-LK wants to merge 2 commits into
Open
vmware: restore legacy VMDK data disk lifecycle (attachVolume HTTP 500 on VMware 8)#13857H-LK wants to merge 2 commits into
H-LK wants to merge 2 commits into
Conversation
Restore the classic worker-VM-based VMDK data-disk lifecycle on VMware 8 to avoid vCenter datastore-browser inconsistencies between FCD and VMDK representations that caused HTTP 500 errors during attachVolume. Port workaround from apc..., upstream of the fix-4.20.3.0 branch 'commit 18f2fd2 vmware: restore legacy VMDK data disk lifecycle'. Changes: - VmwareHelper: add getVirtualDiskType(Storage.ProvisioningType) mapping (THIN->THIN, SPARSE->PREALLOCATED, FAT->EAGER_ZEROED_THICK). - VirtualMachineMO: add createDisk overload accepting ProvisioningType, delegate via VmwareHelper; attachDisk(maxIops) delegates to new overload with updateVmdkAdapter flag defaulting to true (guard only evaluated when updateVmdkAdapter is true). - VmwareStorageLayoutHelper: syncVolumeToVmDefaultFolder now exposes a 7-arg Pair<String,Boolean> overload (targetAdapterType + provisioningType) that moves root/deprecated-legacy or fcd/ base-folder VMDKs into the VM default folder using VirtualDiskManagerMO with an optional FileBackedVirtualDiskSpec adapter/disk mapping, returning whether the adapter type had to change. - VmwareStorageProcessor: createVolume always uses the classic worker-VM VMDK path via getDeprecatedLegacyDatastorePathFromVmdkFileName and createDisk(volume.getProvisioningType()); cleanup wrapped in try/catch with warning instead of throwing. attachVolume uses the new overload and skips the vmdk adapter update when the sync already migrated the disk. Tests: - VmwareHelperTest: add getVirtualDiskTypeMapsCloudStackProvisioningTypes. - VmwareStorageLayoutHelperTest: add three tests covering root move, root copy+delete with adapter/provisioning, and base-folder copy+delete.
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
14 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks the VMware (vSphere API 8) data-disk lifecycle to avoid vCenter datastore-browser (HTTP) interactions during attachVolume, by restoring the legacy worker-VM VMDK creation path and migrating legacy disk locations into the VM’s default folder using vSphere APIs.
Changes:
- Add
ProvisioningType -> VirtualDiskTypemapping viaVmwareHelper.getVirtualDiskType, and use it when creating/copying VMDKs. - Extend disk attach/create flows to support provisioning-type-aware VMDK creation and optionally skip VMDK descriptor adapter updates.
- Update volume attach/create logic to remove the FCD path and to migrate legacy/root/base-folder VMDKs into the VM folder using
VirtualDiskManagerMO(with new unit tests).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vmware-base/src/test/java/com/cloud/hypervisor/vmware/util/VmwareHelperTest.java | Adds unit test coverage for provisioning type → vSphere disk type mapping. |
| vmware-base/src/main/java/com/cloud/hypervisor/vmware/util/VmwareHelper.java | Introduces getVirtualDiskType helper used by create/copy flows. |
| vmware-base/src/main/java/com/cloud/hypervisor/vmware/mo/VirtualMachineMO.java | Adds provisioning-type-aware createDisk overload and attachDisk(..., updateVmdkAdapter) overload. |
| plugins/hypervisors/vmware/src/test/java/com/cloud/storage/resource/VmwareStorageLayoutHelperTest.java | Adds tests for move vs copy+delete migration using VirtualDiskManagerMO. |
| plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageProcessor.java | Removes FCD volume creation path, adds migration+skip-update logic for attach, and hardens worker-VM cleanup. |
| plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageLayoutHelper.java | Adds migration support for deprecated legacy paths using VirtualDiskManagerMO and returns (path, didCopy) result. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2084
to
+2088
| VmdkAdapterType targetAdapterType = VmdkAdapterType.getAdapterType(DiskControllerType.getType(diskController)); | ||
| Pair<String, Boolean> syncResult = VmwareStorageLayoutHelper.syncVolumeToVmDefaultFolder(dsMo.getOwnerDatacenter().first(), vmName, dsMo, | ||
| volumePath, VmwareManager.s_vmwareSearchExcludeFolder.value(), targetAdapterType, volumeTO.getProvisioningType()); | ||
| datastoreVolumePath = syncResult.first(); | ||
| updateVmdkAdapter = !syncResult.second(); |
Contributor
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Contributor
|
I did a quick browse and see no big strangities, needs test/verification though. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13857 +/- ##
======================================
Coverage 3.41% 3.41%
======================================
Files 487 487
Lines 41867 41867
Branches 7912 7912
======================================
Hits 1429 1429
Misses 40238 40238
Partials 200 200
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18839 |
Contributor
|
@H-LK some test errors: |
…ts (apache#13249) Two new VmwareStorageLayoutHelperTest cases (syncVolumeToVmDefaultFolderCopiesRootVolumeWithTargetAdapterAndProvisioningType and syncVolumeToVmDefaultFolderCopiesBaseFolderVolumeWithTargetAdapter) mixed raw values with an ArgumentCaptor while verifying copyVirtualDisk, which triggers Mockito InvalidUseOfMatchers because once any argument is a matcher, all arguments must be matchers. Wrap all non-captor arguments in eq(...) when capturing the FileBackedVirtualDiskSpec, and add the missing org.mockito.Mockito.eq import. The deleteVirtualDisk verifications stay unchanged as they do not use matchers. Tests now pass: - VmwareStorageLayoutHelperTest.syncVolumeToVmDefaultFolderCopiesRootVolumeWithTargetAdapterAndProvisioningType - VmwareStorageLayoutHelperTest.syncVolumeToVmDefaultFolderCopiesBaseFolderVolumeWithTargetAdapter
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageLayoutHelper.java:268
- The adapter-type mapping is currently partial and relies on
targetAdapterType.toString()for all other cases. Since this method already special-cases values due to naming/casing differences, it should explicitly map all supported adapter types (e.g., PVSCSI, LSI Logic SAS, etc.) to the exactVirtualDiskAdapterTypevalues expected by vSphere. A centralized mapping (e.g., onVmdkAdapterType) plus unit tests for each supported adapter type would reduce the risk of subtle casing/enum-name mismatches.
private static FileBackedVirtualDiskSpec createDiskSpec(VmdkAdapterType targetAdapterType, Storage.ProvisioningType provisioningType) {
FileBackedVirtualDiskSpec diskSpec = new FileBackedVirtualDiskSpec();
if (targetAdapterType == VmdkAdapterType.buslogic) {
diskSpec.setAdapterType(VirtualDiskAdapterType.BUS_LOGIC.value());
} else if (targetAdapterType == VmdkAdapterType.lsilogic) {
diskSpec.setAdapterType(VirtualDiskAdapterType.LSI_LOGIC.value());
} else {
diskSpec.setAdapterType(targetAdapterType.toString());
}
plugins/hypervisors/vmware/src/main/java/com/cloud/storage/resource/VmwareStorageLayoutHelper.java:205
- This log statement eagerly builds the string even when INFO logging is disabled. Prefer parameterized logging (placeholders) to avoid unnecessary string concatenation and to keep log formatting consistent across the codebase.
LOGGER.info("sync " + deprecatedLegacyPath + "->" + vmwarePath);
Comment on lines
+225
to
+232
| if (targetAdapterType != null && ds.fileExists(vmdkLinkedCloneModeLegacyPair[0])) { | ||
| String vmwarePath = vmdkLinkedCloneModePair[0]; | ||
| LOGGER.info("sync " + vmdkLinkedCloneModeLegacyPair[0] + "->" + vmwarePath); | ||
| VirtualDiskManagerMO diskManager = new VirtualDiskManagerMO(ds.getContext()); | ||
| FileBackedVirtualDiskSpec diskSpec = createDiskSpec(targetAdapterType, provisioningType); | ||
| diskManager.copyVirtualDisk(vmdkLinkedCloneModeLegacyPair[0], dcMo.getMor(), vmwarePath, dcMo.getMor(), diskSpec, true); | ||
| diskManager.deleteVirtualDisk(vmdkLinkedCloneModeLegacyPair[0], dcMo.getMor()); | ||
| return new Pair<>(vmwarePath, true); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR restores the classic worker-VM-based VMDK data-disk lifecycle on VMware 8 (
cs.vmware.api.version=8.0), removing the FCD (VirtualStorageObjectManagerMO) path for data disks. This avoids vCenter datastore-browser inconsistencies between FCD and VMDK representations that caused HTTP 500 errors duringattachVolumeon a running VM, as reported in #13249.Context — issue #13249
attachVolumefails withHTTP response code: 500from the vCenter datastore browser when attaching a detached volume on a PreSetup/DatastoreCluster storage pool to a running VMware VM.VirtualMachineMO.attachDisk()→getVmdkFileInfo()does an HTTP GET via the vCenter datastore browser, and vCenter routes this to a non-owner ESXi host which cannot serve the locked file of the running VM.This PR is a more comprehensive workaround than the minimal descriptor-update guard proposed in #13773 (by @DaanHoogland, against the 4.20 branch). It supersedes that approach by:
HypervisorHostHelper.createWorkerVM+vmMo.createDisk/detachDisk), usinggetDeprecatedLegacyDatastorePathFromVmdkFileName.fcd/-base-folder VMDKs into the VM default folder duringattachVolumeviaVirtualDiskManagerMO(move, or copy+delete when an adapter-type change is required), instead of relying on the vCenter HTTP datastore browser.ProvisioningType→ vSphereVirtualDiskType(THIN→THIN,SPARSE→PREALLOCATED,FAT→EAGER_ZEROED_THICK) through a newVmwareHelper.getVirtualDiskTypehelper, used both at create time and as theFileBackedVirtualDiskSpec.diskTypeduring adapter-changing copy.attachDiskwhen the sync already migrated the disk into the VM folder (adapter type changed), so the vCenter datastore browser is no longer consulted for a locked file of a running VM.Fixes: #13249
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
N/A
How Has This Been Tested?
Unit tests
VmwareHelperTest.getVirtualDiskTypeMapsCloudStackProvisioningTypes— verifies THIN/SPARSE/FAT/null mappings toVirtualDiskType.VmwareStorageLayoutHelperTest(new) — three tests with MockitomockConstructiononVirtualDiskManagerMO:syncVolumeToVmDefaultFolderMovesRootVolumeWithVirtualDiskManager— root-level (deprecated legacy)[ds] volume.vmdkis moved into[ds] vm-name/volume.vmdk.syncVolumeToVmDefaultFolderCopiesRootVolumeWithTargetAdapterAndProvisioningType— root-level volume is copied with adapter+provisioning spec (lsiLogic/eagerZeroedThick) and the original is deleted;result.second()istrue.syncVolumeToVmDefaultFolderCopiesBaseFolderVolumeWithTargetAdapter—fcd/-base-folder volume is copied with adapter spec (lsiLogic/thin) and the original is deleted;result.second()istrue.Verifies
moveVirtualDisk/copyVirtualDisk+deleteVirtualDiskordering and theFileBackedVirtualDiskSpecadapterType/diskType values.Compilation
All changed files compile clean (no errors). The port was verified line-by-line identical to the workaround already shipped in the downstream
cloudstack-4.20.3.0-vmware-datadisk-fcd-fixbranch (commit18f2fd2c94"vmware: restore legacy VMDK data disk lifecycle"), which is production-deployed.How did you try to break this feature and the system with this change?
String-returningsyncVolumeToVmDefaultFolderoverloads still compile (they delegate.first()internally).attachDisk(maxIops)keeps the existing default behaviour (updateVmdkAdapter=true); only the newattachDisk(..., boolean)overload allows skipping the descriptor update.createVolumecleanup (detachAllDisksAndDestroy) is wrapped intry/catchwith a warning so a worker-VM cleanup failure no longer aborts volume creation.