CSTACKEX-46 Create Async, Attach Cluster/Zone and Grant/Revoke Access by suryag1201 · Pull Request #17 · NetApp/cloudstack

suryag1201 · 2025-11-06T03:23:57Z

Description

Create Async, Attach Cluster/Zone and Grant/Revoke Access

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?

Copilot

Pull Request Overview

This PR implements asynchronous volume creation, cluster/zone attachment with access group management, and grant/revoke access operations for ONTAP storage integration. The changes introduce iSCSI protocol support with proper LUN mapping and unmapping capabilities.

Key Changes:

Implemented access group (igroup) creation, retrieval, and management for SAN protocols
Added logical access control through LUN mapping/unmapping operations
Enhanced cluster/zone attachment flows with host identifier validation and access group provisioning

Reviewed Changes

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

Show a summary per file

File	Description
Utility.java	Added utility methods for access group creation, LUN/igroup naming, OS type mapping, and storage strategy initialization
Constants.java	Renamed PATH_SEPARATOR to SLASH and added new constants for ONTAP API fields and naming conventions
UnifiedSANStrategy.java	Implemented createAccessGroup, getAccessGroup, enableLogicalAccess, and disableLogicalAccess methods for iSCSI LUN operations
UnifiedNASStrategy.java	Updated method signatures to match interface changes (public visibility and parameter types)
StorageStrategy.java	Modified abstract method signatures to use Map parameters and public visibility for access control operations
OntapPrimaryDatastoreLifecycle.java	Enhanced attachCluster and attachZone with protocol validation, host identifier collection, and access group creation
SANFeignClient.java	Updated API endpoints with proper ONTAP REST API paths and added QueryMap support for flexible query parameters
OntapPrimaryDatastoreDriver.java	Implemented grantAccess and revokeAccess methods with volume-level access control through LUN mapping

Comments suppressed due to low confidence (2)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:1

Missing space after 'if' keyword. Add space for consistency with Java coding conventions: if (condition) instead of if(condition).

/*

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:1

Missing space after 'if' keyword. Add space for consistency with Java coding conventions: if (condition) instead of if(condition).

/*

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

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

Copilot

Pull Request Overview

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

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

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

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

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

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

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java:1

The method getAccessGroupByName uses Constants.NAME as a key, but the getAccessGroup method in UnifiedSANStrategy expects Constants.IGROUP_DOT_NAME (line 182). This mismatch will cause the access group retrieval to fail.

/*

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

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

Copilot

Pull Request Overview

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

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

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

sandeeplocharla · 2025-11-08T16:49:13Z

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

    }

+    private void grantAccessForVolume(StoragePoolVO storagePool, VolumeVO volumeVO, Host host) {
+        Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(storagePool.getId());


Retrieving storagepool details is a common requirement, so, these 2 lines of code could be a good candidate for Utility class

sandeeplocharla · 2025-11-08T16:51:43Z

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

+            throw new CloudRuntimeException(errMsg);
+        }
+    }
+    private boolean hostInitiatorFoundInIgroup(String hostInitiator, Igroup igroup) {


Similar to this, getLunName and getIgroupName methods could've been private methods in this class or we can create a helper class for such operations, instead of putting them in Utility

sandeeplocharla · 2025-11-08T16:59:32Z

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

+        switch (protocol) {
+            case ISCSI:
+                // Access group name format: cs_svmName_scopeId
+                String igroupName = Utility.getIgroupName(svmName, scopeId);


This getIgroupName could rather be in some helper class instead of Utility

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

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

Copilot

Pull Request Overview

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

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 21 comments.

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

Copilot

Pull Request Overview

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

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

plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/Utility.java

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

Copilot

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

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

...age/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java

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

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java

github-actions · 2025-12-08T08:56:38Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

Gupta, Surya added 2 commits November 5, 2025 15:28

CSTACKEX-46 Create Async, Attach Cluster/Zone and Grant/Revoke Access

eafc1e7

CSTACKEX-46 Added Logging

f43783c

Copilot AI review requested due to automatic review settings November 6, 2025 03:23

Copilot AI reviewed Nov 6, 2025

View reviewed changes

CSTACKEX-46 Resolve Copilot review comments

ad4e526

suryag1201 commented Nov 6, 2025

View reviewed changes

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java Outdated Show resolved Hide resolved

suryag1201 commented Nov 6, 2025

View reviewed changes

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java Show resolved Hide resolved

CSTACKEX-46 Resolve review comments

54a7eef

Copilot AI review requested due to automatic review settings November 6, 2025 09:47

Copilot AI reviewed Nov 6, 2025

View reviewed changes

CSTACKEX-46 Resolve review bot comments

815af83

rajiv-jain-netapp reviewed Nov 6, 2025

View reviewed changes

CSTACKEX-46 Change method name

15081b5

Copilot AI review requested due to automatic review settings November 7, 2025 05:28

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Gupta, Surya added 2 commits November 7, 2025 11:42

CSTACKEX-46 Resolve check styling issues

65fa626

CSTACKEX-46 Testing: Resolve fetch Storage details issue

3a7d2f8

Copilot AI review requested due to automatic review settings November 7, 2025 07:20

Copilot AI reviewed Nov 7, 2025

View reviewed changes

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java Outdated Show resolved Hide resolved

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java Show resolved Hide resolved

Gupta, Surya added 2 commits November 7, 2025 13:08

CSTACKEX-46 Testing: Protocol Type fetch issue

bc8e216

CSTACKEX-46 Testing: Added debug log

cd87640

Copilot AI review requested due to automatic review settings November 7, 2025 08:03

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Gupta, Surya added 2 commits November 7, 2025 13:36

CSTACKEX-46 Testing: Added debug log

a10981a

CSTACKEX-46 Testing: Corrected the URL for Create Lun and Igroup

c4399c1

Copilot AI review requested due to automatic review settings November 7, 2025 08:20

Copilot AI reviewed Nov 7, 2025

View reviewed changes

CSTACKEX-46 Resolve bot review comments

71daee2

sandeeplocharla requested changes Nov 8, 2025

View reviewed changes

CSTACKEX-46 Resolve review comments

dfde3f8

Copilot AI review requested due to automatic review settings November 10, 2025 07:05

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Gupta, Surya added 2 commits November 10, 2025 14:07

CSTACKEX-46 Resolve review comments again

bce721d

CSTACKEX-46 Change log level

8980303

Copilot AI review requested due to automatic review settings November 10, 2025 08:45

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Gupta, Surya added 2 commits November 10, 2025 15:02

CSTACKEX-46 added valiodation for lun name

70c6b13

CSTACKEX-46 resolve license issue

63b484b

Copilot AI review requested due to automatic review settings November 10, 2025 11:17

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Gupta, Surya added 2 commits November 10, 2025 18:03

CSTACKEX-46 resolve check style issues

241d7f1

CSTACKEX-46 Resolve import issues

016601d

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

Copilot AI reviewed Nov 10, 2025

View reviewed changes

CSTACKEX-46 Resolve copilot comments

79a078f

github-actions bot added the status:has-conflicts label Dec 8, 2025

Conversation

suryag1201 commented Nov 6, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suryag1201 commented Nov 6, 2025 •

edited

Loading