Create volume on a specified storage pool#12966
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12966 +/- ##
============================================
+ Coverage 17.60% 18.00% +0.39%
- Complexity 15676 16463 +787
============================================
Files 5918 5977 +59
Lines 531667 537742 +6075
Branches 65001 66028 +1027
============================================
+ Hits 93617 96834 +3217
- Misses 427491 429990 +2499
- Partials 10559 10918 +359
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17378 |
There was a problem hiding this comment.
Pull request overview
This PR enhances CloudStack’s createVolume API to support creating a data volume directly on a specified primary storage pool (so it can reach Ready state without attaching to a VM), aimed at workflows like vmware2kvm migrations.
Changes:
- Adds a new
storageidparameter tocreateVolume. - Implements a new code path in
VolumeApiServiceImpl.createVolumeto create the volume on the requested primary storage pool viavolService.createVolumeAsync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java | Adds a storageid branch that triggers backend volume creation on a specified primary store. |
| api/src/main/java/org/apache/cloudstack/api/command/user/volume/CreateVolumeCmd.java | Introduces the storageid API parameter and a getter intended to enforce snapshot/storage mutual exclusivity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private VolumeVO allocateVolumeOnStorage(Long volumeId, Long storageId) throws ExecutionException, InterruptedException { | ||
| DataStore destStore = dataStoreMgr.getDataStore(storageId, DataStoreRole.Primary); | ||
| VolumeInfo destVolume = volFactory.getVolume(volumeId, destStore); | ||
| AsyncCallFuture<VolumeApiResult> createVolumeFuture = volService.createVolumeAsync(destVolume, destStore); | ||
| VolumeApiResult createVolumeResult = createVolumeFuture.get(); | ||
| if (createVolumeResult.isFailed()) { | ||
| throw new CloudRuntimeException("Creation of a dest volume failed: " + createVolumeResult.getResult()); | ||
| } | ||
| return _volsDao.findById(destVolume.getId()); |
There was a problem hiding this comment.
allocateVolumeOnStorage() doesn’t validate that the provided storageId resolves to an existing/accessible primary storage pool (dataStoreMgr.getDataStore may return null) and doesn’t check pool conditions (e.g., in-maintenance/disabled, zone mismatch with the volume, storage access groups). As-is, this can lead to NPEs and also allows callers to force volume creation on arbitrary pools bypassing the normal allocation/compatibility checks.
Consider resolving StoragePoolVO/PrimaryDataStore first, rejecting null / in-maintenance, validating the pool is in the expected state (Ready/Up) and in the same zone as the volume, and running the same compatibility checks used elsewhere (e.g., storageMgr.checkIfReadyVolumeFitsInStoragePoolWithStorageAccessGroups and doesStoragePoolSupportDiskOffering / capacity checks) before calling volService.createVolumeAsync.
| private VolumeVO allocateVolumeOnStorage(Long volumeId, Long storageId) throws ExecutionException, InterruptedException { | ||
| DataStore destStore = dataStoreMgr.getDataStore(storageId, DataStoreRole.Primary); | ||
| VolumeInfo destVolume = volFactory.getVolume(volumeId, destStore); | ||
| AsyncCallFuture<VolumeApiResult> createVolumeFuture = volService.createVolumeAsync(destVolume, destStore); | ||
| VolumeApiResult createVolumeResult = createVolumeFuture.get(); |
There was a problem hiding this comment.
allocateVolumeOnStorage() is a bit misleadingly named: it actually triggers backend creation (createVolumeAsync) and blocks waiting for completion. Renaming it to reflect the side-effect (e.g., createVolumeOnStoragePool) would make the control flow in createVolume() clearer and reduce confusion with allocVolume(), which only persists DB state.
| public Long getStorageId() { | ||
| if (snapshotId != null && storageId != null) { | ||
| throw new IllegalArgumentException("StorageId parameter cannot be specified with the SnapshotId parameter."); | ||
| } | ||
| return storageId; | ||
| } |
There was a problem hiding this comment.
The mutual-exclusion check for snapshotId vs storageId is currently ineffective: when snapshotId is provided, createVolume() never calls getStorageId(), so a request with both parameters will silently ignore storageid and proceed down the snapshot path.
To ensure the API contract is enforced, validate this combination in a code path that always runs (e.g., in CreateVolumeCmd.create()/execute() or in VolumeApiServiceImpl.allocVolume/createVolume before branching) and throw an InvalidParameterValueException/ServerApiException rather than letting the conflict slip through.
| @Parameter(name = ApiConstants.STORAGE_ID, | ||
| type = CommandType.UUID, | ||
| entityType = StoragePoolResponse.class, | ||
| description = "Storage pool ID to create the volume in. Exclusive with SnapshotId parameter.") |
There was a problem hiding this comment.
storageid is a user-exposed parameter here, but there’s no indication it’s restricted (no authorized=Admin) and the backend path in VolumeApiServiceImpl doesn’t enforce pool-level access/compatibility checks. If this is intended primarily for admin/migration workflows, consider restricting the parameter to admins (as done in other user commands that expose storage pool IDs) or ensure the service layer performs full access checks before acting on the provided pool ID.
| description = "Storage pool ID to create the volume in. Exclusive with SnapshotId parameter.") | |
| description = "Storage pool ID to create the volume in. Exclusive with SnapshotId parameter.", | |
| authorized = {RoleType.Admin}) |
| } else if (cmd.getStorageId() != null) { | ||
| volume = allocateVolumeOnStorage(cmd.getEntityId(), cmd.getStorageId()); |
There was a problem hiding this comment.
New storageid-driven creation path in createVolume()/allocateVolumeOnStorage isn’t covered by existing unit tests (there are tests for VolumeApiServiceImpl in server/src/test/java/com/cloud/storage/VolumeApiServiceImplTest.java, but none exercise this new branch). Adding a unit test that verifies pool validation + volService.createVolumeAsync invocation and failure handling would help prevent regressions.
Description
This PR adds support for creating volume on the specified storage pool in the Ready state.
This can be done without attaching the volume to any instance. This can be helpful with vmware2kvm scenarios.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Created volume using the
storageidparameter on an NFS storage pool.Verified that volume was in
Readystate with a qcow2 file created with the specified virtual size.How did you try to break this feature and the system with this change?