diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java index 8a5cbb6a753c..bd35fc4f9112 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java @@ -205,7 +205,8 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet * via this path; bootable recovery is createTemplate(snapshotid) → deploy). * - Backend: iSCSI → POST /api/storage/luns (clone.source in .snapshot/); * NFS → POST /api/storage/file/clone with snapshot.name. - * - IOPS: MIN_IOPS/MAX_IOPS may be on snapshot_details; apply is TODO below. + * - IOPS: the new volume's min/max (snapshot values, or the root offering when one + * is supplied). Applied here; the temporary template clone does not get a policy. * * 2) cloneOfTemplate — deploy / create from cached template on this pool. * @@ -219,9 +220,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet CloudStackVolume clonedCloudStackVolume; if (cloneOfSnapshotId != null) { clonedCloudStackVolume = cloneCloudStackVolumeFromSnapshot( - storagePool, volInfo, details, cloneOfSnapshotId); - // TODO(CSTACKEX-306): apply persisted MIN_IOPS / MAX_IOPS from snapshot_details - // onto this CloudStack volume (and ONTAP QoS if applicable) after successful clone. + storagePool, volInfo, details, cloneOfSnapshotId, true); } else if (cloneOfTemplateId != null) { clonedCloudStackVolume = cloneCloudStackVolumeFromTemplate( storagePool, volInfo, details, cloneOfTemplateId); @@ -525,9 +524,15 @@ private Long getSnapshotIdForCloning(long volumeId) { * *

Optional grow when the disk offering is larger than the snapshot size (same pattern as * clone-from-template).

+ * + *

When {@code applyQos} is true, the new volume's min and max IOPS are applied the same way + * as a template clone: iSCSI carries the policy on the clone request, NFS attaches it after + * the file exists. The temporary volume used for {@code createTemplate(snapshotid)} passes + * false and is left without a policy.

