Repository navigation
NetApp ONTAP ASUP Support - #14193
suryag1201 wants to merge 1 commit into
Conversation
|
@surya, I suggest we explicitly call out that the supportability scope is limited to storage systems managed using cluster-based credentials. This will help set clear expectations and avoid ambiguity around other authentication or management models. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #14193 +/- ##
=============================================
- Coverage 20.75% 3.88% -16.88%
=============================================
Files 6496 493 -6003
Lines 591602 42571 -549031
Branches 72583 8069 -64514
=============================================
- Hits 122814 1653 -121161
+ Misses 455456 40687 -414769
+ Partials 13332 231 -13101
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
lock.unlock()is executed even whenlock.lock(...)returns false (earlyreturn), which can… · New The DAO method is documented as “non-destroyed” volumes, but the SQL currently includes volumes in… · New This treats a missing/unknownis_all_flash_optimized(null) asfalse, which classifies the node… · New This allocates an intermediate list and then traverses it three more times. Since this runs once… · New
What changed in this PR
Adds AutoSupport (ASUP) telemetry reporting for the ONTAP primary-storage plugin, including a new global config key to control the reporting interval and best-effort EMS message publishing per cluster (event-id 0) and per pool (event-id 1).
Changes:
- Introduces
OntapAsupManager+OntapConfigurationManagerand wires them into the ONTAP plugin Spring context. - Extends ONTAP Feign support (
ClusterFeignClientnodes endpoint, newEmsFeignClient, new models) and addsStorageStrategyhelpers to fetch cluster/node info and send EMS logs. - Adds DAO support + tests for multi-pool VM detection and ASUP payload correctness/interval validation.
| File | Description |
|---|---|
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java | Adds unit tests for new StorageStrategy.getClusterInfo() model/platform rollups. |
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/asup/OntapAsupManagerTest.java | New test suite validating ASUP event routing, payload fields, interval validation, and scheduling behavior. |
| plugins/storage/volume/ontap/src/main/resources/META-INF/cloudstack/storage-volume-ontap/spring-storage-volume-ontap-context.xml | Registers new ASUP/config manager beans in plugin context. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageUtils.java | Splits strategy creation into “resolve without connect” vs “resolve + connect” paths for ASUP. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java | Adds constants for ASUP payload keys, interval config, and node fields selector. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapConfigurationManager.java | New ValidatedConfigKey registration + validator for ontap.autosupport.interval. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java | Adds cluster/node info retrieval, ONTAP version formatting, and EMS ASUP send method. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/EmsApplicationLog.java | New model for EMS application-log POST payload. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ClusterNode.java | New model for GET /api/cluster/nodes subset used by ASUP. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Cluster.java | Adds non-serialized model / platformType fields for ASUP rollups. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/EmsFeignClient.java | New Feign client for POST /api/support/ems/application-logs. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/ClusterFeignClient.java | Adds GET /api/cluster/nodes endpoint support. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/asup/OntapAsupManager.java | New scheduled manager that composes and pushes ASUP telemetry messages. |
| plugins/storage/volume/ontap/pom.xml | Adds dependency needed for management-server host counting. |
| engine/schema/src/main/java/com/cloud/storage/dao/VolumeDaoImpl.java | Adds existence query to detect multi-pool VMs (root on pool, data on another). |
| engine/schema/src/main/java/com/cloud/storage/dao/VolumeDao.java | Exposes hasMultiPrimaryStoragePoolVm(poolId) in DAO API. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
### Description Upstream PR : apache#14193 1- This allocates an intermediate list and then traverses it three more times. Since this runs once per pool per interval, on very large pools this adds avoidable CPU/GC overhead. Consider computing rootDiskCount, dataDiskCount, and totalLogicalSizeBytes in a single pass over volumes (filtering by CS_VOLUME_STATES inline) to reduce allocations and repeated streaming. 2- This treats a missing/unknown is_all_flash_optimized (null) as false, which classifies the node as fas and can incorrectly set cluster platformType when ONTAP doesn’t return these fields (permissions/older versions/partial responses). Consider returning null (unknown) when the required flags are absent and only adding non-null classifications to the rollup set so ASUP doesn’t misreport hardware type. 3- The DAO method is documented as “non-destroyed” volumes, but the SQL currently includes volumes in transitional/pre-provisioning states (e.g., Allocated) because it only excludes Destroy/Expunged. That can incorrectly flip multiPrimaryStoragePoolVm=true for VMs whose data disk hasn’t actually been created/moved yet. Tighten the state predicates to align with the intended “physically exists” semantics (e.g., include only the same stable states you count elsewhere, or explicitly exclude Allocated/upload-related states). 4- lock.unlock() is executed even when lock.lock(...) returns false (early return), which can attempt to unlock a lock this thread never acquired. Additionally, GlobalLock.getInternLock(...) typically requires releasing the reference (e.g., releaseRef()) to avoid leaking the interned lock object. Track a boolean locked and only unlock() when true, and ensure the lock reference is released in finally (even when not acquired). fixed the above issues <!--- ******************************************************************************* --> <!--- 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): ### 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>
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@suryag1201 could you please resolve the conflicts? |
@RosiKyu |
|
@rajiv-jain-netapp, @piyush5netapp , @sathvikaragi , @sandeeplocharla , this PR needs two reviews to be included in the 24.0 release. Could any of you help review it? Thanks! |
rajiv-jain-netapp
left a comment
There was a problem hiding this comment.
I had reviewed these changes
Co-authored-by: Cursor <cursoragent@cursor.com>
155171b to
d8bdc41
Compare



Description
Adds AutoSupport (ASUP) telemetry for the ONTAP primary-storage plugin. On a configurable interval, It send below 2 details per cycle:
As a part of Event-0 (once per cluster): CloudStack connected to Unified ONTAP cluster — CloudStack version, OS, ONTAP version, cluster model, platform type, cluster UUID, management-server count.
As a part of Event-1 (once per pool): CloudStack storage pool backed by Unified ONTAP flexvolume — pool name/status, protocol, SVM, ONTAP volume UUID, root/data disk counts, logical size, VM/volume snapshot counts, multi-pool VM flag.
Scope: AutoSupport telemetry is supported only for ONTAP storage systems using cluster-based credentials.
Configuration
Global setting (UI: Configuration → Global Settings):
Key: ontap.autosupport.interval
Default : 24 hours (once a day)
Allowed values: 0 = disabled; otherwise 4–168 hours (1 week)
Validation: Out-of-range or non-integer values are rejected in the UI.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Set ontap.autosupport.interval to 0 — no data sent.
Set it to 24 (or any value 4–168) — event-0 once per cluster, event-1 once per pool, on that interval.
Set it below 4 or above 168 — UI validation error.
Create VMs/snapshots (including a VM across pools) and confirm the next event -1 payload updates.
Set back to 0 — events stop. Management server restart is not required.
How did you try to break this feature and the system with this change?