From 3dce185cdf5e16f6ce8a6fb0295d758c98939552 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 08:21:08 +0530 Subject: [PATCH 1/2] 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 900b0980d28869b0622e04e89c1c1ad132e1a168 Mon Sep 17 00:00:00 2001 From: sandeeplocharla Date: Mon, 10 Aug 2026 10:38:18 +0530 Subject: [PATCH 2/2] Added missing UTs --- .../OntapPrimaryDatastoreLifecycleTest.java | 38 ++- .../storage/service/StorageStrategyTest.java | 287 ++++++++++-------- 2 files changed, 196 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 eb3bbff3cfa4..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 @@ -59,6 +59,8 @@ import static org.mockito.ArgumentMatchers.eq; import org.mockito.Mock; import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -424,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"); @@ -466,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); @@ -478,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"); @@ -569,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 @@ -590,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")); } @@ -773,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"); @@ -781,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)); @@ -789,7 +832,7 @@ public void testGetNetworkInterface_nfs() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -807,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"); @@ -814,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)); @@ -822,7 +873,7 @@ public void testGetNetworkInterface_iscsi() { .thenReturn(interfaceResponse); // Execute - Pair result = storageStrategy.getNetworkInterface(); + Pair result = storageStrategy.getNetworkInterface(aggregate); // Verify assertNotNull(result); @@ -832,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"); @@ -848,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"); @@ -870,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")); } @@ -883,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"); @@ -898,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<>()); @@ -913,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); @@ -927,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")); } @@ -938,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"); @@ -956,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"); @@ -977,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"); @@ -995,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")); } /** @@ -1020,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"); @@ -1030,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"); @@ -1054,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"); @@ -1091,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. * @@ -1149,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); @@ -1160,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; }