Export Policy and File operation changes NAS Storage pool creation #11

piyush5netapp · 2025-10-27T06:40:47Z

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?

piyush5netapp · 2025-10-27T07:13:43Z

...ns/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/NASStrategy.java


    public abstract String createExportPolicy(String svmName, String policyName);
+    public abstract boolean deleteExportPolicy(String svmName, String policyName);
+    public abstract boolean exportPolicyExists(String svmName, String policyName);


We can have getExportPolicy method

piyush5netapp · 2025-10-27T07:14:08Z

...ns/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/NASStrategy.java

    }

    public abstract String createExportPolicy(String svmName, String policyName);
+    public abstract boolean deleteExportPolicy(String svmName, String policyName);


make it void to pass back the exact error incase of failure

piyush5netapp · 2025-10-27T07:17:22Z

...ns/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/NASStrategy.java

        super(ontapStorage);
    }

    public abstract String createExportPolicy(String svmName, String policyName);


Return the policy object

piyush5netapp · 2025-10-27T07:20:57Z

...ns/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/NASStrategy.java

+    public abstract boolean exportPolicyExists(String svmName, String policyName);
    public abstract String addExportRule(String policyName, String clientMatch, String[] protocols, String[] roRule, String[] rwRule);
    public abstract String assignExportPolicyToVolume(String volumeUuid, String policyName);
    public abstract String enableNFS(String svmUuid);


We don't need this method, SVM should be pre-configured with NFS enabled.

piyush5netapp · 2025-10-27T07:23:39Z

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

    @RequestMapping(method = RequestMethod.GET, value = "/{uuid}")
    Svm getSvmByUUID(URI baseURL, @RequestHeader("Authorization") String header);

+    @RequestMapping(method = RequestMethod.PATCH)


We can remove this as not required now.

piyush5netapp · 2025-10-27T07:25:15Z

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

     */
    @Override
    public DataStore initialize(Map<String, Object> dsInfos) {
+        s_logger.info("initialize {}", dsInfos);


make it debug and make it meaningful.

piyush5netapp · 2025-10-27T07:28:42Z

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

        } else {
            throw new CloudRuntimeException("ONTAP details validation failed, cannot create primary storage");
        }
+        String storagePath = url + ":/" + storagePoolName;


Put the comment for assuming it is for NFS and URL value.

piyush5netapp · 2025-10-27T07:30:39Z

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

+        s_logger.info("initialize {}", dsInfos);
        if (dsInfos == null) {
            throw new CloudRuntimeException("Datastore info map is null, cannot create primary storage");
        }


Put the sample URL on which below code has been written.

suryag1201 · 2025-10-27T14:25:59Z

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

+            exportPolicy.setSvm(svm);
+
+            // Create URI for export policy creation
+            URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + "/api/protocols/nfs/export-policies");


pass this URL to constant

suryag1201 · 2025-10-27T14:28:41Z

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

+
+            // Get policy ID first
+            URI getUrl = URI.create(Constants.HTTPS + storage.getManagementLIF() +
+                    "/api/protocols/nfs/export-policies?name=" + policyName + "&svm.name=" + svmName);


same, put these in reusable constants

github-actions · 2025-10-29T06:51:06Z

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

Copilot

Pull Request Overview

This PR implements comprehensive NAS support for the ONTAP storage plugin by adding export policy management and file operations. The changes migrate from Spring-based Feign clients to native Feign configuration and enhance the storage strategy pattern with complete NAS functionality.

Removes Spring Framework dependencies and migrates to native Feign client configuration
Implements complete NAS strategy with export policy management and file operations
Refactors storage classes to use immutable fields and proper dependency injection

Reviewed Changes

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

Show a summary per file

File	Description
Utility.java	Removes Spring components and unused URI generation method
Constants.java	Enhances ProtocolType enum with string values
UnifiedNASStrategy.java	Implements comprehensive NAS operations including export policies and file management
UnifiedSANStrategy.java	Adds default constructor and dependency initialization
StorageStrategy.java	Refactors to use FeignClientFactory instead of Spring injection
NASStrategy.java	Updates abstract methods for export policy operations
SANStrategy.java	Adds default constructor support
StorageProviderFactory.java	Removes Spring annotations and refactors strategy creation
OntapPrimaryDatastoreProvider.java	Updates logging imports and adds host listener
OntapHostListener.java	Implements HypervisorHostListener interface
OntapPrimaryDatastoreLifecycle.java	Adds URL parsing and hardcoded configuration for testing
Volume.java	Adds JSON ignore properties annotation
OntapStorage.java	Converts to immutable class with final fields
Job.java	Fixes property mappings and variable naming
Multiple Feign clients	Migrates from Spring to native Feign annotations
FeignConfiguration.java	Replaces Spring configuration with native Feign setup
FeignClientFactory.java	New factory class for creating Feign clients
OntapPrimaryDatastoreDriver.java	Updates logging import
pom.xml	Adds feign-jackson dependency

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

Copilot · 2025-11-04T04:48:58Z

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

+                "10.196.38.171", extractedDetails.get(Constants.SVM_NAME), ProtocolType.NFS,
+                Boolean.parseBoolean(extractedDetails.get(Constants.IS_DISAGGREGATED))); // TODO hardcoded for testing


Hardcoded IP address should not be committed to production code. Consider using configuration or extractedDetails instead.

Suggested change

"10.196.38.171", extractedDetails.get(Constants.SVM_NAME), ProtocolType.NFS,

Boolean.parseBoolean(extractedDetails.get(Constants.IS_DISAGGREGATED))); // TODO hardcoded for testing

mLIF, extractedDetails.get(Constants.SVM_NAME), ProtocolType.NFS,

