Multi-Host support for NFS3 and iSCSI#64
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
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?
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.
| @Override | ||
| AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| public AccessGroup updateAccessGroup(AccessGroup accessGroup) { | ||
| return null; |
| 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; |
| HostVO hostVO = (HostVO) host; | ||
| if (!isNfs3EnabledOnHost(hostVO)) { | ||
| throw new CloudRuntimeException("NFS protocol is not enabled on host with id: " + hostId); | ||
| } |
| Host host = _hostDao.findById(hostId); | ||
| if (host == null) { | ||
| logger.warn("Host not found with id: {}", hostId); | ||
| return false; | ||
| } |
| 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 + "'"); |
| 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"); | ||
| } |
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?