CSTACKEX-191: logical access methods can have return type as primitive#60
Merged
Conversation
rajiv-jain-netapp
requested review from
piyush5netapp,
sandeeplocharla and
suryag1201
as code owners
suryag1201
reviewed
May 25, 2026
suryag1201
reviewed
May 25, 2026
suryag1201
left a comment
suryag1201
left a comment
There was a problem hiding this comment.
Please update the testing section after making these changes
Author
|
Testing results: I got my lab and setup working back yesterday. And I just closed the storage-pool and VM create workflow, tested with the changes. All are working fine. |
Author
updated testing details |
There was a problem hiding this comment.
Pull request overview
This PR updates the ONTAP storage strategy API so logical-access operations return the LUN number as a primitive String rather than a Map, and propagates that signature change through SAN/NAS strategies and tests. It also includes some refactoring/cleanup (logging level changes, method extraction in the ONTAP primary datastore driver, and a small KVM iSCSI wait-time adjustment).
Changes:
- Change
enableLogicalAccess/getLogicalAccessreturn types fromMap<String,String>toString(LUN number) across the ONTAP storage strategy API and implementations. - Update SAN strategy behavior to return the LUN number directly and adjust dependent logic accordingly.
- Refactor ONTAP primary datastore driver access-grant/revoke flows and apply several small logging/formatting tweaks (plus KVM iSCSI wait retry reduction).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/UnifiedSANStrategyTest.java | Updates tests for new String return type (needs stronger assertions for expected LUN values). |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java | Updates test stub to match new abstract method signatures. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java | Returns LUN number as String, adds response validation, adjusts ensure-mapping flow, and changes logging levels. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java | Updates method signatures to return String (currently returns null). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java | Updates abstract method signatures to return String (Javadoc needs alignment). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java | Minor formatting cleanup. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeConcise.java | Formatting cleanup (annotation spacing, brace placement). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java | Extracts iSCSI grant-access logic; refactors revoke-access validation into helper; adds nullable helper result container. |
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java | Reduces wait retries for disk availability and logs exceptions when filesystem size checks fail. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Map<String, String> response = null; | ||
| @Override | ||
| public String enableLogicalAccess(Map<String, String> values) { | ||
| logger.trace("enableLogicalAccess : Creating LunMap with values {} ", values); |
Collaborator
There was a problem hiding this comment.
I don't think there was any ask to convert all the logs to 'trace'. Let's use either 'info' or 'debug' itself?
sandeeplocharla
approved these changes
Jun 11, 2026
sandeeplocharla
left a comment
sandeeplocharla
left a comment
Collaborator
There was a problem hiding this comment.
Except for using trace level logs, rest look fine to me
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.
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?
How did you try to break this feature and the system with this change?