[sandeeplocharla]

Choosing IpInterface based on its status and affinity to the chosen aggregate

Description

This PR...

1. Fixed an issue in picking an unavailable IP while creating the storage pool.
2. When CloudStack creates an ONTAP primary storage pool, it now picks the best available network interface (LIF) using a priority-based selection:
   • Best case: Uses a LIF homed on the same node as the storage aggregate — optimal I/O, no warning
   • Degraded case: All home-node LIFs are down but a failover LIF is running on that node — pool is created, admin is warned
   • Fallback case: No LIF at all on the aggregate's node, pool is created using a LIF from a different node, admin is warned with a latency note
   • Failure case: No usable LIF anywhere, pool creation fails with a clear error

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?

Note: The following images have been captured for NFS3, the same would be the case for iSCSI.

Clearly, by the virtue of free space available, the plugin would choose sti246_vsim_ocvs040d_aggr1 by default.

Scenario-1 [pool_P1]: Happy path; No LIFs were down.



The first best available LIF with current node and home node matching with the chosen node has been picked.

Scenario-2 [pool_P2_1]: LIFs on 040d node were down; with one LIF whose current node: 040d, while its home node: 040c

Scenario-3 [pool_P3]: None of the 040d node LIFs are UP. First best available LIF is picked from 040c.

[Copilot]

Pull request overview

This PR updates the ONTAP primary storage workflow to select a reachable/usable data LIF based on operational status and node affinity (aggregate home node vs. failover vs. cross-node fallback), and surfaces warnings via pool details/alerts when selection is degraded.

Changes:

• Adds node-aware, status-aware LIF selection in StorageStrategy.getNetworkInterface() and returns both the chosen LIF IP + an optional warning.
• Captures the chosen aggregate’s node during volume creation to bias LIF selection toward optimal locality.
• Updates lifecycle + tests to handle the new (lifIp, warning) result and emits storage alerts when degraded selection occurs.

Reviewed changes

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

Show a summary per file

File	Description
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java	Expands unit tests for LIF status and node-affinity tiers; avoids mocking issues on newer JDKs.
plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java	Updates mocks for new Pair<String,String> LIF return type and scope behavior.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java	Adds helper to send storage alerts for degraded LIF selection.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java	Adds constants for aggregate node/space fields and LIF state/location/warning keys.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java	Implements tiered LIF selection and tracks chosenAggregateNode from aggregate selection.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java	Persists selected LIF + warning and emits an alert during pool initialization.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IpInterface.java	Extends model with state, enabled, and location to support selection logic.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Aggregate.java	Extends model with node and setters needed for reading/constructing detailed aggregate info.
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/AggregateFeignClient.java	Adds @QueryMap to request specific aggregate fields (node/space/state).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

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

• If Data LIF selection fails after createStorageVolume() succeeds, initialize() throws and can leave an orphaned ONTAP volume on the array. Add best-effort cleanup (deleteStorageVolume) when getNetworkInterface()/processDataLifSelection fails so failed pool creation doesn’t leak storage resources.

            logger.info("Creating ONTAP volume '" + storagePoolName + "' with size: " + capacityBytes + " bytes (" +
                    (capacityBytes / (1024 * 1024 * 1024)) + " GB)");
            try {
                Volume volume = storageStrategy.createStorageVolume(storagePoolName, capacityBytes);
                if (volume == null) {
                    logger.error("createStorageVolume returned null for volume: " + storagePoolName);
                    throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName);
                }
                logger.info("Volume object retrieved successfully. UUID: " + volume.getUuid() + ", Name: " + volume.getName());
                details.putIfAbsent(OntapStorageConstants.VOLUME_UUID, volume.getUuid());
                details.putIfAbsent(OntapStorageConstants.VOLUME_NAME, volume.getName());
            } catch (Exception e) {
                logger.error("Exception occurred while creating ONTAP volume: " + storagePoolName, e);
                throw new CloudRuntimeException("Failed to create ONTAP volume: " + storagePoolName + ". Error: " + e.getMessage(), e);
            }

            Pair<String, String> 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);

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

