harikrishna-patnala commented on code in PR #8889:
URL: https://github.com/apache/cloudstack/pull/8889#discussion_r1627181071
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -695,37 +695,21 @@ private void
handleVolumeMigrationFromNonManagedStorageToManagedStorage(VolumeIn
if (HypervisorType.XenServer.equals(hypervisorType)) {
handleVolumeMigrationForXenServer(srcVolumeInfo,
destVolumeInfo);
- }
- else {
+ destVolumeInfo =
_volumeDataFactory.getVolume(destVolumeInfo.getId(),
destVolumeInfo.getDataStore());
Review Comment:
@rg9975 why is the copycommand result and callback moved only for XenServer
from finally block. This means we are now skipping for kvm or other hypervisor
types. Am I missing anything ?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1796,31 +1889,20 @@ private void createVolumeFromSnapshot(SnapshotInfo
snapshotInfo) {
* invocation of createVolumeFromSnapshot(SnapshotInfo).
*/
private void deleteVolumeFromSnapshot(SnapshotInfo snapshotInfo) {
- VolumeVO volumeVO = null;
- // cleanup any temporary volume previously created for copy from a
snapshot
- if
("true".equalsIgnoreCase(snapshotInfo.getDataStore().getDriver().getCapabilities().get("CAN_CREATE_TEMP_VOLUME_FROM_SNAPSHOT")))
{
- SnapshotDetailsVO tempUuid = null;
- tempUuid = _snapshotDetailsDao.findDetail(snapshotInfo.getId(),
"TemporaryVolumeCopyUUID");
Review Comment:
is there covered any where in your new code to delete the snapshot detail ?
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -4367,14 +4368,7 @@ public boolean
checkIfDynamicScalingCanBeEnabled(VirtualMachine vm, ServiceOffer
*/
protected long configureCustomRootDiskSize(Map<String, String>
customParameters, VMTemplateVO template, HypervisorType hypervisorType,
DiskOfferingVO rootDiskOffering) {
verifyIfHypervisorSupportsRootdiskSizeOverride(hypervisorType);
- Long rootDiskSizeCustomParam = null;
Review Comment:
why we had to remove the rootDiskSizeCustomParam related code here ?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -870,6 +867,18 @@ private void handleVolumeMigrationForKVM(VolumeInfo
srcVolumeInfo, VolumeInfo de
throw new CloudRuntimeException(errMsg, ex);
}
} finally {
+ // revoke access (for managed volumes)
Review Comment:
@rg9975 do we need to test this with other managed storages like Dell
Powerflex ? because I see we have introduced revoking the access for the
volume. I agree this is required here, my only concern is if it creates any
regression to other managed storages.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -1021,7 +1032,9 @@ public Answer backupSnapshot(final CopyCommand cmd) {
command.add(NAME_OPTION, snapshotName);
command.add("-p", snapshotDestPath);
- descName = UUID.randomUUID().toString();
+ if (isCreatedFromVmSnapshot) {
Review Comment:
can you please explain why we had to keep this specific check for vmsnapshot
case ?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2628,13 +2710,7 @@ private Map<String, String>
getSnapshotDetails(SnapshotInfo snapshotInfo) {
long snapshotId = snapshotInfo.getId();
- // if the snapshot required a temporary volume be created check if the
UUID is set so we can
- // retrieve the temporary volume's path to use during remote copy
- List<SnapshotDetailsVO> storedDetails =
_snapshotDetailsDao.findDetails(snapshotInfo.getId(),
"TemporaryVolumeCopyPath");
- if (storedDetails != null && storedDetails.size() > 0) {
Review Comment:
any reason why we had to remove the first if condition ?
--
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]
