DaanHoogland commented on a change in pull request #4644:
URL: https://github.com/apache/cloudstack/pull/4644#discussion_r623860237
##########
File path:
services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
##########
@@ -811,6 +811,13 @@ public void allocCapacity(long dataCenterId,
SecondaryStorageVm.Role role) {
}
public boolean isZoneReady(Map<Long, ZoneHostInfo> zoneHostInfoMap, long
dataCenterId) {
+ List <HostVO> hosts = _hostDao.listByDataCenterId(dataCenterId);
+ if (CollectionUtils.isEmpty(hosts)) {
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("Zone " + dataCenterId + " has no host
available which is enabled and in Up state");
+ }
+ return false;
+ }
Review comment:
separate method?
##########
File path: server/src/main/java/com/cloud/ha/HighAvailabilityManagerImpl.java
##########
@@ -682,29 +687,44 @@ public void cancelDestroy(VMInstanceVO vm, Long hostId) {
protected Long destroyVM(final HaWorkVO work) {
final VirtualMachine vm = _itMgr.findById(work.getInstanceId());
- s_logger.info("Destroying " + vm.toString());
+ if (vm == null) {
+ s_logger.info("No longer can find VM " + work.getInstanceId() + ".
Throwing away " + work);
+ work.setStep(Step.Done);
+ return null;
+ }
+ boolean expunge =
VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType())
+ || VirtualMachine.Type.ConsoleProxy.equals(vm.getType());
+ if (!expunge &&
VirtualMachine.State.Destroyed.equals(work.getPreviousState())) {
+ s_logger.info("VM " + vm.getUuid() + " already in " +
vm.getState() + " state. Throwing away " + work);
+ work.setStep(Step.Done);
+ return null;
+ }
try {
- if (vm.getState() != State.Destroyed) {
- s_logger.info("VM is no longer in Destroyed state " +
vm.toString());
- return null;
+ if (VirtualMachine.State.Running.equals(work.getPreviousState())) {
+ _itMgr.advanceStop(vm.getUuid(), true);
}
-
- if (vm.getHostId() != null) {
- _itMgr.destroy(vm.getUuid(), false);
- s_logger.info("Successfully destroy " + vm);
- return null;
- } else {
- if (s_logger.isDebugEnabled()) {
- s_logger.debug(vm + " has already been stopped");
+ s_logger.info("Destroying " + vm.toString());
+ if
(!VirtualMachine.State.Expunging.equals(work.getPreviousState())) {
+ s_logger.info("Destroying " + vm.getUuid());
+ if (VirtualMachine.Type.ConsoleProxy.equals(vm.getType())) {
+ consoleProxyManager.destroyProxy(vm.getId());
+ } else if
(VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType())) {
+ secondaryStorageVmManager.destroySecStorageVm(vm.getId());
+ } else {
Review comment:
the total method has grown from 23 to 40 lines, can you factor the new
bits out please.
##########
File path:
server/src/main/java/com/cloud/consoleproxy/ConsoleProxyManagerImpl.java
##########
@@ -1006,6 +1006,13 @@ private void allocCapacity(long dataCenterId) {
}
public boolean isZoneReady(Map<Long, ZoneHostInfo> zoneHostInfoMap, long
dataCenterId) {
+ List <HostVO> hosts = _hostDao.listByDataCenterId(dataCenterId);
+ if (CollectionUtils.isEmpty(hosts)) {
+ if (s_logger.isDebugEnabled()) {
+ s_logger.debug("Zone " + dataCenterId + " has no host
available which is enabled and in Up state");
+ }
+ return false;
+ }
Review comment:
this is the exact same code as
inSecondaryStorageManagerImpl.isZoneReady() can you factor out and reuse?
##########
File path: server/src/main/java/com/cloud/resource/ResourceManagerImpl.java
##########
@@ -1267,8 +1266,14 @@ private boolean doMaintain(final long hostId) {
if (hosts == null || hosts.isEmpty() || !answer.getMigrate()
||
_serviceOfferingDetailsDao.findDetail(vm.getServiceOfferingId(),
GPU.Keys.vgpuType.toString()) != null) {
// Migration is not supported for VGPU Vms so stop them.
- // for the last host in this cluster, stop all the VMs
- s_logger.error("Maintenance: No hosts available for
migrations. Scheduling shutdown instead of migrations.");
+ // for the last host in this cluster, destroy SSVM/CPVM
and stop all other VMs
+ if
(VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType())
+ ||
VirtualMachine.Type.ConsoleProxy.equals(vm.getType())) {
+ s_logger.error(String.format("Maintenance: VM is of
type %s. Destroying VM %s (ID: %s) immediately instead of migration.",
vm.getType().toString(), vm.getInstanceName(), vm.getUuid()));
+ _haMgr.scheduleDestroy(vm, host.getId());
+ continue;
+ }
Review comment:
can you factor this out, including the comment, please?
##########
File path:
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -606,10 +610,18 @@ protected void advanceExpunge(VMInstanceVO vm) throws
ResourceUnavailableExcepti
if (hostId != null) {
final Commands cmds = new Commands(Command.OnError.Stop);
for (final Command command : finalizeExpungeCommands) {
+ if
(VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType()) ||
+
VirtualMachine.Type.ConsoleProxy.equals(vm.getType())) {
+ command.setBypassHostMaintenance(true);
+ }
cmds.addCommand(command);
}
if (nicExpungeCommands != null) {
for (final Command command : nicExpungeCommands) {
+ if
(VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType()) ||
+
VirtualMachine.Type.ConsoleProxy.equals(vm.getType())) {
+ command.setBypassHostMaintenance(true);
+ }
Review comment:
I'm seeing this code fragment a lot, please factor out and reuse.
##########
File path:
plugins/storage/volume/default/src/main/java/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
##########
@@ -214,6 +216,16 @@ public void createAsync(DataStore dataStore, DataObject
data, AsyncCompletionCal
@Override
public void deleteAsync(DataStore dataStore, DataObject data,
AsyncCompletionCallback<CommandResult> callback) {
DeleteCommand cmd = new DeleteCommand(data.getTO());
+ if (DataObjectType.VOLUME.equals(data.getType())) {
+ Volume volume = (Volume)data;
+ if (volume.getInstanceId() != null) {
+ VMInstanceVO vm = vmDao.findById(volume.getInstanceId());
+ if (vm != null &&
(VirtualMachine.Type.SecondaryStorageVm.equals(vm.getType()) ||
+
VirtualMachine.Type.ConsoleProxy.equals(vm.getType()))) {
+ cmd.setBypassHostMaintenance(true);
+ }
+ }
+ }
Review comment:
can you factor out and give a meaningful name?
--
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]
