CloudStack Volume support with ONTAP storage#13053
Open
rajiv-jain-netapp wants to merge 105 commits into
Open
Conversation
… added EOF fixes + correcting license header
Initial primary storage pool plugin skeleton
…POJOs" This reverts commit fe0f752.
…POJOs" This reverts commit 28faca1.
Feignconfiguration and volume feignClient along with desired POJOs with cstack 28
* CSTACKEX-29 Cluster, SVM and Aggr Feign Client * CSTACKEX-29 Change the endpoint method name in feign client * CSTACKEX-29 Make the alignment proper * CSTACKEX-29 Added License Info * CSTACKEX-29 Resolve Review Comments * CSTACKEX-29 Remove Component Annotation from datastoredriverclass * CSTACKEX-29 Resolve Style check issues * CSTACKEX-29 Resolve ALL Style issues * CSTACKEX-29 Resolve Precommits Issues * CSTACKEX-29 Added Method comments and change the ontap response class name --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
* CSTACKEX-31 NAS and Job Feign Client and POJOs * CSTACKEX-31 Fixed Checks Issues * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Resolve Review Comments * CSTACKEX-31 Added Aggr and size to volume model * CSTACKEX-31 Change the export policy endpoint path * CSTACKEX-31 Fixed check styles --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
* CSTACKEX-30 SAN Feign Client * CSTACKEX-30 Fixed check style issues * CSTACKEX-30 Fixed review comments --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
* CSTACKEX-7: ONTAP Primary storage pool --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>
CSTACKEX-34: Upgrade to framework classes design
* CSTACKEX-35 Create Async * CSTACKEX-35 Added Null and empty check * CSTACKEX-35 Resolved review comments * CSTACKEX-35 Removed Type Casting for logger --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
* CSTACKEX-1: Feign changes and fixes for getting storage pool creation to work * CSTACKEX-01: Create Primary Storage pool changes with working code * CSTACKEX-01: Addressed all review comments and updated some code * CSTACKEX-01: Made some changes to fix some errors seen during testing * CSTACKEX-01: Addressed additional comments --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>
14 tasks
Member
|
@piyush5netapp @rajiv-jain-netapp I went through the reviews and resolved the ones that already seem addressed. Could you go through the comments that are still open and address them, or close with a comment if you judge it unnecessary (the indentation ones)? |
- extracted the ONTAP plugin name to interface nane to use it from one place. - Corrected indentation gaps.
Author
|
Thank you for your comments, @winterhazel. |
Author
|
@winterhazel @DaanHoogland @weizhouapache Gentle reminder to review the PR. We are looking to incorporate feedback and progress further—your inputs would be highly valuable. Appreciate your support! |
Contributor
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 17898 |
…intenance mode (#56)
- extracted the ONTAP plugin name to interface nane to use it from one place. - Corrected indentation gaps.
Contributor
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17938 |
Member
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
Member
@rajiv-jain-netapp there are still some comments that were not addressed. Could you have a look? Also, I did not receive any responses on this day you commented, maybe you added them to a review and forgot to submit it? |
winterhazel
reviewed
May 22, 2026
Comment on lines
+32
to
+34
| @JsonProperty("name") | ||
|
|
||
| private String name; |
Member
There was a problem hiding this comment.
Suggested change
| @JsonProperty("name") | |
| private String name; | |
| @JsonProperty("name") | |
| private String name; |
Addressing some formatting issues from the latest commits
Comment on lines
+48
to
+49
| public void setName(String name) { | ||
| this.name = name; } |
Member
There was a problem hiding this comment.
Suggested change
| public void setName(String name) { | |
| this.name = name; } | |
| public void setName(String name) { | |
| this.name = name; | |
| } |
Formatting
| } | ||
| if (StringUtils.isBlank(providerName)) { | ||
|
|
||
| if (StringUtils.isBlank(providerName )) { |
Member
There was a problem hiding this comment.
Suggested change
| if (StringUtils.isBlank(providerName )) { | |
| if (StringUtils.isBlank(providerName)) { |
|
[SF] Trillian test result (tid-16133)
|
|
Hi This is mainly about NFS and iSCSI, what about Fibre Channel SAN with MultiPath? |
Member
good point @rajiv-jain-netapp |
Author
We planned to add support in incremental order for the protocols. Currently, we picked NFS3 and iSCSI and remaining protocols support would be added in subsequent releases. |
…ISCSI protocol is not enabled at the SVM(#61) ### Description This PR... Primary storage-pool is not failing even if NFS3/ISCSI protocol is not enabled at the storage VM <!-- For new features, provide link to FS, dev ML discussion etc. --> <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. --> <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged --> <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" --> <!-- Fixes: # --> <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] 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 - [x] Minor - [ ] Trivial ### Screenshots (if appropriate): <img width="1707" height="848" alt="Screenshot 2026-05-24 at 8 45 02 PM" src="https://github.com/user-attachments/assets/9dbc8386-29b4-4ce2-b863-6933c1c0a8d6" /> <img width="1697" height="937" alt="Screenshot 2026-05-24 at 8 44 46 PM" src="https://github.com/user-attachments/assets/5c8abce4-0343-453e-a980-bd7124034083" /> ### How Has This Been Tested? <!-- Please describe in detail how you tested your changes. --> <!-- Include details of your testing environment, and the tests you ran to --> #### How did you try to break this feature and the system with this change? <!-- see how your change affects other areas of the code, etc. --> <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document --> --------- Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
### Description Storage Pool Creation is failing "message": "Unexpected argument \"svm.iscsiEnabled\" <!-- For new features, provide link to FS, dev ML discussion etc. --> <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. --> <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged --> <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" --> <!-- Fixes: # --> <!--- ******************************************************************************* --> <!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. --> <!--- PLEASE PUT AN 'X' in only **ONE** box --> <!--- ******************************************************************************* --> ### Types of changes - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] New feature (non-breaking change which adds functionality) - [x] 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 - [x] Major - [ ] Minor #### Bug Severity - [ ] BLOCKER - [ ] Critical - [ ] Major - [ ] Minor - [ ] Trivial ### Screenshots (if appropriate): <img width="1471" height="145" alt="image" src="https://github.com/user-attachments/assets/f894ec2d-0769-4f6d-8ad3-13b3e6b2b5cc" /> <img width="2292" height="1672" alt="image" src="https://github.com/user-attachments/assets/403bce30-c9a2-44ec-a783-cf65c1fa2bd9" /> ### How Has This Been Tested? 1- Disable the NFS and ISCSI protocol on svm and checked the storage pool creation: It threw the appropriate error on UI, below is the screenshot 2- Enable the NFS and ISCSI protocol on svm: Storage pool creation went fine #### How did you try to break this feature and the system with this change? <!-- see how your change affects other areas of the code, etc. --> <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document --> Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
Member
|
@piyush5netapp and @rajiv-jain-netapp, could you have a look at the pending comments? @rajiv-jain-netapp commented about providing responses to some of them, but I did not receive any responses, maybe they were added to a review and not submitted? |
nvazquez
reviewed
Jun 9, 2026
nvazquez
left a comment
nvazquez
left a comment
Contributor
There was a problem hiding this comment.
Thanks @rajiv-jain-netapp please find a first round of review comments
| return true; | ||
| } | ||
| String msg = result.toLowerCase(); | ||
| if (msg.contains("already exists") || msg.contains("database exists") || msg.contains("exists")) { |
Contributor
There was a problem hiding this comment.
As an improvement, non-blocking comment, would it be possible to check the exit status code instead of the returned message?
| return true; | ||
| } | ||
| String msg = result.toLowerCase(); | ||
| if (msg.contains("already present") || msg.contains("already logged in") || msg.contains("session exists")) { |
|
|
||
| private void waitForDiskToBecomeAvailable(String volumeUuid, KVMStoragePool pool) { | ||
| int numberOfTries = 10; | ||
| int numberOfTries = 30; |
Contributor
There was a problem hiding this comment.
Is it worth being configurable by administrators?
| return 0L; | ||
| } | ||
| } catch (Exception ignore) { | ||
| // If FS check fails for any reason, fall back to blockdev call |
Contributor
There was a problem hiding this comment.
I think this deserves a log line
|
|
||
| @Override | ||
| public boolean volumesRequireGrantAccessWhenUsed() { | ||
| logger.info("OntapPrimaryDatastoreDriver: volumesRequireGrantAccessWhenUsed: Called"); |
Contributor
There was a problem hiding this comment.
Does it need to be at INFO level?
Comment on lines
+125
to
+133
| if (dataObject == null) { | ||
| throw new InvalidParameterValueException("dataObject should not be null"); | ||
| } | ||
| if (dataStore == null) { | ||
| throw new InvalidParameterValueException("dataStore should not be null"); | ||
| } | ||
| if (callback == null) { | ||
| throw new InvalidParameterValueException("callback should not be null"); | ||
| } |
Contributor
There was a problem hiding this comment.
What about something like this?
Suggested change
| if (dataObject == null) { | |
| throw new InvalidParameterValueException("dataObject should not be null"); | |
| } | |
| if (dataStore == null) { | |
| throw new InvalidParameterValueException("dataStore should not be null"); | |
| } | |
| if (callback == null) { | |
| throw new InvalidParameterValueException("callback should not be null"); | |
| } | |
| if (ObjectUtils.anyNull(dataObject, dataStore, callback)) { | |
| throw new InvalidParameterValueException("dataObject, dataStore and callback must not be null"); | |
| } |
Comment on lines
+358
to
+366
| if (dataStore == null) { | ||
| throw new InvalidParameterValueException("dataStore should not be null"); | ||
| } | ||
| if (dataObject == null) { | ||
| throw new InvalidParameterValueException("dataObject should not be null"); | ||
| } | ||
| if (host == null) { | ||
| throw new InvalidParameterValueException("host should not be null"); | ||
| } |
Comment on lines
+391
to
+429
| // Only retrieve LUN name for iSCSI volumes | ||
| String cloudStackVolumeName = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME).getValue(); | ||
| UnifiedSANStrategy sanStrategy = (UnifiedSANStrategy) OntapStorageUtils.getStrategyByStoragePoolDetails(details); | ||
| String accessGroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); | ||
|
|
||
| // Validate if Igroup exist ONTAP for this host as we may be using delete_on_unmap= true and igroup may be deleted by ONTAP automatically | ||
| Map<String, String> getAccessGroupMap = Map.of( | ||
| OntapStorageConstants.NAME, accessGroupName, | ||
| OntapStorageConstants.SVM_DOT_NAME, svmName | ||
| ); | ||
| AccessGroup accessGroup = sanStrategy.getAccessGroup(getAccessGroupMap); | ||
| if(accessGroup == null || accessGroup.getIgroup() == null) { | ||
| logger.info("grantAccess: Igroup {} does not exist for the host {} : Need to create Igroup for the host ", accessGroupName, host.getName()); | ||
| // create the igroup for the host and perform lun-mapping | ||
| accessGroup = new AccessGroup(); | ||
| List<HostVO> hosts = new ArrayList<>(); | ||
| hosts.add((HostVO) host); | ||
| accessGroup.setHostsToConnect(hosts); | ||
| accessGroup.setStoragePoolId(storagePool.getId()); | ||
| accessGroup = sanStrategy.createAccessGroup(accessGroup); | ||
| }else{ | ||
| logger.info("grantAccess: Igroup {} already exist for the host {}: ", accessGroup.getIgroup().getName() ,host.getName()); | ||
| /* TODO Below cases will be covered later, for now they will be a pre-requisite on customer side | ||
| 1. Igroup exist with the same name but host initiator has been rempved | ||
| 2. Igroup exist with the same name but host initiator has been changed may be due to new NIC or new adapter | ||
| In both cases we need to verify current host initiator is registered in the igroup before allowing access | ||
| Incase it is not , add it and proceed for lun-mapping | ||
| */ | ||
| } | ||
| logger.info("grantAccess: Igroup {} is present now with initiators {} ", accessGroup.getIgroup().getName(), accessGroup.getIgroup().getInitiators()); | ||
| // Create or retrieve existing LUN mapping | ||
| String lunNumber = sanStrategy.ensureLunMapped(svmName, cloudStackVolumeName, accessGroupName); | ||
|
|
||
| // Update volume path if changed (e.g., after migration or re-mapping) | ||
| String iscsiPath = OntapStorageConstants.SLASH + storagePool.getPath() + OntapStorageConstants.SLASH + lunNumber; | ||
| if (volumeVO.getPath() == null || !volumeVO.getPath().equals(iscsiPath)) { | ||
| volumeVO.set_iScsiName(iscsiPath); | ||
| volumeVO.setPath(iscsiPath); | ||
| } |
Contributor
There was a problem hiding this comment.
Can these lines be extracted to a method?
Comment on lines
+504
to
+543
| String accessGroupName = OntapStorageUtils.getIgroupName(svmName, host.getName()); | ||
|
|
||
| // Retrieve LUN name from volume details; if missing, volume may not have been fully created | ||
| VolumeDetailVO lunDetail = volumeDetailsDao.findDetail(volumeVO.getId(), OntapStorageConstants.LUN_DOT_NAME); | ||
| String lunName = lunDetail != null ? lunDetail.getValue() : null; | ||
| if (lunName == null) { | ||
| logger.warn("revokeAccessForVolume: No LUN name found for volume [{}]; skipping revoke", volumeVO.getId()); | ||
| return; | ||
| } | ||
|
|
||
| // Verify LUN still exists on ONTAP (may have been manually deleted) | ||
| CloudStackVolume cloudStackVolume = getCloudStackVolumeByName(storageStrategy, svmName, lunName); | ||
| if (cloudStackVolume == null || cloudStackVolume.getLun() == null || cloudStackVolume.getLun().getUuid() == null) { | ||
| logger.warn("revokeAccessForVolume: LUN for volume [{}] not found on ONTAP, skipping revoke", volumeVO.getId()); | ||
| return; | ||
| } | ||
|
|
||
| // Verify igroup still exists on ONTAP | ||
| AccessGroup accessGroup = getAccessGroupByName(storageStrategy, svmName, accessGroupName); | ||
| if (accessGroup == null || accessGroup.getIgroup() == null || accessGroup.getIgroup().getUuid() == null) { | ||
| logger.warn("revokeAccessForVolume: iGroup [{}] not found on ONTAP, skipping revoke", accessGroupName); | ||
| return; | ||
| } | ||
|
|
||
| // Verify host initiator is in the igroup before attempting to remove mapping | ||
| SANStrategy sanStrategy = (UnifiedSANStrategy) storageStrategy; | ||
| if (!sanStrategy.validateInitiatorInAccessGroup(host.getStorageUrl(), svmName, accessGroup.getIgroup())) { | ||
| logger.warn("revokeAccessForVolume: Initiator [{}] is not in iGroup [{}], skipping revoke", | ||
| host.getStorageUrl(), accessGroupName); | ||
| return; | ||
| } | ||
|
|
||
| // Remove the LUN mapping from the igroup | ||
| Map<String, String> disableLogicalAccessMap = new HashMap<>(); | ||
| disableLogicalAccessMap.put(OntapStorageConstants.LUN_DOT_UUID, cloudStackVolume.getLun().getUuid()); | ||
| disableLogicalAccessMap.put(OntapStorageConstants.IGROUP_DOT_UUID, accessGroup.getIgroup().getUuid()); | ||
| storageStrategy.disableLogicalAccess(disableLogicalAccessMap); | ||
|
|
||
| logger.info("revokeAccessForVolume: Successfully revoked access to LUN [{}] for host [{}]", | ||
| lunName, host.getName()); |
| public Map<String, String> enableLogicalAccess(Map<String, String> values) { | ||
| return null; | ||
| public String enableLogicalAccess(Map<String, String> values) { | ||
| logger.info("enableLogicalAccess : Create LunMap"); |
Contributor
There was a problem hiding this comment.
Are both consecutive log lines needed? Similar pattern is also present in other methods as well
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
Co-authored-by: Sandeep Locharla sandeep.locharla@netapp.com
Co-authored-by: Piyush Srivastava piyush5@netapp.com
Co-authored-by: Surya Gupta suryag@netapp.com
This PR...
1. memory snapshot
2. user input for quicing option during snapshot creation
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Testing Done:
How did you try to break this feature and the system with this change?