NFS Cloudstack volume and file and export policy utils by piyush5netapp · Pull Request #18 · NetApp/cloudstack

piyush5netapp · 2025-11-07T05:12:21Z

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?

Copilot

Pull Request Overview

This PR adds support for NFS CloudStack volume creation and management utilities for the ONTAP storage plugin. The implementation includes file operations, export policy management, and integration with the existing iSCSI workflow.

Key Changes:

Added NFS protocol support alongside existing iSCSI implementation
Implemented file and export policy management utilities for NFS volumes
Added host listener infrastructure for storage pool attachment workflows

Reviewed Changes

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

Show a summary per file

File	Description
Utility.java	Refactored volume creation to support both NFS and iSCSI protocols using switch statement; added storage strategy retrieval utility
Constants.java	Added volume UUID and name constants for storage pool mapping
ProtocolType.java	Renamed NFS3 enum value to NFS
CloudStackVolume.java	Added Volume and cloudstackVolName fields to support NFS workflow
AccessGroup.java	Added fields for host connection and scope information
UnifiedNASStrategy.java	Implemented NFS volume creation and file/export policy management operations
StorageStrategy.java	Enhanced volume creation to return Volume object with UUID for storage pool mapping
StorageProviderFactory.java	Updated protocol enum reference from NFS3 to NFS
OntapPrimaryDatastoreProvider.java	Integrated OntapHostListener for host attachment events
OntapHostListener.java	Added stub implementation of HypervisorHostListener interface
OntapPrimaryDatastoreLifecycle.java	Implemented access group creation in attachCluster and attachZone methods
VolumeFeignClient.java	Added volume query method and simplified updateVolumeRebalancing signature
NASFeignClient.java	Cleaned up parameter formatting and removed unnecessary headers
OntapPrimaryDatastoreDriver.java	Added NFS protocol handling in volume creation workflow

Comments suppressed due to low confidence (2)

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

Corrected duplicated word 'volume' in error message.

/*

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

Missing space between 'catch' and the opening parenthesis. Should be '} catch (Exception e) {'.

            } catch (Exception e) {

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

Copilot · 2025-11-07T05:13:06Z

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

+                FileInfo file = new FileInfo();
+                //file.setName("test1"); // to be replaced with volume name // this should not be passed for dir
+                //file.setName(volumeObject.getName()); // to check whether this needs to be sent or not
+                file.setSize(Long.parseLong("10000"));


Dead code: This line sets a hardcoded size that is immediately overwritten on the next line. Remove this line as the actual size is set from volumeObject.getSize() on line 74.

Suggested change

file.setSize(Long.parseLong("10000"));

Copilot · 2025-11-07T05:13:07Z

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

     * @return the created AccessGroup object
     */
-    abstract AccessGroup createAccessGroup(AccessGroup accessGroup);
+    abstract public AccessGroup createAccessGroup(AccessGroup accessGroup);


The modifier 'public' should come before 'abstract' following Java convention. Change to 'public abstract AccessGroup createAccessGroup(AccessGroup accessGroup);'.

Suggested change

abstract public AccessGroup createAccessGroup(AccessGroup accessGroup);

public abstract AccessGroup createAccessGroup(AccessGroup accessGroup);

Copilot

Pull Request Overview

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

Comments suppressed due to low confidence (1)

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

Export policy name generation should be extracted to a utility method as indicated by the TODO comment.

