vmware: restore legacy VMDK data disk lifecycle (attachVolume HTTP 500 on VMware 8) by H-LK · Pull Request #13857 · apache/cloudstack

H-LK · 2026-08-12T11:24:00Z

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 during attachVolume on a running VM, as reported in #13249.

Context — issue #13249

attachVolume fails with HTTP response code: 500 from 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:

Removing the FCD code path for data disks entirely on VMware 8 — volumes are always created as classic VMDKs via a worker VM (HypervisorHostHelper.createWorkerVM + vmMo.createDisk/detachDisk), using getDeprecatedLegacyDatastorePathFromVmdkFileName.
Migrating legacy root-level (deprecated) and fcd/-base-folder VMDKs into the VM default folder during attachVolume via VirtualDiskManagerMO (move, or copy+delete when an adapter-type change is required), instead of relying on the vCenter HTTP datastore browser.
Mapping CloudStack ProvisioningType → vSphere VirtualDiskType (THIN→THIN, SPARSE→PREALLOCATED, FAT→EAGER_ZEROED_THICK) through a new VmwareHelper.getVirtualDiskType helper, used both at create time and as the FileBackedVirtualDiskSpec.diskType during adapter-changing copy.
Skipping the VMDK adapter descriptor update in attachDisk when 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

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

N/A

How Has This Been Tested?

Unit tests

VmwareHelperTest.getVirtualDiskTypeMapsCloudStackProvisioningTypes — verifies THIN/SPARSE/FAT/null mappings to VirtualDiskType.
VmwareStorageLayoutHelperTest (new) — three tests with Mockito mockConstruction on VirtualDiskManagerMO:
1. syncVolumeToVmDefaultFolderMovesRootVolumeWithVirtualDiskManager — root-level (deprecated legacy) [ds] volume.vmdk is moved into [ds] vm-name/volume.vmdk.
2. syncVolumeToVmDefaultFolderCopiesRootVolumeWithTargetAdapterAndProvisioningType — root-level volume is copied with adapter+provisioning spec (lsiLogic / eagerZeroedThick) and the original is deleted; result.second() is true.
3. syncVolumeToVmDefaultFolderCopiesBaseFolderVolumeWithTargetAdapter — fcd/-base-folder volume is copied with adapter spec (lsiLogic / thin) and the original is deleted; result.second() is true.
  Verifies moveVirtualDisk / copyVirtualDisk+deleteVirtualDisk ordering and the FileBackedVirtualDiskSpec adapterType/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-fix branch (commit 18f2fd2c94 "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?

Verified that callers of the existing String-returning syncVolumeToVmDefaultFolder overloads still compile (they delegate .first() internally).
Verified attachDisk(maxIops) keeps the existing default behaviour (updateVmdkAdapter=true); only the new attachDisk(..., boolean) overload allows skipping the descriptor update.
Verified createVolume cleanup (detachAllDisksAndDestroy) is wrapped in try/catch with a warning so a worker-VM cleanup failure no longer aborts volume creation.

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.

boring-cyborg · 2026-08-12T11:24:04Z

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)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

Copilot

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 -> VirtualDiskType mapping via VmwareHelper.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.

+                        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();


DaanHoogland · 2026-08-12T14:07:43Z

@blueorangutan package

blueorangutan · 2026-08-12T14:10:04Z

@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.

DaanHoogland · 2026-08-12T14:10:43Z

I did a quick browse and see no big strangities, needs test/verification though.

codecov · 2026-08-12T14:15:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 3.41%. Comparing base (1ceb1d0) to head (30a12d1).

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

Flag	Coverage Δ
uitests	`3.41% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2026-08-12T15:04:36Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18839

Copilot AI lite review requested due to automatic review settings August 12, 2026 11:24

boring-cyborg Bot added the component:vmware label Aug 12, 2026

Copilot started reviewing on behalf of H-LK August 12, 2026 11:24 View session

H-LK mentioned this pull request Aug 12, 2026

make updating the vmdk descriptor optional #13773

Open

14 tasks

Copilot AI reviewed Aug 12, 2026

View reviewed changes

Navigation Menu

Uh oh!

vmware: restore legacy VMDK data disk lifecycle (attachVolume HTTP 500 on VMware 8) - #13857

vmware: restore legacy VMDK data disk lifecycle (attachVolume HTTP 500 on VMware 8)#13857
H-LK wants to merge 1 commit into
apache:mainfrom
H-LK:vmware-datadisk-fcd-fix-13249

H-LK commented Aug 12, 2026

Uh oh!

boring-cyborg Bot commented Aug 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

DaanHoogland commented Aug 12, 2026

Uh oh!

blueorangutan commented Aug 12, 2026

Uh oh!

DaanHoogland commented Aug 12, 2026

Uh oh!

codecov Bot commented Aug 12, 2026

Uh oh!

blueorangutan commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

H-LK commented Aug 12, 2026

Description

Context — issue #13249

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Unit tests

Compilation

How did you try to break this feature and the system with this change?

Uh oh!

boring-cyborg Bot commented Aug 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

DaanHoogland commented Aug 12, 2026

Uh oh!

blueorangutan commented Aug 12, 2026

Uh oh!

DaanHoogland commented Aug 12, 2026

Uh oh!

codecov Bot commented Aug 12, 2026

Codecov Report

Uh oh!

blueorangutan commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants