CSTACKEX-127: Primary storage-pool is getting created even if desired data LIFs are not reachable#76
Open
sandeeplocharla wants to merge 1 commit into
Open
CSTACKEX-127: Primary storage-pool is getting created even if desired data LIFs are not reachable#76sandeeplocharla wants to merge 1 commit into
sandeeplocharla wants to merge 1 commit into
Conversation
sandeeplocharla
requested review from
piyush5netapp,
rajiv-jain-netapp and
suryag1201
as code owners
There was a problem hiding this comment.
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.
Comment on lines
+159
to
+165
| 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); | ||
| } |
Comment on lines
+275
to
+277
| if (aggrChosen.getNode() != null) { | ||
| this.chosenAggregateNode = aggrChosen.getNode().getName(); | ||
| } |
Comment on lines
+500
to
+506
| 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; | ||
| } |
Comment on lines
+549
to
552
| } catch (Exception e) { | ||
| logger.error("Exception while retrieving network interfaces: ", e); | ||
| throw new CloudRuntimeException("Failed to retrieve network interfaces: " + e.getMessage()); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Choosing IpInterface based on its status and affinity to the chosen aggregate
Description
This PR...
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
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.
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
040dnode were down; with one LIF whose current node:040d, while its home node:040cScenario-3 [pool_P3]: None of the
040dnode LIFs are UP. First best available LIF is picked from040c.