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?
| throw new CloudRuntimeException("Invalid accessGroup object - hostsToConnect is null or empty"); | ||
| } | ||
|
|
||
| Map<String, String> details = storagePoolDetailsDao.listDetailsKeyPairs(accessGroup.getStoragePoolId()); |
There was a problem hiding this comment.
How can an NFS storage pool exist without an export policy associated with it? Based on that, I’m not sure how the null check below would help or how it would lead to creating a policy for the storage pool.
Also, could you add code comments explaining the scenarios in which the workflow would enter the if and else blocks?
There was a problem hiding this comment.
Thankyou for pointing out. The null check has been auto-added while refactoring the code at the end.
Sure will update the necessary comments.
|
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
| @@ -18,26 +18,13 @@ | |||
| */ | |||
| package org.apache.cloudstack.storage.driver; | |||
There was a problem hiding this comment.
this clas does not have any change, Shall we revert this file?
| 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?
2 participants
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?
How did you try to break this feature and the system with this change?