winterhazel commented on code in PR #14192:
URL: https://github.com/apache/cloudstack/pull/14192#discussion_r4530792369
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:
##########
@@ -148,137 +153,24 @@ public boolean connectPhysicalDisk(String volumeUuid,
KVMStoragePool pool, Map<S
// After a certain number of tries and a certain waiting period in
between tries,
// this method could still return (it should not block indefinitely)
(the race condition
// isn't solved here, but made highly unlikely to be a problem).
- // If the by-path is missing or is a regular file (not the iSCSI block
symlink), size
- // stays 0. Return false so connect does not succeed and a raw file is
not created at
- // that by-path in place of the real LUN device.
- if (!waitForDiskToBecomeAvailable(volumeUuid, pool)) {
- logger.warn("iSCSI device not ready for target {} at {}:{} after
wait", volumeUuid, host, port);
- return false;
- }
+ waitForDiskToBecomeAvailable(volumeUuid, pool);
return true;
}
- /**
- * Checks the result of an iscsiadm node-create command.
- * Returns true if the node was created or already exists, false on
failure.
- */
- boolean handleNodeCreateResult(String result, String volumeUuid) {
Review Comment:
> Since we have introduced new Netapp Ontap pool type as OntapIscsi, all
those changes are replicated to new adapter OntapIscsiStorageAdaptor. I did
this keeping separation of concern as one of the thought. Also, I see
IscsiAdmStorageAdaptor is written mostly in a way to support a design where
iscsi session cannot be shared across multiple cloudstack volume which was not
aligning with Netapp Ontap where same iscsi session can be shared across
multiple cloudstack volumes
Ok, it makes sense to create a new OntapIscsiStorageAdaptor.
> But, I feel refactoring of the changes like idempotency in node creation,
session login or pre-existence can also sit in IscsiAdmStorageAdaptor as an
enhancement. Let me know your thoughts.
I agree that these changes can sit in IscsiAdmStorageAdaptor as
enhancements. As these enhancements were already merged, can we leave this
class as it is in main? This pull request is currently reverting them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
