Refactor code to choose aggregate, network interface and creating storage volume; Also, the corresponding UT changes

[sandeeplocharla]

Description

This PR has changes to refactor choosing an aggregate and its corresponding network interface and also creation of storage volume. Also, with this, volume creation would be done at the end, avoiding volume creation cleanup if in case there's a chance of failure during aggregate or network interface selection.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

[ChoosingAggregateRefactorLogs_iSCSi.rtf](https://github.com/user-attachments/files/30884333/ChoosingAggregateRefactorLogs_iSCSi.rtf) [choosingAggregateRefactorLogs_NFS3.rtf](https://github.com/user-attachments/files/30884337/choosingAggregateRefactorLogs_NFS3.rtf)

[Copilot]

Pull request overview

Refactors the ONTAP primary storage initialization flow to explicitly select an aggregate (and a node-affine data LIF) before creating the backing FlexVol, so volume creation happens last and can avoid cleanup work if earlier selection steps fail.

Changes:

• Split aggregate selection into a dedicated chooseAggregate(size) method and pass the chosen aggregate into volume creation.
• Update data LIF selection to require the chosen aggregate (for deterministic node affinity) and move LIF selection before volume creation in the datastore lifecycle.
• Add stricter validation around aggregate/node presence for LIF affinity.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java	Introduces explicit aggregate selection and requires aggregate input for LIF selection and volume creation.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java	Reorders initialization to choose aggregate + LIF first, then create the FlexVol on the selected aggregate.

Suppressed comments (2)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:610

• getNetworkInterface wraps unexpected exceptions in a CloudRuntimeException but drops the original cause, which makes upstream error handling/debugging harder (especially since callers may rewrap again).

        } 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());
        }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:233

• This refactor changes the public API by removing the previous overloads createStorageVolume(String, Long) and getNetworkInterface(); however, the repo still contains unit tests that call the old signatures (e.g. StorageStrategyTest and OntapPrimaryDatastoreLifecycleTest). As-is, this will fail compilation unless those tests are updated (or compatibility wrappers are added).

    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);
            throw new CloudRuntimeException("No aggregates available to create volume on SVM " + svmName);
        }
        if (size == null || size <= 0) {
            throw new CloudRuntimeException("Invalid volume size provided: " + size);

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:564

• Potential NullPointerException: iface.getIp() is dereferenced without a null check (iface.getIp().getAddress()). If ONTAP returns an interface record without an IP object, this will NPE and abort LIF selection. Consider skipping records with missing IP/address before calling isIPv4Address(...).

            for (IpInterface iface : response.getRecords()) {
                if (!Boolean.TRUE.equals(iface.getEnabled()) || !OntapStorageConstants.LIF_STATE_UP.equals(iface.getState())) {
                    continue;
                }
                if (!isIPv4Address(iface.getIp().getAddress())) {
                    continue;

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:165

• processDataLifSelection(...) can send a storage alert when a LIF warning is present. With the new ordering, this alert may fire before FlexVol creation succeeds; if volume creation later fails, operators may see alerts for a pool that never got created. Consider splitting LIF validation (fail fast before volume creation) from alert emission (only after volume creation succeeds).

            Pair<String, String> 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 (" +

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:224

• chooseAggregate() is documented and implemented to pick the online aggregate with the largest available block space, but connect(true) currently calls validateAndSelectAggregatesForVolumeCreation(...), which overwrites this.aggregates with List.of(aggr) on each match. That means chooseAggregate() will typically see only the last eligible aggregate rather than all candidates, so the “largest available” selection can be wrong depending on SVM aggregate ordering.

     * Selects the best aggregate for a volume of the given size from candidates populated by
     * {@link #connect(boolean)} with aggregate validation enabled.
     *
     * <p>Picks the online aggregate with the largest available block space that can fit
     * {@code size}. The returned aggregate includes node information for LIF affinity.</p>
     *
     * @param size requested volume size in bytes
     * @return the chosen aggregate detail response

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:278

• In chooseAggregate(), the log message says "for volume creation" but the thrown exception says "for volume operations". Keeping these consistent helps troubleshooting and makes it easier to assert on errors in tests/logs.

            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.");

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (11)

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:876

• getNetworkInterface(...) now returns a Map<String,String>, but this test still assigns it to Pair, which will not compile. Convert the returned map into the Pair used by the assertions (or assert on the map directly).

        Pair<String, String> result = storageStrategy.getNetworkInterface(aggregate);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:1008

• getNetworkInterface(...) now returns a Map<String,String>, but this test still uses Pair, which will not compile. Convert the returned map to a Pair for the existing assertions (or update assertions to use map keys).

        Pair<String, String> result = storageStrategy.getNetworkInterface(aggregate);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:1027

• getNetworkInterface(...) now returns a Map<String,String>, but this test still uses Pair, which will not compile. Convert the returned map to a Pair for the existing assertions (or update assertions to use map keys).

        Pair<String, String> result = storageStrategy.getNetworkInterface(aggregate);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:1048

• getNetworkInterface(...) now returns a Map<String,String>, but this test still uses Pair, which will not compile. Convert the returned map to a Pair for the existing assertions (or update assertions to use map keys).

        Pair<String, String> result = storageStrategy.getNetworkInterface(aggregate);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java:1092

• getNetworkInterface(...) now returns a Map<String,String>, but this test still uses Pair, which will not compile. Convert the returned map to a Pair for the existing assertions (or update assertions to use map keys).

        Pair<String, String> result = storageStrategy.getNetworkInterface(aggregate);

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:136

• StorageStrategy.getNetworkInterface(...) now returns Map<String,String>, but the mock is configured to return Pair. This will not compile and also doesn't match how OntapPrimaryDatastoreLifecycle reads DATA_LIF/LIF_WARNING from the map.

        when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("testNetworkInterface", null));

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:449

• StorageStrategy.getNetworkInterface(...) now returns Map<String,String>, but this test still stubs it with Pair. Return a map with DATA_LIF/LIF_WARNING so initialize() can read it correctly.

        when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("10.0.0.1", warningMessage));

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:484

• StorageStrategy.getNetworkInterface(...) now returns Map<String,String>, but this test still stubs it with Pair. Use an empty map (or a map without DATA_LIF) to exercise the null Data LIF path.

        when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(null, null));

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:645

• StorageStrategy.getNetworkInterface(...) now returns Map<String,String>, but this test still stubs it with Pair. Return a map with DATA_LIF so initialize() can proceed.

        when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>(expectedDataLif, null));

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:516

• StorageStrategy.getNetworkInterface(...) now returns Map<String,String>, but this test still stubs it with Pair. Return a map with DATA_LIF set to an empty string to hit the empty-lif validation.

        when(storageStrategy.getNetworkInterface(any())).thenReturn(new Pair<>("", null));

plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java:33

• Once getNetworkInterface(...) stubs are updated to return Map<String,String> (as required by the refactor), com.cloud.utils.Pair is no longer referenced in this test and the import will fail compilation. Remove the unused Pair import.

This issue also appears in the following locations of the same file:

• line 136
• line 449
• line 484
• line 516
• line 645

import org.apache.cloudstack.storage.feign.model.Aggregate;

[github-actions]

🔴 Test Coverage Grade: D — Marginal

Metric	Value
Line coverage	24.59%
Branch coverage	18.76%

Grade Scale

Grade	Line Coverage	Meaning
🟢 A	≥ 80%	Excellent - this code sleeps well at night 😴
🟡 B	60-79%	Good - almost there, don't stop now 😉
🟠 C	40-59%	Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D	20-39%	Marginal - boldly shipping where no test has gone before 🖖
⛔ F	< 20%	Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run