Create & Delete, Enable & Disable, Enter & Cancel maintenance of Prim…#12563
Create & Delete, Enable & Disable, Enter & Cancel maintenance of Prim…#12563sandeeplocharla wants to merge 1 commit intoapache:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12563 +/- ##
============================================
- Coverage 17.89% 17.83% -0.06%
- Complexity 16084 16118 +34
============================================
Files 5936 5979 +43
Lines 532635 535613 +2978
Branches 65148 65370 +222
============================================
+ Hits 95298 95538 +240
- Misses 426667 429379 +2712
- Partials 10670 10696 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c495c7c to
20d4e97
Compare
20d4e97 to
c98cf8c
Compare
|
@blueorangutan package |
|
@kiranchavala 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 16683 |
|
@blueorangutan test |
|
@kiranchavala a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Hey @sandeeplocharla et al, Great work, some issues came up in the github actions. Can you first look at the license check and the pre-commit actions? |
|
Hi @DaanHoogland |
Thank you, @DaanHoogland. We look forward to receiving your review comments and will address them as soon as possible. |
08b53f0 to
b1792b4
Compare
|
Hi, the project level Code coverage seems to be failing, due to mutliple files that this code is indirectly touching, doesn't have coverage. What could be done in such a case? |
@sandeeplocharla , we have ambitions on coverage that we don’t meet historically. Please trty to meet them on your part of the code, more we cannot ask. As for the failing tests, these seem te be the same that are failing on main as well. If you can give us any hints on them that will be very much appreciated ;) |
There was a problem hiding this comment.
did you do license checks on the extra dependencies added in this file?
see https://apache.org/legal/resolved.html for explanation.
There was a problem hiding this comment.
Will check the licenses and get back to you.
There was a problem hiding this comment.
Hi @DaanHoogland , I've cross-checked the licenses of the dependencies added in our pom.xml. I had to remove one which wasn't conforming with ACF guidelines that you've shared.
Currently, all licenses in the ONTAP plugin are compliant with Apache CloudStack norms. All runtime dependencies use Apache License 2.0 (Category A). Test dependencies use MIT and EPL 2.0 licenses, which are also Category A compatible. No Category X licenses are present.
This PR does not have the benchmark coverage that we usually aim for within our team. But, we plan on having maximum code coverage possible with the upcoming PRs. As for this PR, there are around 5400 indirect files because of which the coverage dropped. I'm not sure, how we would able to handle them all. |
|
[SF] Trillian test result (tid-15359)
|
this is good enough until we encounter tangible issues. Please continue with the comment outstanding now. review effort will have to be made and I (or we) will hopefully get to this soon. Please be prepared for some rebasing and conflict resolving over the coming weeks 🥺 … unfortunately I cannot promise when further review will happen, but this PR is on my list. |
…ary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain <Rajiv.Jain@netapp.com> Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain<rajiv1@netapp.com> Edited readme file Fixed license check issue Removed dependency that's not conforming with ACF guidelines
b1792b4 to
b8e5cd6
Compare
|
@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 ✖️ debian ✖️ suse15. SL-JID 16871 |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16874 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15500)
|
Hello @DaanHoogland, let us know if this PR has to be rebased or if anything else should be done for this PR? |
|
Thanks @sandeeplocharla great work! An additional question about the functionalities, would SnapMirror be supported for volume replication? In case it is not, are there any plans to add support for it? |
No, @harikrishna-patnala @sureshanaparti @weizhouapache , can you review please? |
There was a problem hiding this comment.
Pull request overview
Adds a new NetApp ONTAP primary storage plugin module to CloudStack, implementing primary storage pool lifecycle operations (create/delete, enable/disable, maintenance) and ONTAP REST integrations for NFS3 and iSCSI.
Changes:
- Introduces the new
storage/volume/ontapplugin module (provider, lifecycle, driver, host listener, strategy layer). - Adds Feign REST clients + ONTAP model classes used by the strategies.
- Adds initial unit tests and wires the plugin into the build (
plugins/pom.xml,client/pom.xml).
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/provider/OntapPrimaryDatastoreProviderTest.java | Adds unit tests for provider configuration and getters. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycleTest.java | Adds unit tests for lifecycle initialization validation paths. |
| plugins/storage/volume/ontap/src/main/resources/META-INF/cloudstack/storage-volume-ontap/spring-storage-volume-ontap-context.xml | Registers the ONTAP primary datastore provider bean for the plugin. |
| plugins/storage/volume/ontap/src/main/resources/META-INF/cloudstack/storage-volume-ontap/module.properties | Declares the plugin module name and parent module. |
| plugins/storage/volume/ontap/src/main/resources/META-INF/cloudstack/storage-volume-ontap/logback-spring.xml | Adds a Logback configuration intended for Feign logging. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java | Adds helper methods (auth header generation, strategy lookup, naming helpers). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Constants.java | Centralizes ONTAP constants (keys, ports, REST params, naming tokens). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/ProtocolType.java | Defines supported protocol enum values (NFS3, ISCSI). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java | Adds a container model for CloudStack<->ONTAP volume/LUN/file data. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java | Adds a model representing an iGroup/export-policy plus target hosts/scope. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java | Implements SAN-side access group creation/deletion (iGroup) scaffolding. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java | Implements NAS-side access group creation/deletion (export policy) logic. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java | Implements ONTAP connectivity validation, volume create/delete, and helpers. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/SANStrategy.java | Adds SAN strategy base class. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/NASStrategy.java | Adds NAS strategy base class. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/StorageProviderFactory.java | Selects the correct strategy (Unified NAS/SAN) based on protocol/config. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/provider/OntapPrimaryDatastoreProvider.java | Implements the primary datastore provider for ONTAP. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java | Handles host connect/disconnect events to mount/unmount the primary pool. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java | Implements pool lifecycle: initialize/create, attach cluster/zone, maintain, delete. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/response/OntapResponse.java | Adds generic ONTAP list response wrapper. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/response/JobResponse.java | Adds ONTAP job wrapper for async operations. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeSpaceLogicalSpace.java | Adds volume logical-space model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeSpace.java | Adds volume space model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/VolumeQosPolicy.java | Adds QoS policy model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Volume.java | Adds ONTAP volume model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Version.java | Adds cluster version model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Svm.java | Adds SVM model (protocol enabled flags, aggregates, etc.). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Qos.java | Adds QoS wrapper model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Policy.java | Adds QoS policy model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/OntapStorage.java | Adds configuration model for ONTAP connectivity and protocol selection. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Nas.java | Adds NAS settings model (path, export policy). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/LunSpace.java | Adds LUN space model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/LunMap.java | Adds LUN mapping model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Lun.java | Adds LUN model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Job.java | Adds job/status model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IscsiService.java | Adds iSCSI service/target model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IpInterface.java | Adds network interface (LIF) model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Initiator.java | Adds initiator model for iGroups. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Igroup.java | Adds iGroup model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/FileInfo.java | Adds NAS file model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java | Adds export rule model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportPolicy.java | Adds export policy model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Cluster.java | Adds cluster model. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/AntiRansomware.java | Adds anti-ransomware model field wrapper. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Aggregate.java | Adds aggregate model with space/state helpers. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/VolumeFeignClient.java | Adds volume REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SvmFeignClient.java | Adds SVM REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java | Adds SAN REST API client interface (iSCSI, iGroups, LUNs, maps). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NetworkFeignClient.java | Adds network LIF REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java | Adds NAS REST API client interface (files, export policies). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/JobFeignClient.java | Adds job polling REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/ClusterFeignClient.java | Adds cluster REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/AggregateFeignClient.java | Adds aggregate REST API client interface. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/FeignConfiguration.java | Adds Feign client wiring (HTTP client, encoder/decoder, retry, interceptor). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/FeignClientFactory.java | Central factory for building Feign clients with the shared configuration. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java | Adds a primary datastore driver stub with basic capabilities. |
| plugins/storage/volume/ontap/pom.xml | Adds the new module Maven build config and dependencies (Feign, Jackson, tests). |
| plugins/storage/volume/ontap/README.md | Documents plugin features, requirements, configuration and limitations. |
| plugins/pom.xml | Registers storage/volume/ontap as a plugins reactor module. |
| client/pom.xml | Adds the ONTAP plugin artifact to the client module dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String authHeader = Utility.generateAuthHeader(storage.getUsername(), storage.getPassword()); | ||
| String svmName = storage.getSvmName(); | ||
| String exportPolicyName = primaryDataStoreInfo.getDetails().get(Constants.EXPORT_POLICY_NAME); | ||
| String exportPolicyId = primaryDataStoreInfo.getDetails().get(Constants.EXPORT_POLICY_ID); | ||
|
|
||
| try { | ||
| nasFeignClient.deleteExportPolicyById(authHeader,exportPolicyId); | ||
| s_logger.info("deleteAccessGroup: Successfully deleted export policy '{}'", exportPolicyName); |
There was a problem hiding this comment.
exportPolicyId is read from primaryDataStoreInfo.getDetails() and used directly in deleteExportPolicyById without validating it. If the detail is missing/empty (or details weren’t loaded), this will result in an invalid ONTAP call and a hard failure. Please validate exportPolicyId (and/or fall back to lookup by exportPolicyName) before attempting deletion, and handle the "already deleted" case gracefully.
| public static String generateAuthHeader (String username, String password) { | ||
| byte[] encodedBytes = Base64Utils.encode((username + AUTH_HEADER_COLON + password).getBytes()); | ||
| return BASIC + StringUtils.SPACE + new String(encodedBytes); | ||
| } |
There was a problem hiding this comment.
generateAuthHeader uses platform-default encoding for both getBytes() and new String(encodedBytes), which can lead to incorrect auth headers on non-UTF-8 platforms. Use an explicit charset (e.g., UTF-8) for both steps to make the header generation deterministic.
| case NFS3: | ||
| String ip = ""; | ||
| for (HostVO host : hosts) { | ||
| if (host != null) { | ||
| ip = host.getStorageIpAddress() != null ? host.getStorageIpAddress().trim() : ""; | ||
| if (ip.isEmpty() && host.getPrivateIpAddress() != null || host.getPrivateIpAddress().trim().isEmpty()) { | ||
| return false; | ||
| } else { | ||
| ip = ip.isEmpty() ? host.getPrivateIpAddress().trim() : ip; | ||
| } |
There was a problem hiding this comment.
In the NFS3 branch, the condition if (ip.isEmpty() && host.getPrivateIpAddress() != null || host.getPrivateIpAddress().trim().isEmpty()) has incorrect operator precedence and can throw an NPE when getPrivateIpAddress() is null. It also returns false when ip is empty but privateIpAddress is present (the opposite of what you want). Please rewrite this logic with proper null/empty checks and parentheses so it correctly falls back to privateIpAddress when storageIpAddress is missing.
| private SSLConnectionSocketFactory getSSLSocketFactory() { | ||
| try { | ||
| SSLContext sslContext = SSLContexts.custom().loadTrustMaterial(null, new TrustAllStrategy()).build(); | ||
| return new SSLConnectionSocketFactory(sslContext, new NoopHostnameVerifier()); | ||
| } catch (Exception ex) { | ||
| throw new RuntimeException(ex); | ||
| } |
There was a problem hiding this comment.
The Feign HTTP client is configured to trust all TLS certificates and disable hostname verification unconditionally (TrustAllStrategy + NoopHostnameVerifier). This undermines HTTPS security for ONTAP credentials and traffic. Please make TLS verification the default and only allow skipping verification via an explicit, opt-in configuration setting.
| public RequestInterceptor createRequestInterceptor() { | ||
| return template -> { | ||
| logger.info("Feign Request URL: {}", template.url()); | ||
| logger.info("HTTP Method: {}", template.method()); | ||
| logger.info("Headers: {}", template.headers()); | ||
| if (template.body() != null) { | ||
| logger.info("Body: {}", new String(template.body(), StandardCharsets.UTF_8)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The request interceptor logs full headers and request bodies at INFO. This will include the Authorization header (Basic auth) and may log sensitive payload fields, leaking credentials into logs. Please redact sensitive headers (at least Authorization) and consider lowering to DEBUG/TRACE or making request logging opt-in.
| if (Objects.equals(storage.getProtocol(), Constants.NFS) && !svm.getNfsEnabled()) { | ||
| s_logger.error("NFS protocol is not enabled on SVM " + svmName); | ||
| return false; | ||
| } else if (Objects.equals(storage.getProtocol(), Constants.ISCSI) && !svm.getIscsiEnabled()) { | ||
| s_logger.error("iSCSI protocol is not enabled on SVM " + svmName); | ||
| return false; |
There was a problem hiding this comment.
storage.getProtocol() is a ProtocolType, but the checks compare it to Constants.NFS/Constants.ISCSI (strings). This condition will never be true, so protocol enablement on the SVM is effectively not validated. Compare against ProtocolType.NFS3 / ProtocolType.ISCSI (or convert consistently) so an SVM with the protocol disabled fails validation as intended.
| s_logger.info("Selected aggregate: " + aggr.getName() + " for volume operations."); | ||
| this.aggregates = List.of(aggr); | ||
| break; | ||
| } | ||
| if (this.aggregates == null || this.aggregates.isEmpty()) { | ||
| s_logger.error("No suitable aggregates found on SVM " + svmName + " for volume creation."); | ||
| return false; | ||
| } | ||
|
|
||
| this.aggregates = aggrs; | ||
| s_logger.info("Successfully connected to ONTAP cluster and validated ONTAP details provided"); |
There was a problem hiding this comment.
The aggregate selection logic sets this.aggregates = List.of(aggr) when it finds a suitable aggregate, but then immediately overwrites it with this.aggregates = aggrs. This makes the earlier validation/selection misleading and can allow later operations to consider aggregates that were just deemed unsuitable. Either keep only the filtered/selected aggregates, or remove the initial selection loop and rely on createStorageVolume to choose the aggregate.
...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java
Show resolved
Hide resolved
...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java
Show resolved
Hide resolved
| } catch (Exception feignEx) { | ||
| String errMsg = feignEx.getMessage(); | ||
| if (errMsg != null && errMsg.contains(("5374023"))) { | ||
| s_logger.warn("createAccessGroup: Igroup with name {} already exists. Fetching existing Igroup.", igroupName); | ||
| return createdAccessGroup; | ||
| } |
There was a problem hiding this comment.
When ONTAP returns error code 5374023 (iGroup already exists), the code logs that it will fetch the existing iGroup but then returns an empty AccessGroup without setting the iGroup. This can hide real issues and makes the return value misleading. Either fetch and set the existing iGroup (e.g., via getIgroupResponse) or clearly document/return null and let callers handle the "already exists" case.
|
@sandeeplocharla , can you review co-pilot’s sugestions and close/refuse or implement them? |
Hi @nvazquez , we have yet to plan the replication features for the upcoming PRs. However, SnapMirror support will be one of the key ONTAP capabilities that we intend to leverage for the plugin‑side workflow. |
|
Many thanks @rajiv-jain-netapp @sandeeplocharla |
Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage
Co-authored-by: Rajiv Jain rajiv1@netapp.com
Co-authored-by: Piyush Srivastava piyush5@netapp.com
Co-authored-by: Surya Gupta suryag@netapp.com
Description
NetApp ONTAP Storage Plugin
Introduction
This document describes in brief the design and implementation of the NetApp ONTAP storage plugin for Apache CloudStack. The plugin enables CloudStack to use NetApp ONTAP as a primary storage provider.
ONTAP Terminology
Scope
Current Implementation
The following operations are currently implemented:
Supported Configurations
Storage Pool Lifecycle
Create Storage Pool
Attach to Cluster/Zone
Delete Storage Pool
Configuration Parameters
Parameters are passed via URL in semicolon-separated key=value format:
usernamepasswordmanagementLIFsvmNameprotocolNFS3orISCSIThere are few files under feign folder which aren't currently being consumed by the workflows being proposed for review. But, they are definitely required for upcoming workflows.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Management Server Operations:
Primary Storage Pool Operations:
Testing Environment:
NFS3 Primary Storage Pool - CreateNFS3 type ONTAP Volume createdExport Policy created on ONTAPDisabled Primary Storage Pool - NFS3Re-Enabled Primary Storage Pool - NFS3Enter maintenance mode - NFS3Cancel maintenance mode - NFS3Delete Primary StoragePool - NFS3ONTAP Volume deletedONTAP ExportPolicy deletediSCSI Primary Storage Pool - DeleteiSCSI type ONTAP Volume creatediGroup createdDisabled Primary Storage Pool - iSCSIRe-Enabled Primary Storage Pool - iSCSIEnter maintenance mode - iSCSICancel maintenance mode - iSCSIDelete Primary StoragePool - iSCSIONTAP iGroup deletedONTAP Volume deleted