Skip to content

Feature/cstackex 22: Shared NFS pool and volume creation - Approach 1 #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
piyush5netapp merged 30 commits into from
Dec 8, 2025
Merged

Feature/cstackex 22: Shared NFS pool and volume creation - Approach 1 #22

piyush5netapp merged 30 commits into from
Dec 8, 2025

Conversation

@piyush5netapp
Copy link

@piyush5netapp piyush5netapp commented Dec 4, 2025 •
edited
Loading

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

  1. Create Storage Pool Type as NetworkFileSystem
image 2. Storage Pool is mounted on host . image 3. Ontap volume mapped to storage pool is created with custom export policy with host IPs allowed as clients. image 4. Create a VM instance with disk offering same as storage pool disk offering. image image image 5. Attach a volume to already existing vm instance image 6. All volume are as a file inside storage pool volume image

How did you try to break this feature and the system with this change?

rajiv-jain-netapp
rajiv-jain-netapp reviewed Dec 5, 2025
View reviewed changes
...ap/src/main/java/org/apache/cloudstack/storage/lifecycle/OntapPrimaryDatastoreLifecycle.java Outdated
break;
case ISCSI:
parameters.setType(Storage.StoragePoolType.Iscsi);
path = "iqn.1992-08.com.netapp:" + details.get(Constants.SVM_NAME) + "." + storagePoolName;
Copy link

@rajiv-jain-netapp rajiv-jain-netapp Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all these hard-coded values. You can keep them as commented code if required.

rajiv-jain-netapp
rajiv-jain-netapp approved these changes Dec 8, 2025
View reviewed changes
Copy link

@rajiv-jain-netapp rajiv-jain-netapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of your files are not loading for review.
You have a few comments that need attention, but none of them appear to be blockers for merging. Please take note of these comments and ensure they are addressed in subsequent updates.
Additionally, I noticed your test results summary includes scenarios for storage pools using the NFS protocol. We have an open use case for ONTAP NFS protocol that was recently added. Let’s include all these scenarios in your COTV plan and ensure they are covered as part of integration testing.

...me/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java Outdated
throw new CloudRuntimeException("deleteAsync : Storage Pool not found for id: " + store.getId());
}
Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(store.getId());
if (ProtocolType.NFS.name().equalsIgnoreCase(details.get(Constants.PROTOCOL))) {
Copy link

@rajiv-jain-netapp rajiv-jain-netapp Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it a no-op? We have to delete the flex-volume created for storage-pool in the delete workflow, right?

...orage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/FeignConfiguration.java Outdated
result = jsonMapper.readValue(json, javaType);
logger.debug("jsonMapper.readValue() completed successfully");
} catch (Throwable ex) {
logger.error("EXCEPTION in jsonMapper.readValue()! Type: {}, Message: {}", ex.getClass().getName(), ex.getMessage(), ex);
Copy link

@rajiv-jain-netapp rajiv-jain-netapp Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a watch here in case we can handle it with a better response

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

@rajiv-jain-netapp rajiv-jain-netapp Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see if we can modularize the initialization method. it line of code has increased, and further may come when we would be extending to another approach based on a flag.

@piyush5netapp piyush5netapp merged commit 2c61e76 into main Dec 8, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rajiv-jain-netapp rajiv-jain-netapp rajiv-jain-netapp approved these changes

@sandeeplocharla sandeeplocharla Awaiting requested review from sandeeplocharla

@suryag1201 suryag1201 Awaiting requested review from suryag1201

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@piyush5netapp @rajiv-jain-netapp