From c68c74683bb9dd5e75238c45c937e94e0be45b9d Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 08:21:08 +0530 Subject: [PATCH 1/7] Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes --- .../OntapPrimaryDatastoreLifecycle.java | 30 ++-- .../storage/service/StorageStrategy.java | 139 +++++++++++------- 2 files changed, 102 insertions(+), 67 deletions(-) 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 55fb7c868c49..dde00ca25c11 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 @@ -42,6 +42,7 @@ import org.apache.cloudstack.storage.datastore.db.StoragePoolDetailsDao; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.datastore.lifecycle.BasePrimaryDataStoreLifeCycleImpl; +import org.apache.cloudstack.storage.feign.model.Aggregate; import org.apache.cloudstack.storage.feign.model.OntapStorage; import org.apache.cloudstack.storage.feign.model.Volume; import org.apache.cloudstack.storage.provider.StorageProviderFactory; @@ -147,10 +148,28 @@ public DataStore initialize(Map dsInfos) { if (storageStrategy.getResolvedSvmUuid() != null && !storageStrategy.getResolvedSvmUuid().isEmpty()) { details.put(OntapStorageConstants.SVM_UUID, storageStrategy.getResolvedSvmUuid()); } + Aggregate aggregate; + try { + aggregate = storageStrategy.chooseAggregate(capacityBytes); + } catch (Exception e) { + logger.error("Exception occurred while choosing aggregate for pool: " + storagePoolName, e); + throw new CloudRuntimeException("Failed to choose ONTAP aggregate for pool: " + storagePoolName + + ". Error: " + e.getMessage(), e); + } + + Pair lifResult; + try { + lifResult = storageStrategy.getNetworkInterface(aggregate); + } catch (Exception e) { + logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e); + throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e); + } + processDataLifSelection(lifResult, details, storagePoolName, zoneId, podId); + logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" + (capacityBytes / (1024 * 1024 * 1024)) + " GB)"); try { - Volume volume = storageStrategy.createStorageVolume(storagePoolName, capacityBytes); + Volume volume = storageStrategy.createStorageVolume(storagePoolName, capacityBytes, aggregate); if (volume == null) { logger.error("createStorageVolume returned null for volume: " + storagePoolName); throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName); @@ -162,15 +181,6 @@ public DataStore initialize(Map dsInfos) { logger.error("Exception occurred while creating ONTAP volume: " + storagePoolName, e); throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName + ". Error: " + e.getMessage(), e); } - - Pair lifResult; - try { - lifResult = storageStrategy.getNetworkInterface(); - } catch (Exception e) { - logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e); - throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e); - } - processDataLifSelection(lifResult, details, storagePoolName, zoneId, podId); } else { throw new CloudRuntimeException("ONTAP details validation failed, cannot create primary storage"); } 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 4b0e8e29aabf..65ccd1ae6caf 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 @@ -92,12 +92,6 @@ public abstract class StorageStrategy { protected OntapStorage storage; - /** - * Holds the node name of the aggregate chosen during createStorageVolume(). - * Used by getNetworkInterface() to prefer a LIF homed on the same node. - */ - private String chosenAggregateNode; - /** * Presents aggregate object for the unified storage, not eligible for disaggregated */ @@ -390,19 +384,16 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str // Common methods like create/delete etc., should be here /** - * Creates ONTAP Flex-Volume - * Eligible only for Unified ONTAP storage - * throw exception in case of disaggregated ONTAP storage + * Selects the best aggregate for a volume of the given size from candidates populated by + * {@link #connect(boolean)} with aggregate validation enabled. * - * @param volumeName the name of the volume to create - * @param size the size of the volume in bytes - * @return the created Volume object + *

Picks the online aggregate with the largest available block space that can fit + * {@code size}. The returned aggregate includes node information for LIF affinity.

+ * + * @param size requested volume size in bytes + * @return the chosen aggregate detail response */ - public Volume createStorageVolume(String volumeName, Long size) { - logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes"); - - this.chosenAggregateNode = null; - + public Aggregate chooseAggregate(Long size) { String svmName = storage.getSvmName(); if (aggregates == null || aggregates.isEmpty()) { logger.error("No aggregates available to create volume on SVM " + svmName); @@ -413,18 +404,6 @@ public Volume createStorageVolume(String volumeName, Long size) { } String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); - - // Generate the Create Volume Request - Volume volumeRequest = new Volume(); - Svm svm = new Svm(); - svm.setName(svmName); - Nas nas = new Nas(); - nas.setPath(OntapStorageConstants.SLASH + volumeName); - - volumeRequest.setName(volumeName); - volumeRequest.setSvm(svm); - - // Pick the best aggregate for this specific request (largest available, online, and sufficient space). long maxAvailableAggregateSpaceBytes = -1L; Aggregate aggrChosen = null; for (Aggregate aggr : aggregates) { @@ -468,13 +447,55 @@ public Volume createStorageVolume(String volumeName, Long size) { logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation."); throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume operations."); } - logger.info("Selected aggregate: " + aggrChosen.getName() + " for volume operations."); + if (aggrChosen.getNode() == null || aggrChosen.getNode().getName() == null + || aggrChosen.getNode().getName().isEmpty()) { + logger.error("Selected aggregate " + aggrChosen.getName() + " does not have a node name."); + throw new CloudRuntimeException("Selected aggregate " + aggrChosen.getName() + + " does not have a node name required for LIF affinity."); + } + logger.info("Selected aggregate: " + aggrChosen.getName() + " on node " + + aggrChosen.getNode().getName() + " for volume operations."); + return aggrChosen; + } - this.chosenAggregateNode = aggrChosen.getNode() != null ? aggrChosen.getNode().getName() : null; + /** + * Creates ONTAP Flex-Volume on the given aggregate. + * Eligible only for Unified ONTAP storage + * throw exception in case of disaggregated ONTAP storage + * + * @param volumeName the name of the volume to create + * @param size the size of the volume in bytes + * @param aggregate the aggregate previously selected via {@link #chooseAggregate(Long)} + * @return the created Volume object + */ + public Volume createStorageVolume(String volumeName, Long size, Aggregate aggregate) { + logger.info("Creating volume: " + volumeName + " of size: " + size + " bytes"); + + String svmName = storage.getSvmName(); + if (size == null || size <= 0) { + throw new CloudRuntimeException("Invalid volume size provided: " + size); + } + if (aggregate == null || aggregate.getName() == null || aggregate.getUuid() == null) { + throw new CloudRuntimeException("Aggregate is required to create volume on SVM " + svmName); + } + + String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); + + // Generate the Create Volume Request + Volume volumeRequest = new Volume(); + Svm svm = new Svm(); + svm.setName(svmName); + Nas nas = new Nas(); + nas.setPath(OntapStorageConstants.SLASH + volumeName); + + volumeRequest.setName(volumeName); + volumeRequest.setSvm(svm); + + logger.info("Creating volume on aggregate: " + aggregate.getName() + " for volume operations."); Aggregate aggr = new Aggregate(); - aggr.setName(aggrChosen.getName()); - aggr.setUuid(aggrChosen.getUuid()); + aggr.setName(aggregate.getName()); + aggr.setUuid(aggregate.getUuid()); volumeRequest.setAggregates(List.of(aggr)); volumeRequest.setSize(size); volumeRequest.setNas(nas); @@ -668,19 +689,26 @@ public String getStoragePath() { /** * Selects the best available data LIF for storage I/O, preferring one homed on the same node - * as the chosen aggregate to avoid inter-node traffic. + * as the given aggregate to avoid inter-node traffic. * *

Selection order:

*
    - *
  1. LIF whose {@code location.home_node} matches the chosen aggregate's node — no warning
    2. + *
  2. LIF whose {@code location.home_node} matches the aggregate's node — no warning
    3. *
  3. LIF currently running on that node (e.g. after failover) — returned with a warning
    4. - *
  4. Any UP and enabled LIF — returned with a warning when aggregate node is known
    5. + *
  5. Any UP and enabled LIF — returned with a warning
    6. *
* + * @param aggregate the aggregate previously selected via {@link #chooseAggregate(Long)}; + * must include a node name for LIF affinity * @return {@link Pair} where {@code first()} is the LIF's IP address and {@code second()} is * a warning message (null when no warning) */ - public Pair getNetworkInterface() { + public Pair getNetworkInterface(Aggregate aggregate) { + if (aggregate == null || aggregate.getNode() == null || aggregate.getNode().getName() == null + || aggregate.getNode().getName().isEmpty()) { + throw new CloudRuntimeException("Aggregate with a node name is required to select a network interface"); + } + String aggregateNode = aggregate.getNode().getName(); String authHeader = OntapStorageUtils.generateAuthHeader(storage.getUsername(), storage.getPassword()); try { Map queryParams = new HashMap<>(); @@ -722,21 +750,19 @@ public Pair getNetworkInterface() { if (!isIPv4Address(iface.getIp().getAddress())) { continue; } - if (chosenAggregateNode != null) { - // LIF is homed on the aggregate's node - String homeNode = iface.getLocation() != null && iface.getLocation().getHomeNode() != null - ? iface.getLocation().getHomeNode().getName() : null; - if (chosenAggregateNode.equals(homeNode)) { - return new Pair<>(iface.getIp().getAddress(), null); - } - // LIF has failed over and is currently running on the aggregate's node - // (home_node differs). Keep as a candidate; returned with a warning if no match is found earlier. - if (currentNodeInterface == null) { - String currentNode = iface.getLocation() != null && iface.getLocation().getNode() != null - ? iface.getLocation().getNode().getName() : null; - if (chosenAggregateNode.equals(currentNode)) { - currentNodeInterface = iface; - } + // LIF is homed on the aggregate's node + String homeNode = iface.getLocation() != null && iface.getLocation().getHomeNode() != null + ? iface.getLocation().getHomeNode().getName() : null; + if (aggregateNode.equals(homeNode)) { + return new Pair<>(iface.getIp().getAddress(), null); + } + // LIF has failed over and is currently running on the aggregate's node + // (home_node differs). Keep as a candidate; returned with a warning if no match is found earlier. + if (currentNodeInterface == null) { + String currentNode = iface.getLocation() != null && iface.getLocation().getNode() != null + ? iface.getLocation().getNode().getName() : null; + if (aggregateNode.equals(currentNode)) { + currentNodeInterface = iface; } } if (fallbackInterface == null) { @@ -752,21 +778,20 @@ public Pair getNetworkInterface() { if (currentNodeInterface != null) { String ip = currentNodeInterface.getIp().getAddress(); - String warning = "No home-node LIF found for aggregate node '" + chosenAggregateNode + String warning = "No home-node LIF found for aggregate node '" + aggregateNode + "'; using LIF '" + ip + "' currently running on that node (home node LIF may be down)."; logger.warn(warning); return new Pair<>(ip, warning); } String ip = fallbackInterface.getIp().getAddress(); - if (chosenAggregateNode == null) { - return new Pair<>(ip, null); - } - String warning = "No operational LIF found on aggregate's home node '" + chosenAggregateNode + String warning = "No operational LIF found on aggregate's home node '" + aggregateNode + "'; using fallback LIF '" + ip + "' on a different node." + " I/O will traverse an inter-node path, increasing latency."; logger.warn(warning); return new Pair<>(ip, warning); + } catch (CloudRuntimeException e) { + throw e; } catch (Exception e) { logger.error("Exception while retrieving network interfaces: ", e); throw new CloudRuntimeException("Failed to retrieve network interfaces: " + e.getMessage()); From a888647af41260da164a4d08d49a25dcba65a518 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 10:38:18 +0530 Subject: [PATCH 2/7] Added missing UTs # Conflicts: # plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java --- .../OntapPrimaryDatastoreLifecycleTest.java | 38 ++- .../storage/service/StorageStrategyTest.java | 285 ++++++++++-------- 2 files changed, 194 insertions(+), 129 deletions(-) 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 12ff93d71c11..feccf604e62a 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 @@ -31,6 +31,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; +import org.apache.cloudstack.storage.feign.model.Aggregate; import org.apache.cloudstack.storage.feign.model.Volume; import com.cloud.dc.dao.ClusterDao; import com.cloud.exception.InvalidParameterValueException; @@ -60,9 +61,12 @@ import static org.mockito.Mockito.when; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.times; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.withSettings; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import org.mockito.InOrder; import static org.mockito.ArgumentMatchers.contains; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -136,13 +140,20 @@ void setUp() { when(_clusterDao.findById(1L)).thenReturn(clusterVO); when(storageStrategy.connect()).thenReturn(true); + Aggregate aggregate = new Aggregate(); + aggregate.setName("aggr1"); + aggregate.setUuid("aggr-uuid-1"); + Aggregate.Node node = new Aggregate.Node(); + node.setName("node-a"); + aggregate.setNode(node); when(storageStrategy.isAff()).thenReturn(true); - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>("testNetworkInterface", null)); + when(storageStrategy.chooseAggregate(any())).thenReturn(aggregate); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("testNetworkInterface", null)); Volume volume = new Volume(); volume.setUuid("test-volume-uuid"); volume.setName("testVolume"); - when(storageStrategy.createStorageVolume(any(), any())).thenReturn(volume); + when(storageStrategy.createStorageVolume(any(), any(), any())).thenReturn(volume); // Setup for attachCluster tests // Configure dataStore mock with necessary methods (works for both DataStore and PrimaryDataStoreInfo) @@ -586,7 +597,7 @@ public void testInitialize_dataLifWithWarning() { dsInfos.put("details", detailsMap); String warningMessage = "LIF on node-b; expected on node-a;Details about LIF failover"; - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>("10.0.0.1", warningMessage)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("10.0.0.1", warningMessage)); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class); MockedStatic utilityMock = Mockito.mockStatic(OntapStorageUtils.class)) { @@ -621,12 +632,13 @@ public void testInitialize_nullDataLif() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>(null, null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(null, null)); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); Exception ex = assertThrows(CloudRuntimeException.class, () -> ontapPrimaryDatastoreLifecycle.initialize(dsInfos)); assertTrue(ex.getMessage().contains("Failed to retrieve Data LIF from ONTAP, cannot create primary storage")); + verify(storageStrategy, never()).createStorageVolume(any(), any(), any()); } } @@ -652,12 +664,13 @@ public void testInitialize_emptyDataLif() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>("", null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("", null)); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); Exception ex = assertThrows(CloudRuntimeException.class, () -> ontapPrimaryDatastoreLifecycle.initialize(dsInfos)); assertTrue(ex.getMessage().contains("Failed to retrieve Data LIF from ONTAP, cannot create primary storage")); + verify(storageStrategy, never()).createStorageVolume(any(), any(), any()); } } @@ -683,13 +696,14 @@ public void testInitialize_getNetworkInterfaceException() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.getNetworkInterface()).thenThrow(new RuntimeException("ONTAP API error")); + when(storageStrategy.getNetworkInterface(any())).thenThrow(new RuntimeException("ONTAP API error")); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); Exception ex = assertThrows(CloudRuntimeException.class, () -> ontapPrimaryDatastoreLifecycle.initialize(dsInfos)); assertTrue(ex.getMessage().contains("Failed to retrieve Data LIF from ONTAP")); assertTrue(ex.getCause() != null && ex.getCause().getMessage().contains("ONTAP API error")); + verify(storageStrategy, never()).createStorageVolume(any(), any(), any()); } } @@ -715,7 +729,7 @@ public void testInitialize_volumeCreationFailure_nullVolume() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.createStorageVolume(any(), any())).thenReturn(null); + when(storageStrategy.createStorageVolume(any(), any(), any())).thenReturn(null); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); @@ -746,7 +760,7 @@ public void testInitialize_volumeCreationException() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.createStorageVolume(any(), any())).thenThrow(new RuntimeException("Volume creation failed")); + when(storageStrategy.createStorageVolume(any(), any(), any())).thenThrow(new RuntimeException("Volume creation failed")); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); @@ -779,13 +793,19 @@ public void testInitialize_positiveWithDetailAssertions() { dsInfos.put("details", detailsMap); String expectedDataLif = "192.168.1.100"; - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>(expectedDataLif, null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(expectedDataLif, null)); when(storageStrategy.getStoragePath()).thenReturn("/vol/testVolume"); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); ontapPrimaryDatastoreLifecycle.initialize(dsInfos); + // Verify LIF selection completes before FlexVol creation + InOrder inOrder = inOrder(storageStrategy); + inOrder.verify(storageStrategy).chooseAggregate(any()); + inOrder.verify(storageStrategy).getNetworkInterface(any()); + inOrder.verify(storageStrategy).createStorageVolume(any(), any(), any()); + // Verify that createPrimaryDataStore was called and host parameter contains the DATA_LIF verify(_dataStoreHelper, times(1)).createPrimaryDataStore(any()); } 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 db5f0a33b307..f7342d645320 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 @@ -597,19 +597,105 @@ public void testConnect_invalidCredentials() { "Expected the message to prompt verifying username/password but got: " + ex.getMessage()); } - // ========== createStorageVolume() Tests ========== + // ========== chooseAggregate() Tests ========== @Test - public void testCreateStorageVolume_positive() { - // Setup - First connect to populate aggregates + public void testChooseAggregate_positive() { + setupSuccessfulConnect(); + storageStrategy.connect(); + + Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) + .thenReturn(aggregateDetail); + + Aggregate result = storageStrategy.chooseAggregate(5000000000L); + + assertNotNull(result); + assertEquals("aggr1", result.getName()); + assertEquals("aggr-uuid-1", result.getUuid()); + assertEquals("node-a", result.getNode().getName()); + } + + @Test + public void testChooseAggregate_invalidSize() { + setupSuccessfulConnect(); + storageStrategy.connect(); + + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(-1L)); + assertTrue(ex.getMessage().contains("Invalid volume size")); + } + + @Test + public void testChooseAggregate_nullSize() { + setupSuccessfulConnect(); + storageStrategy.connect(); + + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(null)); + assertTrue(ex.getMessage().contains("Invalid volume size")); + } + + @Test + public void testChooseAggregate_noAggregates() { + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(5000000000L)); + assertTrue(ex.getMessage().contains("No aggregates available")); + } + + @Test + public void testChooseAggregate_aggregateNotOnline() { + setupSuccessfulConnect(); + storageStrategy.connect(); + + Aggregate aggregateDetail = new Aggregate(); + aggregateDetail.setName("aggr1"); + aggregateDetail.setUuid("aggr-uuid-1"); + aggregateDetail.setState(null); + + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) + .thenReturn(aggregateDetail); + + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(5000000000L)); + assertTrue(ex.getMessage().contains("No suitable aggregates found")); + } + + @Test + public void testChooseAggregate_insufficientSpace() { + setupSuccessfulConnect(); + storageStrategy.connect(); + + Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 1000000.0, "node-a"); + + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) + .thenReturn(aggregateDetail); + + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(5000000000L)); + assertTrue(ex.getMessage().contains("No suitable aggregates found")); + } + + @Test + public void testChooseAggregate_missingNode() { setupSuccessfulConnect(); storageStrategy.connect(); - // Setup aggregate details Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0); when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) .thenReturn(aggregateDetail); + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(5000000000L)); + assertTrue(ex.getMessage().contains("does not have a node name")); + } + + // ========== createStorageVolume() Tests ========== + + @Test + public void testCreateStorageVolume_positive() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // Setup job response Job job = new Job(); job.setUuid("job-uuid-1"); @@ -639,7 +725,7 @@ public void testCreateStorageVolume_positive() { .thenReturn(volumeResponse); // Execute - Volume result = storageStrategy.createStorageVolume("test-volume", 5000000000L); + Volume result = storageStrategy.createStorageVolume("test-volume", 5000000000L, aggregate); // Verify assertNotNull(result); @@ -651,80 +737,32 @@ public void testCreateStorageVolume_positive() { @Test public void testCreateStorageVolume_invalidSize() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); - // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", -1L)); + () -> storageStrategy.createStorageVolume("test-volume", -1L, aggregate)); assertTrue(ex.getMessage().contains("Invalid volume size")); } @Test public void testCreateStorageVolume_nullSize() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); - // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", null)); + () -> storageStrategy.createStorageVolume("test-volume", null, aggregate)); assertTrue(ex.getMessage().contains("Invalid volume size")); } @Test - public void testCreateStorageVolume_noAggregates() { - // Execute & Verify - without calling connect first + public void testCreateStorageVolume_nullAggregate() { Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", 5000000000L)); - assertTrue(ex.getMessage().contains("No aggregates available")); - } - - @Test - public void testCreateStorageVolume_aggregateNotOnline() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); - - Aggregate aggregateDetail = new Aggregate(); - aggregateDetail.setName("aggr1"); - aggregateDetail.setUuid("aggr-uuid-1"); - aggregateDetail.setState(null); // null state to simulate offline - - when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) - .thenReturn(aggregateDetail); - - // Execute & Verify - Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", 5000000000L)); - assertTrue(ex.getMessage().contains("No suitable aggregates found")); - } - - @Test - public void testCreateStorageVolume_insufficientSpace() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); - - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 1000000.0); // Only 1MB available - - when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) - .thenReturn(aggregateDetail); - - // Execute & Verify - Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", 5000000000L)); // Request 5GB - assertTrue(ex.getMessage().contains("No suitable aggregates found")); + () -> storageStrategy.createStorageVolume("test-volume", 5000000000L, null)); + assertTrue(ex.getMessage().contains("Aggregate is required")); } @Test public void testCreateStorageVolume_jobFailed() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); - - setupAggregateForVolumeCreation(); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); Job job = new Job(); job.setUuid("job-uuid-1"); @@ -742,18 +780,14 @@ public void testCreateStorageVolume_jobFailed() { when(jobFeignClient.getJobByUUID(anyString(), eq("job-uuid-1"))) .thenReturn(failedJob); - // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", 5000000000L)); + () -> storageStrategy.createStorageVolume("test-volume", 5000000000L, aggregate)); assertTrue(ex.getMessage().contains("failed") || ex.getMessage().contains("Job failed")); } @Test public void testCreateStorageVolume_volumeNotFoundAfterCreation() { - // Setup - setupSuccessfulConnect(); - storageStrategy.connect(); - setupAggregateForVolumeCreation(); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); setupSuccessfulJobCreation(); // Setup empty volume response @@ -763,9 +797,8 @@ public void testCreateStorageVolume_volumeNotFoundAfterCreation() { when(volumeFeignClient.getAllVolumes(anyString(), anyMap())) .thenReturn(emptyResponse); - // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.createStorageVolume("test-volume", 5000000000L)); + () -> storageStrategy.createStorageVolume("test-volume", 5000000000L, aggregate)); assertTrue(ex.getMessage() != null && ex.getMessage().contains("not found after creation")); } @@ -949,6 +982,8 @@ public void testGetStoragePath_iscsi_noTargetIqn() { @Test public void testGetNetworkInterface_nfs() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // Setup IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.50"); @@ -957,6 +992,12 @@ public void testGetNetworkInterface_nfs() { ipInterface.setIp(ipInfo); ipInterface.setState(OntapStorageConstants.LIF_STATE_UP); ipInterface.setEnabled(true); + IpInterface.Node homeNode = new IpInterface.Node(); + homeNode.setName("node-a"); + IpInterface.Location location = new IpInterface.Location(); + location.setHomeNode(homeNode); + location.setNode(homeNode); + ipInterface.setLocation(location); OntapResponse interfaceResponse = new OntapResponse<>(); interfaceResponse.setRecords(List.of(ipInterface)); @@ -965,7 +1006,7 @@ public void testGetNetworkInterface_nfs() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -984,6 +1025,8 @@ public void testGetNetworkInterface_iscsi() { jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient, clusterFeignClient); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.51"); @@ -991,6 +1034,12 @@ public void testGetNetworkInterface_iscsi() { ipInterface.setIp(ipInfo); ipInterface.setState(OntapStorageConstants.LIF_STATE_UP); ipInterface.setEnabled(true); + IpInterface.Node homeNode = new IpInterface.Node(); + homeNode.setName("node-a"); + IpInterface.Location location = new IpInterface.Location(); + location.setHomeNode(homeNode); + location.setNode(homeNode); + ipInterface.setLocation(location); OntapResponse interfaceResponse = new OntapResponse<>(); interfaceResponse.setRecords(List.of(ipInterface)); @@ -999,7 +1048,7 @@ public void testGetNetworkInterface_iscsi() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -1009,6 +1058,8 @@ public void testGetNetworkInterface_iscsi() { @Test public void testGetNetworkInterface_nfs_lifDown() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // LIF exists but is operationally down — should fail IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.50"); @@ -1025,12 +1076,14 @@ public void testGetNetworkInterface_nfs_lifDown() { .thenReturn(interfaceResponse); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.getNetworkInterface()); + () -> storageStrategy.getNetworkInterface(aggregate)); assertTrue(ex.getMessage().contains("operationally UP and enabled")); } @Test public void testGetNetworkInterface_nfs_lifDisabled() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // LIF exists but is administratively disabled — should fail IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.50"); @@ -1047,7 +1100,7 @@ public void testGetNetworkInterface_nfs_lifDisabled() { .thenReturn(interfaceResponse); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.getNetworkInterface()); + () -> storageStrategy.getNetworkInterface(aggregate)); assertTrue(ex.getMessage().contains("operationally UP and enabled")); } @@ -1061,6 +1114,8 @@ public void testGetNetworkInterface_iscsi_lifDown() { jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient, clusterFeignClient); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.51"); @@ -1076,12 +1131,14 @@ public void testGetNetworkInterface_iscsi_lifDown() { .thenReturn(interfaceResponse); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.getNetworkInterface()); + () -> storageStrategy.getNetworkInterface(aggregate)); assertTrue(ex.getMessage().contains("operationally UP and enabled")); } @Test public void testGetNetworkInterface_noInterfaces() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // Setup OntapResponse emptyResponse = new OntapResponse<>(); emptyResponse.setRecords(new ArrayList<>()); @@ -1091,12 +1148,14 @@ public void testGetNetworkInterface_noInterfaces() { // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.getNetworkInterface()); + () -> storageStrategy.getNetworkInterface(aggregate)); assertTrue(ex.getMessage().contains("No network interfaces found")); } @Test public void testGetNetworkInterface_feignException() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + // Setup Map> emptyHeaders = Collections.emptyMap(); Request dummyReq = Request.create(Request.HttpMethod.GET, "http://test", emptyHeaders, (byte[]) null, (Charset) null); @@ -1105,7 +1164,7 @@ public void testGetNetworkInterface_feignException() { // Execute & Verify Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.getNetworkInterface()); + () -> storageStrategy.getNetworkInterface(aggregate)); assertTrue(ex.getMessage().contains("Failed to retrieve network interfaces")); } @@ -1116,13 +1175,13 @@ public void testGetNetworkInterface_feignException() { */ @Test public void testGetNetworkInterface_nfs_tier1_homeNodeMatch() { - injectChosenAggregateNode(storageStrategy, "node-a"); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); IpInterface lif = buildLif("10.0.0.1", OntapStorageConstants.LIF_STATE_UP, true, "node-a", "node-a"); when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); assertEquals("10.0.0.1", result.first()); assertTrue(result.second() == null, "Tier 1 should produce no warning"); @@ -1134,14 +1193,14 @@ public void testGetNetworkInterface_nfs_tier1_homeNodeMatch() { */ @Test public void testGetNetworkInterface_nfs_tier2_currentNodeMatch() { - injectChosenAggregateNode(storageStrategy, "node-a"); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); // home node = node-b, currently running on node-a after failover IpInterface lif = buildLif("10.0.0.2", OntapStorageConstants.LIF_STATE_UP, true, "node-b", "node-a"); when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); assertEquals("10.0.0.2", result.first()); assertTrue(result.second() != null, "Tier 2 should produce a warning"); @@ -1155,14 +1214,14 @@ public void testGetNetworkInterface_nfs_tier2_currentNodeMatch() { */ @Test public void testGetNetworkInterface_nfs_tier3_crossNodeFallback() { - injectChosenAggregateNode(storageStrategy, "node-a"); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); // Both home_node and current node are node-b — no affinity to node-a IpInterface lif = buildLif("10.0.0.3", OntapStorageConstants.LIF_STATE_UP, true, "node-b", "node-b"); when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); assertEquals("10.0.0.3", result.first()); assertTrue(result.second() != null, "Tier 3 fallback should produce a warning"); @@ -1173,24 +1232,22 @@ public void testGetNetworkInterface_nfs_tier3_crossNodeFallback() { } /** - * When chosenAggregateNode is null (volume not yet created / no aggregate info), - * any UP/enabled LIF is returned without warning. + * Null aggregate or missing node fails clearly — no silent unaffined LIF selection. */ @Test - public void testGetNetworkInterface_nfs_noAggregateNode_noWarning() { - // chosenAggregateNode is null by default — no node affinity context - IpInterface lif = buildLif("10.0.0.4", OntapStorageConstants.LIF_STATE_UP, true, "node-a", "node-a"); - when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) - .thenReturn(wrapLifs(List.of(lif))); + public void testGetNetworkInterface_nullAggregate_fails() { + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.getNetworkInterface(null)); + assertTrue(ex.getMessage().contains("Aggregate with a node name is required")); + } - Pair result = storageStrategy.getNetworkInterface(); + @Test + public void testGetNetworkInterface_missingNode_fails() { + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0); - assertEquals("10.0.0.4", result.first()); - // With no chosenAggregateNode, tier 1/2 selection is skipped — result falls through to tier 3 - // but since there's no "expected node" in the warning message (chosenAggregateNode is null), - // the message text will still contain "null" — we simply verify no exception is thrown and IP is correct. - // (Tier 3 warning is generated when chosenAggregateNode != null; here it is null so no warning) - assertTrue(result.second() == null, "No warning when chosenAggregateNode is null"); + Exception ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.getNetworkInterface(aggregate)); + assertTrue(ex.getMessage().contains("Aggregate with a node name is required")); } /** @@ -1198,7 +1255,7 @@ public void testGetNetworkInterface_nfs_noAggregateNode_noWarning() { */ @Test public void testGetNetworkInterface_nfs_tier1Down_tier2Used() { - injectChosenAggregateNode(storageStrategy, "node-a"); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); // Tier 1 candidate: home_node = node-a but operationally DOWN IpInterface lifDown = buildLif("10.0.0.5", "down", true, "node-a", "node-a"); @@ -1208,7 +1265,7 @@ public void testGetNetworkInterface_nfs_tier1Down_tier2Used() { when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lifDown, lifFailover))); - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); assertEquals("10.0.0.6", result.first()); assertTrue(result.second() != null, "Should warn that the home-node LIF is not in use"); @@ -1232,16 +1289,10 @@ private void setupSuccessfulConnect() { when(svmFeignClient.getSvmResponse(anyMap(), anyString())).thenReturn(svmResponse); - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0); + Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())).thenReturn(aggregateDetail); } - private void setupAggregateForVolumeCreation() { - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0); - when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) - .thenReturn(aggregateDetail); - } - private void setupSuccessfulJobCreation() { Job job = new Job(); job.setUuid("job-uuid-1"); @@ -1269,21 +1320,6 @@ private void setupSuccessfulJobCreation() { .thenReturn(volumeResponse); } - /** - * Injects a value into the private {@code chosenAggregateNode} field of StorageStrategy - * so node-affinity tests can exercise all three selection tiers without having to drive - * the full {@code createStorageVolume()} flow. - */ - private static void injectChosenAggregateNode(StorageStrategy strategy, String nodeName) { - try { - Field field = StorageStrategy.class.getDeclaredField("chosenAggregateNode"); - field.setAccessible(true); - field.set(strategy, nodeName); - } catch (NoSuchFieldException | IllegalAccessException e) { - throw new RuntimeException("Failed to inject chosenAggregateNode", e); - } - } - /** * Builds an {@link IpInterface} with all node-affinity fields populated. * @@ -1327,6 +1363,10 @@ private static OntapResponse wrapLifs(List lifs) { * {@code mock(Aggregate.class)} which fails on JDK 26+ due to Byte Buddy limitations. */ private static Aggregate buildAggregate(String name, String uuid, double availableBytes) { + return buildAggregate(name, uuid, availableBytes, null); + } + + private static Aggregate buildAggregate(String name, String uuid, double availableBytes, String nodeName) { Aggregate.AggregateSpaceBlockStorage blockStorage = new Aggregate.AggregateSpaceBlockStorage(); blockStorage.setAvailable(availableBytes); @@ -1338,6 +1378,11 @@ private static Aggregate buildAggregate(String name, String uuid, double availab agg.setUuid(uuid); agg.setState(Aggregate.StateEnum.ONLINE); agg.setSpace(space); + if (nodeName != null) { + Aggregate.Node node = new Aggregate.Node(); + node.setName(nodeName); + agg.setNode(node); + } return agg; } From 457eb4838f2a3e17f0188ada740df90293e7b68c Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 12 Aug 2026 11:46:58 +0530 Subject: [PATCH 3/7] Addressed review comments --- .../OntapPrimaryDatastoreLifecycle.java | 13 +++++------ .../storage/service/StorageStrategy.java | 22 +++++++++++++------ 2 files changed, 21 insertions(+), 14 deletions(-) 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 dde00ca25c11..15146d8dc75d 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 @@ -69,7 +69,6 @@ import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; import com.cloud.storage.StoragePoolAutomation; -import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; import com.google.common.base.Preconditions; @@ -157,14 +156,16 @@ public DataStore initialize(Map dsInfos) { + ". Error: " + e.getMessage(), e); } - Pair lifResult; + Map lifResult; try { lifResult = storageStrategy.getNetworkInterface(aggregate); } catch (Exception e) { logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e); throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP: " + e.getMessage(), e); } - processDataLifSelection(lifResult, details, storagePoolName, zoneId, podId); + String dataLif = lifResult.get(OntapStorageConstants.DATA_LIF); + String lifWarning = lifResult.get(OntapStorageConstants.LIF_WARNING); + processDataLifSelection(dataLif, lifWarning, details, storagePoolName, zoneId, podId); logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" + (capacityBytes / (1024 * 1024 * 1024)) + " GB)"); @@ -299,9 +300,8 @@ private void validateInitializeInputs(Long capacityBytes, Long capacityIops, Lon } } - private void processDataLifSelection(Pair lifResult, Map details, + private void processDataLifSelection(String dataLIF, String lifWarning, Map details, String storagePoolName, Long zoneId, Long podId) { - String dataLIF = lifResult.first(); if (dataLIF == null || dataLIF.isEmpty()) { throw new CloudRuntimeException("Failed to retrieve Data LIF from ONTAP, cannot create primary storage"); } @@ -309,8 +309,7 @@ private void processDataLifSelection(Pair lifResult, Map getNetworkInterface(Aggregate aggregate) { + public Map getNetworkInterface(Aggregate aggregate) { if (aggregate == null || aggregate.getNode() == null || aggregate.getNode().getName() == null || aggregate.getNode().getName().isEmpty()) { throw new CloudRuntimeException("Aggregate with a node name is required to select a network interface"); @@ -754,7 +753,7 @@ public Pair getNetworkInterface(Aggregate aggregate) { String homeNode = iface.getLocation() != null && iface.getLocation().getHomeNode() != null ? iface.getLocation().getHomeNode().getName() : null; if (aggregateNode.equals(homeNode)) { - return new Pair<>(iface.getIp().getAddress(), null); + return networkInterfaceResult(iface.getIp().getAddress(), null); } // LIF has failed over and is currently running on the aggregate's node // (home_node differs). Keep as a candidate; returned with a warning if no match is found earlier. @@ -781,7 +780,7 @@ public Pair getNetworkInterface(Aggregate aggregate) { String warning = "No home-node LIF found for aggregate node '" + aggregateNode + "'; using LIF '" + ip + "' currently running on that node (home node LIF may be down)."; logger.warn(warning); - return new Pair<>(ip, warning); + return networkInterfaceResult(ip, warning); } String ip = fallbackInterface.getIp().getAddress(); @@ -789,7 +788,7 @@ public Pair getNetworkInterface(Aggregate aggregate) { + "'; using fallback LIF '" + ip + "' on a different node." + " I/O will traverse an inter-node path, increasing latency."; logger.warn(warning); - return new Pair<>(ip, warning); + return networkInterfaceResult(ip, warning); } catch (CloudRuntimeException e) { throw e; } catch (Exception e) { @@ -798,6 +797,15 @@ public Pair getNetworkInterface(Aggregate aggregate) { } } + private Map networkInterfaceResult(String address, String warning) { + Map result = new HashMap<>(); + result.put(OntapStorageConstants.DATA_LIF, address); + if (warning != null) { + result.put(OntapStorageConstants.LIF_WARNING, warning); + } + return result; + } + /** * Returns true if the given IP address string is an IPv4 address. * IPv6 addresses contain colons; IPv4 addresses do not. From c767c30e07053d7e7108addec0078a7094ff9df7 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 12 Aug 2026 14:47:29 +0530 Subject: [PATCH 4/7] Committed missed test files --- .../OntapPrimaryDatastoreLifecycleTest.java | 23 ++++++---- .../storage/service/StorageStrategyTest.java | 46 ++++++++++--------- 2 files changed, 38 insertions(+), 31 deletions(-) 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 feccf604e62a..cc886cf8d43f 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 @@ -54,7 +54,7 @@ import java.util.Map; import java.util.List; import java.util.ArrayList; -import com.cloud.utils.Pair; +import java.util.HashMap; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; @@ -148,7 +148,8 @@ void setUp() { aggregate.setNode(node); when(storageStrategy.isAff()).thenReturn(true); when(storageStrategy.chooseAggregate(any())).thenReturn(aggregate); - when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("testNetworkInterface", null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn( + Map.of(OntapStorageConstants.DATA_LIF, "testNetworkInterface")); Volume volume = new Volume(); volume.setUuid("test-volume-uuid"); @@ -575,7 +576,7 @@ public void testInitialize_unexpectedDetailKey() { @Test public void testInitialize_dataLifWithWarning() { - // Test when getNetworkInterface returns a warning in the Pair's second value + // Test when getNetworkInterface returns a warning in LIF_WARNING // This exercises the processDataLifSelection path for non-null warning HashMap detailsMap = new HashMap<>(); detailsMap.put(OntapStorageConstants.USERNAME, "testUser"); @@ -597,7 +598,9 @@ public void testInitialize_dataLifWithWarning() { dsInfos.put("details", detailsMap); String warningMessage = "LIF on node-b; expected on node-a;Details about LIF failover"; - when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("10.0.0.1", warningMessage)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(Map.of( + OntapStorageConstants.DATA_LIF, "10.0.0.1", + OntapStorageConstants.LIF_WARNING, warningMessage)); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class); MockedStatic utilityMock = Mockito.mockStatic(OntapStorageUtils.class)) { @@ -612,7 +615,7 @@ public void testInitialize_dataLifWithWarning() { @Test public void testInitialize_nullDataLif() { - // Test when lifResult.first() returns null + // Test when DATA_LIF is missing from the result map HashMap detailsMap = new HashMap<>(); detailsMap.put(OntapStorageConstants.USERNAME, "testUser"); detailsMap.put(OntapStorageConstants.PASSWORD, "testPassword"); @@ -632,7 +635,7 @@ public void testInitialize_nullDataLif() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(null, null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn(new HashMap<>()); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); @@ -644,7 +647,7 @@ public void testInitialize_nullDataLif() { @Test public void testInitialize_emptyDataLif() { - // Test when lifResult.first() returns empty string + // Test when DATA_LIF is an empty string HashMap detailsMap = new HashMap<>(); detailsMap.put(OntapStorageConstants.USERNAME, "testUser"); detailsMap.put(OntapStorageConstants.PASSWORD, "testPassword"); @@ -664,7 +667,8 @@ public void testInitialize_emptyDataLif() { dsInfos.put("isTagARule", false); dsInfos.put("details", detailsMap); - when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("", null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn( + Map.of(OntapStorageConstants.DATA_LIF, "")); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { storageProviderFactory.when(() -> StorageProviderFactory.getStrategy(any())).thenReturn(storageStrategy); @@ -793,7 +797,8 @@ public void testInitialize_positiveWithDetailAssertions() { dsInfos.put("details", detailsMap); String expectedDataLif = "192.168.1.100"; - when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(expectedDataLif, null)); + when(storageStrategy.getNetworkInterface(any())).thenReturn( + Map.of(OntapStorageConstants.DATA_LIF, expectedDataLif)); when(storageStrategy.getStoragePath()).thenReturn("/vol/testVolume"); try (MockedStatic storageProviderFactory = Mockito.mockStatic(StorageProviderFactory.class)) { 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 f7342d645320..fe4f6ccbd69d 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 @@ -73,7 +73,6 @@ import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; -import com.cloud.utils.Pair; import com.cloud.utils.exception.CloudRuntimeException; import feign.FeignException; @@ -1006,12 +1005,13 @@ public void testGetNetworkInterface_nfs() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); - assertEquals("192.168.1.50", result.first()); - assertTrue(result.second() == null, "Expect no warning when a suitable LIF is found"); + assertEquals("192.168.1.50", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) == null, + "Expect no warning when a suitable LIF is found"); verify(networkFeignClient, times(1)).getNetworkIpInterfaces(anyString(), anyMap()); } @@ -1048,12 +1048,13 @@ public void testGetNetworkInterface_iscsi() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); - assertEquals("192.168.1.51", result.first()); - assertTrue(result.second() == null, "Expect no warning when a suitable LIF is found"); + assertEquals("192.168.1.51", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) == null, + "Expect no warning when a suitable LIF is found"); } @Test @@ -1181,10 +1182,10 @@ public void testGetNetworkInterface_nfs_tier1_homeNodeMatch() { when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); - assertEquals("10.0.0.1", result.first()); - assertTrue(result.second() == null, "Tier 1 should produce no warning"); + assertEquals("10.0.0.1", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) == null, "Tier 1 should produce no warning"); } /** @@ -1200,11 +1201,11 @@ public void testGetNetworkInterface_nfs_tier2_currentNodeMatch() { when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); - assertEquals("10.0.0.2", result.first()); - assertTrue(result.second() != null, "Tier 2 should produce a warning"); - assertTrue(result.second().contains("node-a")); + assertEquals("10.0.0.2", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) != null, "Tier 2 should produce a warning"); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING).contains("node-a")); } /** @@ -1221,13 +1222,13 @@ public void testGetNetworkInterface_nfs_tier3_crossNodeFallback() { when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lif))); - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); - assertEquals("10.0.0.3", result.first()); - assertTrue(result.second() != null, "Tier 3 fallback should produce a warning"); - assertTrue(result.second().contains("node-a"), + assertEquals("10.0.0.3", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) != null, "Tier 3 fallback should produce a warning"); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING).contains("node-a"), "Warning should mention the expected node"); - assertTrue(result.second().contains("10.0.0.3"), + assertTrue(result.get(OntapStorageConstants.LIF_WARNING).contains("10.0.0.3"), "Warning should mention the fallback LIF IP"); } @@ -1265,10 +1266,11 @@ public void testGetNetworkInterface_nfs_tier1Down_tier2Used() { when(networkFeignClient.getNetworkIpInterfaces(anyString(), anyMap())) .thenReturn(wrapLifs(List.of(lifDown, lifFailover))); - Pair result = storageStrategy.getNetworkInterface(aggregate); + Map result = storageStrategy.getNetworkInterface(aggregate); - assertEquals("10.0.0.6", result.first()); - assertTrue(result.second() != null, "Should warn that the home-node LIF is not in use"); + assertEquals("10.0.0.6", result.get(OntapStorageConstants.DATA_LIF)); + assertTrue(result.get(OntapStorageConstants.LIF_WARNING) != null, + "Should warn that the home-node LIF is not in use"); } // ========== Helper Methods ========== From 2e324d8bf8ebbd860176a7fbfb19ee923fb1cb4d Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 8 Oct 2026 00:03:15 +0530 Subject: [PATCH 5/7] Addressed review comments --- .../org/apache/cloudstack/storage/service/StorageStrategy.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 db16a6b9f160..acc774baef9c 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 @@ -430,7 +430,7 @@ public Aggregate chooseAggregate(Long size) { final long availableBytes = aggrResp.getAvailableBlockStorageSpace().longValue(); logger.debug("Aggregate " + aggr.getName() + " available bytes=" + availableBytes + ", requested=" + size); - if (availableBytes < size) { + if (availableBytes <= size) { logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Required=" + size + " bytes, available=" + availableBytes + " bytes. Skipping this aggregate."); continue; From 47f6fb710417a922578aa8ca69204a1c323db814 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 8 Oct 2026 15:00:42 +0530 Subject: [PATCH 6/7] Brought back the missing changes --- .../OntapPrimaryDatastoreLifecycle.java | 2 +- .../storage/service/StorageStrategy.java | 24 +++- .../OntapPrimaryDatastoreLifecycleTest.java | 2 +- .../storage/service/StorageStrategyTest.java | 116 +++++++++++------- 4 files changed, 92 insertions(+), 52 deletions(-) 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 15146d8dc75d..83b0c0223ea0 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 @@ -149,7 +149,7 @@ public DataStore initialize(Map dsInfos) { } Aggregate aggregate; try { - aggregate = storageStrategy.chooseAggregate(capacityBytes); + aggregate = storageStrategy.chooseAggregate(storageStrategy.getAggregates(), capacityBytes); } catch (Exception e) { logger.error("Exception occurred while choosing aggregate for pool: " + storagePoolName, e); throw new CloudRuntimeException("Failed to choose ONTAP aggregate for pool: " + storagePoolName 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 acc774baef9c..b02d7e624b93 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 @@ -19,6 +19,7 @@ package org.apache.cloudstack.storage.service; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -348,11 +349,23 @@ public String getResolvedSvmUuid() { return resolvedSvmUuid; } + /** + * Aggregates eligible for new FlexVol creation, populated by {@link #connect(boolean)} with aggregate validation enabled. + */ + public List getAggregates() { + return aggregates; + } + + public void setAggregates(List aggregates) { + this.aggregates = aggregates; + } + private void validateAndSelectAggregatesForVolumeCreation(String authHeader, String svmName, List aggrs) { if (aggrs == null || aggrs.isEmpty()) { logger.error("No aggregates are assigned to SVM " + svmName); throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName); } + List eligibleAggregates = new ArrayList<>(); for (Aggregate aggr : aggrs) { logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid()); Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid(), @@ -372,27 +385,28 @@ private void validateAndSelectAggregatesForVolumeCreation(String authHeader, Str continue; } logger.info("Selected aggregate: " + aggr.getName() + " for volume operations."); - this.aggregates = List.of(aggr); + eligibleAggregates.add(aggr); } - if (this.aggregates == null || this.aggregates.isEmpty()) { + if (eligibleAggregates.isEmpty()) { logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation."); throw new CloudRuntimeException("No suitable aggregates found on SVM " + svmName + " for volume creation."); } + setAggregates(eligibleAggregates); } // Common methods like create/delete etc., should be here /** - * Selects the best aggregate for a volume of the given size from candidates populated by - * {@link #connect(boolean)} with aggregate validation enabled. + * Selects the best aggregate for a volume of the given size from the given candidate aggregates. * *

Picks the online aggregate with the largest available block space that can fit * {@code size}. The returned aggregate includes node information for LIF affinity.

* + * @param aggregates candidate aggregates, for example {@link #getAggregates()} * @param size requested volume size in bytes * @return the chosen aggregate detail response */ - public Aggregate chooseAggregate(Long size) { + public Aggregate chooseAggregate(List aggregates, Long size) { String svmName = storage.getSvmName(); if (aggregates == null || aggregates.isEmpty()) { logger.error("No aggregates available to create volume on SVM " + svmName); 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 cc886cf8d43f..23e142c09638 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 @@ -807,7 +807,7 @@ public void testInitialize_positiveWithDetailAssertions() { // Verify LIF selection completes before FlexVol creation InOrder inOrder = inOrder(storageStrategy); - inOrder.verify(storageStrategy).chooseAggregate(any()); + inOrder.verify(storageStrategy).chooseAggregate(any(), any()); inOrder.verify(storageStrategy).getNetworkInterface(any()); inOrder.verify(storageStrategy).createStorageVolume(any(), any(), any()); 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 fe4f6ccbd69d..c05f8d63a8a3 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 @@ -259,6 +259,39 @@ public void testConnect_positive() { verify(svmFeignClient, times(1)).getSvmResponse(anyMap(), anyString()); } + @Test + public void testConnect_collectsAllEligibleAggregates() { + Svm svm = new Svm(); + svm.setName("svm1"); + svm.setState(OntapStorageConstants.RUNNING); + svm.setNfsEnabled(true); + + Aggregate aggregate1 = new Aggregate(); + aggregate1.setName("aggr1"); + aggregate1.setUuid("aggr-uuid-1"); + Aggregate aggregate2 = new Aggregate(); + aggregate2.setName("aggr2"); + aggregate2.setUuid("aggr-uuid-2"); + svm.setAggregates(List.of(aggregate1, aggregate2)); + + OntapResponse svmResponse = new OntapResponse<>(); + svmResponse.setRecords(List.of(svm)); + + when(svmFeignClient.getSvmResponse(anyMap(), anyString())).thenReturn(svmResponse); + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) + .thenReturn(buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0)); + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-2"), anyMap())) + .thenReturn(buildAggregate("aggr2", "aggr-uuid-2", 20000000000.0)); + + assertTrue(storageStrategy.connect()); + + List aggregates = storageStrategy.getAggregates(); + assertNotNull(aggregates); + assertEquals(2, aggregates.size()); + assertEquals("aggr-uuid-1", aggregates.get(0).getUuid()); + assertEquals("aggr-uuid-2", aggregates.get(1).getUuid()); + } + @Test public void testConnect_operationsOnly_skipsAggregateValidation() { Svm svm = new Svm(); @@ -600,14 +633,11 @@ public void testConnect_invalidCredentials() { @Test public void testChooseAggregate_positive() { - setupSuccessfulConnect(); - storageStrategy.connect(); - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) .thenReturn(aggregateDetail); - Aggregate result = storageStrategy.chooseAggregate(5000000000L); + Aggregate result = storageStrategy.chooseAggregate(candidateAggregates(), 5000000000L); assertNotNull(result); assertEquals("aggr1", result.getName()); @@ -616,37 +646,52 @@ public void testChooseAggregate_positive() { } @Test - public void testChooseAggregate_invalidSize() { - setupSuccessfulConnect(); - storageStrategy.connect(); + public void testChooseAggregate_picksLargestAvailable() { + Aggregate candidate1 = new Aggregate(); + candidate1.setName("aggr1"); + candidate1.setUuid("aggr-uuid-1"); + Aggregate candidate2 = new Aggregate(); + candidate2.setName("aggr2"); + candidate2.setUuid("aggr-uuid-2"); + + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) + .thenReturn(buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a")); + when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-2"), anyMap())) + .thenReturn(buildAggregate("aggr2", "aggr-uuid-2", 20000000000.0, "node-b")); + Aggregate result = storageStrategy.chooseAggregate(List.of(candidate1, candidate2), 5000000000L); + + assertEquals("aggr-uuid-2", result.getUuid()); + assertEquals("node-b", result.getNode().getName()); + } + + @Test + public void testChooseAggregate_invalidSize() { Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(-1L)); + () -> storageStrategy.chooseAggregate(candidateAggregates(), -1L)); assertTrue(ex.getMessage().contains("Invalid volume size")); } @Test public void testChooseAggregate_nullSize() { - setupSuccessfulConnect(); - storageStrategy.connect(); - Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(null)); + () -> storageStrategy.chooseAggregate(candidateAggregates(), null)); assertTrue(ex.getMessage().contains("Invalid volume size")); } @Test public void testChooseAggregate_noAggregates() { Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(5000000000L)); + () -> storageStrategy.chooseAggregate(null, 5000000000L)); + assertTrue(ex.getMessage().contains("No aggregates available")); + + ex = assertThrows(CloudRuntimeException.class, + () -> storageStrategy.chooseAggregate(List.of(), 5000000000L)); assertTrue(ex.getMessage().contains("No aggregates available")); } @Test public void testChooseAggregate_aggregateNotOnline() { - setupSuccessfulConnect(); - storageStrategy.connect(); - Aggregate aggregateDetail = new Aggregate(); aggregateDetail.setName("aggr1"); aggregateDetail.setUuid("aggr-uuid-1"); @@ -656,39 +701,40 @@ public void testChooseAggregate_aggregateNotOnline() { .thenReturn(aggregateDetail); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(5000000000L)); + () -> storageStrategy.chooseAggregate(candidateAggregates(), 5000000000L)); assertTrue(ex.getMessage().contains("No suitable aggregates found")); } @Test public void testChooseAggregate_insufficientSpace() { - setupSuccessfulConnect(); - storageStrategy.connect(); - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 1000000.0, "node-a"); when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) .thenReturn(aggregateDetail); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(5000000000L)); + () -> storageStrategy.chooseAggregate(candidateAggregates(), 5000000000L)); assertTrue(ex.getMessage().contains("No suitable aggregates found")); } @Test public void testChooseAggregate_missingNode() { - setupSuccessfulConnect(); - storageStrategy.connect(); - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0); when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())) .thenReturn(aggregateDetail); Exception ex = assertThrows(CloudRuntimeException.class, - () -> storageStrategy.chooseAggregate(5000000000L)); + () -> storageStrategy.chooseAggregate(candidateAggregates(), 5000000000L)); assertTrue(ex.getMessage().contains("does not have a node name")); } + private List candidateAggregates() { + Aggregate candidate = new Aggregate(); + candidate.setName("aggr1"); + candidate.setUuid("aggr-uuid-1"); + return List.of(candidate); + } + // ========== createStorageVolume() Tests ========== @Test @@ -1275,26 +1321,6 @@ public void testGetNetworkInterface_nfs_tier1Down_tier2Used() { // ========== Helper Methods ========== - private void setupSuccessfulConnect() { - Svm svm = new Svm(); - svm.setName("svm1"); - svm.setState(OntapStorageConstants.RUNNING); - svm.setNfsEnabled(true); - - Aggregate aggregate = new Aggregate(); - aggregate.setName("aggr1"); - aggregate.setUuid("aggr-uuid-1"); - svm.setAggregates(List.of(aggregate)); - - OntapResponse svmResponse = new OntapResponse<>(); - svmResponse.setRecords(List.of(svm)); - - when(svmFeignClient.getSvmResponse(anyMap(), anyString())).thenReturn(svmResponse); - - Aggregate aggregateDetail = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); - when(aggregateFeignClient.getAggregateByUUID(anyString(), eq("aggr-uuid-1"), anyMap())).thenReturn(aggregateDetail); - } - private void setupSuccessfulJobCreation() { Job job = new Job(); job.setUuid("job-uuid-1"); From 6ecdda0d2dc2904218a35048f775b8710085d979 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Thu, 8 Oct 2026 15:38:52 +0530 Subject: [PATCH 7/7] pre-check failure fix --- .../storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java | 2 -- 1 file changed, 2 deletions(-) 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 23e142c09638..a21097180033 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 @@ -65,7 +65,6 @@ import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.withSettings; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import org.mockito.InOrder; import static org.mockito.ArgumentMatchers.contains; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -73,7 +72,6 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.assertFalse; -import java.util.HashMap; import com.cloud.storage.StoragePool; import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStoreLifeCycle; import org.apache.cloudstack.storage.provider.StorageProviderFactory;