*/ private CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storagePool, VolumeInfo volumeInfo, - Map details, long csSnapshotId) { + Map details, long csSnapshotId, + boolean applyQos) { String snapshotName = requireSnapshotDetail(csSnapshotId, OntapStorageConstants.ONTAP_SNAP_NAME); String volumePath = requireSnapshotDetail(csSnapshotId, OntapStorageConstants.VOLUME_PATH); String primaryPoolId = requireSnapshotDetail(csSnapshotId, OntapStorageConstants.PRIMARY_POOL_ID); @@ -546,30 +551,56 @@ private CloudStackVolume cloneCloudStackVolumeFromSnapshot(StoragePoolVO storage } StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); + boolean iscsi = isIscsi(details); + VolumeQosPolicy qosPolicy = null; logger.info("cloneCloudStackVolumeFromSnapshot: Cloning from CS snapshot [{}] (ONTAP snap [{}], path [{}]) " + "for volume [{}] on pool [{}] protocol [{}]", csSnapshotId, snapshotName, volumePath, volumeInfo.getId(), storagePool.getId(), poolProtocol); - CloudStackVolume request = isIscsi(details) - ? createCloneLunFromSnapshotRequest(storagePool, details, volumeInfo, volumePath, snapshotName) - : createCloneFileFromSnapshotRequest(storagePool, details, volumeInfo, volumePath, snapshotName); - CloudStackVolume cloned = storageStrategy.cloneCloudStackVolume(request); - if (cloned == null) { - throw new CloudRuntimeException("ONTAP returned nothing when cloning snapshot [" + csSnapshotId - + "] for volume [" + volumeInfo.getId() + "]"); - } + try { + if (applyQos) { + verifySufficientIopsForStoragePool(storagePool, volumeInfo.getMinIops(), volumeInfo.getId()); + Volume.Type volumeType = volumeInfo.getVolumeType(); + if (volumeType == Volume.Type.DATADISK || volumeType == Volume.Type.ROOT) { + qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, + volumeInfo.getMinIops(), volumeInfo.getMaxIops(), storagePool.getId()); + } + } - long requestedSize = getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool); - long snapshotSize = resolveSnapshotSizeBytes(csSnapshotId); - if (snapshotSize > 0 && requestedSize > snapshotSize) { - logger.info("cloneCloudStackVolumeFromSnapshot: Growing clone of snapshot [{}] from {} to {} bytes for volume [{}]", - csSnapshotId, snapshotSize, requestedSize, volumeInfo.getId()); - resizeClonedVolumeOrCleanup(storageStrategy, cloned, requestedSize, - "cloneCloudStackVolumeFromSnapshot snapshot [" + csSnapshotId + "] volume [" + volumeInfo.getId() + "]"); - } + CloudStackVolume request = iscsi + ? createCloneLunFromSnapshotRequest(storagePool, details, volumeInfo, volumePath, snapshotName, qosPolicy) + : createCloneFileFromSnapshotRequest(storagePool, details, volumeInfo, volumePath, snapshotName); + CloudStackVolume cloned = storageStrategy.cloneCloudStackVolume(request); + if (cloned == null) { + throw new CloudRuntimeException("ONTAP returned nothing when cloning snapshot [" + csSnapshotId + + "] for volume [" + volumeInfo.getId() + "]"); + } + + long requestedSize = getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool); + long snapshotSize = resolveSnapshotSizeBytes(csSnapshotId); + if (snapshotSize > 0 && requestedSize > snapshotSize) { + logger.info("cloneCloudStackVolumeFromSnapshot: Growing clone of snapshot [{}] from {} to {} bytes for volume [{}]", + csSnapshotId, snapshotSize, requestedSize, volumeInfo.getId()); + resizeClonedVolumeOrCleanup(storageStrategy, cloned, requestedSize, + "cloneCloudStackVolumeFromSnapshot snapshot [" + csSnapshotId + "] volume [" + volumeInfo.getId() + "]"); + } - return cloned; + if (!iscsi && qosPolicy != null) { + CloudStackVolume cloudStackVolume = createCloudStackVolumeRequestByProtocol( + storagePool, details, volumeInfo, qosPolicy); + storageStrategy.updateCloudStackVolume(cloudStackVolume); + } + if (applyQos) { + persistQosPolicyDetails(volumeInfo.getId(), qosPolicy); + } + return cloned; + } catch (Exception e) { + if (qosPolicy != null) { + storageStrategy.deleteVolumeQosPolicy(qosPolicy.getUuid()); + } + throw e; + } } /** @@ -779,10 +810,24 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); logger.info("createCloudStackVolumeForTypeVolume: Connection to Ontap SVM [{}] successful, preparing CloudStackVolumeRequest", details.get(OntapStorageConstants.SVM_NAME)); VolumeInfo volumeInfo = (VolumeInfo) data; - VolumeDetailVO qosPolicyDetail = volumeDetailsDao.findDetail( - volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_UUID); - CloudStackVolume cloudStackVolumeRequest = createDeleteCloudStackVolumeRequest(storagePool, details, volumeInfo); - storageStrategy.deleteCloudStackVolume(cloudStackVolumeRequest); + VolumeDetailVO qosPolicyDetail = volumeDetailsDao.findDetail(volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_UUID); + + // NFS file delete leaves the QoS assignment in place unless the file is set to none first. + // iSCSI LUN delete already drops object_count. + // TODO: revisit this NFS none-policy step when the file is deleted from ONTAP instead of KVM. + boolean qosCleared = false; + if (!isIscsi(details)) { + qosCleared = clearQosPolicyBeforeDelete(storageStrategy, storagePool, details, volumeInfo, qosPolicyDetail); + } + try { + CloudStackVolume cloudStackVolumeRequest = createDeleteCloudStackVolumeRequest(storagePool, details, volumeInfo); + storageStrategy.deleteCloudStackVolume(cloudStackVolumeRequest); + } catch (Exception e) { + if (qosCleared) { + restoreQosPolicyAfterFailedDelete(storageStrategy, storagePool, details, volumeInfo, qosPolicyDetail); + } + throw e; + } if (qosPolicyDetail != null) { deleteUnusedQosPolicy(storageStrategy, qosPolicyDetail.getValue()); } @@ -813,6 +858,51 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac } } + /** + * Sets the NFS file QoS policy to {@code none} before the file is deleted. + * Failure must not block volume delete. + * + * @return true when the file was moved off its policy + */ + private boolean clearQosPolicyBeforeDelete(StorageStrategy storageStrategy, StoragePoolVO storagePool, + Map details, VolumeInfo volumeInfo, + VolumeDetailVO qosPolicyDetail) { + if (qosPolicyDetail == null || qosPolicyDetail.getValue() == null) { + return false; + } + VolumeQosPolicy nonePolicy = new VolumeQosPolicy(); + nonePolicy.setName(OntapStorageConstants.QOS_POLICY_NONE); + try { + CloudStackVolume request = createCloudStackVolumeRequestByProtocol(storagePool, details, volumeInfo, nonePolicy); + storageStrategy.updateCloudStackVolume(request); + return true; + } catch (Exception e) { + logger.warn("Could not clear QoS policy [{}] on volume [{}] before delete: {}", + qosPolicyDetail.getValue(), volumeInfo.getId(), e.getMessage()); + return false; + } + } + + /** + * Puts the previous QoS policy back on an NFS file when delete fails after the policy was cleared. + */ + private void restoreQosPolicyAfterFailedDelete(StorageStrategy storageStrategy, StoragePoolVO storagePool, + Map details, VolumeInfo volumeInfo, + VolumeDetailVO qosPolicyDetail) { + if (qosPolicyDetail == null || qosPolicyDetail.getValue() == null) { + return; + } + VolumeQosPolicy previousPolicy = new VolumeQosPolicy(); + previousPolicy.setUuid(qosPolicyDetail.getValue()); + try { + CloudStackVolume request = createCloudStackVolumeRequestByProtocol(storagePool, details, volumeInfo, previousPolicy); + storageStrategy.updateCloudStackVolume(request); + } catch (Exception e) { + logger.warn("Could not restore QoS policy [{}] on volume [{}] after delete failed: {}", + qosPolicyDetail.getValue(), volumeInfo.getId(), e.getMessage()); + } + } + /** * Orchestrates CloudStack volume-snapshot delete on ONTAP. * @@ -984,7 +1074,7 @@ private CopyCommandResult copySnapshotToTemporaryVolume(DataObject srcData, Volu renameTemporarySnapshotCopy(volInfo, volumeVO, srcData.getId()); logger.info("copySnapshotToTemporaryVolume: Cloning CS snapshot [{}] into temporary volume [{}] on pool [{}] for template creation", srcData.getId(), volInfo.getId(), storagePool.getId()); - CloudStackVolume cloned = cloneCloudStackVolumeFromSnapshot(storagePool, volInfo, details, srcData.getId()); + CloudStackVolume cloned = cloneCloudStackVolumeFromSnapshot(storagePool, volInfo, details, srcData.getId(), false); if (ProtocolType.NFS3.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { volumeVO.setPath(volInfo.getUuid()); } @@ -1825,7 +1915,8 @@ public void takeSnapshot(SnapshotInfo snapshot, AsyncCompletionCallback details, VolumeInfo volumeInfo, String sourceVolumePath, - String snapshotName) { + String snapshotName, VolumeQosPolicy qosPolicy) { String lunName = volumeInfo.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE); if (!OntapStorageUtils.isValidName(lunName)) { throw new CloudRuntimeException("Invalid dataObject name [" + lunName @@ -2276,6 +2367,12 @@ private CloudStackVolume createCloneLunFromSnapshotRequest(StoragePoolVO storage lunRequest.setSvm(svm); lunRequest.setName(OntapStorageUtils.getLunName(storagePool.getName(), lunName)); lunRequest.setClone(clone); + if (qosPolicy != null) { + VolumeQosPolicy qosPolicyReference = new VolumeQosPolicy(); + qosPolicyReference.setName(qosPolicy.getName()); + qosPolicyReference.setUuid(qosPolicy.getUuid()); + lunRequest.setQosPolicy(qosPolicyReference); + } CloudStackVolume request = new CloudStackVolume(); request.setLun(lunRequest); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java index 1c8fae171b67..e7ca26cbe7a9 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java @@ -139,7 +139,7 @@ public CloudStackVolume createTemplateCache(StoragePoolVO storagePool, TemplateI public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { if (cloudstackVolume == null || cloudstackVolume.getVolumeInfo() == null || cloudstackVolume.getFlexVolumeUuid() == null || cloudstackVolume.getFile() == null) { - throw new CloudRuntimeException("Invalid NFS volume QoS update request"); + throw new CloudRuntimeException("Invalid NFS volume request"); } FileInfo fileInfo = new FileInfo(); fileInfo.setQosPolicy(cloudstackVolume.getFile().getQosPolicy()); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java index e0166cbf2829..9f9bbe1789ff 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java @@ -196,7 +196,24 @@ private void bestEffortDeleteTemplateCacheLun(String svmName, String lunName, St @Override public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { - return null; + if (cloudstackVolume == null || cloudstackVolume.getLun() == null + || cloudstackVolume.getLun().getUuid() == null) { + throw new CloudRuntimeException("Invalid iSCSI volume request"); + } + Lun lunUpdate = new Lun(); + lunUpdate.setQosPolicy(cloudstackVolume.getLun().getQosPolicy()); + try { + JobResponse response = sanFeignClient.updateLun( + getAuthHeader(), cloudstackVolume.getLun().getUuid(), lunUpdate); + pollJobIfPresent(response, "update QoS policy on LUN [" + cloudstackVolume.getLun().getUuid() + "]"); + } catch (FeignException e) { + throw new CloudRuntimeException("Failed to apply QoS policy to LUN: " + e.getMessage(), e); + } + logger.info("Applied QoS policy [{}] to LUN [{}]", + cloudstackVolume.getLun().getQosPolicy() != null + ? cloudstackVolume.getLun().getQosPolicy().getName() : null, + cloudstackVolume.getLun().getUuid()); + return cloudstackVolume; } @Override diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java index 6fb66d20c215..c12fe7325761 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java @@ -129,8 +129,7 @@ public class OntapStorageConstants { public static final String LUN_DOT_SERIAL_NUMBER = "lun.serial_number"; public static final String QOS_POLICY_UUID = "qosPolicyUuid"; public static final String IS_AFF = "isAFF"; - public static final String MIN_IOPS_NOT_SUPPORTED_ON_FAS = - "Minimum IOPS is not supported on FAS/non-AFF ONTAP platforms; only maximum IOPS is supported"; + public static final String MIN_IOPS_NOT_SUPPORTED_ON_FAS = "Minimum IOPS must be 0 for FAS. Only a Maximum IOPS limit is supported"; public static final String QOS_POLICY_NONE = "none"; public static final String QOS_POLICY_NAME_PREFIX = "cs_"; public static final String QOS_POLICY_NAME_TO = "to_"; diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java index 12e5ff3faced..ec55c72b9ccc 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriverTest.java @@ -1034,6 +1034,134 @@ void testCreateAsync_VolumeClonedFromSnapshot_IscsiSuccessWithoutGrow() { } } + @Test + void testCreateAsync_VolumeClonedFromSnapshot_IscsiWithIops_SetsQosOnLunCreate() { + stubVolumeCloneFromSnapshot(5368709120L, 5368709120L, ProtocolType.ISCSI.name()); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + lenient().when(storagePool.getCapacityIops()).thenReturn(null); + + Lun clonedLun = new Lun(); + clonedLun.setName("/vol/vol1/test_volume"); + clonedLun.setUuid("snap-cloned-lun-uuid"); + CloudStackVolume cloned = new CloudStackVolume(); + cloned.setLun(clonedLun); + VolumeQosPolicy qosPolicy = qosPolicy("qos-snap-uuid", "cs_100_to_200_iops_svm1"); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + stubQosCreateMocks(utilityMock, sanStrategy, cloned, qosPolicy); + when(sanStrategy.cloneCloudStackVolume(any())).thenReturn(cloned); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(CloudStackVolume.class); + verify(sanStrategy).cloneCloudStackVolume(requestCaptor.capture()); + assertTrue(isQosPolicyReference(requestCaptor.getValue().getLun().getQosPolicy(), + "qos-snap-uuid", "cs_100_to_200_iops_svm1")); + verify(sanStrategy, never()).updateCloudStackVolume(any()); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-snap-uuid"), eq(false)); + } + } + + @Test + void testCreateAsync_VolumeClonedFromSnapshot_NfsWithIops_AttachesQosAfterClone() { + stubVolumeCloneFromSnapshot(5368709120L, 5368709120L, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(volumeInfo.getUuid()).thenReturn("new-volume-uuid"); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + lenient().when(storagePool.getCapacityIops()).thenReturn(null); + + CloudStackVolume cloned = new CloudStackVolume(); + VolumeQosPolicy qosPolicy = lookedUpQosPolicy("qos-nfs-snap-uuid", "cs_100_to_200_iops_svm1"); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + stubQosCreateMocks(utilityMock, nasStrategy, cloned, qosPolicy); + when(nasStrategy.cloneCloudStackVolume(any())).thenReturn(cloned); + when(nasStrategy.updateCloudStackVolume(any())).thenReturn(cloned); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(nasStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); + ArgumentCaptor cloneCaptor = ArgumentCaptor.forClass(CloudStackVolume.class); + verify(nasStrategy).cloneCloudStackVolume(cloneCaptor.capture()); + assertNull(cloneCaptor.getValue().getFile().getQosPolicy()); + verify(nasStrategy).updateCloudStackVolume(argThat(request -> + "1".equals(request.getDatastoreId()) + && "flex-uuid".equals(request.getFlexVolumeUuid()) + && request.getVolumeInfo() == volumeInfo + && request.getFile() != null + && isQosPolicyReference(request.getFile().getQosPolicy(), + "qos-nfs-snap-uuid", "cs_100_to_200_iops_svm1"))); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-nfs-snap-uuid"), eq(false)); + } + } + + @Test + void testCreateAsync_VolumeClonedFromSnapshot_NoIops_DoesNotCreatePolicy() { + stubVolumeCloneFromSnapshot(5368709120L, 5368709120L, ProtocolType.ISCSI.name()); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(null); + when(volumeInfo.getMaxIops()).thenReturn(null); + + Lun clonedLun = new Lun(); + clonedLun.setName("/vol/vol1/test_volume"); + clonedLun.setUuid("snap-cloned-lun-uuid"); + CloudStackVolume cloned = new CloudStackVolume(); + cloned.setLun(clonedLun); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())).thenReturn(sanStrategy); + when(sanStrategy.cloneCloudStackVolume(any())).thenReturn(cloned); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + verify(sanStrategy, never()).createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class)); + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(CloudStackVolume.class); + verify(sanStrategy).cloneCloudStackVolume(requestCaptor.capture()); + assertNull(requestCaptor.getValue().getLun().getQosPolicy()); + } + } + + @Test + void testCreateAsync_VolumeClonedFromSnapshot_CloneFails_DeletesNewPolicy() { + stubVolumeCloneFromSnapshot(5368709120L, 5368709120L, ProtocolType.ISCSI.name()); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + lenient().when(storagePool.getCapacityIops()).thenReturn(null); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-snap-uuid", "cs_100_to_200_iops_svm1"); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + stubQosCreateMocks(utilityMock, sanStrategy, new CloudStackVolume(), qosPolicy); + when(sanStrategy.cloneCloudStackVolume(any())) + .thenThrow(new CloudRuntimeException("clone failed")); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).deleteVolumeQosPolicy("qos-snap-uuid"); + verify(volumeDetailsDao, never()).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + anyString(), eq(false)); + } + } + @Test void testCreateAsync_VolumeClonedFromSnapshot_GrowsWhenOfferingIsLarger() { stubVolumeCloneFromSnapshot(5368709120L, 21474836480L, ProtocolType.ISCSI.name()); @@ -1911,6 +2039,36 @@ void testResize_MinIopsBeyondPoolCapacity_Fails() { verify(sanStrategy, never()).updateCloudStackVolume(any()); } + @Test + void testResize_MinIopsEqualToMaxIops_AttachesQosPolicy() { + long currentSize = 4L * 1024 * 1024 * 1024; + stubResizeVolumeRecord(currentSize); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 100L, 100L, null, false, null, null, true)); + when(storagePool.getCapacityIops()).thenReturn(null); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(null); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)) + .thenReturn(new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_UUID, "lun-uuid-123", false)); + + VolumeQosPolicy qosPolicy = lookedUpQosPolicy("qos-uuid", "cs_100_to_100_iops_svm1"); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + stubQosCreateMocks(utilityMock, sanStrategy, iscsiCloudStackVolume(), qosPolicy); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_100_iops_svm1"), eq(100L), eq(100L)); + verify(sanStrategy).resizeCloudStackVolume(argThat(request -> + request.getLun() != null + && "lun-uuid-123".equals(request.getLun().getUuid()) + && isQosPolicyReference(request.getLun().getQosPolicy(), "qos-uuid", "cs_100_to_100_iops_svm1")), + eq(currentSize)); + } + } + @Test void testCreateAsync_MinIopsBeyondPoolCapacity_FailsWithCapacityDetails() { VolumeVO allocatedVolume = mock(VolumeVO.class); @@ -2266,6 +2424,31 @@ void testCreateAsync_MinIopsGreaterThanMaxIops_Fails() { } } + @Test + void testCreateAsync_MinIopsEqualToMaxIops_CreatesQosPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(100L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_100_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_100_iops_svm1"), eq(100L), eq(100L)); + verify(sanStrategy).createCloudStackVolume(argThat(request -> + request.getLun() != null && request.getLun().getQosPolicy() != null + && "qos-uuid".equals(request.getLun().getQosPolicy().getUuid()))); + } + } + @Test void testCreateAsync_NoIops_DoesNotCreateQosPolicy() { stubIscsiVolumeCreate(); @@ -2360,6 +2543,76 @@ void testDeleteAsync_DeletesUnusedQosPolicy() { } } + @Test + void testDeleteAsync_NfsClearsQosBeforeFileDelete() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + when(dataStore.getId()).thenReturn(1L); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)) + .thenReturn(new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false)); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) + .thenReturn(nasStrategy); + when(nasStrategy.updateCloudStackVolume(any())).thenAnswer(invocation -> invocation.getArgument(0)); + doNothing().when(nasStrategy).deleteCloudStackVolume(any()); + + driver.deleteAsync(dataStore, volumeInfo, commandCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CommandResult.class); + verify(commandCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(nasStrategy).updateCloudStackVolume(argThat(request -> + "flex-uuid".equals(request.getFlexVolumeUuid()) + && request.getFile() != null + && isQosPolicyReference(request.getFile().getQosPolicy(), null, OntapStorageConstants.QOS_POLICY_NONE))); + verify(nasStrategy).deleteCloudStackVolume(any()); + verify(nasStrategy).deleteVolumeQosPolicy("qos-uuid"); + } + } + + @Test + void testDeleteAsync_NfsRestoresQosWhenFileDeleteFails() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + when(dataStore.getId()).thenReturn(1L); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)) + .thenReturn(new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false)); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) + .thenReturn(nasStrategy); + when(nasStrategy.updateCloudStackVolume(any())).thenAnswer(invocation -> invocation.getArgument(0)); + doThrow(new CloudRuntimeException("file delete failed")).when(nasStrategy).deleteCloudStackVolume(any()); + + driver.deleteAsync(dataStore, volumeInfo, commandCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CommandResult.class); + verify(commandCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + ArgumentCaptor qosCaptor = ArgumentCaptor.forClass(CloudStackVolume.class); + verify(nasStrategy, times(2)).updateCloudStackVolume(qosCaptor.capture()); + assertTrue(isQosPolicyReference(qosCaptor.getAllValues().get(0).getFile().getQosPolicy(), + null, OntapStorageConstants.QOS_POLICY_NONE)); + VolumeQosPolicy restored = qosCaptor.getAllValues().get(1).getFile().getQosPolicy(); + assertEquals("qos-uuid", restored.getUuid()); + assertNull(restored.getName()); + assertNull(restored.getSvm()); + assertNull(restored.getFixed()); + verify(nasStrategy, never()).deleteVolumeQosPolicy(any()); + } + } + @Test void testDeleteAsync_QosPolicyDeleteFails_VolumeDeleteStillSucceeds() { when(dataStore.getId()).thenReturn(1L); @@ -2674,7 +2927,7 @@ private void stubResizeVolumeRecord(long currentSize) { when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getSize()).thenReturn(currentSize); lenient().when(volumeVO.getPath()).thenReturn("/vol/vol1/lun1"); - when(volumeDao.update(100L, volumeVO)).thenReturn(true); + lenient().when(volumeDao.update(100L, volumeVO)).thenReturn(true); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); @@ -3128,6 +3381,37 @@ void testCopyAsync_IscsiTemporarySnapshotCopy_ClonesLunAndRecordsIdentity() { verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.LUN_DOT_NAME), eq("/vol/vol1/cs_tmp_snap_200_100"), eq(false)); verify(volumeDao).update(anyLong(), eq(dbVolume)); verify(sanStrategy, never()).resizeCloudStackVolume(any(), anyLong()); + verify(sanStrategy, never()).createVolumeQosPolicy(anyString(), any(), any()); + } + } + + @Test + void testCopyAsync_IscsiTemporarySnapshotCopy_IgnoresVolumeIops() { + VolumeVO dbVolume = temporarySnapshotCopyVolume(); + SnapshotInfo snapshotInfo = stubSnapshotToVolumeCopy(dbVolume); + when(volumeInfo.getVolume()).thenReturn(temporarySnapshotCopyVolume()); + stubSnapshotCloneSource(ProtocolType.ISCSI.name()); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); + lenient().when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + lenient().when(volumeInfo.getMinIops()).thenReturn(100L); + lenient().when(volumeInfo.getMaxIops()).thenReturn(200L); + + Lun clonedLun = new Lun(); + clonedLun.setName("/vol/vol1/cs_tmp_snap_200_100"); + clonedLun.setUuid("tmp-lun-uuid"); + CloudStackVolume cloned = new CloudStackVolume(); + cloned.setLun(clonedLun); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())).thenReturn(sanStrategy); + when(sanStrategy.cloneCloudStackVolume(any())).thenReturn(cloned); + + driver.copyAsync(snapshotInfo, volumeInfo, null, null); + + verify(sanStrategy, never()).createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class)); + ArgumentCaptor requestCaptor = ArgumentCaptor.forClass(CloudStackVolume.class); + verify(sanStrategy).cloneCloudStackVolume(requestCaptor.capture()); + assertNull(requestCaptor.getValue().getLun().getQosPolicy()); } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java index 199a847e2136..86fdf1ff4a44 100755 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedNASStrategyTest.java @@ -330,10 +330,23 @@ public void testUpdateCloudStackVolume_AppliesQosPolicy() { @Test public void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() { - assertThrows(CloudRuntimeException.class, () -> strategy.updateCloudStackVolume(new CloudStackVolume())); + CloudStackVolume missingFile = new CloudStackVolume(); + missingFile.setVolumeInfo(mock(VolumeInfo.class)); + missingFile.setFlexVolumeUuid("flex-uuid"); + + assertInvalidNfsVolumeUpdate(null); + assertInvalidNfsVolumeUpdate(new CloudStackVolume()); + assertInvalidNfsVolumeUpdate(missingFile); + verify(nasFeignClient, never()).updateFile(anyString(), anyString(), anyString(), any(FileInfo.class)); } + private void assertInvalidNfsVolumeUpdate(CloudStackVolume request) { + CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, + () -> strategy.updateCloudStackVolume(request)); + assertEquals("Invalid NFS volume request", ex.getMessage()); + } + // Test createCloudStackVolume - Volume Not Found @Test public void testCreateCloudStackVolume_VolumeNotFound() { diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java index 0ffb0166d960..fabdf321e020 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java @@ -1344,20 +1344,50 @@ void testSetOntapStorage() { } @Test - void testUpdateCloudStackVolume_ReturnsNull() { + void testUpdateCloudStackVolume_AppliesQosPolicy() { Lun lun = new Lun(); lun.setUuid("lun-uuid-123"); VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); qosPolicy.setName("cs_0_to_5000_iops_svm1"); + qosPolicy.setUuid("qos-uuid"); lun.setQosPolicy(qosPolicy); CloudStackVolume request = new CloudStackVolume(); request.setLun(lun); - assertNull(unifiedSANStrategy.updateCloudStackVolume(request)); - assertNull(unifiedSANStrategy.updateCloudStackVolume(new CloudStackVolume())); + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.generateAuthHeader("admin", "password")) + .thenReturn(authHeader); + when(sanFeignClient.updateLun(eq(authHeader), eq("lun-uuid-123"), any(Lun.class))) + .thenReturn(null); + + CloudStackVolume result = unifiedSANStrategy.updateCloudStackVolume(request); + + assertSame(request, result); + ArgumentCaptor lunCaptor = ArgumentCaptor.forClass(Lun.class); + verify(sanFeignClient).updateLun(eq(authHeader), eq("lun-uuid-123"), lunCaptor.capture()); + assertEquals("cs_0_to_5000_iops_svm1", lunCaptor.getValue().getQosPolicy().getName()); + assertEquals("qos-uuid", lunCaptor.getValue().getQosPolicy().getUuid()); + } + } + + @Test + void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() { + CloudStackVolume lunWithoutUuid = new CloudStackVolume(); + lunWithoutUuid.setLun(new Lun()); + + assertInvalidIscsiVolumeUpdate(null); + assertInvalidIscsiVolumeUpdate(new CloudStackVolume()); + assertInvalidIscsiVolumeUpdate(lunWithoutUuid); + verify(sanFeignClient, never()).updateLun(any(), any(), any()); } + private void assertInvalidIscsiVolumeUpdate(CloudStackVolume request) { + CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, + () -> unifiedSANStrategy.updateCloudStackVolume(request)); + assertEquals("Invalid iSCSI volume request", ex.getMessage()); + } + @Test void testUpdateAccessGroup_ReturnsNull() { AccessGroup accessGroup = new AccessGroup(); diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 445bad769a06..f3a49aad23db 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -884,7 +884,7 @@ public VolumeVO allocVolume(long ownerId, Long zoneId, Long diskOfferingId, Long minIops = 0L; maxIops = 0L; } else { - if (minIops == null || minIops <= 0) { + if (minIops == null || minIops < 0) { throw new InvalidParameterValueException("The min IOPS must be greater than 0."); } @@ -2328,7 +2328,7 @@ public Volume changeDiskOfferingForVolumeInternal(Long volumeId, Long newDiskOff if (volumeMigrateRequired) { logger.warn(String.format("Volume change offering operation succeeded for volume ID: %s but volume resize operation failed, so please try resize volume operation separately", volume.getUuid())); } else { - throw new CloudRuntimeException(String.format("Volume disk offering change operation failed for volume ID [%s] because the volume resize operation failed.", volume.getUuid())); + throw new CloudRuntimeException(String.format("Volume disk offering change operation failed for volume ID [%s] because the volume resize operation failed: %s", volume.getUuid(), e.getMessage())); } } } @@ -2372,7 +2372,7 @@ public Volume changeDiskOfferingForVolumeInternal(Long volumeId, Long newDiskOff if (volumeMigrateRequired) { logger.warn(String.format("Volume change offering operation succeeded for volume ID: %s but volume resize operation failed, so please try resize volume operation separately", volume.getUuid())); } else { - throw new CloudRuntimeException(String.format("Volume change offering operation failed for volume ID: %s due to resize volume operation failed", volume.getUuid())); + throw new CloudRuntimeException(String.format("Volume change offering operation failed for volume ID: %s due to resize volume operation failed: %s", volume.getUuid(), e.getMessage()), e); } } } diff --git a/ui/src/views/storage/ChangeOfferingForVolume.vue b/ui/src/views/storage/ChangeOfferingForVolume.vue index 66a3cf1442e4..29d00dfe512e 100644 --- a/ui/src/views/storage/ChangeOfferingForVolume.vue +++ b/ui/src/views/storage/ChangeOfferingForVolume.vue @@ -158,7 +158,7 @@ export default { miniops: [{ type: 'number', validator: async (rule, value) => { - if (value && (isNaN(value) || value <= 0)) { + if (value && (isNaN(value) || value < 0)) { return Promise.reject(this.$t('message.error.number')) } return Promise.resolve() diff --git a/ui/src/views/storage/CreateVolume.vue b/ui/src/views/storage/CreateVolume.vue index ae203bdfa9b8..073ff204bba1 100644 --- a/ui/src/views/storage/CreateVolume.vue +++ b/ui/src/views/storage/CreateVolume.vue @@ -311,7 +311,7 @@ export default { size: [{ required: true, message: this.$t('message.error.custom.disk.size') }], miniops: [{ validator: async (rule, value) => { - if (value && (isNaN(value) || value <= 0)) { + if (value && (isNaN(value) || value < 0)) { return Promise.reject(this.$t('message.error.number')) } return Promise.resolve()