DaanHoogland commented on code in PR #8085:
URL: https://github.com/apache/cloudstack/pull/8085#discussion_r1356873391
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -1443,6 +1443,12 @@ public void orchestrateStart(final String vmUuid, final
Map<VirtualMachineProfil
}
if (canRetry) {
try {
+ // Setting pod id to null will result in migration of
Volumes across pods
+ // We set it to null only if migration of volumes
across cluster is enabled
+ // Or volumes are still in allocated state for that VM
(in case of failure during deployment)
+ if
(MIGRATE_VM_ACROSS_CLUSTERS.valueIn(vm.getDataCenterId()) ||
checkForNonAllocatedVolumes(vm.getId())) {
+ vm.setPodIdToDeployIn(null);
+ }
Review Comment:
can this be a separate method and javadoc?
also,
```
// We set it to null only if migration of volumes
across cluster is enabled
```
is clear, but
```
// Or volumes are still in allocated state for that
VM (in case of failure during deployment)
```
is Hindi to me ;) Would you mean
```suggestion
// Setting pod id to null will result in migration
of Volumes across pods
// We set it to null only if migration of volumes
across cluster is enabled,
// or in case some volumes are not in allocated
state for that VM.
// In which case deployment would fail?
if
(MIGRATE_VM_ACROSS_CLUSTERS.valueIn(vm.getDataCenterId()) ||
checkForNonAllocatedVolumes(vm.getId())) {
vm.setPodIdToDeployIn(null);
}
```
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -1460,6 +1466,11 @@ public void orchestrateStart(final String vmUuid, final
Map<VirtualMachineProfil
}
}
+ private boolean checkForNonAllocatedVolumes(long vmId) {
+ final List<VolumeVO> vols = _volsDao.findByInstance(vmId);
+ return CollectionUtils.isEmpty(vols) || vols.stream().allMatch(v ->
Volume.State.Allocated.equals(v.getState()));
Review Comment:
so false if any are not in allocated-state? How does this logically is a
`checkForNonAllocatedVolumes()`? seems like a
`checkThatAllVolumesAreAllocated()` to me.
--
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]
