nvazquez commented on a change in pull request #4282:
URL: https://github.com/apache/cloudstack/pull/4282#discussion_r559732436
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -2372,18 +2368,32 @@ private void
setDestinationPoolAndReallocateNetwork(StoragePool destPool, VMInst
}
}
- private void afterStorageMigrationVmwareVMcleanup(StoragePool destPool,
VMInstanceVO vm, HostVO srcHost, Long srcClusterId) {
+ private void afterStorageMigrationVmwareVMcleanup(StoragePool destPool,
VMInstanceVO vm) {
// OfflineVmwareMigration: this should only happen on storage
migration, else the guru would already have issued the command
- final Long destClusterId = destPool.getClusterId();
- if (srcClusterId != null && destClusterId != null && !
srcClusterId.equals(destClusterId)) {
- final String srcDcName =
_clusterDetailsDao.getVmwareDcName(srcClusterId);
- final String destDcName =
_clusterDetailsDao.getVmwareDcName(destClusterId);
- if (srcDcName != null && destDcName != null &&
!srcDcName.equals(destDcName)) {
- removeStaleVmFromSource(vm, srcHost);
- }
+ Long destClusterId = destPool.getClusterId();
+ Long srcHostId = vm.getHostId() != null ? vm.getHostId() :
vm.getLastHostId();
+ if (srcHostId == null) {
+ s_logger.debug(String.format("No Host ID was found when doing
cleanup after Vmware migration for VM with ID = [%d] and Cluster destination ID
= [%d]", vm.getId(), destClusterId));
+ return;
+ }
+ HostVO srcHost = _hostDao.findById(srcHostId);
+ if (srcHost == null) {
+ s_logger.debug(String.format("When doing cleanup after Vmware
migration could not find a host for the given ID = [%d]", srcHostId));
+ return;
+ }
+ Long srcClusterId = srcHost.getClusterId();
+ if (srcClusterId.equals(destClusterId)) {
Review comment:
Suggestion: why not reversing the logic and moving the code block from
in lines 2377-2383 to this point?
I'm ok anyways but I think the other way is easier to read. If you are
keeping this if block please add null checks for srcClusterId and destClusterId
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -2372,18 +2368,32 @@ private void
setDestinationPoolAndReallocateNetwork(StoragePool destPool, VMInst
}
}
- private void afterStorageMigrationVmwareVMcleanup(StoragePool destPool,
VMInstanceVO vm, HostVO srcHost, Long srcClusterId) {
+ private void afterStorageMigrationVmwareVMcleanup(StoragePool destPool,
VMInstanceVO vm) {
// OfflineVmwareMigration: this should only happen on storage
migration, else the guru would already have issued the command
- final Long destClusterId = destPool.getClusterId();
- if (srcClusterId != null && destClusterId != null && !
srcClusterId.equals(destClusterId)) {
- final String srcDcName =
_clusterDetailsDao.getVmwareDcName(srcClusterId);
- final String destDcName =
_clusterDetailsDao.getVmwareDcName(destClusterId);
- if (srcDcName != null && destDcName != null &&
!srcDcName.equals(destDcName)) {
- removeStaleVmFromSource(vm, srcHost);
- }
+ Long destClusterId = destPool.getClusterId();
+ Long srcHostId = vm.getHostId() != null ? vm.getHostId() :
vm.getLastHostId();
+ if (srcHostId == null) {
+ s_logger.debug(String.format("No Host ID was found when doing
cleanup after Vmware migration for VM with ID = [%d] and Cluster destination ID
= [%d]", vm.getId(), destClusterId));
+ return;
+ }
+ HostVO srcHost = _hostDao.findById(srcHostId);
+ if (srcHost == null) {
+ s_logger.debug(String.format("When doing cleanup after Vmware
migration could not find a host for the given ID = [%d]", srcHostId));
+ return;
+ }
+ Long srcClusterId = srcHost.getClusterId();
+ if (srcClusterId.equals(destClusterId)) {
+ s_logger.debug("Since the Source cluster ID [%s] is equal to the
Destination cluster ID [%s] we do not need to proceed with the clean up after
migration");
+ return;
+ }
+ String srcDcName = _clusterDetailsDao.getVmwareDcName(srcClusterId);
+ String destDcName = _clusterDetailsDao.getVmwareDcName(destClusterId);
+ if(StringUtils.isNotBlank(srcDcName) &&
StringUtils.isNotBlank(destDcName)){
Review comment:
Minor checkstyle bug: please add a space between the if and the
condition and other space before the curly brace after the 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.
For queries about this service, please contact Infrastructure at:
[email protected]
