Multi-Host support for NFS3 and iSCSI#64
Conversation
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
45b634f to
8323d52
Compare
8323d52 to
1f0e3c1
Compare
1f0e3c1 to
8a26a59
Compare
There was a problem hiding this comment.
Pull request overview
Adds multi-host lifecycle handling for the ONTAP volume plugin by updating NFS export policy rules as hosts connect to / are removed from storage pools, and by extending the AccessGroup model/strategy API to support add/remove semantics.
Changes:
- Implemented
UnifiedNASStrategy.updateAccessGroupto add/remove host client matches in an existing NFS export policy. - Added
HostRuleActiontoAccessGroupand wired host-connect / host-removal flows to update NFS3 export policy rules. - Tightened/adjusted model and API surface (e.g.,
StorageStrategy.updateAccessGroupvisibility;ExportRule.ProtocolsEnumJSON annotations).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/service/StorageStrategyTest.java | Updates test scaffolding to match new abstract method visibility; import/method formatting adjustments. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java | Implements export-policy mutation logic for NFS host add/remove (multi-host enablement). |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java | Makes updateAccessGroup explicitly public abstract for consistent overriding/call sites. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/model/AccessGroup.java | Adds HostRuleAction (ADD/REMOVE) to drive update semantics. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/listener/OntapHostListener.java | Updates host lifecycle handling to update NFS3 export policy rules on connect and before removal. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ExportRule.java | Adjusts enum JSON serialization/deserialization behavior for export rule protocols. |
| plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java | Import reordering / formatting-only change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
8a26a59 to
f8a887c
Compare
8179fdb to
d1ed46a
Compare
| } | ||
|
|
||
| @Override | ||
| public boolean hostAboutToBeRemoved(long hostId) { |
There was a problem hiding this comment.
Thinking aloud, do we not have any usecase where this method returns false?
There was a problem hiding this comment.
If export policy rule hasn't been removed, we could catch the exception and return false
| "Cannot update an export policy with no existing rules."); | ||
| } | ||
|
|
||
| // This plugin creates a single NFS export rule per policy. More than one rule means the |
There was a problem hiding this comment.
This is not true for export policy, right? we have one export policy per pool?
There was a problem hiding this comment.
As its an update method, the code is checking to see if atleast one rule exists. But, its not creating one policy per host, though its over doing the check currently. Will correct it as we plan to allow pool creation with empty rules set.
There was a problem hiding this comment.
Okay, check the code in the remove block for the last client entry in that rule. If that is the case, we have to remove the rule itself rather than remove the client from the rule. Validate the values from ONTAP while testing.
rajiv-jain-netapp
left a comment
There was a problem hiding this comment.
In the testing section, I need results for
- User created an NFS pool for cluster scope; validate the export policy on the ONTAP for the rules and client added.
- Now add a new host to this cluster and validate whether the export policy is getting updated for this new host or not.
- Similar to #2, test for host removal as well.
Note: I noticed the term "Live migrate" in your current test case capture. Just to remind you, we do not support live migration yet; these details will lead to a wrong interpretation in audiences. Please correct them accordingly.
Updated the Testing done with the relevant screenshots. |
Description
This PR...
Provides support to Multi-Host for NFS3 and iSCSI
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Only 1 Host present in the Cluster

Created an NFS3 StoragePool with Cluster scope

ONTAP volume created for the SP

Export Policy rule and client

Added a new host

Updated client in the Export Policy

Removed the host from the cluster

The respective client has been removed from the Export Policy
