CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit Maintenance mode #20

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 ontap volume

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?

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

…Storage pool workflows

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>

… and setting host and path

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

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

eb35e8c

…Storage pool workflows

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

Locharla, Sandeep added 2 commits November 17, 2025 16:49

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

997d03b

… and setting host and path

CSTACKEX-50: Fixed some issues seen while testing

04617d3

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

Copilot AI reviewed Nov 18, 2025

View reviewed changes

Locharla, Sandeep added 2 commits November 18, 2025 14:22

CSTACKEX-50: Fixed few EOF lint issues

a2258a5

CSTACKEX-50: Fixed few lint issues

5b2c577

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

Copilot AI reviewed Nov 18, 2025

View reviewed changes

	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

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit Maintenance mode #20

Are you sure you want to change the base?

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit Maintenance mode #20

Uh oh!

Conversation

sandeeplocharla commented Nov 10, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants