slavkap commented on code in PR #8889:
URL: https://github.com/apache/cloudstack/pull/8889#discussion_r1633287838
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -207,31 +215,53 @@ public boolean connectPhysicalDisk(String volumePath,
KVMStoragePool pool, Map<S
@Override
public boolean disconnectPhysicalDisk(String volumePath, KVMStoragePool
pool) {
-
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(volumePath,pool)
called with args (%s, %s) START", volumePath, pool.getUuid()));
+ if (LOGGER.isDebugEnabled())
LOGGER.debug(String.format("disconnectPhysicalDisk(volumePath,pool) called with
args (%s, %s) START", volumePath, pool.getUuid()));
AddressInfo address = this.parseAndValidatePath(volumePath);
+ if (address.getAddress() == null) {
+ if (LOGGER.isDebugEnabled())
LOGGER.debug(String.format("disconnectPhysicalDisk(volumePath,pool) returning
FALSE, volume path has no address field", volumePath, pool.getUuid()));
+ return false;
+ }
ScriptResult result = runScript(disconnectScript, 60000L,
address.getAddress().toLowerCase());
Review Comment:
I think here should be checked the exit code and if it's -1 to return false
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -2463,8 +2473,12 @@ public Answer copyVolumeFromPrimaryToPrimary(CopyCommand
cmd) {
if
(!storagePoolMgr.connectPhysicalDisk(destPrimaryStore.getPoolType(),
destPrimaryStore.getUuid(), destVolumePath, destPrimaryStore.getDetails())) {
s_logger.warn("Failed to connect dest volume at path: " +
destVolumePath + ", in storage pool id: " + destPrimaryStore.getUuid());
}
- String managedStoreTarget = destPrimaryStore.getDetails() !=
null ? destPrimaryStore.getDetails().get("managedStoreTarget") : null;
- destVolumeName = managedStoreTarget != null ?
managedStoreTarget : destVolumePath;
+ if (destPrimaryStore.getPoolType() ==
StoragePoolType.FiberChannel) {
Review Comment:
```suggestion
destVolumeName = derivePath(destPrimaryStore, destData,
destPrimaryStore.getDetails());
```
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -207,31 +215,53 @@ public boolean connectPhysicalDisk(String volumePath,
KVMStoragePool pool, Map<S
@Override
public boolean disconnectPhysicalDisk(String volumePath, KVMStoragePool
pool) {
-
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(volumePath,pool)
called with args (%s, %s) START", volumePath, pool.getUuid()));
+ if (LOGGER.isDebugEnabled())
LOGGER.debug(String.format("disconnectPhysicalDisk(volumePath,pool) called with
args (%s, %s) START", volumePath, pool.getUuid()));
AddressInfo address = this.parseAndValidatePath(volumePath);
+ if (address.getAddress() == null) {
+ if (LOGGER.isDebugEnabled())
LOGGER.debug(String.format("disconnectPhysicalDisk(volumePath,pool) returning
FALSE, volume path has no address field", volumePath, pool.getUuid()));
+ return false;
+ }
ScriptResult result = runScript(disconnectScript, 60000L,
address.getAddress().toLowerCase());
- if (LOGGER.isDebugEnabled()) LOGGER.debug("multipath flush output: " +
result.getResult());
-
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(volumePath,pool)
called with args (%s, %s) COMPLETE [rc=%s]", volumePath, pool.getUuid(),
result.getResult())); return true;
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("multipath flush output: " + result.getResult());
+
LOGGER.debug(String.format("disconnectPhysicalDisk(volumePath,pool) called with
args (%s, %s) COMPLETE [rc=%s]", volumePath, pool.getUuid(),
result.getResult()));
+ }
+ return true;
}
@Override
public boolean disconnectPhysicalDisk(Map<String, String>
volumeToDisconnect) {
-
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(volumeToDisconnect)
called with arg bag [not implemented]:") + " " + volumeToDisconnect);
+ LOGGER.debug(String.format("disconnectPhysicalDisk(volumeToDisconnect)
called with arg bag [not implemented]:") + " " + volumeToDisconnect);
return false;
}
@Override
public boolean disconnectPhysicalDiskByPath(String localPath) {
- LOGGER.debug(String.format("disconnectPhysicalDiskByPath(localPath)
called with args (%s) STARTED", localPath));
- ScriptResult result = runScript(disconnectScript, 60000L,
localPath.replace("/dev/mapper/3", ""));
- if (LOGGER.isDebugEnabled()) LOGGER.debug("multipath flush output: " +
result.getResult());
- LOGGER.debug(String.format("disconnectPhysicalDiskByPath(localPath)
called with args (%s) COMPLETE [rc=%s]", localPath, result.getExitCode()));
return true;
+ if (localPath == null) {
+ return false;
+ }
+ if (LOGGER.isDebugEnabled())
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(localPath) called with
args (%s) START", localPath));
+ if (localPath.startsWith("/dev/mapper/")) {
+ String multipathName = localPath.replace("/dev/mapper/3", "");
+ // this ensures we only disconnect multipath devices supported by
this driver
+ for (String oui: SUPPORTED_OUI_LIST) {
+ if (multipathName.length() > 1 &&
multipathName.substring(2).startsWith(oui)) {
+ ScriptResult result = runScript(disconnectScript, 60000L,
multipathName);
Review Comment:
Maybe check the exit code here either and return false if the disconnecting
failed?
--
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]