• getNetworkInterface() dereferences iface.getIp().getAddress() without checking iface.getIp() (or address) for null. If ONTAP returns a LIF record missing the nested ip object/address, this will throw a NullPointerException and mask the real selection failure.

            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/service/StorageStrategy.java:213

• validateAndSelectAggregatesForVolumeCreation() overwrites this.aggregates on every eligible aggregate and ends up keeping only the last one. Since createStorageVolume() later chooses the ‘best’ aggregate only from this.aggregates, this can cause the plugin to ignore higher-free-space aggregates depending on ONTAP response order.

    private void validateAndSelectAggregatesForVolumeCreation(String authHeader, String svmName, List<Aggregate> aggrs) {
        if (aggrs == null || aggrs.isEmpty()) {
            logger.error("No aggregates are assigned to SVM " + svmName);
            throw new CloudRuntimeException("No aggregates are assigned to SVM " + svmName);
        }
        for (Aggregate aggr : aggrs) {
            logger.debug("Found aggregate: " + aggr.getName() + " with UUID: " + aggr.getUuid());
            Aggregate aggrResp = aggregateFeignClient.getAggregateByUUID(authHeader, aggr.getUuid(),
                        Map.of(OntapStorageConstants.FIELDS, OntapStorageConstants.AGGREGATE_NODE
                                + OntapStorageConstants.COMMA + OntapStorageConstants.AGGREGATE_SPACE
                                + OntapStorageConstants.COMMA + OntapStorageConstants.STATE));
            if (aggrResp == null) {
                logger.warn("Aggregate details response is null for aggregate " + aggr.getName() + ". Skipping.");
                continue;
            }
            if (!Objects.equals(aggrResp.getState(), Aggregate.StateEnum.ONLINE)) {
                logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate.");
                continue;
            } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null ||
                    aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {
                logger.warn("Aggregate " + aggr.getName() + " does not have sufficient available space. Skipping this aggregate.");
                continue;
            }
            logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
            this.aggregates = List.of(aggr);
        }

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (4)

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

• getNetworkInterface() dereferences iface.getIp().getAddress() without checking iface.getIp() / address for null, which can throw an NPE if ONTAP returns a LIF record missing the nested ip object/address (partial/invalid responses).

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

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

• If Data LIF selection/validation fails after createStorageVolume() succeeds, initialize() throws and can leave an orphaned volume on the array. Add best-effort cleanup (delete the created volume) on LIF selection/processing failure to avoid leaking storage resources on failed pool creation.

            Pair<String, String> lifResult;
            try {
                lifResult = storageStrategy.getNetworkInterface();
            } catch (Exception e) {
                logger.error("Exception occurred while retrieving network interface for pool: " + storagePoolName, e);

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

• validateAndSelectAggregatesForVolumeCreation() keeps iterating after selecting a suitable aggregate, overwriting this.aggregates each time. This makes the chosen aggregate depend on iteration order (last suitable wins) and triggers unnecessary extra ONTAP API calls.

            logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
            this.aggregates = List.of(aggr);
        }

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java:181

• updateNfsExportPolicyForConnectedHostIfNeeded() no longer distinguishes between a missing host and a host that isn't eligible for NFS3: when host is null, isNfs3EnabledOnHost(host) returns false and the code throws "NFS protocol is not enabled...", which is misleading compared to the previous explicit "Host was not found" error.

        if (!isNfs3EnabledOnHost(host)) {
            throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId);

[sandeeplocharla]

Refactor the code and modularize aggregate selection, volume creation and fetching network interfaces. Raise a PR for this.

[piyush5netapp]

if the aggregate also has same static inner class Node, lets have a separate Node model

[piyush5netapp]

we can use CollectionsUtil.isEmpty() on records

[rajiv-jain-netapp]

As discussed, we will be picking up the review comments in the separate PR, with this approving this PR.