rp- commented on code in PR #8889:
URL: https://github.com/apache/cloudstack/pull/8889#discussion_r1580611288
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1696,14 +1850,15 @@ private void
handleCreateVolumeFromVolumeOnSecondaryStorage(VolumeInfo srcVolume
private CopyCmdAnswer copyImageToVolume(DataObject srcDataObject,
VolumeInfo destVolumeInfo, HostVO hostVO) {
int primaryStorageDownloadWait =
StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value();
- CopyCommand copyCommand = new CopyCommand(srcDataObject.getTO(),
destVolumeInfo.getTO(), primaryStorageDownloadWait,
- VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand copyCommand = null;
Review Comment:
Why have this mutable and not just down with the `new CopyCommand`?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2496,15 +2599,17 @@ private void
handleCreateTemplateFromManagedVolume(VolumeInfo volumeInfo, Templa
int primaryStorageDownloadWait =
StorageManager.PRIMARY_STORAGE_DOWNLOAD_WAIT.value();
- CopyCommand copyCommand = new CopyCommand(volumeInfo.getTO(),
templateInfo.getTO(), primaryStorageDownloadWait,
VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand copyCommand = null;
Review Comment:
Why have this mutable and not just down with the `new CopyCommand`?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2975,19 +3076,21 @@ private CopyCmdAnswer performCopyOfVdi(VolumeInfo
volumeInfo, SnapshotInfo snaps
srcData = cacheData;
}
- CopyCommand copyCommand = new CopyCommand(srcData.getTO(),
volumeInfo.getTO(), primaryStorageDownloadWait,
VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand copyCommand = null;
Review Comment:
Why have this mutable and not just down with the `new CopyCommand`?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/MultipathSCSIAdapterBase.java:
##########
@@ -223,15 +228,19 @@ public boolean disconnectPhysicalDisk(Map<String, String>
volumeToDisconnect) {
@Override
public boolean disconnectPhysicalDiskByPath(String localPath) {
LOGGER.debug(String.format("disconnectPhysicalDiskByPath(localPath)
called with args (%s) STARTED", localPath));
+ if (localPath == null || (localPath != null &&
!localPath.startsWith("/dev/mapper/"))) {
Review Comment:
`/dev/mapper` is still a bit broad for me?
I don't know if there is any primary storage who might also use
`/dev/mapper` but it would certainly conflict.
Is there a better way to determine if a path belongs to FibreChannel?
##########
server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java:
##########
@@ -561,6 +561,7 @@ private StoragePool getStoragePool(final
UnmanagedInstanceTO.Disk disk, final Da
}
}
}
+
Review Comment:
revert this? as it adds nothing except touching a file
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -266,10 +266,16 @@ public Answer copyTemplateToPrimaryStorage(final
CopyCommand cmd) {
Map<String, String> details = primaryStore.getDetails();
- String path = details != null ?
details.get("managedStoreTarget") : null;
+ String path = null;
+ if (primaryStore.getPoolType() ==
StoragePoolType.FiberChannel) {
+ path = destData.getPath();
+ } else {
+ path = details != null ? details.get("managedStoreTarget")
: null;
+ }
Review Comment:
Move this into a function, as it is also used in the
`cloneVolumeFromBaseTemplate`
--
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]
