Multi-Host support for NFS3 and iSCSI#64
Open
sandeeplocharla wants to merge 5 commits into
Open
Conversation
sandeeplocharla
requested review from
piyush5netapp,
rajiv-jain-netapp and
suryag1201
as code owners
sandeeplocharla
requested review from
piyush5netapp,
rajiv-jain-netapp and
suryag1201
and removed request for
piyush5netapp,
rajiv-jain-netapp and
suryag1201
| if (!hostConnectedToPool) { | ||
| try { | ||
| logger.info("grantAccess: Host [{}] is not connected to NFS pool [{}], reconnecting host to pool", host.getId(), storagePool.getId()); | ||
| boolean connected = storageManager.connectHostToSharedPool(host, storagePool.getId()); |
There was a problem hiding this comment.
The shared pool term is picked from the CloudStack perspective, or from the way we are approaching storage pool design. Please add code comment, if possible
Collaborator
Author
There was a problem hiding this comment.
Though I'm aware that this code is a bit of a stretch, I have added with nfs protocol being enabled on a host post its addition to the storage pool. I wasn't able to simulate the scenario and looks like a rare occurence. So, on discussing with the team in the scrum today, we came to consensus that at this point we can document the pre-requisite of enabling the protocol on host and remove this code. I wanted to take your opinion on the same. So, please let me know if you feel the same?
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
45b634f to
8323d52
Compare
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
8323d52 to
1f0e3c1
Compare
| StoragePool pool = _storagePoolDao.findById(poolId); | ||
| if (pool == null) { | ||
| logger.error("Failed to disconnect host - storage pool not found with id: {}", poolId); | ||
| if (!host.getHypervisorType().equals(Hypervisor.HypervisorType.KVM)) { |
There was a problem hiding this comment.
If this logic is executed during host removal, and the host is not a KVM host, do we really need to return false? Would it make more sense to return true instead?
Returning false could potentially block the host removal workflow, which may not be desirable and does not appear to provide any benefit to our plugin. Could you please confirm the expected behavior here and whether returning false is intentionally required for non-KVM hosts?
Collaborator
Author
There was a problem hiding this comment.
If the host is not KVM, the call shouldn't even come our plugin's host listener. So, this check seems redundant right now. Removing it.
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
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.
Comment on lines
161
to
163
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; |
Comment on lines
45
to
+56
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.ArgumentMatchers.anyMap; | ||
| import static org.mockito.ArgumentMatchers.anyString; | ||
| import static org.mockito.ArgumentMatchers.eq; | ||
| import org.mockito.Mock; |
Comment on lines
+188
to
+191
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
Comment on lines
+228
to
+232
| Host host = _hostDao.findById(hostId); | ||
| if (host == null) { | ||
| logger.warn("Host not found with id: {}", hostId); | ||
| return false; | ||
| } |
Comment on lines
+175
to
+178
| private void updateNfsExportPolicyForConnectedHostIfNeeded(long poolId, long hostId, Host host, Map<String, String> detailsMap) { | ||
| if (!ProtocolType.NFS3.name().equalsIgnoreCase(detailsMap.get(OntapStorageConstants.PROTOCOL))) { | ||
| return; | ||
| } |
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); |
Comment on lines
198
to
+205
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| if (accessGroup == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); | ||
| } | ||
| // Check if an AccessGroup was constructed without associating it to a storage pool. | ||
| if (accessGroup.getStoragePoolId() == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - storagePoolId is null"); | ||
| } |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
8a26a59 to
f8a887c
Compare
Comment on lines
+97
to
+101
| <<<<<<< HEAD | ||
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; | ||
| ======= | ||
| >>>>>>> 8a4a24f841 (Reverted few unnecessary changes) |
Comment on lines
217
to
222
| @Override | ||
| public boolean hostAboutToBeRemoved(long hostId) { | ||
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
+70
to
81
| @JsonCreator | ||
| public static ProtocolsEnum fromValue(String text) { | ||
| if (text == null) { | ||
| return null; | ||
| } | ||
| for (ProtocolsEnum b : ProtocolsEnum.values()) { | ||
| if (String.valueOf(b.value).equals(text)) { | ||
| if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { | ||
| return b; | ||
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); | ||
| } |
Comment on lines
198
to
+210
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| if (accessGroup == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); | ||
| } | ||
| // Check if an AccessGroup was constructed without associating it to a storage pool. | ||
| if (accessGroup.getStoragePoolId() == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - storagePoolId is null"); | ||
| } | ||
| // At least one host is required regardless of ADD or REMOVE action. | ||
| // An empty list means there is nothing to add to or remove from the export policy client list. | ||
| if (accessGroup.getHostsToConnect() == null || accessGroup.getHostsToConnect().isEmpty()) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - hostsToConnect is null or empty"); | ||
| } |
| * @return the updated AccessGroup object | ||
| */ | ||
| abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); | ||
| public abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); |
|
|
||
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { |
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
3cf634c to
3669c61
Compare
Comment on lines
+97
to
+101
| <<<<<<< HEAD | ||
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; | ||
| ======= | ||
| >>>>>>> 8a4a24f841 (Reverted few unnecessary changes) |
Comment on lines
+218
to
222
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
+97
to
+101
| <<<<<<< HEAD | ||
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; | ||
| ======= | ||
| >>>>>>> 8a4a24f841 (Reverted few unnecessary changes) |
Comment on lines
+218
to
222
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
+189
to
+191
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
Comment on lines
+76
to
+80
| if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { | ||
| return b; | ||
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); |
Comment on lines
+24
to
+27
| public class CheckNfsOnHostCommand extends Command { | ||
| private String requiredNfsVersion; | ||
| private boolean strictServiceCheck; | ||
|
|
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
3669c61 to
bb97fb7
Compare
Comment on lines
+96
to
+101
| @Inject private SnapshotDetailsDao snapshotDetailsDao; | ||
| <<<<<<< HEAD | ||
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; | ||
| ======= | ||
| >>>>>>> 8a4a24f841 (Reverted few unnecessary changes) |
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); |
Comment on lines
+188
to
+191
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
|
|
||
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { |
Comment on lines
+97
to
+101
| <<<<<<< HEAD | ||
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; | ||
| ======= | ||
| >>>>>>> 8a4a24f841 (Reverted few unnecessary changes) |
Comment on lines
+76
to
+80
| if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { | ||
| return b; | ||
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); |
|
|
||
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { |
|
|
||
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); |
sandeeplocharla
force-pushed
the
feature/CSTACKEX-192
branch
from
e4ca055 to
60c82ac
Compare
Comment on lines
+188
to
+191
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
|
|
||
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { |
Comment on lines
+218
to
222
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
+69
to
+73
| import com.cloud.storage.ScopeType; | ||
| import com.cloud.storage.SnapshotVO; | ||
| import com.cloud.storage.Storage; | ||
| import com.cloud.storage.StorageManager; | ||
| import com.cloud.storage.StoragePool; |
Comment on lines
+98
to
+99
| @Inject private SnapshotDao snapshotDao; | ||
| @Inject private StorageManager storageManager; |
Comment on lines
+70
to
+74
| @JsonCreator | ||
| public static ProtocolsEnum fromValue(String text) { | ||
| if (text == null) { | ||
| return null; | ||
| } |
Comment on lines
198
to
+202
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| if (accessGroup == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); | ||
| } | ||
| // Check if an AccessGroup was constructed without associating it to a storage pool. |
Comment on lines
75
to
81
| for (ProtocolsEnum b : ProtocolsEnum.values()) { | ||
| if (String.valueOf(b.value).equals(text)) { | ||
| if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { | ||
| return b; | ||
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); | ||
| } |
Comment on lines
+250
to
+255
| String hostStorageIp = host.getStorageIpAddress(); | ||
| String ip = (hostStorageIp != null && !hostStorageIp.isEmpty()) ? hostStorageIp : host.getPrivateIpAddress(); | ||
| // Occurs when a CloudStack host has neither a storage IP nor a private IP configured | ||
| // (misconfigured or partially registered host). Skip it to avoid inserting a broken | ||
| // or empty match entry into the ONTAP export rule. | ||
| if (ip == null || ip.isEmpty()) { |
Comment on lines
+188
to
+191
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
Comment on lines
+218
to
222
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
198
to
+201
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| if (accessGroup == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); | ||
| } |
|
|
||
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { |
Comment on lines
217
to
222
| @Override | ||
| public boolean hostAboutToBeRemoved(long hostId) { | ||
| public boolean hostDisconnected(long hostId, long poolId) { | ||
| logger.info("Disconnect from host " + hostId + " from pool " + poolId); | ||
| // Note: This is not currently being called for NetApp ONTAP storage plugin. | ||
| return false; | ||
| } |
Comment on lines
615
to
621
| * updateExportPolicy example add/remove-Rule for NFS 3.0 and NFS 4.1 protocols | ||
| * //TODO for Nvme/TCP and Nvme/FC protocols | ||
| * @param accessGroup the access group to update | ||
| * @return the updated AccessGroup object | ||
| */ | ||
| abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); | ||
| public abstract AccessGroup updateAccessGroup(AccessGroup accessGroup); | ||
|
|
Comment on lines
71
to
81
| public static ProtocolsEnum fromValue(String text) { | ||
| if (text == null) { | ||
| return null; | ||
| } | ||
| for (ProtocolsEnum b : ProtocolsEnum.values()) { | ||
| if (String.valueOf(b.value).equals(text)) { | ||
| if (String.valueOf(b.value).equalsIgnoreCase(text) || b.name().equalsIgnoreCase(text)) { | ||
| return b; | ||
| } | ||
| } | ||
| return null; | ||
| throw new IllegalArgumentException("Unexpected protocol value '" + text + "'"); | ||
| } |
Comment on lines
168
to
171
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| } |
Comment on lines
198
to
+202
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; | ||
| if (accessGroup == null) { | ||
| throw new CloudRuntimeException("Invalid accessGroup object - accessGroup is null"); | ||
| } | ||
| // Check if an AccessGroup was constructed without associating it to a storage pool. |
sandeeplocharla
commented
Jul 10, 2026
| } | ||
|
|
||
| // Update NFS export policy for this connected host when the pool protocol is NFS3. | ||
| updateNfsExportPolicyForConnectedHostIfNeeded(poolId, hostId, host, detailsMap); |
Collaborator
Author
There was a problem hiding this comment.
Add to TODO to throw an alert if protocol wasn't enabled on the host
sandeeplocharla
commented
Jul 10, 2026
| } | ||
|
|
||
| logger.info("Removing export policy rule for host {} from storage pool {}", host.getName(), pool.getName()); | ||
| HostVO hostVO = (HostVO) host; |
Collaborator
Author
There was a problem hiding this comment.
Remove the typecasting and checking the instance of HostVO
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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?