CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit Maintenance mode by sandeeplocharla · Pull Request #20 · NetApp/cloudstack

sandeeplocharla · 2025-11-10T13:25:16Z

…Storage pool workflows

Description

This PR has the following changes implemented:

Pick a single aggregate instead of passing the entire array of aggregates while creating a storage pool
Disable and re-enable storage pool
Enter and exit maintenance mode
Delete primary storage pool and the underlying igroup

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
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

Screenshots (if appropriate):

How Has This Been Tested?

Create Primary storage pool of type iscsi (Cluster scoped)

Corresponding igroup created on ontap

StoragePool details

Disable StoragePool

Enable StoragePool

Enter Maintenance mode

Cancel Maintenance mode

Enter Maintenance mode and Delete StoragePool

 Igroups on Ontap

Create Primary StoragePool of type iscsi (Zone scoped)

Avg time taken for each operation:

Note: An average of 3 trials has been considered and the system tested on has both Management Server, Cloudstack Agent and host running on the same machine, though on different network interfaces.

cluster scoped SP => 5s 48ms
zone scoped SP => 5s 79ms
disable cluster scoped SP => ~ 54ms
disable zone scoped SP => ~ 56ms
enable cluster scoped SP => ~ 50ms
enable zone scoped SP => ~ 40ms
enable maintenance mode on cluster scoped SP => 3s 58ms
enable maintenance mode on zone scoped SP => 3s 52ms
cancel maintenance mode on cluster scoped SP => 3s 59ms
cancel maintenance mode on zone scoped SP => 3s 50ms
delete cluster scoped SP => 3s
delete zone scoped SP => 2s 85ms

How did you try to break this feature and the system with this change?

Copilot

Pull Request Overview

This PR implements storage pool lifecycle management operations for ONTAP integration, enabling users to manage storage pools through disable/re-enable, maintenance mode, and deletion workflows.

Key Changes:

Enhanced aggregate selection logic to choose a single suitable aggregate based on online state and available space
Implemented storage pool lifecycle operations: enable, disable, maintenance mode (enter/exit), and deletion
Added underlying ONTAP volume deletion when removing storage pools

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Constants.java	Added constants for volume name and UUID tracking
StorageStrategy.java	Implemented aggregate selection, volume creation/deletion, and job polling logic
OntapPrimaryDatastoreLifecycle.java	Implemented lifecycle methods for enable, disable, maintain, cancelMaintain, and deleteDataStore
OntapStorage.java	Added size field to OntapStorage model
Job.java	Fixed JSON property annotations for Links and Self classes
Aggregate.java	Added state enum, space tracking, and methods to check aggregate availability
VolumeFeignClient.java	Added getAllVolumes method and updated deleteVolume return type
FeignConfiguration.java	Switched from JsonMapper to ObjectMapper and added Apache license header
OntapPrimaryDatastoreDriver.java	Updated OntapStorage constructor call to include size parameter
pom.xml	Downgraded jackson-databind version from 2.15.2 to 2.13.4

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-10T13:26:05Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+                    continue;
+                }
+                s_logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
+                this.aggregates = List.of(aggr);


The space comparison uses <= which will reject aggregates that have exactly the required size. This should use < instead to allow aggregates with space equal to the required size.

Copilot · 2025-11-10T13:26:06Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

