You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/01/18 18:12:46 UTC
[GitHub] [cloudstack] nvazquez commented on a change in pull request #4282: Prevents null pointers when doing consecutive VM migrates
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:
users@infra.apache.org