CSTACKEX-7: ONTAP Primary storage pool #9

sandeeplocharla · 2025-10-21T05:52:46Z

Description

This PR...

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?

suryag1201 · 2025-10-22T06:09:55Z

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

    //this method to get all svms and also filtered svms based on query params as a part of URL
    @RequestMapping(method = RequestMethod.GET)
-    OntapResponse<Svm> getSvmResponse(URI baseURL, @RequestHeader("Authorization") String header);
+    OntapResponse<Svm> getSvms(URI baseURL, @RequestHeader("Authorization") String header);


we kept this naming convention for other feign client and this can return only one record also

Reverted it back

suryag1201 · 2025-10-22T06:11:30Z

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

+    public static String Username;
+    public static String Password;
+    public static String ManagementLIF;
+    public static String Svm;


svm is an object which can not be string use svmName instead

sure, will rename it

suryag1201 · 2025-10-22T06:12:17Z

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

+    public static String Password;
+    public static String ManagementLIF;
+    public static String Svm;
+    public static String Protocol;


Can we have type as enum with supported protocols?

suryag1201 · 2025-10-22T06:18:24Z

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

+        @SuppressWarnings("unchecked")
+        Map<String, String> details = (Map<String, String>)dsInfos.get("details");
+        // Validations
+        if (podId != null && clusterId == null) {


we can combine this check into one check bcz both are required, also clusterId can be empty

Yeah, in my previous review to this change i nanother PR, I gave comment to use trim command and then empty check. Please add them for each mandatory parameter

Taken care of it, done

suryag1201 · 2025-10-22T06:20:10Z

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

+        }
+
+        PrimaryDataStoreParameters parameters = new PrimaryDataStoreParameters();
+        if (clusterId != null) {


why this check is required again? it is already checked above

Customer has an option to not send both cluster and pod ids, in case of creating a zone level primary storage pool.

suryag1201 · 2025-10-22T06:23:24Z

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

+        details.put(Constants.MANAGEMENTLIF, url);

+        // Validate the ONTAP details
+        if(details.get(Constants.ISDISAGGREGATED) == null || details.get(Constants.ISDISAGGREGATED).isEmpty()) {


add _ for ISDISAGGREGATED constant like IS_DISAGGREGATED

suryag1201 · 2025-10-22T06:32:42Z

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

+            // Call the SVM API to check if the SVM exists
+            Svm svm = null;
+            URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GETSVMs);
+            OntapResponse<Svm> svms = svmFeignClient.getSvms(url, authHeader);


add a check for svms null check object

suryag1201 · 2025-10-22T06:34:48Z

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

+public class Constants {
+    public static final String NFS = "nfs";
+    public static final String ISCSI = "iscsi";
+    public static final String PROTOCOL = "protocol";


add _ where ever possible

suryag1201 · 2025-10-22T06:40:27Z

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

+            // Create URI for POST CreateVolume API
+            URI url = utils.generateURI(Constants.CREATEVOLUME);
+            // Call the VolumeFeignClient to create the volume
+            JobResponse jobResponse = volumeFeignClient.createVolumeWithJob(url, authHeader, volumeRequest);


add null checks

done, thanks

suryag1201 · 2025-10-22T07:11:46Z

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

+            url = utils.generateURI(Constants.GETJOBBYUUID);
+            int jobRetryCount = 0, maxJobRetries = Constants.JOBMAXRETRIES;
+            Job createVolumeJob = null;
+            while(createVolumeJob == null || createVolumeJob.getState().equals(Constants.JOBRUNNING) || createVolumeJob.getState().equals(Constants.JOBQUEUE) || createVolumeJob.getState().equals(Constants.JOBPAUSED)) {


instead of checking for all state of job, check for not success

this seems to be lost during rebase, thanks, done

suryag1201 · 2025-10-22T09:28:23Z

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

+            Svm svm = null;
+            URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GETSVMs);
+            OntapResponse<Svm> svms = svmFeignClient.getSvms(url, authHeader);
+            for (Svm storageVM : svms.getRecords()) {


Instead of getting all svm and picking based on name, better to call getSVMByName. We may need to add this method in svmFeignClient

Yes, this approach is not optimized. fetch the SVM using name as query parameter instead fetch all then find for desired name.

Made the necessary changes

rajiv-jain-netapp · 2025-10-22T11:10:27Z

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


    @RequestMapping(method = RequestMethod.DELETE, value="/{uuid}")
-    void deleteVolume(@RequestHeader("Authorization") String authHeader, @PathVariable("uuid") String uuid);
+    void deleteVolume(URI baseURL, @RequestHeader("Authorization") String authHeader, @PathVariable("uuid") String uuid);


with this change, why do we need to add url parameter at class level annotation?

If we don't pass it explicitly, we need to set a property statically in application.properties or somewhere like that, which in our case is not feasible. Also, user might ask us to deal with multiple filers, so, the IP wouldn't be static

rajiv-jain-netapp · 2025-10-22T11:13:02Z

...me/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/response/OntapResponse.java

I am not seeing any significant changes here, better remove this class from the PR

This came in as part of rebase. It just has some space changes. Sure, will try to revert it

rajiv-jain-netapp · 2025-10-22T11:15:41Z

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

+        @SuppressWarnings("unchecked")
+        Map<String, String> details = (Map<String, String>)dsInfos.get("details");
+        // Validations
+        if (podId != null && clusterId == null) {


Yeah, in my previous review to this change i nanother PR, I gave comment to use trim command and then empty check. Please add them for each mandatory parameter

rajiv-jain-netapp · 2025-10-22T11:19:41Z

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

+            Svm svm = null;
+            URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GETSVMs);
+            OntapResponse<Svm> svms = svmFeignClient.getSvms(url, authHeader);
+            for (Svm storageVM : svms.getRecords()) {


Yes, this approach is not optimized. fetch the SVM using name as query parameter instead fetch all then find for desired name.

sandeeplocharla · 2025-10-23T07:25:27Z

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

+            ClusterVO clusterVO = _clusterDao.findById(clusterId);
+            Preconditions.checkNotNull(clusterVO, "Unable to locate the specified cluster");
+            if (clusterVO.getHypervisorType() != Hypervisor.HypervisorType.KVM) {
+                throw new CloudRuntimeException("ONTAP primary storage is not supported for KVM hypervisor");


change the message

suryag1201 · 2025-10-23T09:36:27Z

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

+import org.apache.cloudstack.storage.utils.Constants.ProtocolType;
+
+public class OntapStorage {
+    public static String Username;


Follow naming convention for all fields

…iable names

rajiv-jain-netapp · 2025-10-24T05:08:12Z

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

why you added only inports in this class ?

rajiv-jain-netapp · 2025-10-24T05:09:38Z

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

+public class OntapStorage {
+    public static String _username;
+    public static String _password;
+    public static String _managementLIF;


Lets use camel casing for the variable name

rajiv-jain-netapp · 2025-10-24T05:10:51Z

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

+            }
+        }
+
+        if (storagePoolName == null || storagePoolName.isEmpty()) {


empty check on the sting parameters must use trim method as well

rajiv-jain-netapp · 2025-10-24T05:17:52Z

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

+                throw new CloudRuntimeException("iSCSI protocol is not enabled on SVM " + svmName);
+            }
+            List<Aggregate> aggrs = svm.getAggregates();
+            if (aggrs == null || aggrs.isEmpty()) {


One use case to validate here is that if the passed credentials belong to the cluster, then by default, all the aggregates are mapped to the underlying SVMs. However, if credentials belong to an SVM user, then aggregates must be assigned to the SVM to access them.

@suryag1201 can you help @sandeeplocharla to close on this use case implementation.

rajiv-jain-netapp

there are dependencies on these changes for other engineers, approving this PR for now. Ensure you are picking up the given comments in subsequent PR

sandeeplocharla self-assigned this Oct 21, 2025

sandeeplocharla requested review from piyush5netapp, rajiv-jain-netapp and suryag1201 October 21, 2025 05:53

suryag1201 reviewed Oct 22, 2025

View reviewed changes

rajiv-jain-netapp reviewed Oct 22, 2025

View reviewed changes

Locharla, Sandeep added 2 commits October 22, 2025 20:08

CSTACKEX-7: ONTAP Primary storage pool

9547f02

CSTACKEX-7: Addressed review comments

a9c7f65

sandeeplocharla force-pushed the feature/CSTACKEX-7 branch from 1573d0c to a9c7f65 Compare October 22, 2025 14:38

sandeeplocharla commented Oct 23, 2025

View reviewed changes

CSTACKEX-7: Addressed few more comments

24d3275

suryag1201 reviewed Oct 23, 2025

View reviewed changes

CSTACKEX-7: Made changes to correct an exception message and some var…

2274e60

…iable names

rajiv-jain-netapp reviewed Oct 24, 2025

View reviewed changes

sandeeplocharla requested review from rajiv-jain-netapp and suryag1201 October 24, 2025 08:09

piyush5netapp approved these changes Oct 24, 2025

View reviewed changes

rajiv-jain-netapp approved these changes Oct 24, 2025

View reviewed changes

sandeeplocharla merged commit 25353c2 into main Oct 24, 2025
13 of 16 checks passed

Uh oh!

CSTACKEX-7: ONTAP Primary storage pool #9

CSTACKEX-7: ONTAP Primary storage pool #9

Uh oh!

Conversation

sandeeplocharla commented Oct 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suryag1201 Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

suryag1201 Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

suryag1201 Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

sandeeplocharla commented Oct 21, 2025 •

edited

Loading

suryag1201 Oct 22, 2025 •

edited

Loading

suryag1201 Oct 22, 2025 •

edited

Loading

suryag1201 Oct 22, 2025 •

edited

Loading

suryag1201 Oct 23, 2025 •

edited

Loading