Skip to content

Commit 90aaf4e

Browse files
CSTACKEX-306: incorporate review comments
1 parent db96c19 commit 90aaf4e

5 files changed

Lines changed: 73 additions & 82 deletions

File tree

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java‎

Lines changed: 40 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@
8989
import java.util.ArrayList;
9090
import java.util.Arrays;
9191
import java.util.HashMap;
92+
import java.util.LinkedHashMap;
9293
import java.util.List;
9394
import java.util.Map;
9495

@@ -194,21 +195,21 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
194195
* 3) else — blank LUN (iSCSI) or qcow2 (NFS).
195196
*
196197
* Mutually exclusive from the motion/orchestrator layer; snapshot is checked first
197-
* (SolidFire-style) so a restore never accidentally falls through to blank create.
198+
* (SolidFire-style) so a create-from-snapshot never accidentally falls through to blank create.
198199
*/
199200
Long cloneOfSnapshotId = getSnapshotIdForCloning(volInfo.getId());
201+
Long cloneOfTemplateId = getTemplateIdForCloning(volInfo.getId());
200202
CloudStackVolume clonedCloudStackVolume;
201203
if (cloneOfSnapshotId != null) {
202204
clonedCloudStackVolume = cloneCloudStackVolumeFromSnapshot(
203205
storagePool, volInfo, details, cloneOfSnapshotId);
204206
// TODO(CSTACKEX-306): apply persisted MIN_IOPS / MAX_IOPS from snapshot_details
205207
// onto this CloudStack volume (and ONTAP QoS if applicable) after successful clone.
208+
} else if (cloneOfTemplateId != null) {
209+
clonedCloudStackVolume = cloneCloudStackVolumeFromTemplate(
210+
storagePool, volInfo, details, cloneOfTemplateId);
206211
} else {
207-
Long cloneOfTemplateId = getTemplateIdForCloning(volInfo.getId());
208-
clonedCloudStackVolume = cloneOfTemplateId != null
209-
? cloneCloudStackVolumeFromTemplate(
210-
storagePool, volInfo, details, cloneOfTemplateId)
211-
: createCloudStackVolume(storagePool, volInfo, details);
212+
clonedCloudStackVolume = createCloudStackVolume(storagePool, volInfo, details);
212213
}
213214

214215
createCmdResult = recordCreatedVolume(storagePool, volInfo, volumeVO, details, clonedCloudStackVolume);
@@ -375,7 +376,7 @@ private Long getTemplateIdForCloning(long volumeId) {
375376

376377
/**
377378
* Returns the CloudStack snapshot id to clone from when {@code volume_details.cloneOfSnapshot}
378-
* is set, or null when this create is not a restore-from-snapshot.
379+
* is set, or null when this create is not a create-volume-from-snapshot.
379380
*
380381
* <p>Set by {@code StorageSystemDataMotionStrategy.handleCreateManagedVolumeFromManagedSnapshot}
381382
* for the duration of {@code createAsync} only (same pattern as {@link #getTemplateIdForCloning}).</p>
@@ -395,7 +396,7 @@ private Long getSnapshotIdForCloning(long volumeId) {
395396
* <p><b>Combinations (product + plugin v1):</b></p>
396397
* <ul>
397398
* <li><b>Same pool only</b> — {@code snapshot_details.PRIMARY_POOL_ID} must equal this
398-
* {@code storagePool}. Cross-pool restore is descoped; use migrate later if needed.</li>
399+
* {@code storagePool}. Creating the volume on a different pool is descoped; use migrate later if needed.</li>
399400
* <li><b>DATA snapshot</b> → attachable data disk (disk offering usually inherited).</li>
400401
* <li><b>ROOT snapshot</b> → still a data disk here (not bootable). Bootable path remains
401402
* {@code createTemplate(snapshotid)} then deploy.</li>
@@ -420,7 +421,7 @@ private CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storage
420421
if (!String.valueOf(storagePool.getId()).equals(primaryPoolId)) {
421422
throw new CloudRuntimeException("Create volume from snapshot [" + csSnapshotId
422423
+ "] requires the snapshot's primary pool [" + primaryPoolId
423-
+ "]; requested pool is [" + storagePool.getId() + "] (cross-pool restore is not supported in v1)");
424+
+ "]; requested pool is [" + storagePool.getId() + "] (creating a volume from a snapshot on a different pool is not supported)");
424425
}
425426
String poolProtocol = details.get(OntapStorageConstants.PROTOCOL);
426427
if (poolProtocol == null || !poolProtocol.equalsIgnoreCase(snapProtocol)) {
@@ -1388,9 +1389,20 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback<CreateCm
13881389
// Persist snapshot_details so deleteAsync can resolve ONTAP FlexVol/snapshot UUIDs
13891390
// (see deleteCloudStackVolumeSnapshot and StorageStrategy.deleteFlexVolSnapshotForCloudStackVolume).
13901391
// Also store source volume min/max IOPS when configured for later create-volume-from-snapshot.
1391-
updateSnapshotDetails(snapshot.getId(), volumeInfo.getId(), flexVolUuid,
1392-
ontapSnapshotUuid, snapshotName, volumePath, volumeVO.getPoolId(), protocol, lunUuid,
1393-
volumeVO.getMinIops(), volumeVO.getMaxIops());
1392+
Map<String, String> snapshotDetails = new LinkedHashMap<>();
1393+
snapshotDetails.put(OntapStorageConstants.SRC_CS_VOLUME_ID, String.valueOf(volumeInfo.getId()));
1394+
snapshotDetails.put(OntapStorageConstants.BASE_ONTAP_FV_ID, flexVolUuid);
1395+
snapshotDetails.put(OntapStorageConstants.ONTAP_SNAP_ID, ontapSnapshotUuid);
1396+
snapshotDetails.put(OntapStorageConstants.ONTAP_SNAP_NAME, snapshotName);
1397+
snapshotDetails.put(OntapStorageConstants.VOLUME_PATH, volumePath);
1398+
snapshotDetails.put(OntapStorageConstants.PRIMARY_POOL_ID, String.valueOf(volumeVO.getPoolId()));
1399+
snapshotDetails.put(OntapStorageConstants.PROTOCOL, protocol);
1400+
// iSCSI only — needed for LUN restore / identity; null (skipped) for NFS.
1401+
snapshotDetails.put(OntapStorageConstants.LUN_DOT_UUID, lunUuid);
1402+
// TODO(CSTACKEX-306): on create-volume-from-snapshot, read these and apply to the new volume.
1403+
putPositiveIopsDetail(snapshotDetails, OntapStorageConstants.MIN_IOPS, volumeVO.getMinIops());
1404+
putPositiveIopsDetail(snapshotDetails, OntapStorageConstants.MAX_IOPS, volumeVO.getMaxIops());
1405+
updateSnapshotDetails(snapshot.getId(), snapshotDetails);
13941406

13951407
CreateObjectAnswer createObjectAnswer = new CreateObjectAnswer(snapshotObjectTo);
13961408
result = new CreateCmdResult(null, createObjectAnswer);
@@ -1789,57 +1801,34 @@ private Storage.ImageFormat getImageFormat(StoragePoolVO storagePool) {
17891801
* <p>All rows go through {@link #persistSnapshotDetail} so DAO writes stay consistent;
17901802
* optional fields (LUN uuid, IOPS) are skipped when unset.</p>
17911803
*
1792-
* @param csSnapshotId CloudStack snapshot ID
1793-
* @param csVolumeId Source CloudStack volume ID
1794-
* @param flexVolUuid ONTAP FlexVolume UUID
1795-
* @param ontapSnapshotUuid ONTAP FlexVolume snapshot UUID
1796-
* @param snapshotName ONTAP snapshot name
1797-
* @param volumePath Path of the volume file/LUN within the FlexVolume (for restore)
1798-
* @param storagePoolId Primary storage pool ID
1799-
* @param protocol Storage protocol (NFS3 or ISCSI)
1800-
* @param lunUuid LUN UUID (only for iSCSI, null for NFS)
1801-
* @param minIops Source volume min IOPS if configured; null/&lt;=0 skipped
1802-
* @param maxIops Source volume max IOPS if configured; null/&lt;=0 skipped
1804+
* @param csSnapshotId CloudStack snapshot ID
1805+
* @param details {@code snapshot_details} name to value, keyed by {@link OntapStorageConstants};
1806+
* entries with null or blank values are skipped
18031807
*/
1804-
private void updateSnapshotDetails(long csSnapshotId, long csVolumeId, String flexVolUuid,
1805-
String ontapSnapshotUuid, String snapshotName,
1806-
String volumePath, long storagePoolId, String protocol,
1807-
String lunUuid, Long minIops, Long maxIops) {
1808-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.SRC_CS_VOLUME_ID, String.valueOf(csVolumeId));
1809-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.BASE_ONTAP_FV_ID, flexVolUuid);
1810-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.ONTAP_SNAP_ID, ontapSnapshotUuid);
1811-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.ONTAP_SNAP_NAME, snapshotName);
1812-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.VOLUME_PATH, volumePath);
1813-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.PRIMARY_POOL_ID, String.valueOf(storagePoolId));
1814-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.PROTOCOL, protocol);
1815-
// iSCSI only — needed for LUN restore / identity; omitted for NFS.
1816-
persistSnapshotDetail(csSnapshotId, OntapStorageConstants.LUN_DOT_UUID, lunUuid);
1817-
// Optional: only when the source volume has IOPS configured (no-op otherwise).
1818-
// TODO(CSTACKEX-306): on create-volume-from-snapshot, read these and apply to the new volume.
1819-
persistSnapshotIopsDetail(csSnapshotId, OntapStorageConstants.MIN_IOPS, minIops);
1820-
persistSnapshotIopsDetail(csSnapshotId, OntapStorageConstants.MAX_IOPS, maxIops);
1808+
private void updateSnapshotDetails(long csSnapshotId, Map<String, String> details) {
1809+
for (Map.Entry<String, String> detail : details.entrySet()) {
1810+
persistSnapshotDetail(csSnapshotId, detail.getKey(), detail.getValue());
1811+
}
18211812
}
18221813

18231814
/**
1824-
* Persists one {@code snapshot_details} row. No-op when {@code value} is null or blank so
1825-
* optional keys (e.g. LUN uuid on NFS) share the same DAO path as required keys.
1815+
* Adds an IOPS entry to {@code details} only when {@code iops} is non-null and positive.
18261816
*/
1827-
private void persistSnapshotDetail(long csSnapshotId, String detailName, String value) {
1828-
if (value == null || value.isEmpty()) {
1829-
return;
1817+
private static void putPositiveIopsDetail(Map<String, String> details, String detailName, Long iops) {
1818+
if (iops != null && iops > 0) {
1819+
details.put(detailName, String.valueOf(iops));
18301820
}
1831-
snapshotDetailsDao.persist(new SnapshotDetailsVO(csSnapshotId, detailName, value, false));
18321821
}
18331822

18341823
/**
1835-
* Persists a snapshot IOPS detail when {@code iops} is non-null and positive; otherwise no-op.
1824+
* Persists one {@code snapshot_details} row. No-op when {@code value} is null or blank so
1825+
* optional keys (e.g. LUN uuid on NFS) share the same DAO path as required keys.
18361826
*/
1837-
private void persistSnapshotIopsDetail(long csSnapshotId, String detailName, Long iops) {
1838-
if (iops == null || iops <= 0) {
1827+
private void persistSnapshotDetail(long csSnapshotId, String detailName, String value) {
1828+
if (value == null || value.isEmpty()) {
18391829
return;
18401830
}
1841-
persistSnapshotDetail(csSnapshotId, detailName, String.valueOf(iops));
1842-
logger.debug("updateSnapshotDetails: persisted {}={} for snapshot [{}]", detailName, iops, csSnapshotId);
1831+
snapshotDetailsDao.persist(new SnapshotDetailsVO(csSnapshotId, detailName, value, false));
18431832
}
18441833

18451834
}

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -833,8 +833,8 @@ abstract public CloudStackVolume createTemplateCache(StoragePoolVO storagePool,
833833
/**
834834
* Creates a new file/LUN in the same FlexVolume by cloning from a FlexVolume snapshot.
835835
*
836-
* <p><b>Product scope (v1):</b> same primary pool / FlexVol only. Cross-pool restore is
837-
* descoped — operators may later {@code migrateVolume} if another pool is required.</p>
836+
* <p><b>Product scope (v1):</b> same primary pool / FlexVol only. Creating the volume on a
837+
* different pool is descoped — operators may later {@code migrateVolume} if another pool is required.</p>
838838
*
839839
* <p>ONTAP backends (protocol-specific; each subclass builds its own request):</p>
840840
* <ul>
@@ -844,14 +844,14 @@ abstract public CloudStackVolume createTemplateCache(StoragePoolVO storagePool,
844844
* </ul>
845845
*
846846
* @param storagePool target CloudStack primary pool (same FlexVol as the snapshot)
847-
* @param details pool details (SVM, FlexVol name/uuid, protocol, …)
847+
* @param poolDetails pool details (SVM, FlexVol name/uuid, protocol, …)
848848
* @param volumeInfo destination CloudStack volume being created
849849
* @param sourceVolumePath snapshotted object path from {@code snapshot_details.VOLUME_PATH}
850850
* @param snapshotName ONTAP FlexVol snapshot name from {@code snapshot_details}
851851
* @return created CloudStackVolume with protocol-specific identity (LUN uuid or file path)
852852
*/
853853
abstract public CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storagePool,
854-
Map<String, String> details,
854+
Map<String, String> poolDetails,
855855
VolumeInfo volumeInfo,
856856
String sourceVolumePath,
857857
String snapshotName);

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ public CloudStackVolume cloneCloudStackVolume(CloudStackVolume cloudstackVolume)
158158
String flexVolUuid = details.get(OntapStorageConstants.VOLUME_UUID);
159159
String flexVolName = details.get(OntapStorageConstants.VOLUME_NAME);
160160
if (flexVolUuid == null || flexVolUuid.isEmpty()) {
161-
throw new CloudRuntimeException("Failed to clone file, FlexVolume uuid is missing from pool details");
161+
throw new CloudRuntimeException("Failed to clone file, FlexVolume uuid is missing from pool poolDetails");
162162
}
163163
String sourcePath = cloudstackVolume.getFile().getPath();
164164
String destinationPath = cloudstackVolume.getDestinationPath();
@@ -206,10 +206,10 @@ public CloudStackVolume cloneCloudStackVolume(CloudStackVolume cloudstackVolume)
206206
* </ul>
207207
*/
208208
@Override
209-
public CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storagePool, Map<String, String> details,
209+
public CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storagePool, Map<String, String> poolDetails,
210210
VolumeInfo volumeInfo, String sourceVolumePath,
211211
String snapshotName) {
212-
if (storagePool == null || details == null || volumeInfo == null) {
212+
if (storagePool == null || poolDetails == null || volumeInfo == null) {
213213
throw new CloudRuntimeException("Failed to clone file from snapshot, invalid request");
214214
}
215215
if (sourceVolumePath == null || sourceVolumePath.isEmpty()) {
@@ -219,10 +219,10 @@ public CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storageP
219219
throw new CloudRuntimeException("Failed to clone file from snapshot, snapshot name is required");
220220
}
221221

222-
String flexVolUuid = details.get(OntapStorageConstants.VOLUME_UUID);
223-
String flexVolName = details.get(OntapStorageConstants.VOLUME_NAME);
222+
String flexVolUuid = poolDetails.get(OntapStorageConstants.VOLUME_UUID);
223+
String flexVolName = poolDetails.get(OntapStorageConstants.VOLUME_NAME);
224224
if (flexVolUuid == null || flexVolUuid.isEmpty()) {
225-
throw new CloudRuntimeException("Failed to clone file from snapshot, FlexVolume uuid is missing from pool details");
225+
throw new CloudRuntimeException("Failed to clone file from snapshot, FlexVolume uuid is missing from pool poolDetails");
226226
}
227227

228228
String sourcePath = OntapStorageUtils.toFlexVolRelativePath(sourceVolumePath, flexVolName);
@@ -337,7 +337,7 @@ public AccessGroup createAccessGroup(AccessGroup accessGroup) {
337337
logger.info("createAccessGroup: ExportPolicy created: {}, now attaching this policy to storage pool volume", createdPolicy.getName());
338338
// attach export policy to volume of storage pool
339339
assignExportPolicyToVolume(volumeUUID,createdPolicy.getName());
340-
// save the export policy details in storage pool details
340+
// save the export policy poolDetails in storage pool poolDetails
341341
storagePoolDetailsDao.addDetail(accessGroup.getStoragePoolId(), OntapStorageConstants.EXPORT_POLICY_ID, String.valueOf(createdPolicy.getId()), true);
342342
storagePoolDetailsDao.addDetail(accessGroup.getStoragePoolId(), OntapStorageConstants.EXPORT_POLICY_NAME, createdPolicy.getName(), true);
343343
logger.info("Successfully assigned exportPolicy {} to volume {}", policyRequest.getName(), volumeName);
@@ -398,7 +398,7 @@ public AccessGroup updateAccessGroup(AccessGroup accessGroup) {
398398

399399
Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId());
400400
if (details == null || details.isEmpty()) {
401-
throw new CloudRuntimeException("No storage pool details found for storagePoolId: " + accessGroup.getStoragePoolId());
401+
throw new CloudRuntimeException("No storage pool poolDetails found for storagePoolId: " + accessGroup.getStoragePoolId());
402402
}
403403
String exportPolicyId = details.get(OntapStorageConstants.EXPORT_POLICY_ID);
404404
if (exportPolicyId == null || exportPolicyId.isEmpty()) {

0 commit comments

Comments
 (0)