CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit maintenance mode workflows #19

sandeeplocharla · 2025-11-10T13:09:22Z

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?

… to work

…Storage pool workflows

Copilot

Pull Request Overview

This PR implements several critical ONTAP storage pool management workflows including:

Storage pool lifecycle operations (create with underlying ONTAP volume, disable/enable, enter/exit maintenance mode, and delete with volume cleanup)
Migration from Spring Cloud OpenFeign to vanilla Feign clients with custom factory pattern
Enhanced aggregate selection logic to validate state and available space during connection
Protocol naming correction from NFS to NFS3

Key Changes:

Replaced Spring dependency injection with direct FeignClientFactory instantiation for better control
Added volume creation/deletion with job polling mechanism
Implemented storage pool maintenance mode and disable/enable operations
Added comprehensive validation for ONTAP connection details and aggregate selection

Reviewed Changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
Utility.java	Converted utility methods to static, removed unused dependencies
Constants.java	Added new constants for volume operations, query parameters, and ONTAP port
ProtocolType.java	Renamed NFS enum to NFS3 for protocol accuracy
UnifiedSANStrategy.java	Replaced @Inject with FeignClientFactory for client initialization
UnifiedNASStrategy.java	Added FeignClientFactory setup and visibility modifiers to methods
StorageStrategy.java	Added aggregate validation logic, volume deletion, and job polling utility
StorageProviderFactory.java	Refactored to static factory pattern, removed Spring component annotations
OntapPrimaryDatastoreLifecycle.java	Implemented volume creation/deletion, maintenance mode, and enhanced validation
Model classes (OntapResponse, Volume, Svm, Aggregate, etc.)	Added @JsonIgnoreProperties for robust JSON deserialization
Feign clients (VolumeFeignClient, SvmFeignClient, etc.)	Migrated from Spring annotations to vanilla Feign annotations
FeignConfiguration.java	Converted from Spring bean configuration to factory methods
FeignClientFactory.java	New factory class for creating configured Feign clients
OntapPrimaryDatastoreDriver.java	Moved getStrategyByStoragePoolDetails from Utility to driver
pom.xml	Replaced Spring Cloud OpenFeign with vanilla Feign dependencies

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

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Job.java

+        @JsonProperty("self")
        private Self self;


Incorrect @JsonProperty annotation value. The property should be 'self' not 'message', but the field variable is already named 'self'. The annotation on line 90 should map to the actual JSON property name returned by the ONTAP API. Please verify the correct JSON property name from the API documentation.

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/Job.java

+        @JsonProperty("href")
        private String href;


Incorrect @JsonProperty annotation value. The property should be 'href' not 'message', but the field variable is already named 'href'. The annotation on line 97 should map to the actual JSON property name returned by the ONTAP API. Please verify the correct JSON property name from the API documentation.

Copilot · 2025-11-10T13:11:27Z

...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?


Remove or resolve the TODO comment. The return value logic needs to be decided: returning false when storage pool is not found may prevent cleanup of orphaned resources, while returning true could incorrectly signal successful deletion. This should be resolved before merging.

Suggested change

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

// Storage pool does not exist; treat as successful deletion (idempotent)

return true;

Copilot · 2025-11-10T13:11:27Z

...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()) {


Incorrect comparison operator. The condition should use < instead of <= because an aggregate with exactly the required size should be acceptable. The current logic incorrectly skips aggregates that have exactly the needed capacity.

Suggested change

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

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

Copilot · 2025-11-10T13:11:27Z

...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java


-@Lazy
-@FeignClient(name = "SANClient", url = "", configuration = FeignConfiguration.class )
+//TODO: Proper URLs should be added in the RequestLine annotations below


The TODO indicates that proper API endpoint paths are missing from @RequestLine annotations. All SAN client methods use placeholder paths like 'POST /' and 'GET /' instead of actual ONTAP REST API endpoints (e.g., '/api/storage/luns'). These need to be corrected for the client to function properly.

Copilot · 2025-11-10T13:11:28Z

...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/NASFeignClient.java

- */
-@Lazy
-@FeignClient(name = "NASClient", url = "" , configuration = FeignConfiguration.class)
+//TODO: Proper URLs should be added in the RequestLine annotations below


The TODO indicates that proper API endpoint paths are missing from @RequestLine annotations. All NAS client methods use placeholder or incomplete paths instead of full ONTAP REST API endpoints. These need to be corrected for the client to function properly.

Copilot · 2025-11-10T13:11:28Z

...ge/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/SANFeignClient.java

+    @RequestLine("GET /")
+    @Headers({"Authorization: {authHeader}"}) // TODO: Check this again, uuid should be part of the path?


The TODO indicates uncertainty about API design. The uuid parameter appears in the method signature but isn't used in the @RequestLine path. Either remove the uuid parameter if it should be a query parameter, or add it to the path as '/{uuid}' in the @RequestLine annotation.

Suggested change

@RequestLine("GET /")

@Headers({"Authorization: {authHeader}"}) // TODO: Check this again, uuid should be part of the path?

@RequestLine("GET /{uuid}")

@Headers({"Authorization: {authHeader}"})

Copilot · 2025-11-10T13:11:28Z

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

+        // Calling the VolumeFeignClient to delete the volume
+        String authHeader = Utility.generateAuthHeader(storage.getUsername(), storage.getPassword());
+        try {
+            // TODO: Implement lun and file deletion, if any, before deleting the volume


The TODO indicates incomplete implementation. Before deleting an ONTAP volume, any LUNs or files within it should be deleted first to prevent orphaned resources. This logic needs to be implemented to ensure proper cleanup.

Copilot · 2025-11-10T13:11:28Z

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

                s_logger.error("iSCSI protocol is not enabled on SVM " + svmName);
                throw new CloudRuntimeException("iSCSI protocol is not enabled on SVM " + svmName);
            }
+            // TODO: Implement logic to select appropriate aggregate based on storage requirements


Misleading TODO comment. The code following this comment (lines 117-140) already implements aggregate selection logic based on state and available space. The TODO should be removed or updated to reflect what additional selection criteria might be needed.

Suggested change

// TODO: Implement logic to select appropriate aggregate based on storage requirements

Locharla, Sandeep added 6 commits November 3, 2025 11:22

CSTACKEX-1: Feign changes and fixes for getting storage pool creation…

ec1707b

… to work

CSTACKEX-01: Create Primary Storage pool changes with working code

b855306

CSTACKEX-01: Addressed all review comments and updated some code

fd67a41

CSTACKEX-01: Made some changes to fix some errors seen during testing

43c3684

CSTACKEX-01: Addressed additional comments

80b24ca

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

accb2bf

…Storage pool workflows

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

sandeeplocharla closed this Nov 10, 2025

sandeeplocharla deleted the feature/CSTACKEX-50 branch November 10, 2025 13:11

sandeeplocharla restored the feature/CSTACKEX-50 branch November 10, 2025 13:11

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit maintenance mode workflows #19

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit maintenance mode workflows #19

Uh oh!

sandeeplocharla commented Nov 10, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Copilot AI Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return false;// TODO: As the CS entity is not present, should we return true here?
	// Storage pool does not exist; treat as successful deletion (idempotent)
	return true;

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

		@RequestLine("GET /")
		@Headers({"Authorization: {authHeader}"}) // TODO: Check this again, uuid should be part of the path?

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit maintenance mode workflows #19

CSTACKEX-50: Disable, Re-Enable, Delete Storage pool and Enter, Exit maintenance mode workflows #19

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 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 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants