From 4bee534a6b51a23f7c31c45c828257a91b704039 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 08:21:08 +0530 Subject: [PATCH 1/4] 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 c002db728dd1..c780701d4ecf 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; @@ -143,10 +144,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); @@ -158,15 +177,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 ac142edf57ae..a97cf20c6abf 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 @@ -77,12 +77,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 */ @@ -220,19 +214,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); @@ -243,18 +234,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) { @@ -298,13 +277,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); @@ -480,19 +501,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<>(); @@ -534,21 +562,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) { @@ -564,21 +590,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 1205458d9ea0a66050a2a3af81e216b3bf1c9df0 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 10:38:18 +0530 Subject: [PATCH 2/4] Added missing UTs --- .../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 ed538de4a49c..5d9d887a57a7 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 @@ -30,6 +30,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.utils.exception.CloudRuntimeException; @@ -55,7 +56,10 @@ 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 org.mockito.InOrder; import static org.mockito.ArgumentMatchers.contains; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -122,12 +126,19 @@ void setUp() { when(_clusterDao.findById(1L)).thenReturn(clusterVO); when(storageStrategy.connect()).thenReturn(true); - when(storageStrategy.getNetworkInterface()).thenReturn(new Pair<>("testNetworkInterface", null)); + 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.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) @@ -435,7 +446,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)) { @@ -470,12 +481,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()); } } @@ -501,12 +513,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()); } } @@ -532,13 +545,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()); } } @@ -564,7 +578,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); @@ -595,7 +609,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); @@ -628,13 +642,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 d8a249a4447a..8568d57dc416 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 @@ -426,19 +426,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"); @@ -468,7 +554,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); @@ -480,80 +566,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"); @@ -571,18 +609,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 @@ -592,9 +626,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")); } @@ -775,6 +808,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"); @@ -783,6 +818,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)); @@ -791,7 +832,7 @@ public void testGetNetworkInterface_nfs() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -809,6 +850,8 @@ public void testGetNetworkInterface_iscsi() { aggregateFeignClient, volumeFeignClient, svmFeignClient, jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.51"); @@ -816,6 +859,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)); @@ -824,7 +873,7 @@ public void testGetNetworkInterface_iscsi() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -834,6 +883,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"); @@ -850,12 +901,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"); @@ -872,7 +925,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")); } @@ -885,6 +938,8 @@ public void testGetNetworkInterface_iscsi_lifDown() { aggregateFeignClient, volumeFeignClient, svmFeignClient, jobFeignClient, networkFeignClient, sanFeignClient, snapshotFeignClient); + Aggregate aggregate = buildAggregate("aggr1", "aggr-uuid-1", 10000000000.0, "node-a"); + IpInterface.IpInfo ipInfo = new IpInterface.IpInfo(); ipInfo.setAddress("192.168.1.51"); @@ -900,12 +955,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<>()); @@ -915,12 +972,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); @@ -929,7 +988,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")); } @@ -940,13 +999,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"); @@ -958,14 +1017,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"); @@ -979,14 +1038,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"); @@ -997,24 +1056,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")); } /** @@ -1022,7 +1079,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"); @@ -1032,7 +1089,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"); @@ -1056,16 +1113,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"); @@ -1093,21 +1144,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. * @@ -1151,6 +1187,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); @@ -1162,6 +1202,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 195a052218fecbf67fb257fdd1a4087c2782b815 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 12 Aug 2026 11:46:58 +0530 Subject: [PATCH 3/4] 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 c780701d4ecf..4b7cecf3a380 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 @@ -68,7 +68,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; @@ -153,14 +152,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)"); @@ -292,9 +293,8 @@ private long validateInitializeInputs(Long capacityBytes, Long podId, Long clust return capacityBytes; } - 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"); } @@ -302,8 +302,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"); @@ -566,7 +565,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. @@ -593,7 +592,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(); @@ -601,7 +600,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) { @@ -610,6 +609,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 92bd9ce84a4f2e3eca12c95c77efa7ca4fb4dafa Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Wed, 12 Aug 2026 14:47:29 +0530 Subject: [PATCH 4/4] Committed missed test files --- .../OntapPrimaryDatastoreLifecycleTest.java | 24 ++++++---- .../storage/service/StorageStrategyTest.java | 46 ++++++++++--------- 2 files changed, 38 insertions(+), 32 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 5d9d887a57a7..9a11e37fe7f1 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,7 +49,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; @@ -64,7 +64,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 org.apache.cloudstack.storage.provider.StorageProviderFactory; import org.apache.cloudstack.storage.service.StorageStrategy; import org.apache.cloudstack.storage.volume.datastore.PrimaryDataStoreHelper; @@ -133,7 +132,8 @@ void setUp() { node.setName("node-a"); aggregate.setNode(node); 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"); @@ -424,7 +424,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"); @@ -446,7 +446,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)) { @@ -461,7 +463,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"); @@ -481,7 +483,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); @@ -493,7 +495,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"); @@ -513,7 +515,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); @@ -642,7 +645,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 8568d57dc416..c90856852c6c 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 @@ -68,7 +68,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; @@ -832,12 +831,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()); } @@ -873,12 +873,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 @@ -1005,10 +1006,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"); } /** @@ -1024,11 +1025,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")); } /** @@ -1045,13 +1046,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"); } @@ -1089,10 +1090,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 ==========