From 329e8ff55d49219b39c467164084799252db2ba6 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Wed, 9 Sep 2026 00:26:46 +0530 Subject: [PATCH 01/13] [CSTACKEX-279] Add Capacity IOPS for Storage Pool --- .../driver/OntapPrimaryDatastoreDriver.java | 59 +++++++- .../OntapPrimaryDatastoreLifecycle.java | 40 ++++- .../OntapPrimaryDatastoreDriverTest.java | 142 ++++++++++++++++++ .../OntapPrimaryDatastoreLifecycleTest.java | 78 ++++++++++ ui/src/views/infra/AddPrimaryStorage.vue | 6 + .../infra/zone/ZoneWizardAddResources.vue | 2 +- .../views/infra/zone/ZoneWizardLaunchZone.vue | 3 + 7 files changed, 325 insertions(+), 5 deletions(-) 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 ed942b438e16..3ee065110246 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 @@ -226,6 +226,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet * Creates a volume on the ONTAP backend. */ private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, VolumeInfo volumeObject, Map details) { + verifySufficientIopsForStoragePool(storagePool, volumeObject.getMinIops(), volumeObject.getId()); StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); return storageStrategy.createCloudStackVolume(createVolumeRequest(storagePool, details, volumeObject)); } @@ -440,6 +441,28 @@ private void deleteNfsTemplateCache(Map details, TemplateInfo te filePath, templateInfo.getId()); } + /** + * Rejects the request when the minimum IOPS being asked for would push the pool past its + * configured IOPS capacity. Pools without an IOPS capacity enforce no ceiling. + * + * Used IOPS include volumes still in {@link Volume.State#Creating} so a second overlapping + * create sees the first reservation. The volume being checked is omitted so its own min IOPS + * are not counted twice (CloudStack has already moved it to Creating before this runs). + */ + private void verifySufficientIopsForStoragePool(StoragePoolVO storagePool, Long requestedMinIops, long excludeVolumeId) { + Long capacityIops = storagePool.getCapacityIops(); + if (capacityIops == null || requestedMinIops == null || requestedMinIops <= 0) { + return; + } + + long requestedTotalIops = getAllocatedMinIops(storagePool, excludeVolumeId) + requestedMinIops; + if (requestedTotalIops > capacityIops) { + throw new CloudRuntimeException(String.format( + "Insufficient IOPS capacity on storage pool %s: requested total of %d IOPS exceeds the pool IOPS capacity of %d", + storagePool.getName(), requestedTotalIops, capacityIops)); + } + } + /** * Deletes a volume or snapshot from the ONTAP storage system. * @@ -595,8 +618,18 @@ public boolean canCopy(DataObject srcData, DataObject destData) { return false; } + /** + * Volume size and IOPS updates are not applied here. Pool IOPS capacity is enforced at + * volume create; resize and QoS belong to a later offering story. + */ @Override - public void resize(DataObject data, AsyncCompletionCallback callback) {} + public void resize(DataObject data, AsyncCompletionCallback callback) { + String path = data instanceof VolumeInfo ? ((VolumeInfo) data).getPath() : null; + String errMsg = "Volume resize is not supported for ONTAP primary storage"; + CreateCmdResult result = new CreateCmdResult(path, new Answer(null, false, errMsg)); + result.setResult(errMsg); + callback.complete(result); + } @Override public ChapInfo getChapInfo(DataObject dataObject) { @@ -1023,9 +1056,31 @@ public long getUsedBytes(StoragePool storagePool) { return 0; } + /** + * Returns min IOPS reserved on the pool, including volumes still being created. + * Destroyed and expunged volumes are omitted by the DAO query. + */ @Override public long getUsedIops(StoragePool storagePool) { - return 0; + return getAllocatedMinIops(storagePool, null); + } + + private long getAllocatedMinIops(StoragePool storagePool, Long excludeVolumeId) { + long usedIops = 0; + + List volumes = volumeDao.findNonDestroyedVolumesByPoolId(storagePool.getId(), null); + if (volumes != null) { + for (VolumeVO volume : volumes) { + if (excludeVolumeId != null && excludeVolumeId.equals(volume.getId())) { + continue; + } + if (volume.getMinIops() != null) { + usedIops += volume.getMinIops(); + } + } + } + + return usedIops; } /** diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java index f6fff96f9ca9..626e8e11f574 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java @@ -57,6 +57,7 @@ import com.cloud.agent.api.StoragePoolInfo; import com.cloud.alert.AlertManager; +import com.cloud.capacity.CapacityManager; import com.cloud.dc.ClusterVO; import com.cloud.dc.dao.ClusterDao; import com.cloud.exception.InvalidParameterValueException; @@ -81,6 +82,7 @@ public class OntapPrimaryDatastoreLifecycle extends BasePrimaryDataStoreLifeCycl @Inject private PrimaryDataStoreDao storagePoolDao; @Inject private StoragePoolDetailsDao storagePoolDetailsDao; @Inject private AlertManager _alertMgr; + @Inject private CapacityManager _capacityMgr; private static final Logger logger = LogManager.getLogger(OntapPrimaryDatastoreLifecycle.class); private static final long ONTAP_MIN_VOLUME_SIZE_IN_BYTES = 20971520L; @@ -101,6 +103,7 @@ public DataStore initialize(Map dsInfos) { String storagePoolName = (String) dsInfos.get("name"); String providerName = (String) dsInfos.get("providerName"); Long capacityBytes = (Long) dsInfos.get("capacityBytes"); + Long capacityIops = (Long) dsInfos.get("capacityIops"); boolean managed = (boolean) dsInfos.get("managed"); String tags = (String) dsInfos.get("tags"); Boolean isTagARule = (Boolean) dsInfos.get("isTagARule"); @@ -113,7 +116,7 @@ public DataStore initialize(Map dsInfos) { @SuppressWarnings("unchecked") Map details = (Map) dsInfos.get("details"); - validateInitializeInputs(capacityBytes, podId, clusterId, zoneId, storagePoolName, providerName, managed, details); + validateInitializeInputs(capacityBytes, capacityIops, podId, clusterId, zoneId, storagePoolName, providerName, managed, details); PrimaryDataStoreParameters parameters = new PrimaryDataStoreParameters(); if (clusterId != null) { @@ -207,12 +210,13 @@ public DataStore initialize(Map dsInfos) { parameters.setProviderName(providerName); parameters.setManaged(managed); parameters.setCapacityBytes(capacityBytes); + parameters.setCapacityIops(capacityIops); parameters.setUsedBytes(0); return _dataStoreHelper.createPrimaryDataStore(parameters); } - private void validateInitializeInputs(Long capacityBytes, Long podId, Long clusterId, Long zoneId, + private void validateInitializeInputs(Long capacityBytes, Long capacityIops, Long podId, Long clusterId, Long zoneId, String storagePoolName, String providerName, boolean managed, Map details) { if (capacityBytes == null || capacityBytes <= 0) { @@ -223,6 +227,10 @@ private void validateInitializeInputs(Long capacityBytes, Long podId, Long clust throw new InvalidParameterValueException("Storage pool capacity " + capacityBytes + " bytes is below the ONTAP minimum volume size of " + ONTAP_MIN_VOLUME_SIZE_IN_BYTES + " bytes (20 MB)"); } + // IOPS capacity is optional; when left blank no pool-level IOPS ceiling is enforced. + if (capacityIops != null && capacityIops <= 0) { + throw new InvalidParameterValueException("Storage pool IOPS capacity must be greater than 0"); + } // Validate scope if (podId == null ^ clusterId == null) { @@ -551,6 +559,11 @@ public boolean migrateToObjectStore(DataStore store) { @Override public void updateStoragePool(StoragePool storagePool, Map details) { + String newCapacityIopsStr = details.get(PrimaryDataStoreLifeCycle.CAPACITY_IOPS); + if (newCapacityIopsStr != null) { + validateUpdatedCapacityIops(storagePool, newCapacityIopsStr); + } + String newCapacityStr = details.get(PrimaryDataStoreLifeCycle.CAPACITY_BYTES); if (newCapacityStr == null) { logger.debug("No capacity change requested for pool: {}, skipping FlexVolume resize", storagePool.getName()); @@ -581,6 +594,29 @@ public void updateStoragePool(StoragePool storagePool, Map detai } } + private void validateUpdatedCapacityIops(StoragePool storagePool, String newCapacityIopsStr) { + long newCapacityIops; + try { + newCapacityIops = Long.parseLong(newCapacityIopsStr); + } catch (NumberFormatException e) { + throw new InvalidParameterValueException("Invalid storage pool IOPS capacity: " + newCapacityIopsStr); + } + if (newCapacityIops <= 0) { + throw new InvalidParameterValueException("Storage pool IOPS capacity must be greater than 0"); + } + + StoragePoolVO storagePoolVO = storagePoolDao.findById(storagePool.getId()); + if (storagePoolVO == null) { + throw new InvalidParameterValueException("Storage pool not found for id: " + storagePool.getId()); + } + long allocatedIops = _capacityMgr.getUsedIops(storagePoolVO); + if (newCapacityIops < allocatedIops) { + throw new InvalidParameterValueException(String.format( + "Cannot set IOPS capacity of storage pool %s to %d IOPS because %d IOPS are already allocated", + storagePool.getName(), newCapacityIops, allocatedIops)); + } + } + @Override public void enableStoragePool(DataStore store) { _dataStoreHelper.enable(store); 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 db1806c8473e..fe29fb595977 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 @@ -61,6 +61,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.util.HashMap; +import java.util.List; import java.util.Map; import static com.cloud.agent.api.to.DataObjectType.TEMPLATE; @@ -1226,6 +1227,147 @@ void testRevokeAccess_Template_UnmapsCacheLun() { } } + @Test + void testGetUsedIops_SumsMinIopsIncludingCreatingVolumes() { + VolumeVO readyVolume = mock(VolumeVO.class); + VolumeVO creatingVolume = mock(VolumeVO.class); + VolumeVO volumeWithoutMinIops = mock(VolumeVO.class); + + when(storagePool.getId()).thenReturn(1L); + when(readyVolume.getMinIops()).thenReturn(700L); + when(creatingVolume.getMinIops()).thenReturn(300L); + when(volumeWithoutMinIops.getMinIops()).thenReturn(null); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)) + .thenReturn(List.of(readyVolume, creatingVolume, volumeWithoutMinIops)); + + assertEquals(1000L, driver.getUsedIops(storagePool)); + } + + @Test + void testResize_IsNotSupported() { + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Volume resize is not supported")); + } + + @Test + void testCreateAsync_MinIopsBeyondPoolCapacity_FailsWithCapacityDetails() { + VolumeVO allocatedVolume = mock(VolumeVO.class); + when(allocatedVolume.getId()).thenReturn(50L); + when(allocatedVolume.getMinIops()).thenReturn(800L); + + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + when(volumeInfo.getMinIops()).thenReturn(300L); + + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("ontap-pool"); + when(storagePool.getCapacityIops()).thenReturn(1000L); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(List.of(allocatedVolume)); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains( + "storage pool ontap-pool: requested total of 1100 IOPS exceeds the pool IOPS capacity of 1000")); + verify(sanStrategy, never()).createCloudStackVolume(any()); + } + + @Test + void testCreateAsync_OtherCreatingVolumeCountsAgainstCapacity() { + VolumeVO readyVolume = mock(VolumeVO.class); + when(readyVolume.getId()).thenReturn(50L); + when(readyVolume.getMinIops()).thenReturn(500L); + + VolumeVO otherCreatingVolume = mock(VolumeVO.class); + when(otherCreatingVolume.getId()).thenReturn(60L); + when(otherCreatingVolume.getMinIops()).thenReturn(300L); + + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + when(volumeInfo.getMinIops()).thenReturn(300L); + + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("ontap-pool"); + when(storagePool.getCapacityIops()).thenReturn(1000L); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)) + .thenReturn(List.of(readyVolume, otherCreatingVolume)); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains( + "storage pool ontap-pool: requested total of 1100 IOPS exceeds the pool IOPS capacity of 1000")); + verify(sanStrategy, never()).createCloudStackVolume(any()); + } + + @Test + void testCreateAsync_DoesNotDoubleCountSelfWhenAlreadyCreating() { + VolumeVO readyVolume = mock(VolumeVO.class); + when(readyVolume.getId()).thenReturn(50L); + when(readyVolume.getMinIops()).thenReturn(500L); + + VolumeVO selfCreatingVolume = mock(VolumeVO.class); + when(selfCreatingVolume.getId()).thenReturn(100L); + + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + when(volumeInfo.getMinIops()).thenReturn(300L); + + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("vol1"); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); + when(storagePool.getCapacityIops()).thenReturn(1000L); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)) + .thenReturn(List.of(readyVolume, selfCreatingVolume)); + + Lun mockLun = new Lun(); + mockLun.setName("/vol/vol1/lun1"); + mockLun.setUuid("lun-uuid-123"); + CloudStackVolume cloudStackVolume = new CloudStackVolume(); + cloudStackVolume.setLun(mockLun); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).createCloudStackVolume(any()); + } + } + @Test void testCanHostAccessStoragePool_ReturnsTrue() { assertTrue(driver.canHostAccessStoragePool(host, storagePool)); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java index f4e3b1196e4f..71c8a2ba1143 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java @@ -49,6 +49,7 @@ import org.apache.cloudstack.storage.service.model.AccessGroup; import com.cloud.hypervisor.Hypervisor; import com.cloud.alert.AlertManager; +import com.cloud.capacity.CapacityManager; import java.util.Map; import java.util.List; import java.util.ArrayList; @@ -64,6 +65,7 @@ import static org.mockito.Mockito.never; import static org.mockito.ArgumentMatchers.contains; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -107,6 +109,9 @@ public class OntapPrimaryDatastoreLifecycleTest { @Mock private AlertManager _alertMgr; + @Mock + private CapacityManager _capacityMgr; + // Mock object that implements both DataStore and PrimaryDataStoreInfo // This is needed because attachCluster(DataStore) casts DataStore to PrimaryDataStoreInfo internally private DataStore dataStore; @@ -243,6 +248,41 @@ public void testInitialize_nfsPoolKeepsNetworkFilesystemType() { assertEquals(Storage.StoragePoolType.NetworkFilesystem, initializeAndCapturePoolType("NFS3")); } + private Long initializeAndCaptureCapacityIops(Map dsInfos) { + try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { + storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); + ontapPrimaryDatastoreLifecycle.initialize(dsInfos); + } + ArgumentCaptor captor = ArgumentCaptor.forClass(PrimaryDataStoreParameters.class); + verify(_dataStoreHelper).createPrimaryDataStore(captor.capture()); + return captor.getValue().getCapacityIops(); + } + + @Test + public void testInitialize_blankCapacityIopsLeavesPoolWithoutIopsCeiling() { + assertNull(initializeAndCaptureCapacityIops(buildDsInfosForProtocol("NFS3"))); + } + + @Test + public void testInitialize_capacityIopsIsStoredOnPool() { + Map dsInfos = buildDsInfosForProtocol("NFS3"); + dsInfos.put("capacityIops", 5000L); + + assertEquals(Long.valueOf(5000L), initializeAndCaptureCapacityIops(dsInfos)); + } + + @Test + public void testInitialize_nonPositiveCapacityIopsIsRejected() { + Map dsInfos = buildDsInfosForProtocol("NFS3"); + dsInfos.put("capacityIops", 0L); + + Exception ex = assertThrows(InvalidParameterValueException.class, + () -> ontapPrimaryDatastoreLifecycle.initialize(dsInfos)); + + assertTrue(ex.getMessage().contains("IOPS capacity must be greater than 0")); + verify(_dataStoreHelper, never()).createPrimaryDataStore(any()); + } + @Test public void testInitialize_null_Arg() { Exception ex = assertThrows(CloudRuntimeException.class,() -> @@ -1246,6 +1286,44 @@ public void testUpdateStoragePool_missingVolumeUuid_throwsCloudRuntimeException( } } + @Test + public void testUpdateStoragePool_capacityIopsBelowAllocated_throwsInvalidParameterValueException() { + StoragePool storagePool = mock(StoragePool.class); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("test-pool"); + + Map details = new HashMap<>(); + details.put(PrimaryDataStoreLifeCycle.CAPACITY_IOPS, "400"); + details.put(OntapStorageConstants.VOLUME_UUID, "flex-vol-uuid-123"); + details.put("protocol", "NFS3"); + + when(_capacityMgr.getUsedIops(any(StoragePoolVO.class))).thenReturn(900L); + + Exception ex = assertThrows(InvalidParameterValueException.class, + () -> ontapPrimaryDatastoreLifecycle.updateStoragePool(storagePool, details)); + + assertTrue(ex.getMessage().contains("900 IOPS are already allocated")); + verify(storageStrategy, never()).updateStorageVolume(any(Volume.class)); + } + + @Test + public void testUpdateStoragePool_capacityIopsAboveAllocated_isAccepted() { + StoragePool storagePool = mock(StoragePool.class); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("test-pool"); + + Map details = new HashMap<>(); + details.put(PrimaryDataStoreLifeCycle.CAPACITY_IOPS, "2000"); + details.put("protocol", "NFS3"); + // No CAPACITY_BYTES key — only the IOPS ceiling is being raised. + + when(_capacityMgr.getUsedIops(any(StoragePoolVO.class))).thenReturn(900L); + + ontapPrimaryDatastoreLifecycle.updateStoragePool(storagePool, details); + + verify(storageStrategy, never()).updateStorageVolume(any(Volume.class)); + } + @Test public void testUpdateStoragePool_updateStorageVolumeThrows_propagatesCloudRuntimeException() { // Setup diff --git a/ui/src/views/infra/AddPrimaryStorage.vue b/ui/src/views/infra/AddPrimaryStorage.vue index 7d189032f098..e5d9e6aa81af 100644 --- a/ui/src/views/infra/AddPrimaryStorage.vue +++ b/ui/src/views/infra/AddPrimaryStorage.vue @@ -301,6 +301,12 @@ + + + +
diff --git a/ui/src/views/infra/zone/ZoneWizardAddResources.vue b/ui/src/views/infra/zone/ZoneWizardAddResources.vue index 9bd9c6d37aef..faf9a10679f6 100644 --- a/ui/src/views/infra/zone/ZoneWizardAddResources.vue +++ b/ui/src/views/infra/zone/ZoneWizardAddResources.vue @@ -611,7 +611,7 @@ export default { title: 'label.capacityiops', key: 'capacityIops', hidden: { - provider: ['DefaultPrimary', 'PowerFlex', 'Linstor', 'NetApp ONTAP'] + provider: ['DefaultPrimary', 'PowerFlex', 'Linstor'] } }, { diff --git a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue index f21201572dff..9402fe99a9c9 100644 --- a/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue +++ b/ui/src/views/infra/zone/ZoneWizardLaunchZone.vue @@ -1619,6 +1619,9 @@ export default { if (this.prefillContent.capacityBytes && this.prefillContent.capacityBytes.length > 0) { params.capacityBytes = this.prefillContent.capacityBytes.split(',').join('') } + if (this.prefillContent.capacityIops && this.prefillContent.capacityIops.length > 0) { + params.capacityIops = this.prefillContent.capacityIops.split(',').join('') + } } params.tags = this.prefillContent?.primaryStorageTags || '' From f859caab349c6abd6b266c23b2bedade94a92c19 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Wed, 9 Sep 2026 22:09:34 +0530 Subject: [PATCH 02/13] [CSTACKEX-279] QoS for Disk and Compute Offering --- .../driver/OntapPrimaryDatastoreDriver.java | 105 ++++++- .../storage/feign/client/SANFeignClient.java | 2 +- .../storage/feign/model/FileInfo.java | 11 + .../cloudstack/storage/feign/model/Lun.java | 11 + .../storage/feign/model/VolumeQosPolicy.java | 83 ++++-- .../storage/service/StorageStrategy.java | 99 +++++++ .../storage/service/UnifiedNASStrategy.java | 39 ++- .../storage/service/UnifiedSANStrategy.java | 27 +- .../storage/utils/OntapStorageConstants.java | 5 + .../storage/utils/OntapStorageUtils.java | 58 ++++ .../OntapPrimaryDatastoreDriverTest.java | 274 ++++++++++++++++++ .../service/UnifiedNASStrategyTest.java | 76 +++++ .../service/UnifiedSANStrategyTest.java | 63 +++- .../storage/utils/OntapStorageUtilsTest.java | 61 ++++ 14 files changed, 871 insertions(+), 43 deletions(-) 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 3ee065110246..df893f544119 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 @@ -68,6 +68,7 @@ import org.apache.cloudstack.storage.feign.model.Lun; import org.apache.cloudstack.storage.feign.model.LunSpace; import org.apache.cloudstack.storage.feign.model.Svm; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import org.apache.cloudstack.storage.service.SANStrategy; @@ -228,7 +229,102 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, VolumeInfo volumeObject, Map details) { verifySufficientIopsForStoragePool(storagePool, volumeObject.getMinIops(), volumeObject.getId()); StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); - return storageStrategy.createCloudStackVolume(createVolumeRequest(storagePool, details, volumeObject)); + VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, volumeObject); + CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + storagePool, details, volumeObject, qosPolicy); + try { + CloudStackVolume created = storageStrategy.createCloudStackVolume(request); + persistQosPolicyDetails(volumeObject.getId(), qosPolicy); + return created; + } catch (RuntimeException e) { + if (qosPolicy != null) { + deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volumeObject.getId()); + } + throw e; + } + } + + private VolumeQosPolicy createQosPolicyIfNeeded(StorageStrategy storageStrategy, Map details, + VolumeInfo volumeObject) { + Volume.Type volumeType = volumeObject.getVolumeType(); + if (volumeType != Volume.Type.DATADISK && volumeType != Volume.Type.ROOT) { + return null; + } + Long minIops = volumeObject.getMinIops(); + Long maxIops = volumeObject.getMaxIops(); + if (!validateIops(minIops, maxIops)) { + return null; + } + String policyName = getQosPolicyName(details.get(OntapStorageConstants.SVM_NAME), minIops, maxIops); + return storageStrategy.createVolumeQosPolicy(policyName, minIops, maxIops); + } + + /** + * Builds a reusable SVM-scoped QoS policy name: cs_{min}_to_{max}_iops_{svmName}. + * Dots in the SVM name are replaced with underscores (ONTAP QoS names cannot contain '.'). + */ + private String getQosPolicyName(String svmName, Long minIops, Long maxIops) { + String sanitizedSvmName = svmName == null ? "" : svmName.replace(".", OntapStorageConstants.UNDERSCORE); + long min = minIops != null && minIops > 0 ? minIops : 0; + long max = maxIops != null && maxIops > 0 ? maxIops : 0; + return OntapStorageConstants.QOS_POLICY_NAME_PREFIX + min + OntapStorageConstants.UNDERSCORE + + OntapStorageConstants.QOS_POLICY_NAME_TO + max + OntapStorageConstants.UNDERSCORE + + OntapStorageConstants.QOS_POLICY_NAME_IOPS + sanitizedSvmName; + } + + /** + * Returns true when the volume has a positive min or max IOPS to apply as an ONTAP QoS policy. + * Throws when both limits are set and min IOPS is greater than max IOPS. + */ + private boolean validateIops(Long minIops, Long maxIops) { + boolean hasMinIops = minIops != null && minIops > 0; + boolean hasMaxIops = maxIops != null && maxIops > 0; + if (!hasMinIops && !hasMaxIops) { + return false; + } + if (hasMinIops && hasMaxIops && minIops > maxIops) { + throw new InvalidParameterValueException( + "Minimum IOPS cannot be greater than maximum IOPS"); + } + return true; + } + + private void persistQosPolicyDetails(long volumeId, VolumeQosPolicy qosPolicy) { + volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_NAME); + volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID); + if (qosPolicy == null) { + return; + } + volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.QOS_POLICY_NAME, qosPolicy.getName(), false); + volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID, qosPolicy.getUuid(), false); + } + + private boolean isQosPolicyUsedByOtherVolumes(String policyUuid, Long excludeVolumeId) { + if (policyUuid == null || policyUuid.isEmpty()) { + return false; + } + List references = volumeDetailsDao.findDetails( + OntapStorageConstants.QOS_POLICY_UUID, policyUuid, null); + if (references == null) { + return false; + } + for (VolumeDetailVO reference : references) { + if (excludeVolumeId == null || reference.getResourceId() != excludeVolumeId) { + return true; + } + } + return false; + } + + private void deleteQosPolicyIfUnused(StorageStrategy storageStrategy, String policyUuid, Long excludeVolumeId) { + if (policyUuid == null || policyUuid.isEmpty()) { + return; + } + if (isQosPolicyUsedByOtherVolumes(policyUuid, excludeVolumeId)) { + logger.info("QoS policy [{}] is still assigned to other volumes; skipping delete", policyUuid); + return; + } + storageStrategy.deleteVolumeQosPolicy(policyUuid); } /** @@ -493,8 +589,15 @@ 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); + if (qosPolicyDetail != null) { + volumeDetailsDao.removeDetail(volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_UUID); + volumeDetailsDao.removeDetail(volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_NAME); + deleteQosPolicyIfUnused(storageStrategy, qosPolicyDetail.getValue(), volumeInfo.getId()); + } logger.info("deleteAsync: Volume deleted: " + volumeInfo.getId()); commandResult.setResult(null); commandResult.setSuccess(true); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java index d365468cee10..240b8ab1323e 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java @@ -52,7 +52,7 @@ public interface SANFeignClient { @RequestLine("PATCH /api/storage/luns/{uuid}") @Headers({"Authorization: {authHeader}", "Content-Type: application/json"}) - void updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Lun lun); + JobResponse updateLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Lun lun); @RequestLine("DELETE /api/storage/luns/{uuid}") @Headers({"Authorization: {authHeader}"}) diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileInfo.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileInfo.java index a5dd24a3a286..71bf4c980bd4 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileInfo.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileInfo.java @@ -49,6 +49,8 @@ public class FileInfo { private Boolean overwriteEnabled = null; @JsonProperty("path") private String path = null; + @JsonProperty("qos_policy") + private VolumeQosPolicy qosPolicy = null; @JsonProperty("size") private Long size = null; @JsonProperty("target") @@ -178,6 +180,15 @@ public String getPath() { public void setPath(String path) { this.path = path; } + + public VolumeQosPolicy getQosPolicy() { + return qosPolicy; + } + + public void setQosPolicy(VolumeQosPolicy qosPolicy) { + this.qosPolicy = qosPolicy; + } + public Long getSize() { return size; } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java index 922751c9a77a..0b2e09dbec45 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java @@ -83,6 +83,9 @@ public static PropertyClassEnum fromValue(String value) { @JsonProperty("name") private String name = null; + @JsonProperty("qos_policy") + private VolumeQosPolicy qosPolicy = null; + @JsonProperty("clone") private Clone clone = null; @@ -202,6 +205,14 @@ public void setName(String name) { this.name = name; } + public VolumeQosPolicy getQosPolicy() { + return qosPolicy; + } + + public void setQosPolicy(VolumeQosPolicy qosPolicy) { + this.qosPolicy = qosPolicy; + } + public Lun osType(OsTypeEnum osType) { this.osType = osType; return this; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java index 7a9a4307ab1a..077ddcb2d734 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java @@ -26,43 +26,21 @@ @JsonIgnoreProperties(ignoreUnknown = true) @JsonInclude(JsonInclude.Include.NON_NULL) public class VolumeQosPolicy { - @JsonProperty("max_throughput_iops") - private Integer maxThroughputIops = null; - - @JsonProperty("max_throughput_mbps") - private Integer maxThroughputMbps = null; - - @JsonProperty("min_throughput_iops") - private Integer minThroughputIops = null; - + @JsonProperty("fixed") + private Fixed fixed; @JsonProperty("name") private String name = null; - @JsonProperty("uuid") private String uuid = null; + @JsonProperty("svm") + private Svm svm; - public Integer getMaxThroughputIops() { - return maxThroughputIops; + public Fixed getFixed() { + return fixed; } - public void setMaxThroughputIops(Integer maxThroughputIops) { - this.maxThroughputIops = maxThroughputIops; - } - - public Integer getMaxThroughputMbps() { - return maxThroughputMbps; - } - - public void setMaxThroughputMbps(Integer maxThroughputMbps) { - this.maxThroughputMbps = maxThroughputMbps; - } - - public Integer getMinThroughputIops() { - return minThroughputIops; - } - - public void setMinThroughputIops(Integer minThroughputIops) { - this.minThroughputIops = minThroughputIops; + public void setFixed(Fixed fixed) { + this.fixed = fixed; } public String getName() { @@ -80,4 +58,49 @@ public String getUuid() { public void setUuid(String uuid) { this.uuid = uuid; } + + public Svm getSvm() { + return svm; + } + + public void setSvm(Svm svm) { + this.svm = svm; + } + + @JsonIgnoreProperties(ignoreUnknown = true) + @JsonInclude(JsonInclude.Include.NON_NULL) + public static class Fixed { + @JsonProperty("capacity_shared") + private Boolean capacityShared; + + @JsonProperty("min_throughput_iops") + private Long minThroughputIops; + + @JsonProperty("max_throughput_iops") + private Long maxThroughputIops; + + public Boolean getCapacityShared() { + return capacityShared; + } + + public void setCapacityShared(Boolean capacityShared) { + this.capacityShared = capacityShared; + } + + public Long getMinThroughputIops() { + return minThroughputIops; + } + + public void setMinThroughputIops(Long minThroughputIops) { + this.minThroughputIops = minThroughputIops; + } + + public Long getMaxThroughputIops() { + return maxThroughputIops; + } + + public void setMaxThroughputIops(Long maxThroughputIops) { + this.maxThroughputIops = maxThroughputIops; + } + } } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index e482301967f1..aa9e9c7bd3fc 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -31,6 +31,7 @@ import org.apache.cloudstack.storage.feign.client.JobFeignClient; import org.apache.cloudstack.storage.feign.client.NASFeignClient; import org.apache.cloudstack.storage.feign.client.NetworkFeignClient; +import org.apache.cloudstack.storage.feign.client.QosFeignClient; import org.apache.cloudstack.storage.feign.client.SANFeignClient; import org.apache.cloudstack.storage.feign.client.SnapshotFeignClient; import org.apache.cloudstack.storage.feign.client.EmsFeignClient; @@ -48,6 +49,7 @@ import org.apache.cloudstack.storage.feign.model.Svm; import org.apache.cloudstack.storage.feign.model.Version; import org.apache.cloudstack.storage.feign.model.Volume; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; @@ -81,6 +83,7 @@ public abstract class StorageStrategy { protected SvmFeignClient svmFeignClient; protected JobFeignClient jobFeignClient; protected NetworkFeignClient networkFeignClient; + protected QosFeignClient qosFeignClient; protected SANFeignClient sanFeignClient; protected NASFeignClient nasFeignClient; protected SnapshotFeignClient snapshotFeignClient; @@ -114,6 +117,7 @@ public StorageStrategy(OntapStorage ontapStorage) { this.svmFeignClient = feignClientFactory.createClient(SvmFeignClient.class, baseURL); this.jobFeignClient = feignClientFactory.createClient(JobFeignClient.class, baseURL); this.networkFeignClient = feignClientFactory.createClient(NetworkFeignClient.class, baseURL); + this.qosFeignClient = feignClientFactory.createClient(QosFeignClient.class, baseURL); this.sanFeignClient = feignClientFactory.createClient(SANFeignClient.class, baseURL); this.nasFeignClient = feignClientFactory.createClient(NASFeignClient.class, baseURL); this.snapshotFeignClient = feignClientFactory.createClient(SnapshotFeignClient.class, baseURL); @@ -957,6 +961,101 @@ public String getAuthHeader() { return OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); } + public VolumeQosPolicy createVolumeQosPolicy(String policyName, Long minIops, Long maxIops) { + VolumeQosPolicy existingPolicy = getVolumeQosPolicy(policyName); + if (existingPolicy != null) { + logger.info("Reusing existing ONTAP QoS policy [{}]", policyName); + return existingPolicy; + } + + VolumeQosPolicy policy = buildVolumeQosPolicy(policyName, minIops, maxIops); + Svm svm = new Svm(); + svm.setName(storage.getSvmName()); + policy.setSvm(svm); + try { + JobResponse response = qosFeignClient.createPolicy(getAuthHeader(), policy); + pollJobIfPresent(response, "create QoS policy [" + policyName + "]"); + } catch (FeignException e) { + if (e.status() != 409) { + throw new CloudRuntimeException("Failed to create ONTAP QoS policy [" + policyName + "]: " + + e.getMessage(), e); + } + logger.info("QoS policy [{}] already exists; using the existing policy", policyName); + } + + VolumeQosPolicy createdPolicy = getVolumeQosPolicy(policyName); + if (createdPolicy == null || createdPolicy.getUuid() == null) { + throw new CloudRuntimeException("Unable to resolve ONTAP QoS policy [" + policyName + "] after creation"); + } + return createdPolicy; + } + + /** + * Prefers the ONTAP REST error body (includes codes such as 8454269) over Feign's status line. + */ + protected String getOntapErrorDetail(Throwable error) { + if (error instanceof FeignException) { + try { + String body = ((FeignException) error).contentUTF8(); + if (body != null && !body.isBlank()) { + return body; + } + } catch (RuntimeException ignored) { + // Mocked or empty Feign responses may not expose a body. + } + } + return error != null ? error.getMessage() : null; + } + + protected CloudRuntimeException wrapOntapApiFailure(String operation, Throwable error) { + return new CloudRuntimeException(operation + ": " + getOntapErrorDetail(error), error); + } + + public void deleteVolumeQosPolicy(String policyUuid) { + if (policyUuid == null || policyUuid.isEmpty()) { + return; + } + try { + JobResponse response = qosFeignClient.deletePolicy(getAuthHeader(), policyUuid); + pollJobIfPresent(response, "delete QoS policy [" + policyUuid + "]"); + } catch (FeignException e) { + if (OntapStorageUtils.isOntapObjectNotFoundError(e)) { + logger.info("QoS policy [{}] is already absent", policyUuid); + return; + } + throw new CloudRuntimeException("Failed to delete ONTAP QoS policy [" + policyUuid + "]: " + + e.getMessage(), e); + } + } + + private VolumeQosPolicy getVolumeQosPolicy(String policyName) { + Map queryParams = new HashMap<>(); + queryParams.put(OntapStorageConstants.NAME, policyName); + queryParams.put(OntapStorageConstants.SVM_DOT_NAME, storage.getSvmName()); + OntapResponse response = qosFeignClient.getPolicies(getAuthHeader(), queryParams); + if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + return null; + } + return response.getRecords().get(0); + } + + private VolumeQosPolicy buildVolumeQosPolicy(String policyName, Long minIops, Long maxIops) { + VolumeQosPolicy.Fixed fixed = new VolumeQosPolicy.Fixed(); + fixed.setCapacityShared(false); + // ONTAP rejects a policy whose throughput limit is zero, so only unlimited-side values are omitted. + if (minIops != null && minIops > 0) { + fixed.setMinThroughputIops(minIops); + } + if (maxIops != null && maxIops > 0) { + fixed.setMaxThroughputIops(maxIops); + } + + VolumeQosPolicy policy = new VolumeQosPolicy(); + policy.setName(policyName); + policy.setFixed(fixed); + return policy; + } + /** * Polls an ONTAP async job for successful completion. * 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 4a9f45f7301e..81884dd3f5b7 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 @@ -95,8 +95,28 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume logger.error("createCloudStackVolume: " + errMsg); throw new CloudRuntimeException(errMsg); } + if (cloudstackVolume.getFile() != null && cloudstackVolume.getFile().getQosPolicy() != null) { + try { + updateCloudStackVolume(cloudstackVolume); + } catch (RuntimeException qosError) { + logger.error("createCloudStackVolume: QoS attach failed; deleting leftover NFS volume file", qosError); + try { + Answer cleanup = deleteVolumeOnKVMHost(cloudstackVolume.getVolumeInfo()); + if (cleanup == null || !cleanup.getResult()) { + logger.error("createCloudStackVolume: leftover NFS file may remain after QoS attach failure: {}", + cleanup != null ? cleanup.getDetails() : "null answer"); + } + } catch (Exception cleanupError) { + logger.error("createCloudStackVolume: failed to delete leftover NFS volume file after QoS attach failure", + cleanupError); + } + throw qosError; + } + } return cloudstackVolume; - }catch (Exception e) { + } catch (CloudRuntimeException e) { + throw e; + } catch (Exception e) { logger.error("createCloudStackVolume: error occured " + e); throw new CloudRuntimeException(e); } @@ -116,7 +136,22 @@ public CloudStackVolume createTemplateCache(StoragePoolVO storagePool, TemplateI @Override CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { - return null; + if (cloudstackVolume == null || cloudstackVolume.getVolumeInfo() == null + || cloudstackVolume.getFlexVolumeUuid() == null || cloudstackVolume.getFile() == null) { + throw new CloudRuntimeException("Invalid NFS volume QoS update request"); + } + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(cloudstackVolume.getFile().getQosPolicy()); + String filePath = cloudstackVolume.getVolumeInfo().getUuid(); + try { + nasFeignClient.updateFile(getAuthHeader(), cloudstackVolume.getFlexVolumeUuid(), filePath, fileInfo); + } catch (FeignException e) { + throw wrapOntapApiFailure("Failed to apply QoS policy to NFS volume file", e); + } + logger.info("Applied QoS policy [{}] to NFS volume file [{}]", + cloudstackVolume.getFile().getQosPolicy() != null + ? cloudstackVolume.getFile().getQosPolicy().getName() : null, filePath); + return cloudstackVolume; } @Override 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 b9e32b081e4d..aec642634f53 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 @@ -90,12 +90,12 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume } catch (FeignException e) { logger.error("FeignException occurred while creating LUN: {}, Status: {}, Exception: {}", cloudstackVolume.getLun().getName(), e.status(), e.getMessage()); - throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage()); + throw wrapOntapApiFailure("Failed to create Lun", e); } catch (CloudRuntimeException e) { throw e; } catch (Exception e) { logger.error("Exception occurred while creating LUN: {}, Exception: {}", cloudstackVolume.getLun().getName(), e.getMessage()); - throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage()); + throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage(), e); } } @@ -184,7 +184,24 @@ private void bestEffortDeleteTemplateCacheLun(String svmName, String lunName, St @Override CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { - return null; + if (cloudstackVolume == null || cloudstackVolume.getLun() == null + || cloudstackVolume.getLun().getUuid() == null) { + throw new CloudRuntimeException("Invalid iSCSI volume QoS update 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 wrapOntapApiFailure("Failed to apply QoS policy to LUN", e); + } + logger.info("Applied QoS policy [{}] to LUN [{}]", + cloudstackVolume.getLun().getQosPolicy() != null + ? cloudstackVolume.getLun().getQosPolicy().getName() : null, + cloudstackVolume.getLun().getUuid()); + return cloudstackVolume; } @Override @@ -298,9 +315,7 @@ public void resizeCloudStackVolume(CloudStackVolume cloudstackVolume, long sizeI sanFeignClient.updateLun(authHeader, lunUuid, patch); logger.debug("resizeCloudStackVolume: Lun {} resized to {} bytes", lunUuid, sizeInBytes); } catch (FeignException e) { - logger.error("FeignException occurred while resizing LUN: {}, Status: {}, Exception: {}", - lunUuid, e.status(), e.getMessage()); - throw new CloudRuntimeException("Failed to resize Lun: " + e.getMessage()); + throw wrapOntapApiFailure("Failed to resize Lun", e); } catch (Exception e) { logger.error("Exception occurred while resizing LUN: {}, Exception: {}", lunUuid, e.getMessage()); throw new CloudRuntimeException("Failed to resize Lun: " + e.getMessage()); 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 4ac49c95dfa1..0330d77051a8 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 @@ -102,6 +102,11 @@ public class OntapStorageConstants { public static final String LUN_DOT_NAME = "lun.name"; public static final String IQN = "iqn"; public static final String LUN_DOT_UUID = "lun.uuid"; + public static final String QOS_POLICY_NAME = "qosPolicyName"; + public static final String QOS_POLICY_UUID = "qosPolicyUuid"; + public static final String QOS_POLICY_NAME_PREFIX = "cs_"; + public static final String QOS_POLICY_NAME_TO = "to"; + public static final String QOS_POLICY_NAME_IOPS = "iops_"; public static final String LOGICAL_UNIT_NUMBER = "logical_unit_number"; public static final String IGROUP_DOT_NAME = "igroup.name"; public static final String IGROUP_DOT_UUID = "igroup.uuid"; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index e2b419ea46f0..a23121c46adf 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -23,10 +23,17 @@ import java.util.Map; import feign.FeignException; +import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.feign.model.FileInfo; import org.apache.cloudstack.storage.feign.model.Lun; +import org.apache.cloudstack.storage.feign.model.LunSpace; import org.apache.cloudstack.storage.feign.model.OntapStorage; +import org.apache.cloudstack.storage.feign.model.Svm; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.provider.StorageProviderFactory; import org.apache.cloudstack.storage.service.StorageStrategy; +import org.apache.cloudstack.storage.service.model.CloudStackVolume; import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -55,6 +62,57 @@ public static String generateAuthHeader (String username, String password) { return BASIC + StringUtils.SPACE + new String(encodedBytes); } + public static CloudStackVolume createCloudStackVolumeRequestByProtocol(StoragePoolVO storagePool, Map details, + DataObject volumeObject, VolumeQosPolicy qosPolicy) { + CloudStackVolume cloudStackVolumeRequest = null; + VolumeQosPolicy qosPolicyReference = null; + if (qosPolicy != null) { + qosPolicyReference = new VolumeQosPolicy(); + qosPolicyReference.setName(qosPolicy.getName()); + qosPolicyReference.setUuid(qosPolicy.getUuid()); + } + + String protocol = details.get(OntapStorageConstants.PROTOCOL); + ProtocolType protocolType = ProtocolType.valueOf(protocol); + switch (protocolType) { + case NFS3: + cloudStackVolumeRequest = new CloudStackVolume(); + cloudStackVolumeRequest.setDatastoreId(String.valueOf(storagePool.getId())); + cloudStackVolumeRequest.setFlexVolumeUuid(details.get(OntapStorageConstants.VOLUME_UUID)); + cloudStackVolumeRequest.setVolumeInfo(volumeObject); + if (qosPolicyReference != null) { + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(qosPolicyReference); + cloudStackVolumeRequest.setFile(fileInfo); + } + break; + case ISCSI: + Svm svm = new Svm(); + svm.setName(details.get(OntapStorageConstants.SVM_NAME)); + cloudStackVolumeRequest = new CloudStackVolume(); + Lun lunRequest = new Lun(); + lunRequest.setSvm(svm); + + LunSpace lunSpace = new LunSpace(); + lunSpace.setSize(volumeObject.getSize()); + lunRequest.setSpace(lunSpace); + String lunName = volumeObject.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE); + if (!isValidName(lunName)) { + String errMsg = "createAsync: Invalid dataObject name [" + lunName + + "]. It must start with a letter and can only contain letters, digits, and underscores, and be up to 200 characters long."; + throw new InvalidParameterValueException(errMsg); + } + lunRequest.setName(getLunName(storagePool.getName(), lunName)); + lunRequest.setOsType(Lun.OsTypeEnum.valueOf(getOSTypeFromHypervisor(storagePool.getHypervisor().name()))); + lunRequest.setQosPolicy(qosPolicyReference); + cloudStackVolumeRequest.setLun(lunRequest); + break; + default: + throw new CloudRuntimeException("Unsupported protocol " + protocol); + } + return cloudStackVolumeRequest; + } + public static boolean isValidName(String name) { // Check for null and length constraint first if (name == null || name.length() > 200) { 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 fe29fb595977..cc22e3b5165e 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 @@ -25,6 +25,7 @@ import com.cloud.storage.ScopeType; import com.cloud.storage.Storage; import com.cloud.storage.VMTemplateStoragePoolVO; +import com.cloud.storage.Volume; import com.cloud.storage.VolumeVO; import com.cloud.storage.VolumeDetailVO; import com.cloud.storage.dao.VMTemplatePoolDao; @@ -44,6 +45,8 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.feign.model.Igroup; import org.apache.cloudstack.storage.feign.model.Lun; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; +import org.apache.cloudstack.storage.service.StorageStrategy; import org.apache.cloudstack.storage.service.UnifiedNASStrategy; import org.apache.cloudstack.storage.service.UnifiedSANStrategy; import org.apache.cloudstack.storage.service.model.AccessGroup; @@ -77,6 +80,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; @@ -210,6 +215,8 @@ void testCreateAsync_VolumeWithISCSI_Success() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(responseVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(responseVolume); // Execute @@ -254,6 +261,8 @@ void testCreateAsync_VolumeWithNFS_Success() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) .thenReturn(nasStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(mockCloudStackVolume); when(nasStrategy.createCloudStackVolume(any())).thenReturn(mockCloudStackVolume); @@ -353,6 +362,7 @@ void testDeleteAsync_ISCSIVolume_Success() { when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_NAME)).thenReturn(lunNameDetail); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(null); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) @@ -1357,6 +1367,8 @@ void testCreateAsync_DoesNotDoubleCountSelfWhenAlreadyCreating() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1368,6 +1380,268 @@ void testCreateAsync_DoesNotDoubleCountSelfWhenAlreadyCreating() { } } + @Test + void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to200_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + 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_to200_iops_svm1"), eq(100L), eq(200L)); + utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), argThat(policy -> policy != null && "qos-uuid".equals(policy.getUuid())))); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_NAME), + eq("cs_100_to200_iops_svm1"), eq(false)); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-uuid"), eq(false)); + } + } + + @Test + void testCreateAsync_RootDiskCustomIops_CreatesAndPersistsQosPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volumeInfo.getMinIops()).thenReturn(111L); + when(volumeInfo.getMaxIops()).thenReturn(999L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-root-uuid", "cs_111_to999_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + 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_111_to999_iops_svm1"), eq(111L), eq(999L)); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-root-uuid"), eq(false)); + } + } + + @Test + void testCreateAsync_NfsDataDiskWithIops_CreatesQosPolicy() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); + lenient().when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-nfs-uuid", "cs_100_to200_iops_svm1"); + CloudStackVolume cloudStackVolume = new CloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + stubQosCreateMocks(utilityMock, nasStrategy, cloudStackVolume, qosPolicy); + + 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_to200_iops_svm1"), eq(100L), eq(200L)); + utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), argThat(policy -> policy != null && "qos-nfs-uuid".equals(policy.getUuid())))); + } + } + + @Test + void testCreateAsync_MinIopsGreaterThanMaxIops_Fails() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(200L); + when(volumeInfo.getMaxIops()).thenReturn(100L); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains( + "Minimum IOPS cannot be greater than maximum IOPS")); + verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); + verify(sanStrategy, never()).createCloudStackVolume(any()); + } + } + + @Test + void testCreateAsync_NoIops_DoesNotCreateQosPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); + utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), isNull())); + } + } + + @Test + void testCreateAsync_SwapVolumeWithIops_DoesNotCreateQosPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.SWAP); + when(volumeInfo.getMinIops()).thenReturn(100L); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); + } + } + + @Test + void testCreateAsync_QosCreateThenLunCreateFails_DeletesUnusedPolicy() { + stubIscsiVolumeCreate(); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to200_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + when(sanStrategy.createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class))) + .thenReturn(qosPolicy); + when(sanStrategy.createCloudStackVolume(any())).thenThrow(new CloudRuntimeException( + "Failed to create Lun: {\"error\":{\"code\":\"8454269\"}}")); + when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) + .thenReturn(List.of()); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); + } + } + + @Test + void testDeleteAsync_DeletesUnusedQosPolicy() { + when(dataStore.getId()).thenReturn(1L); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + + VolumeDetailVO lunNameDetail = new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_NAME, "/vol/vol1/lun1", false); + VolumeDetailVO lunUuidDetail = new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_UUID, "lun-uuid-123", false); + VolumeDetailVO qosDetail = new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); + + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_NAME)).thenReturn(lunNameDetail); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); + when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) + .thenReturn(List.of(qosDetail)); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) + .thenReturn(sanStrategy); + doNothing().when(sanStrategy).deleteCloudStackVolume(any()); + + driver.deleteAsync(dataStore, volumeInfo, commandCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CommandResult.class); + verify(commandCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(volumeDetailsDao).removeDetail(100L, OntapStorageConstants.QOS_POLICY_UUID); + verify(volumeDetailsDao).removeDetail(100L, OntapStorageConstants.QOS_POLICY_NAME); + verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); + } + } + + private void stubIscsiVolumeCreate() { + when(dataStore.getId()).thenReturn(1L); + when(dataStore.getName()).thenReturn("ontap-pool"); + when(volumeInfo.getType()).thenReturn(VOLUME); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getName()).thenReturn("test-volume"); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + lenient().when(storagePool.getId()).thenReturn(1L); + lenient().when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); + lenient().when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); + lenient().when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + lenient().when(volumeVO.getId()).thenReturn(100L); + } + + private CloudStackVolume iscsiCloudStackVolume() { + Lun mockLun = new Lun(); + mockLun.setName("/vol/vol1/lun1"); + mockLun.setUuid("lun-uuid-123"); + CloudStackVolume volume = new CloudStackVolume(); + volume.setLun(mockLun); + return volume; + } + + private VolumeQosPolicy qosPolicy(String uuid, String name) { + VolumeQosPolicy policy = new VolumeQosPolicy(); + policy.setUuid(uuid); + policy.setName(name); + return policy; + } + + private void stubQosCreateMocks(MockedStatic utilityMock, + StorageStrategy strategy, + CloudStackVolume cloudStackVolume, VolumeQosPolicy qosPolicy) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(strategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + when(strategy.createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class))) + .thenReturn(qosPolicy); + when(strategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); + } + @Test void testCanHostAccessStoragePool_ReturnsTrue() { assertTrue(driver.canHostAccessStoragePool(host, storagePool)); 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 dd90363af045..4cdb8d33ec24 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 @@ -29,6 +29,7 @@ import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector; import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; import org.apache.cloudstack.storage.command.CreateObjectCommand; +import org.apache.cloudstack.storage.command.DeleteCommand; import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; @@ -45,6 +46,7 @@ import org.apache.cloudstack.storage.feign.model.FileInfo; import org.apache.cloudstack.storage.feign.model.Job; import org.apache.cloudstack.storage.feign.model.OntapStorage; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.feign.model.response.JobResponse; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import org.apache.cloudstack.storage.service.model.AccessGroup; @@ -72,10 +74,12 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; @@ -226,6 +230,78 @@ public void testCreateTemplateCache_IsNoOp() { assertNull(result); } + + @Test + public void testCreateCloudStackVolume_AppliesQosPolicyToNfsFile() throws Exception { + CloudStackVolume cloudStackVolume = mock(CloudStackVolume.class); + VolumeObject volumeObject = mock(VolumeObject.class); + VolumeVO volumeVO = mock(VolumeVO.class); + EndPoint endPoint = mock(EndPoint.class); + Answer answer = new Answer(null, true, "Success"); + + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(qosPolicy); + + when(cloudStackVolume.getDatastoreId()).thenReturn("1"); + when(cloudStackVolume.getVolumeInfo()).thenReturn(volumeObject); + when(cloudStackVolume.getFlexVolumeUuid()).thenReturn("flex-uuid"); + when(cloudStackVolume.getFile()).thenReturn(fileInfo); + when(volumeObject.getId()).thenReturn(100L); + when(volumeObject.getUuid()).thenReturn("volume-uuid-123"); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeDao.update(anyLong(), any(VolumeVO.class))).thenReturn(true); + when(epSelector.select(volumeObject)).thenReturn(endPoint); + when(endPoint.sendMessage(any(CreateObjectCommand.class))).thenReturn(answer); + + CloudStackVolume result = strategy.createCloudStackVolume(cloudStackVolume); + + assertNotNull(result); + verify(nasFeignClient).updateFile(anyString(), eq("flex-uuid"), eq("volume-uuid-123"), + argThat(file -> file.getQosPolicy() != null + && "cs_100_to200_iops_svm1".equals(file.getQosPolicy().getName()))); + } + + @Test + public void testCreateCloudStackVolume_QosAttachFails_DeletesLeftoverNfsFile() { + CloudStackVolume cloudStackVolume = mock(CloudStackVolume.class); + VolumeObject volumeObject = mock(VolumeObject.class); + VolumeVO volumeVO = mock(VolumeVO.class); + EndPoint endPoint = mock(EndPoint.class); + Answer createAnswer = new Answer(null, true, "Success"); + Answer deleteAnswer = new Answer(null, true, "Deleted"); + + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(qosPolicy); + + when(cloudStackVolume.getDatastoreId()).thenReturn("1"); + when(cloudStackVolume.getVolumeInfo()).thenReturn(volumeObject); + when(cloudStackVolume.getFlexVolumeUuid()).thenReturn("flex-uuid"); + when(cloudStackVolume.getFile()).thenReturn(fileInfo); + when(volumeObject.getId()).thenReturn(100L); + when(volumeObject.getUuid()).thenReturn("volume-uuid-123"); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeDao.update(anyLong(), any(VolumeVO.class))).thenReturn(true); + when(epSelector.select(volumeObject)).thenReturn(endPoint); + when(endPoint.sendMessage(any(CreateObjectCommand.class))).thenReturn(createAnswer); + when(endPoint.sendMessage(any(DeleteCommand.class))).thenReturn(deleteAnswer); + + FeignException feignException = mock(FeignException.class); + when(feignException.contentUTF8()).thenReturn( + "{\"error\":{\"code\":\"8454269\",\"message\":\"Invalid QoS policy group specified\"}}"); + when(feignException.getMessage()).thenReturn("Bad Request"); + doThrow(feignException).when(nasFeignClient).updateFile(anyString(), eq("flex-uuid"), + eq("volume-uuid-123"), any(FileInfo.class)); + + CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, + () -> strategy.createCloudStackVolume(cloudStackVolume)); + assertTrue(ex.getMessage().contains("8454269")); + verify(endPoint).sendMessage(any(DeleteCommand.class)); + } + // 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 700b63d15575..b429b54b9149 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 @@ -30,6 +30,7 @@ import org.apache.cloudstack.storage.feign.model.Lun; import org.apache.cloudstack.storage.feign.model.LunMap; import org.apache.cloudstack.storage.feign.model.OntapStorage; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.feign.model.response.OntapResponse; import org.apache.cloudstack.storage.service.model.AccessGroup; import org.apache.cloudstack.storage.service.model.CloudStackVolume; @@ -354,6 +355,31 @@ void testCreateCloudStackVolume_FeignException_ThrowsCloudRuntimeException() { } } + @Test + void testCreateCloudStackVolume_MinThroughputRejected_PropagatesOntapError() { + Lun lun = new Lun(); + lun.setName("/vol/vol1/lun1"); + CloudStackVolume request = new CloudStackVolume(); + request.setLun(lun); + + FeignException feignException = mock(FeignException.class); + when(feignException.status()).thenReturn(400); + when(feignException.contentUTF8()).thenReturn( + "{\"error\":{\"code\":\"8454269\",\"message\":\"Invalid QoS policy group specified\"}}"); + when(feignException.getMessage()).thenReturn("Bad Request"); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.generateAuthHeader("admin", "password")) + .thenReturn(authHeader); + when(sanFeignClient.createLun(eq(authHeader), eq(true), any(Lun.class))) + .thenThrow(feignException); + + CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, + () -> unifiedSANStrategy.createCloudStackVolume(request)); + assertTrue(ex.getMessage().contains("8454269")); + } + } + @Test void testDeleteCloudStackVolume_Success() { // Setup @@ -1122,10 +1148,10 @@ void testSetOntapStorage() { } @Test - void testUpdateCloudStackVolume_ReturnsNull() { + void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() { CloudStackVolume request = new CloudStackVolume(); - CloudStackVolume result = unifiedSANStrategy.updateCloudStackVolume(request); - assertNull(result); + assertThrows(CloudRuntimeException.class, + () -> unifiedSANStrategy.updateCloudStackVolume(request)); } @Test @@ -2182,4 +2208,35 @@ void testEnsureLunMapped_ExistingMapping_ReturnsExistingNumber() { verify(sanFeignClient, never()).createLunMap(any(), anyBoolean(), any(LunMap.class)); } } + + @Test + void testCreateCloudStackVolume_PassesQosPolicyOnLunCreate() { + Lun lun = new Lun(); + lun.setName("/vol/vol1/lun1"); + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + lun.setQosPolicy(qosPolicy); + CloudStackVolume request = new CloudStackVolume(); + request.setLun(lun); + + Lun createdLun = new Lun(); + createdLun.setName("/vol/vol1/lun1"); + createdLun.setUuid("lun-uuid-123"); + OntapResponse response = new OntapResponse<>(); + response.setRecords(List.of(createdLun)); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.generateAuthHeader("admin", "password")) + .thenReturn(authHeader); + when(sanFeignClient.createLun(eq(authHeader), eq(true), any(Lun.class))) + .thenReturn(response); + + unifiedSANStrategy.createCloudStackVolume(request); + + ArgumentCaptor lunCaptor = ArgumentCaptor.forClass(Lun.class); + verify(sanFeignClient).createLun(eq(authHeader), eq(true), lunCaptor.capture()); + assertNotNull(lunCaptor.getValue().getQosPolicy()); + assertEquals("cs_100_to200_iops_svm1", lunCaptor.getValue().getQosPolicy().getName()); + } + } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java index ebe7da25ed12..594f5b33fb5d 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java @@ -18,12 +18,24 @@ */ package org.apache.cloudstack.storage.utils; +import java.util.HashMap; +import java.util.Map; + +import com.cloud.hypervisor.Hypervisor; import com.cloud.utils.exception.CloudRuntimeException; +import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; +import org.apache.cloudstack.storage.service.model.CloudStackVolume; +import org.apache.cloudstack.storage.service.model.ProtocolType; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class OntapStorageUtilsTest { @@ -82,6 +94,55 @@ public void getIgroupName_truncates_whenOneCharOverMaxLength() { assertEquals(OntapStorageConstants.IGROUP_NAME_MAX_LENGTH, result.length()); } + @Test + public void createCloudStackVolumeRequestByProtocol_attachesQosToIscsiLun() { + StoragePoolVO storagePool = mock(StoragePoolVO.class); + when(storagePool.getName()).thenReturn("pool1"); + when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); + + VolumeInfo volumeInfo = mock(VolumeInfo.class); + when(volumeInfo.getName()).thenReturn("data_disk"); + when(volumeInfo.getSize()).thenReturn(1073741824L); + + Map details = new HashMap<>(); + details.put(OntapStorageConstants.PROTOCOL, ProtocolType.ISCSI.name()); + details.put(OntapStorageConstants.SVM_NAME, "svm1"); + + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + qosPolicy.setUuid("qos-uuid"); + + CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + storagePool, details, volumeInfo, qosPolicy); + + assertNotNull(request.getLun().getQosPolicy()); + assertEquals("qos-uuid", request.getLun().getQosPolicy().getUuid()); + assertEquals("cs_100_to200_iops_svm1", request.getLun().getQosPolicy().getName()); + } + + @Test + public void createCloudStackVolumeRequestByProtocol_attachesQosToNfsFile() { + StoragePoolVO storagePool = mock(StoragePoolVO.class); + when(storagePool.getId()).thenReturn(1L); + + VolumeInfo volumeInfo = mock(VolumeInfo.class); + + Map details = new HashMap<>(); + details.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + details.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + qosPolicy.setUuid("qos-uuid"); + + CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + storagePool, details, volumeInfo, qosPolicy); + + assertEquals("flex-uuid", request.getFlexVolumeUuid()); + assertNotNull(request.getFile().getQosPolicy()); + assertEquals("qos-uuid", request.getFile().getQosPolicy().getUuid()); + } + @Test public void isOntapSnapshotNotFoundError_matchesEntryDoesNotExist() { CloudRuntimeException ex = new CloudRuntimeException("Job failed with error: entry doesn't exist"); From 87f71bdd4bdfe186bfeff1857fa4805953809d6f Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Wed, 9 Sep 2026 22:28:07 +0530 Subject: [PATCH 03/13] [CSTACKEX-279] Added QoS Client --- .../storage/feign/client/QosFeignClient.java | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java new file mode 100644 index 000000000000..2a8113f10b81 --- /dev/null +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cloudstack.storage.feign.client; + +import java.util.Map; + +import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; +import org.apache.cloudstack.storage.feign.model.response.JobResponse; +import org.apache.cloudstack.storage.feign.model.response.OntapResponse; + +import feign.Headers; +import feign.Param; +import feign.QueryMap; +import feign.RequestLine; + +public interface QosFeignClient { + + @RequestLine("POST /api/storage/qos/policies?return_timeout=0") + @Headers({"Authorization: {authHeader}"}) + JobResponse createPolicy(@Param("authHeader") String authHeader, VolumeQosPolicy policy); + + @RequestLine("GET /api/storage/qos/policies") + @Headers({"Authorization: {authHeader}"}) + OntapResponse getPolicies(@Param("authHeader") String authHeader, + @QueryMap Map queryParams); + + @RequestLine("PATCH /api/storage/qos/policies/{uuid}?return_timeout=0") + @Headers({"Authorization: {authHeader}"}) + JobResponse updatePolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid, + VolumeQosPolicy policy); + + @RequestLine("DELETE /api/storage/qos/policies/{uuid}?return_timeout=0") + @Headers({"Authorization: {authHeader}"}) + JobResponse deletePolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid); +} From 7c5e657a3526bbfb04b5dcfbcd15c7a508202ecf Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Wed, 9 Sep 2026 23:38:15 +0530 Subject: [PATCH 04/13] [CSTACKEX-279] Fix CustomIOPS bug --- .../cloudstack/engine/orchestration/CloudOrchestrator.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java index 9f6d02cc1234..d1d4e261a1c0 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/CloudOrchestrator.java @@ -66,9 +66,6 @@ import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.dao.VMInstanceDao; -import static org.apache.cloudstack.api.ApiConstants.MAX_IOPS; -import static org.apache.cloudstack.api.ApiConstants.MIN_IOPS; - @Component public class CloudOrchestrator implements OrchestrationService { @@ -205,8 +202,8 @@ public VirtualMachineEntity createVirtualMachine(String id, String owner, String Map userVmDetails = _vmInstanceDetailsDao.listDetailsKeyPairs(vm.getId()); if (userVmDetails != null) { - String minIops = userVmDetails.get(MIN_IOPS); - String maxIops = userVmDetails.get(MAX_IOPS); + String minIops = userVmDetails.get("minIops"); + String maxIops = userVmDetails.get("maxIops"); rootDiskOfferingInfo.setMinIops(minIops != null && minIops.trim().length() > 0 ? Long.parseLong(minIops) : null); rootDiskOfferingInfo.setMaxIops(maxIops != null && maxIops.trim().length() > 0 ? Long.parseLong(maxIops) : null); From f4d4c0e0dc438d722f9739d1067170aa28027ed8 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Thu, 10 Sep 2026 00:40:07 +0530 Subject: [PATCH 05/13] [CSTACKEX-279] Fix Volume resize bug --- ui/src/views/storage/ResizeVolume.vue | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ui/src/views/storage/ResizeVolume.vue b/ui/src/views/storage/ResizeVolume.vue index 5f9efd6ed506..b6d4f1899391 100644 --- a/ui/src/views/storage/ResizeVolume.vue +++ b/ui/src/views/storage/ResizeVolume.vue @@ -105,14 +105,21 @@ export default { }, fetchData () { this.loading = true + if (this.resource.size != null) { + this.form.size = this.resource.size / (1024 * 1024 * 1024) + } getAPI('listDiskOfferings', { zoneid: this.resource.zoneid, listall: true }).then(json => { this.offerings = json.listdiskofferingsresponse.diskoffering || [] - this.form.diskofferingid = this.offerings[0].id || '' - this.customDiskOffering = this.offerings[0].iscustomized || false - this.customDiskOfferingIops = this.offerings[0].iscustomizediops || false + const currentOffering = this.offerings.find(offering => offering.id === this.resource.diskofferingid) + this.customDiskOffering = currentOffering?.iscustomized || false + this.customDiskOfferingIops = currentOffering?.iscustomizediops || false + if (this.customDiskOfferingIops) { + this.form.miniops = this.resource.miniops + this.form.maxiops = this.resource.maxiops + } }).finally(() => { this.loading = false }) From d44c670b66eb2f0b4f7738794b88d51c5bb1e852 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Thu, 10 Sep 2026 01:18:30 +0530 Subject: [PATCH 06/13] [CSTACKEX-279] Fix Volume resize bug --- ui/src/views/storage/ResizeVolume.vue | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ui/src/views/storage/ResizeVolume.vue b/ui/src/views/storage/ResizeVolume.vue index b6d4f1899391..90b1a42a2a53 100644 --- a/ui/src/views/storage/ResizeVolume.vue +++ b/ui/src/views/storage/ResizeVolume.vue @@ -96,7 +96,9 @@ export default { methods: { initForm () { this.formRef = ref() - this.form = reactive({}) + this.form = reactive({ + size: this.resource.size != null ? this.resource.size / (1024 * 1024 * 1024) : undefined + }) this.rules = reactive({ size: [{ required: true, message: this.$t('message.error.size') }], miniops: [{ required: true, message: this.$t('message.error.number') }], From 52a19fecd5d1113005284b1cf0bbec940cde3d37 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Thu, 10 Sep 2026 01:37:25 +0530 Subject: [PATCH 07/13] [CSTACKEX-279] Fix Volume resize bug --- ui/src/views/storage/ResizeVolume.vue | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ui/src/views/storage/ResizeVolume.vue b/ui/src/views/storage/ResizeVolume.vue index 90b1a42a2a53..b6d4f1899391 100644 --- a/ui/src/views/storage/ResizeVolume.vue +++ b/ui/src/views/storage/ResizeVolume.vue @@ -96,9 +96,7 @@ export default { methods: { initForm () { this.formRef = ref() - this.form = reactive({ - size: this.resource.size != null ? this.resource.size / (1024 * 1024 * 1024) : undefined - }) + this.form = reactive({}) this.rules = reactive({ size: [{ required: true, message: this.$t('message.error.size') }], miniops: [{ required: true, message: this.$t('message.error.number') }], From 7f2b957743d3aaeb49b807c3983e1788dcfd5b47 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Fri, 11 Sep 2026 00:19:22 +0530 Subject: [PATCH 08/13] [CSTACKEX-279] Volume Resize QoS --- .../admin/storage/UpdateStoragePoolCmd.java | 4 +- .../driver/OntapPrimaryDatastoreDriver.java | 95 +++++++- .../storage/service/StorageStrategy.java | 2 +- .../storage/service/UnifiedNASStrategy.java | 2 +- .../storage/service/UnifiedSANStrategy.java | 2 +- .../storage/utils/OntapStorageConstants.java | 2 +- .../OntapPrimaryDatastoreDriverTest.java | 213 +++++++++++++++++- .../storage/service/StorageStrategyTest.java | 2 +- .../service/UnifiedNASStrategyTest.java | 28 +++ .../service/UnifiedSANStrategyTest.java | 27 +++ 10 files changed, 353 insertions(+), 24 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java index 4b0a6ba00b28..90498b9a8cb8 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java @@ -22,6 +22,7 @@ import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiArgValidator; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.BaseCmd; @@ -53,7 +54,8 @@ public class UpdateStoragePoolCmd extends BaseCmd { @Parameter(name = ApiConstants.TAGS, type = CommandType.LIST, collectionType = CommandType.STRING, description = "Comma-separated list of tags for the storage pool") private List tags; - @Parameter(name = ApiConstants.CAPACITY_IOPS, type = CommandType.LONG, required = false, description = "IOPS CloudStack can provision from this storage pool") + @Parameter(name = ApiConstants.CAPACITY_IOPS, type = CommandType.LONG, required = false, description = "IOPS CloudStack can provision from this storage pool", + validations = {ApiArgValidator.PositiveNumber}) private Long capacityIops; @Parameter(name = ApiConstants.CAPACITY_BYTES, type = CommandType.LONG, required = false, description = "Bytes CloudStack can provision from this storage pool") 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 df893f544119..965164d86a19 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 @@ -27,6 +27,7 @@ import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor.HypervisorType; +import com.cloud.storage.ResizeVolumePayload; import com.cloud.storage.Storage; import com.cloud.storage.StoragePool; import com.cloud.storage.Volume; @@ -250,8 +251,11 @@ private VolumeQosPolicy createQosPolicyIfNeeded(StorageStrategy storageStrategy, if (volumeType != Volume.Type.DATADISK && volumeType != Volume.Type.ROOT) { return null; } - Long minIops = volumeObject.getMinIops(); - Long maxIops = volumeObject.getMaxIops(); + return createQosPolicyIfNeeded(storageStrategy, details, volumeObject.getMinIops(), volumeObject.getMaxIops()); + } + + private VolumeQosPolicy createQosPolicyIfNeeded(StorageStrategy storageStrategy, Map details, + Long minIops, Long maxIops) { if (!validateIops(minIops, maxIops)) { return null; } @@ -722,18 +726,95 @@ public boolean canCopy(DataObject srcData, DataObject destData) { } /** - * Volume size and IOPS updates are not applied here. Pool IOPS capacity is enforced at - * volume create; resize and QoS belong to a later offering story. + * Applies min/max IOPS as an ONTAP QoS policy. */ @Override public void resize(DataObject data, AsyncCompletionCallback callback) { - String path = data instanceof VolumeInfo ? ((VolumeInfo) data).getPath() : null; - String errMsg = "Volume resize is not supported for ONTAP primary storage"; - CreateCmdResult result = new CreateCmdResult(path, new Answer(null, false, errMsg)); + String errMsg = null; + String path = null; + try { + if (!(data instanceof VolumeInfo)) { + throw new CloudRuntimeException("Invalid DataObjectType (" + + (data != null ? data.getType() : null) + ") passed to resize"); + } + VolumeInfo volumeInfo = (VolumeInfo) data; + path = volumeInfo.getPath(); + ResizeVolumePayload resizeVolumePayload = (ResizeVolumePayload) volumeInfo.getpayload(); + if (resizeVolumePayload == null) { + throw new CloudRuntimeException("Missing resize payload for volume " + volumeInfo.getId()); + } + + VolumeVO volume = volumeDao.findById(volumeInfo.getId()); + if (volume == null || volume.getPoolId() == null) { + throw new CloudRuntimeException("Unable to resolve volume or storage pool for IOPS update"); + } + + StoragePoolVO storagePool = storagePoolDao.findById(volume.getPoolId()); + if (storagePool == null) { + throw new CloudRuntimeException("Storage pool not found for volume " + volume.getId()); + } + + verifySufficientIopsForStoragePool(storagePool, resizeVolumePayload.newMinIops, volume.getId()); + + Map details = storagePoolDetailsDao.listDetailsKeyPairs(storagePool.getId()); + StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); + VolumeDetailVO qosPolicyUuidDetail = volumeDetailsDao.findDetail( + volume.getId(), OntapStorageConstants.QOS_POLICY_UUID); + String previousPolicyUuid = qosPolicyUuidDetail != null ? qosPolicyUuidDetail.getValue() : null; + + VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, + resizeVolumePayload.newMinIops, resizeVolumePayload.newMaxIops); + if (qosPolicy != null) { + if (previousPolicyUuid == null || !previousPolicyUuid.equals(qosPolicy.getUuid())) { + try { + attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, qosPolicy); + persistQosPolicyDetails(volume.getId(), qosPolicy); + deleteQosPolicyIfUnused(storageStrategy, previousPolicyUuid, volume.getId()); + } catch (RuntimeException e) { + deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volume.getId()); + throw e; + } + } + } else if (previousPolicyUuid != null) { + detachQosPolicy(storageStrategy, storagePool, details, volumeInfo); + persistQosPolicyDetails(volume.getId(), null); + deleteQosPolicyIfUnused(storageStrategy, previousPolicyUuid, volume.getId()); + } + } catch (Exception e) { + errMsg = e.getMessage(); + logger.error("Failed to update IOPS for volume [{}]: {}", data != null ? data.getId() : null, errMsg, e); + } + + CreateCmdResult result = new CreateCmdResult(path, new Answer(null, errMsg == null, errMsg)); result.setResult(errMsg); callback.complete(result); } + private void attachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO storagePool, + Map details, VolumeInfo volumeInfo, + VolumeQosPolicy qosPolicy) { + CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + storagePool, details, volumeInfo, qosPolicy); + if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { + VolumeDetailVO lunUuid = volumeDetailsDao.findDetail(volumeInfo.getId(), OntapStorageConstants.LUN_DOT_UUID); + if (lunUuid == null || lunUuid.getValue() == null) { + throw new CloudRuntimeException("LUN UUID is missing for volume " + volumeInfo.getId()); + } + if (request.getLun() == null) { + throw new CloudRuntimeException("Missing LUN on QoS update request for volume " + volumeInfo.getId()); + } + request.getLun().setUuid(lunUuid.getValue()); + } + storageStrategy.updateCloudStackVolume(request); + } + + private void detachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO storagePool, + Map details, VolumeInfo volumeInfo) { + VolumeQosPolicy noPolicy = new VolumeQosPolicy(); + noPolicy.setName(""); + attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, noPolicy); + } + @Override public ChapInfo getChapInfo(DataObject dataObject) { return null; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index aa9e9c7bd3fc..ca1c2102d6c3 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -804,7 +804,7 @@ abstract public CloudStackVolume createTemplateCache(StoragePoolVO storagePool, * @param cloudstackVolume the CloudStack volume to update * @return the updated CloudStackVolume object */ - abstract CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume); + public abstract CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume); /** * Method encapsulates the behavior based on the opted protocol in subclasses. 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 81884dd3f5b7..11e6df844b81 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 @@ -135,7 +135,7 @@ public CloudStackVolume createTemplateCache(StoragePoolVO storagePool, TemplateI } @Override - CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { + 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"); 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 aec642634f53..b6b9f7434346 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 @@ -183,7 +183,7 @@ private void bestEffortDeleteTemplateCacheLun(String svmName, String lunName, St } @Override - CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { + public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { if (cloudstackVolume == null || cloudstackVolume.getLun() == null || cloudstackVolume.getLun().getUuid() == null) { throw new CloudRuntimeException("Invalid iSCSI volume QoS update request"); 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 0330d77051a8..a99c3201e596 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 @@ -105,7 +105,7 @@ public class OntapStorageConstants { public static final String QOS_POLICY_NAME = "qosPolicyName"; public static final String QOS_POLICY_UUID = "qosPolicyUuid"; public static final String QOS_POLICY_NAME_PREFIX = "cs_"; - public static final String QOS_POLICY_NAME_TO = "to"; + public static final String QOS_POLICY_NAME_TO = "to_"; public static final String QOS_POLICY_NAME_IOPS = "iops_"; public static final String LOGICAL_UNIT_NUMBER = "logical_unit_number"; public static final String IGROUP_DOT_NAME = "igroup.name"; 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 cc22e3b5165e..383051db05fd 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 @@ -22,6 +22,7 @@ import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.hypervisor.Hypervisor; +import com.cloud.storage.ResizeVolumePayload; import com.cloud.storage.ScopeType; import com.cloud.storage.Storage; import com.cloud.storage.VMTemplateStoragePoolVO; @@ -43,6 +44,7 @@ import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.feign.model.FileInfo; import org.apache.cloudstack.storage.feign.model.Igroup; import org.apache.cloudstack.storage.feign.model.Lun; import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; @@ -1254,13 +1256,193 @@ void testGetUsedIops_SumsMinIopsIncludingCreatingVolumes() { } @Test - void testResize_IsNotSupported() { + void testResize_SizeChange_IsRejected() { + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(8L * 1024 * 1024 * 1024, 0L, 3333L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(4L * 1024 * 1024 * 1024); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Volume size change is not supported")); + } + + @Test + void testResize_IopsUpdate_AttachesQosPolicy() { + long currentSize = 4L * 1024 * 1024 * 1024; + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 0L, 5000L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(currentSize); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(null); + VolumeDetailVO lunUuidDetail = new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_UUID, "lun-uuid-123", false); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_5000_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); + when(sanStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).updateCloudStackVolume(argThat(request -> + request.getLun() != null && "lun-uuid-123".equals(request.getLun().getUuid()))); + verify(volumeDetailsDao).addDetail(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); + verify(volumeDetailsDao).addDetail(100L, OntapStorageConstants.QOS_POLICY_NAME, "cs_0_to_5000_iops_svm1", false); + } + } + + @Test + void testResize_IopsUpdate_NfsAttachesQosPolicy() { + long currentSize = 4L * 1024 * 1024 * 1024; + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 100L, 200L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(currentSize); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(null); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); + CloudStackVolume cloudStackVolume = nfsCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + stubQosCreateMocks(utilityMock, nasStrategy, cloudStackVolume, qosPolicy); + when(nasStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(nasStrategy).updateCloudStackVolume(any()); + verify(volumeDetailsDao).addDetail(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); + } + } + + @Test + void testResize_SameQosPolicy_SkipsAttach() { + long currentSize = 4L * 1024 * 1024 * 1024; + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 0L, 3333L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(currentSize); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + VolumeDetailVO qosDetail = new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_3333_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy, never()).updateCloudStackVolume(any()); + } + } + + @Test + void testResize_ClearIops_DetachesQosPolicy() { + long currentSize = 4L * 1024 * 1024 * 1024; + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 0L, 0L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(currentSize); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getCapacityIops()).thenReturn(null); + when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); + VolumeDetailVO qosDetail = new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); + when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) + .thenReturn(List.of(qosDetail)); + VolumeDetailVO lunUuidDetail = new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_UUID, "lun-uuid-123", false); + when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); + + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + when(sanStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); + + driver.resize(volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertTrue(resultCaptor.getValue().isSuccess()); + verify(sanStrategy).updateCloudStackVolume(any()); + verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); + } + } + + @Test + void testResize_MinIopsBeyondPoolCapacity_Fails() { + long currentSize = 4L * 1024 * 1024 * 1024; + VolumeVO otherVolume = mock(VolumeVO.class); + when(otherVolume.getId()).thenReturn(50L); + when(otherVolume.getMinIops()).thenReturn(800L); + + when(volumeInfo.getId()).thenReturn(100L); + when(volumeInfo.getpayload()).thenReturn( + new ResizeVolumePayload(currentSize, 300L, 1000L, null, false, null, null, true)); + when(volumeDao.findById(100L)).thenReturn(volumeVO); + when(volumeVO.getId()).thenReturn(100L); + when(volumeVO.getPoolId()).thenReturn(1L); + when(volumeVO.getSize()).thenReturn(currentSize); + when(storagePoolDao.findById(1L)).thenReturn(storagePool); + when(storagePool.getId()).thenReturn(1L); + when(storagePool.getName()).thenReturn("ontap-pool"); + when(storagePool.getCapacityIops()).thenReturn(1000L); + when(volumeDao.findNonDestroyedVolumesByPoolId(1L, null)).thenReturn(List.of(otherVolume, volumeVO)); + driver.resize(volumeInfo, createCallback); ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); verify(createCallback).complete(resultCaptor.capture()); assertFalse(resultCaptor.getValue().isSuccess()); - assertTrue(resultCaptor.getValue().getResult().contains("Volume resize is not supported")); + assertTrue(resultCaptor.getValue().getResult().contains( + "requested total of 1100 IOPS exceeds the pool IOPS capacity of 1000")); + verify(sanStrategy, never()).updateCloudStackVolume(any()); } @Test @@ -1387,7 +1569,7 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { when(volumeInfo.getMinIops()).thenReturn(100L); when(volumeInfo.getMaxIops()).thenReturn(200L); - VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to200_iops_svm1"); + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { @@ -1398,11 +1580,11 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); - verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to200_iops_svm1"), eq(100L), eq(200L)); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( any(), any(), any(), argThat(policy -> policy != null && "qos-uuid".equals(policy.getUuid())))); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_NAME), - eq("cs_100_to200_iops_svm1"), eq(false)); + eq("cs_100_to_200_iops_svm1"), eq(false)); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), eq(false)); } @@ -1415,7 +1597,7 @@ void testCreateAsync_RootDiskCustomIops_CreatesAndPersistsQosPolicy() { when(volumeInfo.getMinIops()).thenReturn(111L); when(volumeInfo.getMaxIops()).thenReturn(999L); - VolumeQosPolicy qosPolicy = qosPolicy("qos-root-uuid", "cs_111_to999_iops_svm1"); + VolumeQosPolicy qosPolicy = qosPolicy("qos-root-uuid", "cs_111_to_999_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { @@ -1426,7 +1608,7 @@ void testCreateAsync_RootDiskCustomIops_CreatesAndPersistsQosPolicy() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); - verify(sanStrategy).createVolumeQosPolicy(eq("cs_111_to999_iops_svm1"), eq(111L), eq(999L)); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_111_to_999_iops_svm1"), eq(111L), eq(999L)); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-root-uuid"), eq(false)); } @@ -1452,7 +1634,7 @@ void testCreateAsync_NfsDataDiskWithIops_CreatesQosPolicy() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); - VolumeQosPolicy qosPolicy = qosPolicy("qos-nfs-uuid", "cs_100_to200_iops_svm1"); + VolumeQosPolicy qosPolicy = qosPolicy("qos-nfs-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = new CloudStackVolume(); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { @@ -1463,7 +1645,7 @@ void testCreateAsync_NfsDataDiskWithIops_CreatesQosPolicy() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); - verify(nasStrategy).createVolumeQosPolicy(eq("cs_100_to200_iops_svm1"), eq(100L), eq(200L)); + verify(nasStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( any(), any(), any(), argThat(policy -> policy != null && "qos-nfs-uuid".equals(policy.getUuid())))); } @@ -1540,7 +1722,7 @@ void testCreateAsync_QosCreateThenLunCreateFails_DeletesUnusedPolicy() { when(volumeInfo.getMinIops()).thenReturn(100L); when(volumeInfo.getMaxIops()).thenReturn(200L); - VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to200_iops_svm1"); + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { @@ -1623,6 +1805,15 @@ private CloudStackVolume iscsiCloudStackVolume() { return volume; } + private CloudStackVolume nfsCloudStackVolume() { + FileInfo fileInfo = new FileInfo(); + CloudStackVolume volume = new CloudStackVolume(); + volume.setFlexVolumeUuid("flex-uuid"); + volume.setFile(fileInfo); + volume.setVolumeInfo(volumeInfo); + return volume; + } + private VolumeQosPolicy qosPolicy(String uuid, String name) { VolumeQosPolicy policy = new VolumeQosPolicy(); policy.setUuid(uuid); @@ -1639,7 +1830,7 @@ private void stubQosCreateMocks(MockedStatic utilityMock, any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(strategy.createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class))) .thenReturn(qosPolicy); - when(strategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); + lenient().when(strategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); } @Test diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index 2c516544cd49..ac2eac29e79e 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -154,7 +154,7 @@ public CloudStackVolume createTemplateCache(org.apache.cloudstack.storage.datast } @Override - CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { + public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume) { return null; } 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 4cdb8d33ec24..1e0172a1e06e 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 @@ -302,6 +302,34 @@ public void testCreateCloudStackVolume_QosAttachFails_DeletesLeftoverNfsFile() { verify(endPoint).sendMessage(any(DeleteCommand.class)); } + @Test + public void testUpdateCloudStackVolume_AppliesQosPolicy() { + VolumeInfo volumeInfo = mock(VolumeInfo.class); + when(volumeInfo.getUuid()).thenReturn("volume-uuid-123"); + + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_100_to200_iops_svm1"); + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(qosPolicy); + + CloudStackVolume request = new CloudStackVolume(); + request.setVolumeInfo(volumeInfo); + request.setFlexVolumeUuid("flex-uuid"); + request.setFile(fileInfo); + + CloudStackVolume result = strategy.updateCloudStackVolume(request); + + assertSame(request, result); + verify(nasFeignClient).updateFile(anyString(), eq("flex-uuid"), eq("volume-uuid-123"), + argThat(file -> file.getQosPolicy() != null + && "cs_100_to200_iops_svm1".equals(file.getQosPolicy().getName()))); + } + + @Test + public void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() { + assertThrows(CloudRuntimeException.class, () -> strategy.updateCloudStackVolume(new CloudStackVolume())); + } + // 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 b429b54b9149..c12eab03ddb8 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 @@ -55,11 +55,13 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; @@ -1154,6 +1156,31 @@ void testUpdateCloudStackVolume_InvalidRequest_ThrowsException() { () -> unifiedSANStrategy.updateCloudStackVolume(request)); } + @Test + void testUpdateCloudStackVolume_AppliesQosPolicyToLun() { + Lun lun = new Lun(); + lun.setUuid("lun-uuid-123"); + VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); + qosPolicy.setName("cs_0_to5000_iops_svm1"); + lun.setQosPolicy(qosPolicy); + CloudStackVolume request = new CloudStackVolume(); + request.setLun(lun); + + 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); + verify(sanFeignClient).updateLun(eq(authHeader), eq("lun-uuid-123"), argThat(update -> + update.getQosPolicy() != null + && "cs_0_to5000_iops_svm1".equals(update.getQosPolicy().getName()))); + } + } + @Test void testUpdateAccessGroup_ReturnsNull() { AccessGroup accessGroup = new AccessGroup(); From 1e709f47a6ba4d6ae5cdeeb9d4cdf42e5c358f76 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Tue, 15 Sep 2026 22:45:04 +0530 Subject: [PATCH 09/13] [CSTACKEX-279] Refactoring --- .../driver/OntapPrimaryDatastoreDriver.java | 66 +++++---- .../OntapPrimaryDatastoreLifecycle.java | 1 + .../storage/service/StorageStrategy.java | 20 +++ .../storage/utils/OntapStorageConstants.java | 1 + .../OntapPrimaryDatastoreDriverTest.java | 134 ++++++++++++++---- .../OntapPrimaryDatastoreLifecycleTest.java | 28 ++++ .../storage/service/StorageStrategyTest.java | 30 ++++ 7 files changed, 226 insertions(+), 54 deletions(-) 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 965164d86a19..1930772cf8d3 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 @@ -230,7 +230,12 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, VolumeInfo volumeObject, Map details) { verifySufficientIopsForStoragePool(storagePool, volumeObject.getMinIops(), volumeObject.getId()); StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); - VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, volumeObject); + VolumeQosPolicy qosPolicy = null; + Volume.Type volumeType = volumeObject.getVolumeType(); + if (volumeType == Volume.Type.DATADISK || volumeType == Volume.Type.ROOT) { + qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, + volumeObject.getMinIops(), volumeObject.getMaxIops(), storagePool.getId()); + } CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( storagePool, details, volumeObject, qosPolicy); try { @@ -246,21 +251,41 @@ private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, Volum } private VolumeQosPolicy createQosPolicyIfNeeded(StorageStrategy storageStrategy, Map details, - VolumeInfo volumeObject) { - Volume.Type volumeType = volumeObject.getVolumeType(); - if (volumeType != Volume.Type.DATADISK && volumeType != Volume.Type.ROOT) { + Long minIops, Long maxIops, Long poolId) { + if (!validateIops(storageStrategy, details, poolId, minIops, maxIops)) { return null; } - return createQosPolicyIfNeeded(storageStrategy, details, volumeObject.getMinIops(), volumeObject.getMaxIops()); + String policyName = getQosPolicyName(details.get(OntapStorageConstants.SVM_NAME), minIops, maxIops); + return storageStrategy.createVolumeQosPolicy(policyName, minIops, maxIops); } - private VolumeQosPolicy createQosPolicyIfNeeded(StorageStrategy storageStrategy, Map details, - Long minIops, Long maxIops) { - if (!validateIops(minIops, maxIops)) { - return null; + /** + * Empty/zero IOPS means no policy. Min greater than max is rejected. + * Min IOPS is AFF-only; older pools without {@code isAFF} are probed once and persisted. + */ + private boolean validateIops(StorageStrategy storageStrategy, Map details, + Long poolId, Long minIops, Long maxIops) { + long min = minIops == null ? 0 : minIops; + long max = maxIops == null ? 0 : maxIops; + if (min <= 0 && max <= 0) { + return false; } - String policyName = getQosPolicyName(details.get(OntapStorageConstants.SVM_NAME), minIops, maxIops); - return storageStrategy.createVolumeQosPolicy(policyName, minIops, maxIops); + if (min > 0 && max > 0 && min > max) { + throw new InvalidParameterValueException("Minimum IOPS cannot be greater than maximum IOPS"); + } + if (min > 0) { + String isAff = details.get(OntapStorageConstants.IS_AFF); + if (StringUtils.isBlank(isAff)) { + isAff = Boolean.toString(storageStrategy.isAff()); + details.put(OntapStorageConstants.IS_AFF, isAff); + storagePoolDetailsDao.addDetail(poolId, OntapStorageConstants.IS_AFF, isAff, false); + } + if (!Boolean.parseBoolean(isAff)) { + throw new InvalidParameterValueException( + "Minimum IOPS is not supported on FAS/non-AFF ONTAP platforms; only maximum IOPS is supported"); + } + } + return true; } /** @@ -276,23 +301,6 @@ private String getQosPolicyName(String svmName, Long minIops, Long maxIops) { + OntapStorageConstants.QOS_POLICY_NAME_IOPS + sanitizedSvmName; } - /** - * Returns true when the volume has a positive min or max IOPS to apply as an ONTAP QoS policy. - * Throws when both limits are set and min IOPS is greater than max IOPS. - */ - private boolean validateIops(Long minIops, Long maxIops) { - boolean hasMinIops = minIops != null && minIops > 0; - boolean hasMaxIops = maxIops != null && maxIops > 0; - if (!hasMinIops && !hasMaxIops) { - return false; - } - if (hasMinIops && hasMaxIops && minIops > maxIops) { - throw new InvalidParameterValueException( - "Minimum IOPS cannot be greater than maximum IOPS"); - } - return true; - } - private void persistQosPolicyDetails(long volumeId, VolumeQosPolicy qosPolicy) { volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_NAME); volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID); @@ -763,7 +771,7 @@ public void resize(DataObject data, AsyncCompletionCallback cal String previousPolicyUuid = qosPolicyUuidDetail != null ? qosPolicyUuidDetail.getValue() : null; VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, - resizeVolumePayload.newMinIops, resizeVolumePayload.newMaxIops); + resizeVolumePayload.newMinIops, resizeVolumePayload.newMaxIops, volume.getPoolId()); if (qosPolicy != null) { if (previousPolicyUuid == null || !previousPolicyUuid.equals(qosPolicy.getUuid())) { try { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java index 626e8e11f574..55fb7c868c49 100755 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java @@ -143,6 +143,7 @@ public DataStore initialize(Map dsInfos) { StorageStrategy storageStrategy = StorageProviderFactory.getStrategy(ontapStorage); boolean isValid = storageStrategy.connect(); if (isValid) { + details.put(OntapStorageConstants.IS_AFF, Boolean.toString(storageStrategy.isAff())); if (storageStrategy.getResolvedSvmUuid() != null && !storageStrategy.getResolvedSvmUuid().isEmpty()) { details.put(OntapStorageConstants.SVM_UUID, storageStrategy.getResolvedSvmUuid()); } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index ca1c2102d6c3..4e4fafb0b2b2 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -233,6 +233,26 @@ private static String rollupPlatformType(LinkedHashSet platformTypes) { return OntapStorageConstants.ASUP_PLATFORM_TYPE_COMPOSITE; } + /** + * True when every cluster node reports {@code is_all_flash_optimized} (AFF, including C-series). + * Any FAS node makes this false. Used for min-throughput QoS support. + */ + public boolean isAff() { + Map query = new HashMap<>(); + query.put(OntapStorageConstants.FIELDS, OntapStorageConstants.CLUSTER_NODE_ASUP_FIELDS); + OntapResponse response = clusterFeignClient.getClusterNodes(getAuthHeader(), query); + if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + throw new CloudRuntimeException( + "Unable to determine whether the ONTAP cluster is AFF or FAS"); + } + for (ClusterNode node : response.getRecords()) { + if (node == null || !Boolean.TRUE.equals(node.getAllFlashOptimized())) { + return false; + } + } + return true; + } + /** * Pushes a single ASUP (AutoSupport) EMS application-log message to the ONTAP cluster. * 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 a99c3201e596..17c37a58a200 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 @@ -104,6 +104,7 @@ public class OntapStorageConstants { public static final String LUN_DOT_UUID = "lun.uuid"; public static final String QOS_POLICY_NAME = "qosPolicyName"; public static final String QOS_POLICY_UUID = "qosPolicyUuid"; + public static final String IS_AFF = "isAFF"; public static final String QOS_POLICY_NAME_PREFIX = "cs_"; public static final String QOS_POLICY_NAME_TO = "to_"; public static final String QOS_POLICY_NAME_IOPS = "iops_"; 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 383051db05fd..f4c4bb78d3fe 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 @@ -155,6 +155,7 @@ void setUp() { storagePoolDetails = new HashMap<>(); storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.ISCSI.name()); storagePoolDetails.put(OntapStorageConstants.SVM_NAME, "svm1"); + storagePoolDetails.put(OntapStorageConstants.IS_AFF, "true"); } @Test @@ -198,8 +199,8 @@ void testCreateAsync_VolumeWithISCSI_Success() { when(volumeInfo.getName()).thenReturn("test-volume"); when(storagePoolDao.findById(1L)).thenReturn(storagePool); - when(storagePool.getId()).thenReturn(1L); - when(storagePool.getName()).thenReturn("vol1"); + lenient().when(storagePool.getId()).thenReturn(1L); + lenient().when(storagePool.getName()).thenReturn("vol1"); when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); @@ -1255,23 +1256,6 @@ void testGetUsedIops_SumsMinIopsIncludingCreatingVolumes() { assertEquals(1000L, driver.getUsedIops(storagePool)); } - @Test - void testResize_SizeChange_IsRejected() { - when(volumeInfo.getId()).thenReturn(100L); - when(volumeInfo.getpayload()).thenReturn( - new ResizeVolumePayload(8L * 1024 * 1024 * 1024, 0L, 3333L, null, false, null, null, true)); - when(volumeDao.findById(100L)).thenReturn(volumeVO); - when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(4L * 1024 * 1024 * 1024); - - driver.resize(volumeInfo, createCallback); - - ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); - verify(createCallback).complete(resultCaptor.capture()); - assertFalse(resultCaptor.getValue().isSuccess()); - assertTrue(resultCaptor.getValue().getResult().contains("Volume size change is not supported")); - } - @Test void testResize_IopsUpdate_AttachesQosPolicy() { long currentSize = 4L * 1024 * 1024 * 1024; @@ -1281,7 +1265,6 @@ void testResize_IopsUpdate_AttachesQosPolicy() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(currentSize); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getCapacityIops()).thenReturn(null); @@ -1320,7 +1303,6 @@ void testResize_IopsUpdate_NfsAttachesQosPolicy() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(currentSize); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getCapacityIops()).thenReturn(null); @@ -1353,7 +1335,6 @@ void testResize_SameQosPolicy_SkipsAttach() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(currentSize); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getCapacityIops()).thenReturn(null); @@ -1385,7 +1366,6 @@ void testResize_ClearIops_DetachesQosPolicy() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(currentSize); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getCapacityIops()).thenReturn(null); @@ -1428,7 +1408,6 @@ void testResize_MinIopsBeyondPoolCapacity_Fails() { when(volumeDao.findById(100L)).thenReturn(volumeVO); when(volumeVO.getId()).thenReturn(100L); when(volumeVO.getPoolId()).thenReturn(1L); - when(volumeVO.getSize()).thenReturn(currentSize); when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); when(storagePool.getName()).thenReturn("ontap-pool"); @@ -1530,7 +1509,7 @@ void testCreateAsync_DoesNotDoubleCountSelfWhenAlreadyCreating() { when(storagePoolDao.findById(1L)).thenReturn(storagePool); when(storagePool.getId()).thenReturn(1L); - when(storagePool.getName()).thenReturn("vol1"); + lenient().when(storagePool.getName()).thenReturn("vol1"); when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.OntapiSCSI); when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); when(storagePool.getCapacityIops()).thenReturn(1000L); @@ -1587,6 +1566,111 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { eq("cs_100_to_200_iops_svm1"), eq(false)); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), eq(false)); + verify(sanStrategy, never()).isAff(); + } + } + + @Test + void testCreateAsync_FasPoolWithMinIops_FailsWithoutCallingOntap() { + stubIscsiVolumeCreate(); + storagePoolDetails.put(OntapStorageConstants.IS_AFF, "false"); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Minimum IOPS is not supported on FAS")); + verify(sanStrategy, never()).isAff(); + verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); + } + } + + @Test + void testCreateAsync_FasPoolWithMaxIopsOnly_CreatesQosPolicyWithoutNodeCall() { + stubIscsiVolumeCreate(); + storagePoolDetails.put(OntapStorageConstants.IS_AFF, "false"); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(0L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_200_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + 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, never()).isAff(); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_0_to_200_iops_svm1"), eq(0L), eq(200L)); + } + } + + @Test + void testCreateAsync_MissingAffDetail_FetchesFromNodesAndPersists() { + stubIscsiVolumeCreate(); + storagePoolDetails.remove(OntapStorageConstants.IS_AFF); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + when(sanStrategy.isAff()).thenReturn(true); + + VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + 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).isAff(); + verify(storagePoolDetailsDao).addDetail(1L, OntapStorageConstants.IS_AFF, "true", false); + verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); + } + } + + @Test + void testCreateAsync_MissingAffDetailOnFas_PersistsFalseAndRejectsMinIops() { + stubIscsiVolumeCreate(); + storagePoolDetails.remove(OntapStorageConstants.IS_AFF); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + when(sanStrategy.isAff()).thenReturn(false); + CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); + + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) + .thenReturn(sanStrategy); + utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); + + driver.createAsync(dataStore, volumeInfo, createCallback); + + ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); + verify(createCallback).complete(resultCaptor.capture()); + assertFalse(resultCaptor.getValue().isSuccess()); + assertTrue(resultCaptor.getValue().getResult().contains("Minimum IOPS is not supported on FAS")); + verify(sanStrategy).isAff(); + verify(storagePoolDetailsDao).addDetail(1L, OntapStorageConstants.IS_AFF, "false", false); + verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java index 71c8a2ba1143..12ff93d71c11 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java @@ -136,6 +136,7 @@ void setUp() { when(_clusterDao.findById(1L)).thenReturn(clusterVO); when(storageStrategy.connect()).thenReturn(true); + when(storageStrategy.isAff()).thenReturn(true); when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>("testNetworkInterface", null)); Volume volume = new Volume(); @@ -271,6 +272,33 @@ public void testInitialize_capacityIopsIsStoredOnPool() { assertEquals(Long.valueOf(5000L), initializeAndCaptureCapacityIops(dsInfos)); } + @Test + public void testInitialize_persistsAffPlatformDetail() { + Map details = initializeAndCaptureDetails(buildDsInfosForProtocol("NFS3")); + + assertEquals("true", details.get(OntapStorageConstants.IS_AFF)); + verify(storageStrategy).isAff(); + } + + @Test + public void testInitialize_persistsFasPlatformDetail() { + when(storageStrategy.isAff()).thenReturn(false); + + Map details = initializeAndCaptureDetails(buildDsInfosForProtocol("NFS3")); + + assertEquals("false", details.get(OntapStorageConstants.IS_AFF)); + } + + private Map initializeAndCaptureDetails(Map dsInfos) { + try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { + storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); + ontapPrimaryDatastoreLifecycle.initialize(dsInfos); + } + ArgumentCaptor captor = ArgumentCaptor.forClass(PrimaryDataStoreParameters.class); + verify(_dataStoreHelper).createPrimaryDataStore(captor.capture()); + return captor.getValue().getDetails(); + } + @Test public void testInitialize_nonPositiveCapacityIopsIsRejected() { Map dsInfos = buildDsInfosForProtocol("NFS3"); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java index ac2eac29e79e..c894496d46af 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java @@ -50,6 +50,7 @@ import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.cloudstack.storage.utils.OntapStorageConstants; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -379,6 +380,35 @@ public void testGetClusterInfo_nodesGetFailureLeavesModelUnset() { assertNull(result.getPlatformType()); } + @Test + public void testIsAff_allNodesAllFlash_returnsTrue() { + when(clusterFeignClient.getClusterNodes(anyString(), anyMap())) + .thenReturn(new OntapResponse<>(List.of( + clusterNode("AFF-A400", true, true, false), + clusterNode("AFF-A400", true, true, false)))); + + assertTrue(storageStrategy.isAff()); + } + + @Test + public void testIsAff_anyNodeNotAllFlash_returnsFalse() { + when(clusterFeignClient.getClusterNodes(anyString(), anyMap())) + .thenReturn(new OntapResponse<>(List.of( + clusterNode("AFF-A400", true, true, false), + clusterNode("FAS8300", false, false, false)))); + + assertFalse(storageStrategy.isAff()); + } + + @Test + public void testIsAff_noNodes_throws() { + when(clusterFeignClient.getClusterNodes(anyString(), anyMap())) + .thenReturn(new OntapResponse<>(List.of())); + + CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, () -> storageStrategy.isAff()); + assertTrue(ex.getMessage().contains("Unable to determine whether the ONTAP cluster is AFF or FAS")); + } + private Cluster stubClusterGet() { Cluster cluster = new Cluster(); when(clusterFeignClient.getCluster(anyString(), eq(true))).thenReturn(cluster); From 2dc4653c1afd56df95dc6f0d1562d02b0a0843dd Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Tue, 15 Sep 2026 23:16:57 +0530 Subject: [PATCH 10/13] [CSTACKEX-279] Refactoring --- .../driver/OntapPrimaryDatastoreDriver.java | 91 ++++++++++--------- .../storage/feign/client/QosFeignClient.java | 5 - .../storage/service/StorageStrategy.java | 29 +----- .../storage/service/UnifiedNASStrategy.java | 2 +- .../storage/service/UnifiedSANStrategy.java | 6 +- .../OntapPrimaryDatastoreDriverTest.java | 3 - .../service/UnifiedNASStrategyTest.java | 2 +- .../service/UnifiedSANStrategyTest.java | 2 +- 8 files changed, 53 insertions(+), 87 deletions(-) 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 1930772cf8d3..e66996f1255c 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 @@ -91,6 +91,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; /** * Primary datastore driver for NetApp ONTAP storage systems. @@ -307,7 +308,6 @@ private void persistQosPolicyDetails(long volumeId, VolumeQosPolicy qosPolicy) { if (qosPolicy == null) { return; } - volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.QOS_POLICY_NAME, qosPolicy.getName(), false); volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID, qosPolicy.getUuid(), false); } @@ -733,9 +733,6 @@ public boolean canCopy(DataObject srcData, DataObject destData) { return false; } - /** - * Applies min/max IOPS as an ONTAP QoS policy. - */ @Override public void resize(DataObject data, AsyncCompletionCallback callback) { String errMsg = null; @@ -747,47 +744,7 @@ public void resize(DataObject data, AsyncCompletionCallback cal } VolumeInfo volumeInfo = (VolumeInfo) data; path = volumeInfo.getPath(); - ResizeVolumePayload resizeVolumePayload = (ResizeVolumePayload) volumeInfo.getpayload(); - if (resizeVolumePayload == null) { - throw new CloudRuntimeException("Missing resize payload for volume " + volumeInfo.getId()); - } - - VolumeVO volume = volumeDao.findById(volumeInfo.getId()); - if (volume == null || volume.getPoolId() == null) { - throw new CloudRuntimeException("Unable to resolve volume or storage pool for IOPS update"); - } - - StoragePoolVO storagePool = storagePoolDao.findById(volume.getPoolId()); - if (storagePool == null) { - throw new CloudRuntimeException("Storage pool not found for volume " + volume.getId()); - } - - verifySufficientIopsForStoragePool(storagePool, resizeVolumePayload.newMinIops, volume.getId()); - - Map details = storagePoolDetailsDao.listDetailsKeyPairs(storagePool.getId()); - StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); - VolumeDetailVO qosPolicyUuidDetail = volumeDetailsDao.findDetail( - volume.getId(), OntapStorageConstants.QOS_POLICY_UUID); - String previousPolicyUuid = qosPolicyUuidDetail != null ? qosPolicyUuidDetail.getValue() : null; - - VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, - resizeVolumePayload.newMinIops, resizeVolumePayload.newMaxIops, volume.getPoolId()); - if (qosPolicy != null) { - if (previousPolicyUuid == null || !previousPolicyUuid.equals(qosPolicy.getUuid())) { - try { - attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, qosPolicy); - persistQosPolicyDetails(volume.getId(), qosPolicy); - deleteQosPolicyIfUnused(storageStrategy, previousPolicyUuid, volume.getId()); - } catch (RuntimeException e) { - deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volume.getId()); - throw e; - } - } - } else if (previousPolicyUuid != null) { - detachQosPolicy(storageStrategy, storagePool, details, volumeInfo); - persistQosPolicyDetails(volume.getId(), null); - deleteQosPolicyIfUnused(storageStrategy, previousPolicyUuid, volume.getId()); - } + applyVolumeQos(volumeInfo); } catch (Exception e) { errMsg = e.getMessage(); logger.error("Failed to update IOPS for volume [{}]: {}", data != null ? data.getId() : null, errMsg, e); @@ -798,6 +755,50 @@ public void resize(DataObject data, AsyncCompletionCallback cal callback.complete(result); } + private void applyVolumeQos(VolumeInfo volumeInfo) { + ResizeVolumePayload payload = (ResizeVolumePayload) volumeInfo.getpayload(); + if (payload == null) { + throw new CloudRuntimeException("Missing resize payload for volume " + volumeInfo.getId()); + } + VolumeVO volume = volumeDao.findById(volumeInfo.getId()); + if (volume == null || volume.getPoolId() == null) { + throw new CloudRuntimeException("Unable to resolve volume or storage pool for IOPS update"); + } + StoragePoolVO storagePool = storagePoolDao.findById(volume.getPoolId()); + if (storagePool == null) { + throw new CloudRuntimeException("Storage pool not found for volume " + volume.getId()); + } + + verifySufficientIopsForStoragePool(storagePool, payload.newMinIops, volume.getId()); + + Map details = storagePoolDetailsDao.listDetailsKeyPairs(storagePool.getId()); + StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); + VolumeDetailVO qosDetail = volumeDetailsDao.findDetail(volume.getId(), OntapStorageConstants.QOS_POLICY_UUID); + String previousUuid = qosDetail != null ? qosDetail.getValue() : null; + VolumeQosPolicy qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, + payload.newMinIops, payload.newMaxIops, volume.getPoolId()); + + if (qosPolicy != null && Objects.equals(previousUuid, qosPolicy.getUuid())) { + return; + } + if (qosPolicy != null) { + try { + attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, qosPolicy); + persistQosPolicyDetails(volume.getId(), qosPolicy); + deleteQosPolicyIfUnused(storageStrategy, previousUuid, volume.getId()); + } catch (RuntimeException e) { + deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volume.getId()); + throw e; + } + return; + } + if (previousUuid != null) { + detachQosPolicy(storageStrategy, storagePool, details, volumeInfo); + persistQosPolicyDetails(volume.getId(), null); + deleteQosPolicyIfUnused(storageStrategy, previousUuid, volume.getId()); + } + } + private void attachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO storagePool, Map details, VolumeInfo volumeInfo, VolumeQosPolicy qosPolicy) { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java index 2a8113f10b81..19cfa59e26a0 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/QosFeignClient.java @@ -40,11 +40,6 @@ public interface QosFeignClient { OntapResponse getPolicies(@Param("authHeader") String authHeader, @QueryMap Map queryParams); - @RequestLine("PATCH /api/storage/qos/policies/{uuid}?return_timeout=0") - @Headers({"Authorization: {authHeader}"}) - JobResponse updatePolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid, - VolumeQosPolicy policy); - @RequestLine("DELETE /api/storage/qos/policies/{uuid}?return_timeout=0") @Headers({"Authorization: {authHeader}"}) JobResponse deletePolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 4e4fafb0b2b2..a5abf75d64a0 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -246,7 +246,7 @@ public boolean isAff() { "Unable to determine whether the ONTAP cluster is AFF or FAS"); } for (ClusterNode node : response.getRecords()) { - if (node == null || !Boolean.TRUE.equals(node.getAllFlashOptimized())) { + if (node == null || Boolean.FALSE.equals(node.getAllFlashOptimized())) { return false; } } @@ -982,12 +982,6 @@ public String getAuthHeader() { } public VolumeQosPolicy createVolumeQosPolicy(String policyName, Long minIops, Long maxIops) { - VolumeQosPolicy existingPolicy = getVolumeQosPolicy(policyName); - if (existingPolicy != null) { - logger.info("Reusing existing ONTAP QoS policy [{}]", policyName); - return existingPolicy; - } - VolumeQosPolicy policy = buildVolumeQosPolicy(policyName, minIops, maxIops); Svm svm = new Svm(); svm.setName(storage.getSvmName()); @@ -1010,27 +1004,6 @@ public VolumeQosPolicy createVolumeQosPolicy(String policyName, Long minIops, Lo return createdPolicy; } - /** - * Prefers the ONTAP REST error body (includes codes such as 8454269) over Feign's status line. - */ - protected String getOntapErrorDetail(Throwable error) { - if (error instanceof FeignException) { - try { - String body = ((FeignException) error).contentUTF8(); - if (body != null && !body.isBlank()) { - return body; - } - } catch (RuntimeException ignored) { - // Mocked or empty Feign responses may not expose a body. - } - } - return error != null ? error.getMessage() : null; - } - - protected CloudRuntimeException wrapOntapApiFailure(String operation, Throwable error) { - return new CloudRuntimeException(operation + ": " + getOntapErrorDetail(error), error); - } - public void deleteVolumeQosPolicy(String policyUuid) { if (policyUuid == null || policyUuid.isEmpty()) { return; 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 11e6df844b81..f0ed59bdbb98 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 @@ -146,7 +146,7 @@ public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume try { nasFeignClient.updateFile(getAuthHeader(), cloudstackVolume.getFlexVolumeUuid(), filePath, fileInfo); } catch (FeignException e) { - throw wrapOntapApiFailure("Failed to apply QoS policy to NFS volume file", e); + throw new CloudRuntimeException("Failed to apply QoS policy to NFS volume file: " + e.getMessage(), e); } logger.info("Applied QoS policy [{}] to NFS volume file [{}]", cloudstackVolume.getFile().getQosPolicy() != null 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 b6b9f7434346..580acab66ba6 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 @@ -90,7 +90,7 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume } catch (FeignException e) { logger.error("FeignException occurred while creating LUN: {}, Status: {}, Exception: {}", cloudstackVolume.getLun().getName(), e.status(), e.getMessage()); - throw wrapOntapApiFailure("Failed to create Lun", e); + throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage(), e); } catch (CloudRuntimeException e) { throw e; } catch (Exception e) { @@ -195,7 +195,7 @@ public CloudStackVolume updateCloudStackVolume(CloudStackVolume cloudstackVolume getAuthHeader(), cloudstackVolume.getLun().getUuid(), lunUpdate); pollJobIfPresent(response, "update QoS policy on LUN [" + cloudstackVolume.getLun().getUuid() + "]"); } catch (FeignException e) { - throw wrapOntapApiFailure("Failed to apply QoS policy to LUN", 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 @@ -315,7 +315,7 @@ public void resizeCloudStackVolume(CloudStackVolume cloudstackVolume, long sizeI sanFeignClient.updateLun(authHeader, lunUuid, patch); logger.debug("resizeCloudStackVolume: Lun {} resized to {} bytes", lunUuid, sizeInBytes); } catch (FeignException e) { - throw wrapOntapApiFailure("Failed to resize Lun", e); + throw new CloudRuntimeException("Failed to resize Lun: " + e.getMessage(), e); } catch (Exception e) { logger.error("Exception occurred while resizing LUN: {}, Exception: {}", lunUuid, e.getMessage()); throw new CloudRuntimeException("Failed to resize Lun: " + e.getMessage()); 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 f4c4bb78d3fe..dca18f8b9275 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 @@ -1288,7 +1288,6 @@ void testResize_IopsUpdate_AttachesQosPolicy() { verify(sanStrategy).updateCloudStackVolume(argThat(request -> request.getLun() != null && "lun-uuid-123".equals(request.getLun().getUuid()))); verify(volumeDetailsDao).addDetail(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); - verify(volumeDetailsDao).addDetail(100L, OntapStorageConstants.QOS_POLICY_NAME, "cs_0_to_5000_iops_svm1", false); } } @@ -1562,8 +1561,6 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( any(), any(), any(), argThat(policy -> policy != null && "qos-uuid".equals(policy.getUuid())))); - verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_NAME), - eq("cs_100_to_200_iops_svm1"), eq(false)); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), eq(false)); verify(sanStrategy, never()).isAff(); 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 1e0172a1e06e..75b530d22d99 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 @@ -298,7 +298,7 @@ public void testCreateCloudStackVolume_QosAttachFails_DeletesLeftoverNfsFile() { CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, () -> strategy.createCloudStackVolume(cloudStackVolume)); - assertTrue(ex.getMessage().contains("8454269")); + assertTrue(ex.getMessage().contains("Failed to apply QoS policy to NFS volume file")); verify(endPoint).sendMessage(any(DeleteCommand.class)); } 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 c12eab03ddb8..12058738ef73 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 @@ -378,7 +378,7 @@ void testCreateCloudStackVolume_MinThroughputRejected_PropagatesOntapError() { CloudRuntimeException ex = assertThrows(CloudRuntimeException.class, () -> unifiedSANStrategy.createCloudStackVolume(request)); - assertTrue(ex.getMessage().contains("8454269")); + assertTrue(ex.getMessage().contains("Failed to create Lun")); } } From fe6a8165ed3bd4b8376c23996bf5360d2638bff4 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Tue, 15 Sep 2026 23:43:57 +0530 Subject: [PATCH 11/13] [CSTACKEX-279] Refactoring --- .../driver/OntapPrimaryDatastoreDriver.java | 42 +++-------------- .../storage/feign/model/VolumeQosPolicy.java | 10 ++++ .../storage/service/StorageStrategy.java | 46 +++++++++++++++++-- .../storage/utils/OntapStorageConstants.java | 4 +- .../storage/utils/OntapStorageUtils.java | 25 ++++++++++ .../OntapPrimaryDatastoreDriverTest.java | 10 ++-- .../storage/utils/OntapStorageUtilsTest.java | 13 ++++++ 7 files changed, 103 insertions(+), 47 deletions(-) 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 e66996f1255c..a603da185fc1 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 @@ -245,7 +245,7 @@ private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, Volum return created; } catch (RuntimeException e) { if (qosPolicy != null) { - deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volumeObject.getId()); + storageStrategy.deleteVolumeQosPolicy(qosPolicy.getUuid()); } throw e; } @@ -303,7 +303,6 @@ private String getQosPolicyName(String svmName, Long minIops, Long maxIops) { } private void persistQosPolicyDetails(long volumeId, VolumeQosPolicy qosPolicy) { - volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_NAME); volumeDetailsDao.removeDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID); if (qosPolicy == null) { return; @@ -311,34 +310,6 @@ private void persistQosPolicyDetails(long volumeId, VolumeQosPolicy qosPolicy) { volumeDetailsDao.addDetail(volumeId, OntapStorageConstants.QOS_POLICY_UUID, qosPolicy.getUuid(), false); } - private boolean isQosPolicyUsedByOtherVolumes(String policyUuid, Long excludeVolumeId) { - if (policyUuid == null || policyUuid.isEmpty()) { - return false; - } - List references = volumeDetailsDao.findDetails( - OntapStorageConstants.QOS_POLICY_UUID, policyUuid, null); - if (references == null) { - return false; - } - for (VolumeDetailVO reference : references) { - if (excludeVolumeId == null || reference.getResourceId() != excludeVolumeId) { - return true; - } - } - return false; - } - - private void deleteQosPolicyIfUnused(StorageStrategy storageStrategy, String policyUuid, Long excludeVolumeId) { - if (policyUuid == null || policyUuid.isEmpty()) { - return; - } - if (isQosPolicyUsedByOtherVolumes(policyUuid, excludeVolumeId)) { - logger.info("QoS policy [{}] is still assigned to other volumes; skipping delete", policyUuid); - return; - } - storageStrategy.deleteVolumeQosPolicy(policyUuid); - } - /** * Creates the backend object that caches a template on this pool's FlexVolume. * @@ -607,8 +578,7 @@ public void deleteAsync(DataStore store, DataObject data, AsyncCompletionCallbac storageStrategy.deleteCloudStackVolume(cloudStackVolumeRequest); if (qosPolicyDetail != null) { volumeDetailsDao.removeDetail(volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_UUID); - volumeDetailsDao.removeDetail(volumeInfo.getId(), OntapStorageConstants.QOS_POLICY_NAME); - deleteQosPolicyIfUnused(storageStrategy, qosPolicyDetail.getValue(), volumeInfo.getId()); + storageStrategy.deleteVolumeQosPolicy(qosPolicyDetail.getValue()); } logger.info("deleteAsync: Volume deleted: " + volumeInfo.getId()); commandResult.setResult(null); @@ -785,9 +755,9 @@ private void applyVolumeQos(VolumeInfo volumeInfo) { try { attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, qosPolicy); persistQosPolicyDetails(volume.getId(), qosPolicy); - deleteQosPolicyIfUnused(storageStrategy, previousUuid, volume.getId()); + storageStrategy.deleteVolumeQosPolicy(previousUuid); } catch (RuntimeException e) { - deleteQosPolicyIfUnused(storageStrategy, qosPolicy.getUuid(), volume.getId()); + storageStrategy.deleteVolumeQosPolicy(qosPolicy.getUuid()); throw e; } return; @@ -795,7 +765,7 @@ private void applyVolumeQos(VolumeInfo volumeInfo) { if (previousUuid != null) { detachQosPolicy(storageStrategy, storagePool, details, volumeInfo); persistQosPolicyDetails(volume.getId(), null); - deleteQosPolicyIfUnused(storageStrategy, previousUuid, volume.getId()); + storageStrategy.deleteVolumeQosPolicy(previousUuid); } } @@ -820,7 +790,7 @@ private void attachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO stor private void detachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO storagePool, Map details, VolumeInfo volumeInfo) { VolumeQosPolicy noPolicy = new VolumeQosPolicy(); - noPolicy.setName(""); + noPolicy.setName(OntapStorageConstants.QOS_POLICY_NONE); attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, noPolicy); } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java index 077ddcb2d734..2e6ce1b6fa3c 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java @@ -34,6 +34,8 @@ public class VolumeQosPolicy { private String uuid = null; @JsonProperty("svm") private Svm svm; + @JsonProperty("object_count") + private Integer objectCount; public Fixed getFixed() { return fixed; @@ -67,6 +69,14 @@ public void setSvm(Svm svm) { this.svm = svm; } + public Integer getObjectCount() { + return objectCount; + } + + public void setObjectCount(Integer objectCount) { + this.objectCount = objectCount; + } + @JsonIgnoreProperties(ignoreUnknown = true) @JsonInclude(JsonInclude.Include.NON_NULL) public static class Fixed { diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index a5abf75d64a0..4576d3fecf2f 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -1008,19 +1008,59 @@ public void deleteVolumeQosPolicy(String policyUuid) { if (policyUuid == null || policyUuid.isEmpty()) { return; } + VolumeQosPolicy policy = getVolumeQosPolicyByUuid(policyUuid); + if (policy == null) { + return; + } + if (policy.getObjectCount() != null && policy.getObjectCount() > 0) { + logger.info("QoS policy [{}] still has object_count={}; skipping delete", + policyUuid, policy.getObjectCount()); + return; + } try { JobResponse response = qosFeignClient.deletePolicy(getAuthHeader(), policyUuid); pollJobIfPresent(response, "delete QoS policy [" + policyUuid + "]"); - } catch (FeignException e) { - if (OntapStorageUtils.isOntapObjectNotFoundError(e)) { - logger.info("QoS policy [{}] is already absent", policyUuid); + } catch (Exception e) { + if (isSkippableQosPolicyDeleteError(e)) { + logger.info("QoS policy [{}] was not deleted on ONTAP (already absent or still in use): {}", + policyUuid, e.getMessage()); return; } + if (e instanceof CloudRuntimeException) { + throw (CloudRuntimeException) e; + } throw new CloudRuntimeException("Failed to delete ONTAP QoS policy [" + policyUuid + "]: " + e.getMessage(), e); } } + private VolumeQosPolicy getVolumeQosPolicyByUuid(String policyUuid) { + Map queryParams = new HashMap<>(); + queryParams.put(OntapStorageConstants.UUID, policyUuid); + queryParams.put(OntapStorageConstants.FIELDS, OntapStorageConstants.QOS_POLICY_OBJECT_COUNT_FIELDS); + try { + OntapResponse response = qosFeignClient.getPolicies(getAuthHeader(), queryParams); + if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + return null; + } + return response.getRecords().get(0); + } catch (FeignException e) { + if (OntapStorageUtils.isOntapObjectNotFoundError(e)) { + return null; + } + throw new CloudRuntimeException("Failed to fetch ONTAP QoS policy [" + policyUuid + "]: " + + e.getMessage(), e); + } + } + + private boolean isSkippableQosPolicyDeleteError(Throwable error) { + if (error instanceof FeignException && ((FeignException) error).status() == 409) { + return true; + } + return OntapStorageUtils.isOntapObjectNotFoundError(error) + || OntapStorageUtils.isOntapObjectInUseError(error); + } + private VolumeQosPolicy getVolumeQosPolicy(String policyName) { Map queryParams = new HashMap<>(); queryParams.put(OntapStorageConstants.NAME, policyName); 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 17c37a58a200..bb407013f6a7 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 @@ -102,12 +102,14 @@ public class OntapStorageConstants { public static final String LUN_DOT_NAME = "lun.name"; public static final String IQN = "iqn"; public static final String LUN_DOT_UUID = "lun.uuid"; - public static final String QOS_POLICY_NAME = "qosPolicyName"; public static final String QOS_POLICY_UUID = "qosPolicyUuid"; public static final String IS_AFF = "isAFF"; + 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_"; public static final String QOS_POLICY_NAME_IOPS = "iops_"; + public static final String UUID = "uuid"; + public static final String QOS_POLICY_OBJECT_COUNT_FIELDS = "uuid,object_count"; public static final String LOGICAL_UNIT_NUMBER = "logical_unit_number"; public static final String IGROUP_DOT_NAME = "igroup.name"; public static final String IGROUP_DOT_UUID = "igroup.uuid"; diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index a23121c46adf..9a8cfef2d364 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -295,4 +295,29 @@ public static boolean isOntapObjectNotFoundError(Throwable error) { return false; } + /** + * Returns true when ONTAP rejected the operation because the object is still assigned. + */ + public static boolean isOntapObjectInUseError(Throwable error) { + if (error == null) { + return false; + } + String text = error.getMessage(); + if (error instanceof FeignException) { + try { + String body = ((FeignException) error).contentUTF8(); + if (body != null && !body.isBlank()) { + text = text == null ? body : text + " " + body; + } + } catch (RuntimeException ignored) { + // Mocked Feign responses may not expose a body. + } + } + if (text == null) { + return false; + } + String lower = text.toLowerCase(); + return lower.contains("in use") || lower.contains("being used") || lower.contains("still assigned"); + } + } 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 dca18f8b9275..681387671fe7 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 @@ -1371,8 +1371,6 @@ void testResize_ClearIops_DetachesQosPolicy() { when(storagePoolDetailsDao.listDetailsKeyPairs(1L)).thenReturn(storagePoolDetails); VolumeDetailVO qosDetail = new VolumeDetailVO(100L, OntapStorageConstants.QOS_POLICY_UUID, "qos-uuid", false); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); - when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) - .thenReturn(List.of(qosDetail)); VolumeDetailVO lunUuidDetail = new VolumeDetailVO(100L, OntapStorageConstants.LUN_DOT_UUID, "lun-uuid-123", false); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); @@ -1390,6 +1388,9 @@ void testResize_ClearIops_DetachesQosPolicy() { verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); verify(sanStrategy).updateCloudStackVolume(any()); + utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + any(), any(), any(), argThat(policy -> policy != null + && OntapStorageConstants.QOS_POLICY_NONE.equals(policy.getName())))); verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); } } @@ -1815,8 +1816,6 @@ void testCreateAsync_QosCreateThenLunCreateFails_DeletesUnusedPolicy() { .thenReturn(qosPolicy); when(sanStrategy.createCloudStackVolume(any())).thenThrow(new CloudRuntimeException( "Failed to create Lun: {\"error\":{\"code\":\"8454269\"}}")); - when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) - .thenReturn(List.of()); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1842,8 +1841,6 @@ void testDeleteAsync_DeletesUnusedQosPolicy() { when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_NAME)).thenReturn(lunNameDetail); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); - when(volumeDetailsDao.findDetails(eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), isNull())) - .thenReturn(List.of(qosDetail)); try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) @@ -1856,7 +1853,6 @@ void testDeleteAsync_DeletesUnusedQosPolicy() { verify(commandCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); verify(volumeDetailsDao).removeDetail(100L, OntapStorageConstants.QOS_POLICY_UUID); - verify(volumeDetailsDao).removeDetail(100L, OntapStorageConstants.QOS_POLICY_NAME); verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); } } diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java index 594f5b33fb5d..b41f1cda96e6 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java @@ -154,4 +154,17 @@ public void isOntapSnapshotNotFoundError_rejectsUnrelatedErrors() { assertFalse(OntapStorageUtils.isOntapObjectNotFoundError( new CloudRuntimeException("Job failed with error: permission denied"))); } + + @Test + public void isOntapObjectInUseError_matchesPolicyStillAssigned() { + CloudRuntimeException ex = new CloudRuntimeException( + "Job failed with error: The QoS policy group is in use by one or more objects."); + assertTrue(OntapStorageUtils.isOntapObjectInUseError(ex)); + } + + @Test + public void isOntapObjectInUseError_rejectsUnrelatedErrors() { + assertFalse(OntapStorageUtils.isOntapObjectInUseError( + new CloudRuntimeException("Job failed with error: permission denied"))); + } } From a4541152634a3b387b7fccb6f5836dd56dd54966 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Tue, 15 Sep 2026 23:58:55 +0530 Subject: [PATCH 12/13] [CSTACKEX-279] Refactoring --- .../driver/OntapPrimaryDatastoreDriver.java | 76 +++++++++++------ .../storage/service/StorageStrategy.java | 16 +--- .../storage/utils/OntapStorageUtils.java | 83 ------------------- .../OntapPrimaryDatastoreDriverTest.java | 83 +++++++------------ .../storage/utils/OntapStorageUtilsTest.java | 74 ----------------- 5 files changed, 84 insertions(+), 248 deletions(-) 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 a603da185fc1..e952acf5dd0c 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 @@ -237,7 +237,7 @@ private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool, Volum qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, volumeObject.getMinIops(), volumeObject.getMaxIops(), storagePool.getId()); } - CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + CloudStackVolume request = createCloudStackVolumeRequestByProtocol( storagePool, details, volumeObject, qosPolicy); try { CloudStackVolume created = storageStrategy.createCloudStackVolume(request); @@ -772,7 +772,7 @@ private void applyVolumeQos(VolumeInfo volumeInfo) { private void attachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO storagePool, Map details, VolumeInfo volumeInfo, VolumeQosPolicy qosPolicy) { - CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( + CloudStackVolume request = createCloudStackVolumeRequestByProtocol( storagePool, details, volumeInfo, qosPolicy); if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { VolumeDetailVO lunUuid = volumeDetailsDao.findDetail(volumeInfo.getId(), OntapStorageConstants.LUN_DOT_UUID); @@ -1585,34 +1585,56 @@ private boolean isIscsi(Map details) { } /** - * Builds the request that creates a blank volume (LUN for iSCSI, qcow2 file for NFS). + * Builds the request that creates or updates a volume (LUN for iSCSI, qcow2 file for NFS), + * attaching a QoS policy reference when one is provided. */ - private CloudStackVolume createVolumeRequest(StoragePoolVO storagePool, Map details, DataObject volumeObject) { - CloudStackVolume request = new CloudStackVolume(); + private CloudStackVolume createCloudStackVolumeRequestByProtocol(StoragePoolVO storagePool, Map details, + DataObject volumeObject, VolumeQosPolicy qosPolicy) { + VolumeQosPolicy qosPolicyReference = null; + if (qosPolicy != null) { + qosPolicyReference = new VolumeQosPolicy(); + qosPolicyReference.setName(qosPolicy.getName()); + qosPolicyReference.setUuid(qosPolicy.getUuid()); + } + String protocol = details.get(OntapStorageConstants.PROTOCOL); - if (ProtocolType.NFS3.name().equalsIgnoreCase(protocol)) { - request.setDatastoreId(String.valueOf(storagePool.getId())); - request.setVolumeInfo(volumeObject); - } else if (ProtocolType.ISCSI.name().equalsIgnoreCase(protocol)) { - Lun lunRequest = new Lun(); - Svm svm = new Svm(); - svm.setName(details.get(OntapStorageConstants.SVM_NAME)); - String lunName = volumeObject.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE); - if (!OntapStorageUtils.isValidName(lunName)) { - throw new InvalidParameterValueException("Invalid dataObject name [" + lunName - + "]. It must start with a letter and can only contain letters, digits, and underscores, and be up to 200 characters long."); - } - lunRequest.setSvm(svm); - lunRequest.setName(OntapStorageUtils.getLunName(storagePool.getName(), lunName)); - lunRequest.setOsType(Lun.OsTypeEnum.valueOf(OntapStorageUtils.getOSTypeFromHypervisor(storagePool.getHypervisor().name()))); - LunSpace lunSpace = new LunSpace(); - lunSpace.setSize(volumeObject.getSize()); - lunRequest.setSpace(lunSpace); - request.setLun(lunRequest); - } else { - throw new CloudRuntimeException("Unsupported protocol " + protocol); + ProtocolType protocolType = ProtocolType.valueOf(protocol); + switch (protocolType) { + case NFS3: + CloudStackVolume nfsRequest = new CloudStackVolume(); + nfsRequest.setDatastoreId(String.valueOf(storagePool.getId())); + nfsRequest.setFlexVolumeUuid(details.get(OntapStorageConstants.VOLUME_UUID)); + nfsRequest.setVolumeInfo(volumeObject); + if (qosPolicyReference != null) { + FileInfo fileInfo = new FileInfo(); + fileInfo.setQosPolicy(qosPolicyReference); + nfsRequest.setFile(fileInfo); + } + return nfsRequest; + case ISCSI: + Svm svm = new Svm(); + svm.setName(details.get(OntapStorageConstants.SVM_NAME)); + CloudStackVolume iscsiRequest = new CloudStackVolume(); + Lun lunRequest = new Lun(); + lunRequest.setSvm(svm); + + LunSpace lunSpace = new LunSpace(); + lunSpace.setSize(volumeObject.getSize()); + lunRequest.setSpace(lunSpace); + String lunName = volumeObject.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE); + if (!OntapStorageUtils.isValidName(lunName)) { + throw new InvalidParameterValueException("Invalid dataObject name [" + lunName + + "]. It must start with a letter and can only contain letters, digits, and underscores, and be up to 200 characters long."); + } + lunRequest.setName(OntapStorageUtils.getLunName(storagePool.getName(), lunName)); + lunRequest.setOsType(Lun.OsTypeEnum.valueOf( + OntapStorageUtils.getOSTypeFromHypervisor(storagePool.getHypervisor().name()))); + lunRequest.setQosPolicy(qosPolicyReference); + iscsiRequest.setLun(lunRequest); + return iscsiRequest; + default: + throw new CloudRuntimeException("Unsupported protocol " + protocol); } - return request; } /** diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index 4576d3fecf2f..fa255363a25d 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -1021,14 +1021,12 @@ public void deleteVolumeQosPolicy(String policyUuid) { JobResponse response = qosFeignClient.deletePolicy(getAuthHeader(), policyUuid); pollJobIfPresent(response, "delete QoS policy [" + policyUuid + "]"); } catch (Exception e) { - if (isSkippableQosPolicyDeleteError(e)) { - logger.info("QoS policy [{}] was not deleted on ONTAP (already absent or still in use): {}", + if ((e instanceof FeignException && ((FeignException) e).status() == 409) + || OntapStorageUtils.isOntapObjectNotFoundError(e)) { + logger.info("QoS policy [{}] was not deleted on ONTAP (already absent or conflict): {}", policyUuid, e.getMessage()); return; } - if (e instanceof CloudRuntimeException) { - throw (CloudRuntimeException) e; - } throw new CloudRuntimeException("Failed to delete ONTAP QoS policy [" + policyUuid + "]: " + e.getMessage(), e); } @@ -1053,14 +1051,6 @@ private VolumeQosPolicy getVolumeQosPolicyByUuid(String policyUuid) { } } - private boolean isSkippableQosPolicyDeleteError(Throwable error) { - if (error instanceof FeignException && ((FeignException) error).status() == 409) { - return true; - } - return OntapStorageUtils.isOntapObjectNotFoundError(error) - || OntapStorageUtils.isOntapObjectInUseError(error); - } - private VolumeQosPolicy getVolumeQosPolicy(String policyName) { Map queryParams = new HashMap<>(); queryParams.put(OntapStorageConstants.NAME, policyName); diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java index 9a8cfef2d364..e2b419ea46f0 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java @@ -23,17 +23,10 @@ import java.util.Map; import feign.FeignException; -import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; -import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; -import org.apache.cloudstack.storage.feign.model.FileInfo; import org.apache.cloudstack.storage.feign.model.Lun; -import org.apache.cloudstack.storage.feign.model.LunSpace; import org.apache.cloudstack.storage.feign.model.OntapStorage; -import org.apache.cloudstack.storage.feign.model.Svm; -import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; import org.apache.cloudstack.storage.provider.StorageProviderFactory; import org.apache.cloudstack.storage.service.StorageStrategy; -import org.apache.cloudstack.storage.service.model.CloudStackVolume; import org.apache.cloudstack.storage.service.model.ProtocolType; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -62,57 +55,6 @@ public static String generateAuthHeader (String username, String password) { return BASIC + StringUtils.SPACE + new String(encodedBytes); } - public static CloudStackVolume createCloudStackVolumeRequestByProtocol(StoragePoolVO storagePool, Map details, - DataObject volumeObject, VolumeQosPolicy qosPolicy) { - CloudStackVolume cloudStackVolumeRequest = null; - VolumeQosPolicy qosPolicyReference = null; - if (qosPolicy != null) { - qosPolicyReference = new VolumeQosPolicy(); - qosPolicyReference.setName(qosPolicy.getName()); - qosPolicyReference.setUuid(qosPolicy.getUuid()); - } - - String protocol = details.get(OntapStorageConstants.PROTOCOL); - ProtocolType protocolType = ProtocolType.valueOf(protocol); - switch (protocolType) { - case NFS3: - cloudStackVolumeRequest = new CloudStackVolume(); - cloudStackVolumeRequest.setDatastoreId(String.valueOf(storagePool.getId())); - cloudStackVolumeRequest.setFlexVolumeUuid(details.get(OntapStorageConstants.VOLUME_UUID)); - cloudStackVolumeRequest.setVolumeInfo(volumeObject); - if (qosPolicyReference != null) { - FileInfo fileInfo = new FileInfo(); - fileInfo.setQosPolicy(qosPolicyReference); - cloudStackVolumeRequest.setFile(fileInfo); - } - break; - case ISCSI: - Svm svm = new Svm(); - svm.setName(details.get(OntapStorageConstants.SVM_NAME)); - cloudStackVolumeRequest = new CloudStackVolume(); - Lun lunRequest = new Lun(); - lunRequest.setSvm(svm); - - LunSpace lunSpace = new LunSpace(); - lunSpace.setSize(volumeObject.getSize()); - lunRequest.setSpace(lunSpace); - String lunName = volumeObject.getName().replace(OntapStorageConstants.HYPHEN, OntapStorageConstants.UNDERSCORE); - if (!isValidName(lunName)) { - String errMsg = "createAsync: Invalid dataObject name [" + lunName - + "]. It must start with a letter and can only contain letters, digits, and underscores, and be up to 200 characters long."; - throw new InvalidParameterValueException(errMsg); - } - lunRequest.setName(getLunName(storagePool.getName(), lunName)); - lunRequest.setOsType(Lun.OsTypeEnum.valueOf(getOSTypeFromHypervisor(storagePool.getHypervisor().name()))); - lunRequest.setQosPolicy(qosPolicyReference); - cloudStackVolumeRequest.setLun(lunRequest); - break; - default: - throw new CloudRuntimeException("Unsupported protocol " + protocol); - } - return cloudStackVolumeRequest; - } - public static boolean isValidName(String name) { // Check for null and length constraint first if (name == null || name.length() > 200) { @@ -295,29 +237,4 @@ public static boolean isOntapObjectNotFoundError(Throwable error) { return false; } - /** - * Returns true when ONTAP rejected the operation because the object is still assigned. - */ - public static boolean isOntapObjectInUseError(Throwable error) { - if (error == null) { - return false; - } - String text = error.getMessage(); - if (error instanceof FeignException) { - try { - String body = ((FeignException) error).contentUTF8(); - if (body != null && !body.isBlank()) { - text = text == null ? body : text + " " + body; - } - } catch (RuntimeException ignored) { - // Mocked Feign responses may not expose a body. - } - } - if (text == null) { - return false; - } - String lower = text.toLowerCase(); - return lower.contains("in use") || lower.contains("being used") || lower.contains("still assigned"); - } - } 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 681387671fe7..d84e57872b00 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 @@ -82,7 +82,6 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.doNothing; @@ -156,6 +155,10 @@ void setUp() { storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.ISCSI.name()); storagePoolDetails.put(OntapStorageConstants.SVM_NAME, "svm1"); storagePoolDetails.put(OntapStorageConstants.IS_AFF, "true"); + lenient().when(volumeInfo.getName()).thenReturn("test-volume"); + lenient().when(volumeInfo.getSize()).thenReturn(1073741824L); + lenient().when(storagePool.getName()).thenReturn("vol1"); + lenient().when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); } @Test @@ -218,8 +221,6 @@ void testCreateAsync_VolumeWithISCSI_Success() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(responseVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(responseVolume); // Execute @@ -264,8 +265,6 @@ void testCreateAsync_VolumeWithNFS_Success() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) .thenReturn(nasStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(mockCloudStackVolume); when(nasStrategy.createCloudStackVolume(any())).thenReturn(mockCloudStackVolume); @@ -286,7 +285,7 @@ void testCreateAsync_VolumeWithNFS_Success() { @Test void testCreateAsync_UnsupportedHypervisor_FailsWithError() { - // Use NFS so createVolumeRequest does not fail earlier in getOSTypeFromHypervisor; + // Use NFS so LUN OS-type resolution does not fail earlier in getOSTypeFromHypervisor; // the failure under test is image-format resolution for non-KVM hypervisors. storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); @@ -1276,7 +1275,7 @@ void testResize_IopsUpdate_AttachesQosPolicy() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_5000_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); when(sanStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); @@ -1311,7 +1310,7 @@ void testResize_IopsUpdate_NfsAttachesQosPolicy() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = nfsCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, nasStrategy, cloudStackVolume, qosPolicy); when(nasStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); @@ -1344,7 +1343,7 @@ void testResize_SameQosPolicy_SkipsAttach() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_3333_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); driver.resize(volumeInfo, createCallback); @@ -1375,11 +1374,9 @@ void testResize_ClearIops_DetachesQosPolicy() { when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.updateCloudStackVolume(any())).thenReturn(cloudStackVolume); driver.resize(volumeInfo, createCallback); @@ -1387,10 +1384,9 @@ void testResize_ClearIops_DetachesQosPolicy() { ArgumentCaptor resultCaptor = ArgumentCaptor.forClass(CreateCmdResult.class); verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); - verify(sanStrategy).updateCloudStackVolume(any()); - utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), argThat(policy -> policy != null - && OntapStorageConstants.QOS_POLICY_NONE.equals(policy.getName())))); + verify(sanStrategy).updateCloudStackVolume(argThat(request -> + request.getLun() != null && request.getLun().getQosPolicy() != null + && OntapStorageConstants.QOS_POLICY_NONE.equals(request.getLun().getQosPolicy().getName()))); verify(sanStrategy).deleteVolumeQosPolicy("qos-uuid"); } } @@ -1528,8 +1524,6 @@ void testCreateAsync_DoesNotDoubleCountSelfWhenAlreadyCreating() { try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1551,7 +1545,7 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1560,8 +1554,9 @@ void testCreateAsync_DataDiskFixedIops_CreatesAndPersistsQosPolicy() { verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); verify(sanStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); - utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), argThat(policy -> policy != null && "qos-uuid".equals(policy.getUuid())))); + verify(sanStrategy).createCloudStackVolume(argThat(request -> + request.getLun() != null && request.getLun().getQosPolicy() != null + && "qos-uuid".equals(request.getLun().getQosPolicy().getUuid()))); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), eq("qos-uuid"), eq(false)); verify(sanStrategy, never()).isAff(); @@ -1575,13 +1570,10 @@ void testCreateAsync_FasPoolWithMinIops_FailsWithoutCallingOntap() { when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); when(volumeInfo.getMinIops()).thenReturn(100L); when(volumeInfo.getMaxIops()).thenReturn(200L); - CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1605,7 +1597,7 @@ void testCreateAsync_FasPoolWithMaxIopsOnly_CreatesQosPolicyWithoutNodeCall() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_0_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1630,7 +1622,7 @@ void testCreateAsync_MissingAffDetail_FetchesFromNodesAndPersists() { VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1652,13 +1644,10 @@ void testCreateAsync_MissingAffDetailOnFas_PersistsFalseAndRejectsMinIops() { when(volumeInfo.getMinIops()).thenReturn(100L); when(volumeInfo.getMaxIops()).thenReturn(200L); when(sanStrategy.isAff()).thenReturn(false); - CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1682,7 +1671,7 @@ void testCreateAsync_RootDiskCustomIops_CreatesAndPersistsQosPolicy() { VolumeQosPolicy qosPolicy = qosPolicy("qos-root-uuid", "cs_111_to_999_iops_svm1"); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, sanStrategy, cloudStackVolume, qosPolicy); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1719,7 +1708,7 @@ void testCreateAsync_NfsDataDiskWithIops_CreatesQosPolicy() { VolumeQosPolicy qosPolicy = qosPolicy("qos-nfs-uuid", "cs_100_to_200_iops_svm1"); CloudStackVolume cloudStackVolume = new CloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { stubQosCreateMocks(utilityMock, nasStrategy, cloudStackVolume, qosPolicy); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1728,8 +1717,9 @@ void testCreateAsync_NfsDataDiskWithIops_CreatesQosPolicy() { verify(createCallback).complete(resultCaptor.capture()); assertTrue(resultCaptor.getValue().isSuccess()); verify(nasStrategy).createVolumeQosPolicy(eq("cs_100_to_200_iops_svm1"), eq(100L), eq(200L)); - utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), argThat(policy -> policy != null && "qos-nfs-uuid".equals(policy.getUuid())))); + verify(nasStrategy).createCloudStackVolume(argThat(request -> + request.getFile() != null && request.getFile().getQosPolicy() != null + && "qos-nfs-uuid".equals(request.getFile().getQosPolicy().getUuid()))); } } @@ -1740,7 +1730,7 @@ void testCreateAsync_MinIopsGreaterThanMaxIops_Fails() { when(volumeInfo.getMinIops()).thenReturn(200L); when(volumeInfo.getMaxIops()).thenReturn(100L); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); @@ -1762,18 +1752,16 @@ void testCreateAsync_NoIops_DoesNotCreateQosPolicy() { when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.DATADISK); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); verify(sanStrategy, never()).createVolumeQosPolicy(any(), any(), any()); - utilityMock.verify(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), isNull())); + verify(sanStrategy).createCloudStackVolume(argThat(request -> + request.getLun() != null && request.getLun().getQosPolicy() == null)); } } @@ -1784,11 +1772,9 @@ void testCreateAsync_SwapVolumeWithIops_DoesNotCreateQosPolicy() { when(volumeInfo.getMinIops()).thenReturn(100L); CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); driver.createAsync(dataStore, volumeInfo, createCallback); @@ -1805,13 +1791,10 @@ void testCreateAsync_QosCreateThenLunCreateFails_DeletesUnusedPolicy() { when(volumeInfo.getMaxIops()).thenReturn(200L); VolumeQosPolicy qosPolicy = qosPolicy("qos-uuid", "cs_100_to_200_iops_svm1"); - CloudStackVolume cloudStackVolume = iscsiCloudStackVolume(); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(sanStrategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(sanStrategy.createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class))) .thenReturn(qosPolicy); when(sanStrategy.createCloudStackVolume(any())).thenThrow(new CloudRuntimeException( @@ -1842,7 +1825,7 @@ void testDeleteAsync_DeletesUnusedQosPolicy() { when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.LUN_DOT_UUID)).thenReturn(lunUuidDetail); when(volumeDetailsDao.findDetail(100L, OntapStorageConstants.QOS_POLICY_UUID)).thenReturn(qosDetail); - try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class)) { + try (MockedStatic utilityMock = mockStatic(OntapStorageUtils.class, CALLS_REAL_METHODS)) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(storagePoolDetails)) .thenReturn(sanStrategy); doNothing().when(sanStrategy).deleteCloudStackVolume(any()); @@ -1903,8 +1886,6 @@ private void stubQosCreateMocks(MockedStatic utilityMock, CloudStackVolume cloudStackVolume, VolumeQosPolicy qosPolicy) { utilityMock.when(() -> OntapStorageUtils.getStrategyByStoragePoolDetails(any())) .thenReturn(strategy); - utilityMock.when(() -> OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - any(), any(), any(), nullable(VolumeQosPolicy.class))).thenReturn(cloudStackVolume); when(strategy.createVolumeQosPolicy(nullable(String.class), nullable(Long.class), nullable(Long.class))) .thenReturn(qosPolicy); lenient().when(strategy.createCloudStackVolume(any())).thenReturn(cloudStackVolume); diff --git a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java index b41f1cda96e6..ebe7da25ed12 100644 --- a/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java +++ b/plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/utils/OntapStorageUtilsTest.java @@ -18,24 +18,12 @@ */ package org.apache.cloudstack.storage.utils; -import java.util.HashMap; -import java.util.Map; - -import com.cloud.hypervisor.Hypervisor; import com.cloud.utils.exception.CloudRuntimeException; -import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; -import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; -import org.apache.cloudstack.storage.feign.model.VolumeQosPolicy; -import org.apache.cloudstack.storage.service.model.CloudStackVolume; -import org.apache.cloudstack.storage.service.model.ProtocolType; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class OntapStorageUtilsTest { @@ -94,55 +82,6 @@ public void getIgroupName_truncates_whenOneCharOverMaxLength() { assertEquals(OntapStorageConstants.IGROUP_NAME_MAX_LENGTH, result.length()); } - @Test - public void createCloudStackVolumeRequestByProtocol_attachesQosToIscsiLun() { - StoragePoolVO storagePool = mock(StoragePoolVO.class); - when(storagePool.getName()).thenReturn("pool1"); - when(storagePool.getHypervisor()).thenReturn(Hypervisor.HypervisorType.KVM); - - VolumeInfo volumeInfo = mock(VolumeInfo.class); - when(volumeInfo.getName()).thenReturn("data_disk"); - when(volumeInfo.getSize()).thenReturn(1073741824L); - - Map details = new HashMap<>(); - details.put(OntapStorageConstants.PROTOCOL, ProtocolType.ISCSI.name()); - details.put(OntapStorageConstants.SVM_NAME, "svm1"); - - VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); - qosPolicy.setName("cs_100_to200_iops_svm1"); - qosPolicy.setUuid("qos-uuid"); - - CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - storagePool, details, volumeInfo, qosPolicy); - - assertNotNull(request.getLun().getQosPolicy()); - assertEquals("qos-uuid", request.getLun().getQosPolicy().getUuid()); - assertEquals("cs_100_to200_iops_svm1", request.getLun().getQosPolicy().getName()); - } - - @Test - public void createCloudStackVolumeRequestByProtocol_attachesQosToNfsFile() { - StoragePoolVO storagePool = mock(StoragePoolVO.class); - when(storagePool.getId()).thenReturn(1L); - - VolumeInfo volumeInfo = mock(VolumeInfo.class); - - Map details = new HashMap<>(); - details.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); - details.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); - - VolumeQosPolicy qosPolicy = new VolumeQosPolicy(); - qosPolicy.setName("cs_100_to200_iops_svm1"); - qosPolicy.setUuid("qos-uuid"); - - CloudStackVolume request = OntapStorageUtils.createCloudStackVolumeRequestByProtocol( - storagePool, details, volumeInfo, qosPolicy); - - assertEquals("flex-uuid", request.getFlexVolumeUuid()); - assertNotNull(request.getFile().getQosPolicy()); - assertEquals("qos-uuid", request.getFile().getQosPolicy().getUuid()); - } - @Test public void isOntapSnapshotNotFoundError_matchesEntryDoesNotExist() { CloudRuntimeException ex = new CloudRuntimeException("Job failed with error: entry doesn't exist"); @@ -154,17 +93,4 @@ public void isOntapSnapshotNotFoundError_rejectsUnrelatedErrors() { assertFalse(OntapStorageUtils.isOntapObjectNotFoundError( new CloudRuntimeException("Job failed with error: permission denied"))); } - - @Test - public void isOntapObjectInUseError_matchesPolicyStillAssigned() { - CloudRuntimeException ex = new CloudRuntimeException( - "Job failed with error: The QoS policy group is in use by one or more objects."); - assertTrue(OntapStorageUtils.isOntapObjectInUseError(ex)); - } - - @Test - public void isOntapObjectInUseError_rejectsUnrelatedErrors() { - assertFalse(OntapStorageUtils.isOntapObjectInUseError( - new CloudRuntimeException("Job failed with error: permission denied"))); - } } From e439f2aebbcfc04da4d60a01d703bccf3030bac2 Mon Sep 17 00:00:00 2001 From: "Gupta, Surya" Date: Wed, 16 Sep 2026 02:15:58 +0530 Subject: [PATCH 13/13] [CSTACKEX-279] Resync the code and fix the QoS issue for ROOT volume --- .../driver/OntapPrimaryDatastoreDriver.java | 60 +++++++++++----- .../storage/feign/client/QosFeignClient.java | 9 ++- .../storage/feign/client/SANFeignClient.java | 2 +- .../storage/service/StorageStrategy.java | 11 ++- .../storage/service/UnifiedSANStrategy.java | 3 +- .../storage/utils/OntapStorageConstants.java | 2 +- .../OntapPrimaryDatastoreDriverTest.java | 69 +++++++++++++++++++ .../service/UnifiedSANStrategyTest.java | 2 +- 8 files changed, 127 insertions(+), 31 deletions(-) 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 e952acf5dd0c..eb95b968883e 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 @@ -369,28 +369,46 @@ private CloudStackVolume cloneCloudStackVolumeFromTemplate(StoragePoolVO storage StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details); boolean iscsi = isIscsi(details); + verifySufficientIopsForStoragePool(storagePool, volumeInfo.getMinIops(), volumeInfo.getId()); + VolumeQosPolicy qosPolicy = null; + Volume.Type volumeType = volumeInfo.getVolumeType(); + if (volumeType == Volume.Type.DATADISK || volumeType == Volume.Type.ROOT) { + qosPolicy = createQosPolicyIfNeeded(storageStrategy, details, + volumeInfo.getMinIops(), volumeInfo.getMaxIops(), storagePool.getId()); + } + CloudStackVolume request = iscsi - ? createCloneLunRequest(storagePool, details, volumeInfo, templatePoolRef, templateId) + ? createCloneLunRequest(storagePool, details, volumeInfo, templatePoolRef, templateId, qosPolicy) : createCloneFileRequest(storagePool, volumeInfo, templatePoolRef, templateId); - CloudStackVolume cloned = storageStrategy.cloneCloudStackVolume(request); - // SAN cloneCloudStackVolume validates the Feign response (LUN name + uuid) before returning - if (cloned == null) { - throw new CloudRuntimeException("ONTAP returned nothing when cloning template [" + templateId - + "] for volume [" + volumeInfo.getId() + "]"); - } + try { + CloudStackVolume cloned = storageStrategy.cloneCloudStackVolume(request); + if (cloned == null) { + throw new CloudRuntimeException("ONTAP returned nothing when cloning template [" + templateId + + "] for volume [" + volumeInfo.getId() + "]"); + } - logger.info("cloneCloudStackVolumeFromTemplate: Cloned template [{}] for volume [{}] on pool [{}]", - templateId, volumeInfo.getId(), storagePool.getId()); + logger.info("cloneCloudStackVolumeFromTemplate: Cloned template [{}] for volume [{}] on pool [{}]", + templateId, volumeInfo.getId(), storagePool.getId()); - long requestedSize = getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool); - if (requestedSize > templatePoolRef.getTemplateSize()) { - logger.info("cloneCloudStackVolumeFromTemplate: Growing clone of template [{}] from {} to {} bytes for volume [{}]", - templateId, templatePoolRef.getTemplateSize(), requestedSize, volumeInfo.getId()); - storageStrategy.resizeCloudStackVolume(cloned, requestedSize); - } + long requestedSize = getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool); + if (requestedSize > templatePoolRef.getTemplateSize()) { + logger.info("cloneCloudStackVolumeFromTemplate: Growing clone of template [{}] from {} to {} bytes for volume [{}]", + templateId, templatePoolRef.getTemplateSize(), requestedSize, volumeInfo.getId()); + storageStrategy.resizeCloudStackVolume(cloned, requestedSize); + } - return cloned; + if (!iscsi && qosPolicy != null) { + attachQosPolicy(storageStrategy, storagePool, details, volumeInfo, qosPolicy); + } + persistQosPolicyDetails(volumeInfo.getId(), qosPolicy); + return cloned; + } catch (Exception e) { + if (qosPolicy != null) { + storageStrategy.deleteVolumeQosPolicy(qosPolicy.getUuid()); + } + throw e; + } } /** @@ -774,7 +792,7 @@ private void attachQosPolicy(StorageStrategy storageStrategy, StoragePoolVO stor VolumeQosPolicy qosPolicy) { CloudStackVolume request = createCloudStackVolumeRequestByProtocol( storagePool, details, volumeInfo, qosPolicy); - if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) { + if (isIscsi(details)) { VolumeDetailVO lunUuid = volumeDetailsDao.findDetail(volumeInfo.getId(), OntapStorageConstants.LUN_DOT_UUID); if (lunUuid == null || lunUuid.getValue() == null) { throw new CloudRuntimeException("LUN UUID is missing for volume " + volumeInfo.getId()); @@ -1657,7 +1675,7 @@ private String getTemplateLunName(StoragePoolVO storagePool, long templateId) { */ private CloudStackVolume createCloneLunRequest(StoragePoolVO storagePool, Map details, VolumeInfo volumeObject, VMTemplateStoragePoolVO templatePoolRef, - long templateId) { + long templateId, VolumeQosPolicy qosPolicy) { String sourceLunUuid = templatePoolRef.getLocalDownloadPath(); if (sourceLunUuid == null || sourceLunUuid.isEmpty()) { throw new CloudRuntimeException("Template [" + templateId + "] has no cached LUN on pool [" @@ -1683,6 +1701,12 @@ private CloudStackVolume createCloneLunRequest(StoragePoolVO storagePool, Map getPolicies(@Param("authHeader") String authHeader, @QueryMap Map queryParams); - @RequestLine("DELETE /api/storage/qos/policies/{uuid}?return_timeout=0") + @RequestLine("GET /api/storage/qos/policies/{uuid}") + @Headers({"Authorization: {authHeader}"}) + VolumeQosPolicy getPolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid, + @QueryMap Map queryParams); + + @RequestLine("DELETE /api/storage/qos/policies/{uuid}") @Headers({"Authorization: {authHeader}"}) JobResponse deletePolicy(@Param("authHeader") String authHeader, @Param("uuid") String uuid); } diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java index 240b8ab1323e..8a60a9a3e2bb 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java @@ -56,7 +56,7 @@ public interface SANFeignClient { @RequestLine("DELETE /api/storage/luns/{uuid}") @Headers({"Authorization: {authHeader}"}) - void deleteLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, @QueryMap Map queryMap); + JobResponse deleteLun(@Param("authHeader") String authHeader, @Param("uuid") String uuid, @QueryMap Map queryMap); // iGroup Operation APIs @RequestLine("POST /api/protocols/san/igroups?return_records={returnRecords}") diff --git a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java index fa255363a25d..4eb7e809687c 100644 --- a/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java +++ b/plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Objects; +import com.cloud.utils.StringUtils; import org.apache.cloudstack.storage.feign.FeignClientFactory; import org.apache.cloudstack.storage.feign.client.AggregateFeignClient; import org.apache.cloudstack.storage.feign.client.ClusterFeignClient; @@ -1009,9 +1010,6 @@ public void deleteVolumeQosPolicy(String policyUuid) { return; } VolumeQosPolicy policy = getVolumeQosPolicyByUuid(policyUuid); - if (policy == null) { - return; - } if (policy.getObjectCount() != null && policy.getObjectCount() > 0) { logger.info("QoS policy [{}] still has object_count={}; skipping delete", policyUuid, policy.getObjectCount()); @@ -1034,14 +1032,13 @@ public void deleteVolumeQosPolicy(String policyUuid) { private VolumeQosPolicy getVolumeQosPolicyByUuid(String policyUuid) { Map queryParams = new HashMap<>(); - queryParams.put(OntapStorageConstants.UUID, policyUuid); queryParams.put(OntapStorageConstants.FIELDS, OntapStorageConstants.QOS_POLICY_OBJECT_COUNT_FIELDS); try { - OntapResponse response = qosFeignClient.getPolicies(getAuthHeader(), queryParams); - if (response == null || response.getRecords() == null || response.getRecords().isEmpty()) { + VolumeQosPolicy policy = qosFeignClient.getPolicy(getAuthHeader(), policyUuid, queryParams); + if (policy == null || StringUtils.isEmpty(policy.getUuid())) { return null; } - return response.getRecords().get(0); + return policy; } catch (FeignException e) { if (OntapStorageUtils.isOntapObjectNotFoundError(e)) { return null; 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 580acab66ba6..cca1982cdaf4 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 @@ -215,7 +215,8 @@ public void deleteCloudStackVolume(CloudStackVolume cloudstackVolume) { String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); Map queryParams = Map.of("allow_delete_while_mapped", "true"); try { - sanFeignClient.deleteLun(authHeader, cloudstackVolume.getLun().getUuid(), queryParams); + JobResponse response = sanFeignClient.deleteLun(authHeader, cloudstackVolume.getLun().getUuid(), queryParams); + pollJobIfPresent(response, "delete Lun [" + cloudstackVolume.getLun().getName() + "]"); } catch (FeignException feignEx) { if (feignEx.status() == 404) { logger.warn("deleteCloudStackVolume: Lun {} does not exist (status 404), skipping deletion", cloudstackVolume.getLun().getName()); 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 bb407013f6a7..6f68dd6c4604 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 @@ -109,7 +109,7 @@ public class OntapStorageConstants { public static final String QOS_POLICY_NAME_TO = "to_"; public static final String QOS_POLICY_NAME_IOPS = "iops_"; public static final String UUID = "uuid"; - public static final String QOS_POLICY_OBJECT_COUNT_FIELDS = "uuid,object_count"; + public static final String QOS_POLICY_OBJECT_COUNT_FIELDS = "uuid,name,object_count"; public static final String LOGICAL_UNIT_NUMBER = "logical_unit_number"; public static final String IGROUP_DOT_NAME = "igroup.name"; public static final String IGROUP_DOT_UUID = "igroup.uuid"; 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 d84e57872b00..bd86964ce51c 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 @@ -937,6 +937,7 @@ void testCreateAsync_VolumeClonedFromTemplate_ClonesWithoutGrowing() { assertEquals("template-lun-uuid", requestCaptor.getValue().getLun().getClone().getSource().getUuid()); verify(sanStrategy, never()).createCloudStackVolume(any()); verify(sanStrategy, never()).resizeCloudStackVolume(any(), anyLong()); + verify(sanStrategy, never()).updateCloudStackVolume(any()); verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.LUN_DOT_UUID), eq("cloned-lun-uuid"), eq(false)); } } @@ -1135,6 +1136,74 @@ void testCreateAsync_VolumeClonedFromTemplateNFS_ClonesFile() { assertEquals("template-uuid", requestCaptor.getValue().getFile().getPath()); assertEquals("volume-uuid", requestCaptor.getValue().getDestinationPath()); verify(nasStrategy, never()).resizeCloudStackVolume(any(), anyLong()); + verify(nasStrategy, never()).updateCloudStackVolume(any()); + } + } + + @Test + void testCreateAsync_VolumeClonedFromTemplate_RootWithIops_SetsQosOnLunCreate() { + stubVolumeCloneFromTemplate(5368709120L, 5368709120L); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + + Lun clonedLun = new Lun(); + clonedLun.setName("/vol/vol1/test_volume"); + clonedLun.setUuid("cloned-lun-uuid"); + CloudStackVolume cloned = new CloudStackVolume(); + cloned.setLun(clonedLun); + VolumeQosPolicy qosPolicy = qosPolicy("qos-root-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)); + verify(sanStrategy).cloneCloudStackVolume(argThat(request -> + request.getLun() != null && request.getLun().getQosPolicy() != null + && "qos-root-uuid".equals(request.getLun().getQosPolicy().getUuid()))); + verify(sanStrategy, never()).updateCloudStackVolume(any()); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-root-uuid"), eq(false)); + } + } + + @Test + void testCreateAsync_VolumeClonedFromTemplateNFS_RootWithIops_AttachesQosToFile() { + storagePoolDetails.put(OntapStorageConstants.PROTOCOL, ProtocolType.NFS3.name()); + storagePoolDetails.put(OntapStorageConstants.VOLUME_UUID, "flex-uuid"); + stubVolumeCloneFromTemplate(5368709120L, 5368709120L); + when(storagePool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(volumeInfo.getUuid()).thenReturn("volume-uuid"); + when(volumeInfo.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(volumeInfo.getMinIops()).thenReturn(100L); + when(volumeInfo.getMaxIops()).thenReturn(200L); + when(templatePoolRef.getInstallPath()).thenReturn("template-uuid"); + + CloudStackVolume cloned = new CloudStackVolume(); + VolumeQosPolicy qosPolicy = qosPolicy("qos-nfs-root-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)); + verify(nasStrategy).updateCloudStackVolume(argThat(request -> + request.getFile() != null && request.getFile().getQosPolicy() != null + && "qos-nfs-root-uuid".equals(request.getFile().getQosPolicy().getUuid()))); + verify(volumeDetailsDao).addDetail(eq(100L), eq(OntapStorageConstants.QOS_POLICY_UUID), + eq("qos-nfs-root-uuid"), eq(false)); } } 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 12058738ef73..5776ccdc81db 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 @@ -395,7 +395,7 @@ void testDeleteCloudStackVolume_Success() { utilityMock.when(() -> OntapStorageUtils.generateAuthHeader("admin", "password")) .thenReturn(authHeader); - doNothing().when(sanFeignClient).deleteLun(eq(authHeader), eq("lun-uuid-123"), anyMap()); + when(sanFeignClient.deleteLun(eq(authHeader), eq("lun-uuid-123"), anyMap())).thenReturn(null); // Execute unifiedSANStrategy.deleteCloudStackVolume(request);