/*

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

Copilot · 2025-11-10T14:04:39Z

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

+                FileInfo file = new FileInfo();
+                //file.setName("test1"); // to be replaced with volume name // this should not be passed for dir
+                //file.setName(volumeObject.getName()); // to check whether this needs to be sent or not
+                file.setSize(Long.parseLong("10000"));


Line 73 sets a hardcoded size that is immediately overwritten on line 74. Remove the hardcoded assignment on line 73.

Suggested change

file.setSize(Long.parseLong("10000"));

Copilot · 2025-11-10T14:04:39Z

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

+                //file.setName("test1"); // to be replaced with volume name // this should not be passed for dir
+                //file.setName(volumeObject.getName()); // to check whether this needs to be sent or not
+                file.setSize(Long.parseLong("10000"));
+                file.setSize(volumeObject.getSize());
+                file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir
+                file.setType(FileInfo.TypeEnum.DIRECTORY); // We are creating file for a cloudstack volume . Should it be dir ? // TODO change once multipart is done


TODO comments indicate uncertainty about implementation details. These should be resolved or clarified before merging, particularly whether the type should be DIRECTORY and whether permissions are appropriate.

Suggested change

//file.setName("test1"); // to be replaced with volume name // this should not be passed for dir

//file.setName(volumeObject.getName()); // to check whether this needs to be sent or not

file.setSize(Long.parseLong("10000"));

file.setSize(volumeObject.getSize());

file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir

file.setType(FileInfo.TypeEnum.DIRECTORY); // We are creating file for a cloudstack volume . Should it be dir ? // TODO change once multipart is done

file.setName(volumeObject.getName()); // Set file name to volume name

file.setSize(volumeObject.getSize());

file.setUnixPermissions(644); // Set permissions appropriate for a file

file.setType(FileInfo.TypeEnum.FILE); // Set type to FILE for NFS volume

Copilot · 2025-11-10T14:04:40Z

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

+            s_logger.debug("Successfully created file in ONTAP under volume with path {} or name {}  ", cloudstackVolume.getVolume().getUuid(), cloudstackVolume.getCloudstackVolName());
+            FileInfo responseFile = cloudstackVolume.getFile();
+            responseFile.setPath(cloudstackVolume.getCloudstackVolName());
+        }catch (Exception e) {


Missing space between closing brace and 'catch' keyword. Should be '} catch (Exception e) {'.

Suggested change

}catch (Exception e) {

} catch (Exception e) {

Copilot · 2025-11-10T14:04:40Z

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

+            s_logger.info("Successfully assigned exportPolicy {} to volume {}", exportPolicy.getName(), volumeName);
+            accessGroup.setPolicy(exportPolicy);
+            return accessGroup;
+        }catch (Exception e){


Missing space between closing brace and 'catch' keyword, and between 'e)' and '{'. Should be '} catch (Exception e) {'.

Suggested change

}catch (Exception e){

} catch (Exception e) {

Copilot · 2025-11-10T14:04:40Z

...volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java

    private FileInfo file;
    private Lun lun;
+    private Volume volume;
+    // will be replaced after testing


Comment indicates temporary field. Either remove the comment if the field is permanent, or create a tracking issue for its removal if it's truly temporary.

Suggested change

// will be replaced after testing

Copilot · 2025-11-10T14:04:41Z

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

+    private String addExportRule(String policyName, String clientMatch, String[] protocols, String[] roRule, String[] rwRule) {
+        return "";
+    }
+


Unused private method 'addExportRule' with stub implementation should be removed or implemented if needed.

Suggested change

private String addExportRule(String policyName, String clientMatch, String[] protocols, String[] roRule, String[] rwRule) {

return "";

}

Copilot · 2025-11-10T14:04:41Z

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

    @RequestLine("GET /")
    @Headers({"Authorization: {authHeader}"})
-    OntapResponse<ExportPolicy> getExportPolicyResponse(@Param("authHeader") String authHeader);
+    ExportPolicy getExportPolicyResponse(@Param("authHeader") String authHeader);


Return type changed from 'OntapResponse' to 'ExportPolicy'. This could be a breaking change if the method is used elsewhere expecting the wrapped response type.

Suggested change

ExportPolicy getExportPolicyResponse(@Param("authHeader") String authHeader);

OntapResponse<ExportPolicy> getExportPolicyResponse(@Param("authHeader") String authHeader);

Copilot · 2025-11-10T14:04:41Z

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

    @Headers({"Authorization: {authHeader}"})
-    OntapResponse<ExportPolicy> getExportPolicyById(@Param("authHeader") String authHeader,
-                                                   @Param("id") String id);
+    ExportPolicy getExportPolicyById(@Param("authHeader") String authHeader,


Return type changed from 'OntapResponse' to 'ExportPolicy'. This could be a breaking change if the method is used elsewhere expecting the wrapped response type.

Suggested change

ExportPolicy getExportPolicyById(@Param("authHeader") String authHeader,

OntapResponse<ExportPolicy> getExportPolicyById(@Param("authHeader") String authHeader,

Copilot · 2025-11-10T14:04:41Z

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

+    @Headers({ "Authorization: {authHeader}"})
+    JobResponse updateVolumeRebalancing(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Volume volumeRequest);


Removed 'acceptHeader' parameter from method signature. This is a breaking change that could affect existing callers of this method.

Suggested change

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

JobResponse updateVolumeRebalancing(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Volume volumeRequest);

@Headers({ "Authorization: {authHeader}", "Accept: {acceptHeader}" })

JobResponse updateVolumeRebalancing(@Param("authHeader") String authHeader, @Param("acceptHeader") String acceptHeader, @Param("uuid") String uuid, Volume volumeRequest);

Copilot

Pull Request Overview

Copilot reviewed 17 out of 20 changed files in this pull request and generated 13 comments.

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

Copilot · 2025-11-14T08:20:44Z

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

+                cloudStackVolumeRequest = new CloudStackVolume();
+                FileInfo file = new FileInfo();
+                //file.setName("test1"); // to be replaced with volume name // this should not be passed for dir
+                //file.setName(volumeObject.getName()); // to check whether this needs to be sent or not


Remove these commented-out lines. If this logic needs investigation, track it with a TODO comment instead of leaving commented code.

Suggested change

//file.setName(volumeObject.getName()); // to check whether this needs to be sent or not

// TODO: Investigate whether file.setName(volumeObject.getName()) needs to be sent for NFS file creation.

Copilot · 2025-11-14T08:20:44Z

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

+                //file.setName(volumeObject.getName()); // to check whether this needs to be sent or not
+                file.setSize(Long.parseLong("10000"));
+                file.setSize(volumeObject.getSize());
+                file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir


The comment is unclear and contradictory ('check if it is needed only for dir ? it is needed for dir'). Either remove the question or clarify whether this is always required or only for directories.

Suggested change

file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir

file.setUnixPermissions(755); // Set permissions because the file is a directory

Copilot · 2025-11-14T08:20:44Z

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

+                file.setSize(Long.parseLong("10000"));
+                file.setSize(volumeObject.getSize());
+                file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir
+                file.setType(FileInfo.TypeEnum.DIRECTORY); // We are creating file for a cloudstack volume . Should it be dir ? // TODO change once multipart is done


The comment contains conflicting information about whether this should be a file or directory. Update the TODO to be more specific about what needs to change and when.

Suggested change

file.setType(FileInfo.TypeEnum.DIRECTORY); // We are creating file for a cloudstack volume . Should it be dir ? // TODO change once multipart is done

file.setType(FileInfo.TypeEnum.DIRECTORY); // TODO: Currently set to DIRECTORY for NFS volumes. Change to FILE when multipart file support is implemented.

Copilot · 2025-11-14T08:20:45Z

...volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/CloudStackVolume.java

    private FileInfo file;
    private Lun lun;
+    private Volume volume;
+    // will be replaced after testing


The comment 'will be replaced after testing' is vague. Specify what will replace this field and under what conditions, or create a TODO if this is temporary.

Suggested change

// will be replaced after testing

// TODO: Replace cloudstackVolName with the final volume naming scheme after testing is complete

Copilot · 2025-11-14T08:20:45Z

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

+        this.volumeFeignClient = feignClientFactory.createClient(VolumeFeignClient.class,baseURL );
+        this.jobFeignClient = feignClientFactory.createClient(JobFeignClient.class, baseURL );


[nitpick] Inconsistent spacing before closing parentheses. Line 69 has a space before the closing parenthesis while line 70 has a space after 'baseURL'. Standardize the formatting.

Suggested change

this.volumeFeignClient = feignClientFactory.createClient(VolumeFeignClient.class,baseURL );

this.jobFeignClient = feignClientFactory.createClient(JobFeignClient.class, baseURL );

this.volumeFeignClient = feignClientFactory.createClient(VolumeFeignClient.class, baseURL);

this.jobFeignClient = feignClientFactory.createClient(JobFeignClient.class, baseURL);

Copilot · 2025-11-14T08:20:46Z

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

+                // CloudStack will construct the full mount path as: hostAddress + ":" + path
+                path = "/" + storagePoolName;
                s_logger.info("Setting NFS path for storage pool: " + path);
+                host = "10.193.192.136"; // TODO hardcoded for now


Hardcoded IP address will cause failures in different environments. This must be replaced with the actual management LIF from details.get(Constants.MANAGEMENT_LIF) before merging.

Suggested change

host = "10.193.192.136"; // TODO hardcoded for now

host = details.get(Constants.MANAGEMENT_LIF);

Copilot · 2025-11-14T08:20:46Z

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

+        PrimaryDataStoreInfo primaryStore = (PrimaryDataStoreInfo)dataStore;
+        List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInClusterForStorageConnection(primaryStore);
+
+        logger.debug(" datastore object received is  {} ",primaryStore );


[nitpick] Extra spaces in the log message format string. Change ' datastore object received is {} ' to 'Datastore object received is {}'.

Suggested change

logger.debug(" datastore object received is {} ",primaryStore );

logger.debug("Datastore object received is {}", primaryStore);

Copilot · 2025-11-14T08:20:47Z

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

+        AccessGroup accessGroupRequest = new AccessGroup();
+        accessGroupRequest.setHostsToConnect(hostsToConnect);
+        accessGroupRequest.setScope(scope);
+        primaryStore.setDetails(details);// setting details as it does not come from cloudstack


[nitpick] Missing space after the semicolon on line 282. Add a space between '//' and 'setting' for consistent comment formatting.

Suggested change

primaryStore.setDetails(details);// setting details as it does not come from cloudstack

primaryStore.setDetails(details); // setting details as it does not come from cloudstack

Copilot · 2025-11-14T08:20:47Z

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

+        accessGroupRequest.setPolicy(exportPolicy);
+        strategy.createAccessGroup(accessGroupRequest);
+
+        logger.debug("attachCluster: Attaching the pool to each of the host in the cluster: {}", primaryStore.getClusterId());


[nitpick] Missing space after the semicolon on line 282. Add a space between '//' and 'setting' for consistent comment formatting.

Copilot · 2025-11-14T08:20:47Z

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

            @Override
            public Object decode(Response response, Type type) throws IOException, DecodeException {
                if (response.body() == null) {
+                    logger.debug("Response body is null, returning null");


Excessive debug logging added throughout the decoder (lines 140-169). This level of verbose logging should be removed or converted to trace level before merging to production to avoid log pollution.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

NFS Cloudstack volume and export policy utils

5f71168

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

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Srivastava, Piyush added 3 commits November 7, 2025 11:38

Licencse add in files

c88483a

accessgroup create recode

e3c44ae

creatacessgroup for NFS impl

09caf90

Copilot AI review requested due to automatic review settings November 10, 2025 14:02

Copilot AI reviewed Nov 10, 2025

View reviewed changes

Srivastava, Piyush added 2 commits November 14, 2025 10:22

storage pool mounting on host

aa14ab5

storage pool mounting on host 1

f89bb1b

Copilot AI review requested due to automatic review settings November 14, 2025 08:17

Copilot AI reviewed Nov 14, 2025

View reviewed changes

Srivastava, Piyush added 2 commits November 17, 2025 17:00

vm restart issue

fe19c55

vm restart issue 1

e27fad2

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

Copilot AI reviewed Nov 17, 2025

View reviewed changes

vm restart issue 2

e8062fd

piyush5netapp closed this Dec 4, 2025

	abstract public AccessGroup createAccessGroup(AccessGroup accessGroup);
	public abstract AccessGroup createAccessGroup(AccessGroup accessGroup);

	private String addExportRule(String policyName, String clientMatch, String[] protocols, String[] roRule, String[] rwRule) {
	return "";
	}

	ExportPolicy getExportPolicyResponse(@Param("authHeader") String authHeader);
	OntapResponse<ExportPolicy> getExportPolicyResponse(@Param("authHeader") String authHeader);

	ExportPolicy getExportPolicyById(@Param("authHeader") String authHeader,
	OntapResponse<ExportPolicy> getExportPolicyById(@Param("authHeader") String authHeader,

		@Headers({ "Authorization: {authHeader}"})
		JobResponse updateVolumeRebalancing(@Param("authHeader") String authHeader, @Param("uuid") String uuid, Volume volumeRequest);

	//file.setName(volumeObject.getName()); // to check whether this needs to be sent or not
	// TODO: Investigate whether file.setName(volumeObject.getName()) needs to be sent for NFS file creation.

	file.setUnixPermissions(755); // check if it is needed only for dir ? it is needed for dir
	file.setUnixPermissions(755); // Set permissions because the file is a directory

	file.setType(FileInfo.TypeEnum.DIRECTORY); // We are creating file for a cloudstack volume . Should it be dir ? // TODO change once multipart is done
	file.setType(FileInfo.TypeEnum.DIRECTORY); // TODO: Currently set to DIRECTORY for NFS volumes. Change to FILE when multipart file support is implemented.

	// will be replaced after testing
	// TODO: Replace cloudstackVolName with the final volume naming scheme after testing is complete

		this.volumeFeignClient = feignClientFactory.createClient(VolumeFeignClient.class,baseURL );
		this.jobFeignClient = feignClientFactory.createClient(JobFeignClient.class, baseURL );

	host = "10.193.192.136"; // TODO hardcoded for now
	host = details.get(Constants.MANAGEMENT_LIF);

	logger.debug(" datastore object received is {} ",primaryStore );
	logger.debug("Datastore object received is {}", primaryStore);

	primaryStore.setDetails(details);// setting details as it does not come from cloudstack
	primaryStore.setDetails(details); // setting details as it does not come from cloudstack

Conversation

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

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

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

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

piyush5netapp commented Nov 7, 2025 •

edited

Loading