+        StoragePool storagePool = _storageMgr.getStoragePool(storagePoolId);
+        if (storagePool == null) {
+            s_logger.warn("Storage pool not found for id: " + storagePoolId + ", cannot delete underlying ONTAP volume");
+            return false;// TODO: As the CS entity is not present, should we return true here?


[nitpick] Remove the TODO comment or move it to a separate line. Inline TODO comments can make the code harder to read and should be placed on their own line above the code they reference.

Suggested change

return false;// TODO: As the CS entity is not present, should we return true here?

// TODO: As the CS entity is not present, should we return true here?

return false;

Copilot · 2025-11-10T13:26:06Z

plugins/storage/volume/ontap/pom.xml

        <openfeign.version>11.0</openfeign.version>
        <json.version>20230227</json.version>
-        <jackson-databind.version>2.15.2</jackson-databind.version>
+        <jackson-databind.version>2.13.4</jackson-databind.version>


Downgrading jackson-databind from 2.15.2 to 2.13.4 may introduce known security vulnerabilities. Version 2.13.4 was released in September 2022 and several CVEs have been fixed in later versions. Consider using a more recent version or document the reason for this downgrade.

Suggested change

<jackson-databind.version>2.13.4</jackson-databind.version>

<jackson-databind.version>2.15.2</jackson-databind.version>

Copilot

Pull Request Overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-18T06:02:36Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+            if (storage.getProtocol() != null) {
+                switch (storage.getProtocol()) {
+                    case NFS3:
+                        queryParams .put(Constants.SERVICES, Constants.DATA_NFS);


Extra space before the dot operator in queryParams .put. Should be queryParams.put.

Copilot · 2025-11-18T06:02:37Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

+        StoragePool storagePool = _storageMgr.getStoragePool(storagePoolId);
+        if (storagePool == null) {
+            s_logger.warn("Storage pool not found for id: " + storagePoolId + ", cannot delete underlying ONTAP volume");
+            return false;// TODO: As the CS entity is not present, should we return true here?


Missing space between false; and the comment. Should be return false; // TODO:.

Copilot · 2025-11-18T06:02:37Z

...olume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NetworkFeignClient.java

@@ -0,0 +1,16 @@
+package org.apache.cloudstack.storage.feign.client;


Missing Apache license header. All source files should include the standard Apache Software Foundation license header at the top of the file.

Copilot

Pull Request Overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-18T08:55:56Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

+        StoragePool storagePool = _storageMgr.getStoragePool(storagePoolId);
+        if (storagePool == null) {
+            s_logger.warn("Storage pool not found for id: " + storagePoolId + ", cannot delete underlying ONTAP volume");
+            return false;// TODO: As the CS entity is not present, should we return true here?


The inline TODO comment creates ambiguity about the correct return value. Move this TODO to a separate line above the return statement or resolve the question before merging.

Suggested change

return false;// TODO: As the CS entity is not present, should we return true here?

// TODO: As the CS entity is not present, should we return true here?

return false;

Copilot · 2025-11-18T08:55:56Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+                    s_logger.warn("Aggregate " + aggr.getName() + " is not in online state. Skipping this aggregate.");
+                    continue;
+                } else if (aggrResp.getSpace() == null || aggrResp.getAvailableBlockStorageSpace() == null ||
+                        aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {


The space comparison is inverted. An aggregate should be skipped if available space is less than required size, but the condition uses <= which will skip aggregates with exactly enough space. Change to < or better yet < storage.getSize().doubleValue() to allow exact matches.

Suggested change

aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {

aggrResp.getAvailableBlockStorageSpace() < storage.getSize().doubleValue()) {

Copilot · 2025-11-18T08:55:57Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+                    continue;
+                }
+                s_logger.info("Selected aggregate: " + aggr.getName() + " for volume operations.");
+                this.aggregates = List.of(aggr);


The loop continues after finding the first suitable aggregate but the assignment inside the loop overwrites this.aggregates each time. Either add a break statement after line 140, or restructure the logic to set aggregates only once outside the loop after finding a suitable one.

Suggested change

this.aggregates = List.of(aggr);

this.aggregates = List.of(aggr);

break;

Copilot · 2025-11-18T08:55:58Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

+            OntapResponse<IpInterface> response =
+                    networkFeignClient.getNetworkIpInterfaces(authHeader, queryParams);
+            if (response != null && response.getRecords() != null && !response.getRecords().isEmpty()) {
+                // For simplicity, return the first interface's name


Comment says 'return the first interface's name' but the code returns the IP address, not the name. Update comment to 'return the first interface's IP address'.

Suggested change

// For simplicity, return the first interface's name

// For simplicity, return the first interface's IP address

…Storage pool workflows

… and setting host and path

…me methods according to the latest design

rajiv-jain-netapp · 2025-12-05T05:20:08Z

plugins/storage/volume/ontap/pom.xml

same changes are available in Piyush's PR also. whoever is merging second, do ensure to take latest and remove redundant changes before merge

rajiv-jain-netapp · 2025-12-05T05:21:42Z

...orage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/FeignConfiguration.java

same as earlier comment

rajiv-jain-netapp · 2025-12-05T05:29:01Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

file did not load

File is still not loading

rajiv-jain-netapp · 2025-12-05T05:29:52Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java

file did not load

rajiv-jain-netapp

Some of your files are not appearing for review. Let’s plan an online review during our upcoming scrum to cover the missing files.
In the test results you shared, I noticed references to cloudstack-volume. This seems incorrect—you are actually referring to the storage pool. Please update that accordingly.
Also, make sure to capture the time taken for each operation in your results.
I’m approving your changes for now, considering them as a partial implementation.

rajiv-jain-netapp · 2025-12-08T04:39:31Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IpInterface.java

+            return false;
+        }
+        IpInterface that = (IpInterface) o;
+        return Objects.equals(uuid, that.uuid) &&


this can be optimized by using only UUID feild instead using all the fields

rajiv-jain-netapp · 2025-12-08T04:40:09Z

...torage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/IpInterface.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(uuid, name, ip, svm, services);


this method would also change as per the updates doenf rot he equals method.

rajiv-jain-netapp · 2025-12-08T04:40:45Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

File is still not loading

rajiv-jain-netapp · 2025-12-08T04:43:57Z

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java

+
+    @Override
+    public boolean hostRemoved(long hostId, long clusterId) {
+        logger.info("hostRemoved: Host {} removed from cluster {}", hostId, clusterId);


Ithese loggers are added for now to debug the workflow, lets keep them as debug

rajiv-jain-netapp · 2025-12-08T05:08:33Z

...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java

+        String path;
+        int port;
+        switch (protocol) {
+            case NFS3:


I noticed that we have replaced NFS3 with NFS in some places. Keep them the same. I am inclined to keep these as NFS3, though.

…Maintenance mode (#20) * CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit Storage pool workflows * CSTACKEX-50: Changes for selecting aggregate, retrieving ip interface and setting host and path * CSTACKEX-50: Fixed some issues seen while testing * CSTACKEX-50: Fixed few EOF lint issues * CSTACKEX-50: Fixed few lint issues * CSTACKEX-50: Included changes for Delete storage pool and modified some methods according to the latest design * CSTACKEX-050: Rebased the code with the latest in master * CSTACKEX-050: Fixed lint issues * CSTACKEX-050: Added some missing changes --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>

Copilot AI review requested due to automatic review settings November 10, 2025 13:25

sandeeplocharla self-assigned this Nov 10, 2025

Copilot AI reviewed Nov 10, 2025

View reviewed changes

sandeeplocharla requested review from piyush5netapp, rajiv-jain-netapp and suryag1201 November 10, 2025 13:26

Copilot AI review requested due to automatic review settings November 18, 2025 06:02

Copilot AI reviewed Nov 18, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 18, 2025 08:54

Copilot AI reviewed Nov 18, 2025

View reviewed changes

Locharla, Sandeep added 7 commits December 4, 2025 21:39

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit …

00e8a25

…Storage pool workflows

CSTACKEX-50: Changes for selecting aggregate, retrieving ip interface…

59224d0

… and setting host and path

CSTACKEX-50: Fixed some issues seen while testing

5f8f744

CSTACKEX-50: Fixed few EOF lint issues

ffa4927

CSTACKEX-50: Fixed few lint issues

bd107b3

CSTACKEX-50: Included changes for Delete storage pool and modified so…

1b0dbba

…me methods according to the latest design

CSTACKEX-050: Rebased the code with the latest in master

1809077

sandeeplocharla force-pushed the feature/CSTACKEX-050 branch from 96c2aa2 to 1809077 Compare December 4, 2025 16:10

CSTACKEX-050: Fixed lint issues

9319777

rajiv-jain-netapp reviewed Dec 5, 2025

View reviewed changes

CSTACKEX-050: Added some missing changes

a4347d6

rajiv-jain-netapp approved these changes Dec 8, 2025

View reviewed changes

sandeeplocharla merged commit b23ac40 into main Dec 8, 2025
11 of 16 checks passed

	return false;// TODO: As the CS entity is not present, should we return true here?
	// TODO: As the CS entity is not present, should we return true here?
	return false;

	<jackson-databind.version>2.13.4</jackson-databind.version>
	<jackson-databind.version>2.15.2</jackson-databind.version>

		@@ -0,0 +1,16 @@
		package org.apache.cloudstack.storage.feign.client;

	aggrResp.getAvailableBlockStorageSpace() <= storage.getSize().doubleValue()) {
	aggrResp.getAvailableBlockStorageSpace() < storage.getSize().doubleValue()) {

	this.aggregates = List.of(aggr);
	this.aggregates = List.of(aggr);
	break;

	// For simplicity, return the first interface's name
	// For simplicity, return the first interface's IP address

Conversation

sandeeplocharla commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajiv-jain-netapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

sandeeplocharla commented Nov 10, 2025 •

edited

Loading