Boolean.parseBoolean(extractedDetails.get(Constants.IS_DISAGGREGATED)));

Copilot · 2025-11-04T04:48:58Z

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

        parameters.setName(storagePoolName);
        parameters.setProviderName(providerName);
        parameters.setManaged(true);
+        parameters.setHost("10.196.38.171");


Another hardcoded IP address that should be replaced with proper configuration or variable.

Copilot · 2025-11-04T04:48:58Z

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

+//                .logger(feignConfiguration.createLogger())
+                .retryer(feignConfiguration.createRetryer())
+                .requestInterceptor(feignConfiguration.createRequestInterceptor())
+                .target(clientClass, "https://10.196.38.171/");


Hardcoded base URL in FeignClientFactory should be configurable to support different environments.

Copilot · 2025-11-04T04:48:58Z

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

+            URI updateVolumeUrl = URI.create(Constants.HTTPS + storage.getManagementLIF() +
+                    "/api/storage/volumes/" + volumeUuid);
+
+            //volumeFeignClient.updateVolumeRebalancing(updateVolumeUrl, authHeader, volumeUuid, volumeUpdate);


Commented out code should be removed or properly implemented before production deployment.

Suggested change

//volumeFeignClient.updateVolumeRebalancing(updateVolumeUrl, authHeader, volumeUuid, volumeUpdate);

volumeFeignClient.updateVolumeRebalancing(updateVolumeUrl, authHeader, volumeUuid, volumeUpdate);

Copilot · 2025-11-04T04:48:59Z

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

+        s_logger.info(" storage object is {} ", storage.getPassword());
+        s_logger.info(" storage object is {} ", storage.getUsername());


Logging sensitive information like passwords should be avoided as it can expose credentials in log files.

Suggested change

s_logger.info(" storage object is {} ", storage.getPassword());

s_logger.info(" storage object is {} ", storage.getUsername());

// Do not log sensitive information such as password or username

Copilot · 2025-11-04T04:48:59Z

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

+        utils = ComponentContext.inject(Utility.class);
+        // Initialize FeignClientFactory and create NAS client
+        feignClientFactory = new FeignClientFactory();
+        sanFeignClient = feignClientFactory.createClient(SANFeignClient.class);// TODO needs to be changed


TODO comment indicates incomplete implementation that should be addressed before production.

Copilot · 2025-11-04T04:48:59Z

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

 public class UnifiedNASStrategy extends NASStrategy{
+
+    private Utility utils;
+//    // Add missing Feign client setup for NAS operations


Commented out code should be removed to improve code clarity.

Suggested change

// // Add missing Feign client setup for NAS operations

rajiv-jain-netapp · 2025-11-06T11:32:59Z

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

+//                .logger(feignConfiguration.createLogger())
+                .retryer(feignConfiguration.createRetryer())
+                .requestInterceptor(feignConfiguration.createRequestInterceptor())
+                .target(clientClass, "https://10.196.38.171/");


lets not merge such changes, you can do this for internal testing.

rajiv-jain-netapp · 2025-11-06T11:34:59Z

...ume/ontap/src/main/java/org/apache/cloudstack/storage/feign/client/AggregateFeignClient.java

-
-    @RequestMapping(method=RequestMethod.GET, value="/{uuid}")
-    Aggregate getAggregateByUUID(URI baseURL,@RequestHeader("Authorization") String header, @PathVariable(name = "uuid", required = true) String uuid);
+    @RequestLine("GET /")


rajiv-jain-netapp · 2025-11-06T11:38:56Z

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

+        details.put(Constants.IS_DISAGGREGATED, "false");
+
+        OntapStorage ontapStorage = new OntapStorage(extractedDetails.get(Constants.USERNAME), extractedDetails.get(Constants.PASSWORD),
+                "10.196.38.171", extractedDetails.get(Constants.SVM_NAME), ProtocolType.NFS,


no hard code values

Pilot commit for NAS support operations

b83ce23

piyush5netapp requested review from rajiv-jain-netapp, sandeeplocharla and suryag1201 October 27, 2025 06:42

piyush5netapp commented Oct 27, 2025

View reviewed changes

suryag1201 reviewed Oct 27, 2025

View reviewed changes

github-actions bot added the status:has-conflicts label Oct 29, 2025

commit for nfs storage pool creation

0049858

Copilot AI review requested due to automatic review settings November 4, 2025 04:47

piyush5netapp changed the title ~~Export Policy and File operation changes NAS support~~ Export Policy and File operation changes NAS Storage pool creation Nov 4, 2025

Copilot AI reviewed Nov 4, 2025

View reviewed changes

rajiv-jain-netapp reviewed Nov 6, 2025

View reviewed changes

		"10.196.38.171", extractedDetails.get(Constants.SVM_NAME), ProtocolType.NFS,
		Boolean.parseBoolean(extractedDetails.get(Constants.IS_DISAGGREGATED))); // TODO hardcoded for testing

	//volumeFeignClient.updateVolumeRebalancing(updateVolumeUrl, authHeader, volumeUuid, volumeUpdate);
	volumeFeignClient.updateVolumeRebalancing(updateVolumeUrl, authHeader, volumeUuid, volumeUpdate);

		s_logger.info(" storage object is {} ", storage.getPassword());
		s_logger.info(" storage object is {} ", storage.getUsername());

	s_logger.info(" storage object is {} ", storage.getPassword());
	s_logger.info(" storage object is {} ", storage.getUsername());
	// Do not log sensitive information such as password or username

Export Policy and File operation changes NAS Storage pool creation #11

Are you sure you want to change the base?

Export Policy and File operation changes NAS Storage pool creation #11

Uh oh!

Conversation

piyush5netapp commented Oct 27, 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!

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!

github-actions bot commented Oct 29, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants