DaanHoogland commented on code in PR #14207:
URL: https://github.com/apache/cloudstack/pull/14207#discussion_r4204664137
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -696,6 +696,11 @@ protected void advanceExpunge(VMInstanceVO vm) throws
ResourceUnavailableExcepti
advanceStop(vm.getUuid(), VmDestroyForcestop.value());
vm = _vmDao.findByUuid(vm.getUuid());
+ // advanceStop() returns without contacting the host when the database
already has the instance as Stopped,
+ // Error, Destroyed or Expunging. The host may still be running it.
Expunging is about to release its
+ // addresses and delete its volumes, so make sure no domain is left
behind for it.
Review Comment:
`ensureInstanceIsStoppedOnLastKnownHost` has good javadoc, I don't think
this comment is needed
```suggestion
```
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -5452,11 +5468,81 @@ private void
handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
case Error:
default:
logger.info("Receive power on report when Instance is in error or
unexpected state. Instance: {}, state: {}.", vm, vm.getState());
+ _alertMgr.sendAlert(AlertManager.AlertType.ALERT_TYPE_SYNC,
vm.getDataCenterId(), vm.getPodIdToDeployIn(),
+ VM_SYNC_ALERT_SUBJECT, String.format("Instance %s is
reported running by host %s but is in %s state. "
+ + "It is not managed by CloudStack and may need to
be stopped on the host.",
+ vm.getInstanceName(), vm.getPowerHostId(),
vm.getState()));
break;
}
}
- private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO
vm) {
+ /**
+ * Send a StopCommand for an instance to the last host it is known to have
run on, whatever the database state
+ * says. Used before expunging, where the instance's addresses and volumes
are about to be released and a domain
+ * left running on the host would keep using them.
+ *
+ * External instances are skipped: their teardown is done by their
extension in finalizeExpunge, and a
+ * StopCommand issued from here would not carry the details that path
needs.
+ */
+ protected void ensureInstanceIsStoppedOnLastKnownHost(final VMInstanceVO
vm) {
+ if (vm == null ||
HypervisorType.External.equals(vm.getHypervisorType())) {
+ return;
+ }
+ final Long hostId = vm.getHostId() != null ? vm.getHostId() :
vm.getLastHostId();
+ if (hostId == null) {
+ return;
+ }
+ try {
+ // checkBeforeCleanup is false on purpose: a running domain is
what has to be removed here.
+ final StopCommand stop = buildStopCommand(vm, new
VirtualMachineProfileImpl(vm), false);
+ final Answer answer = _agentMgr.send(hostId, stop);
+ if (answer != null && answer.getResult()) {
+ return;
+ }
+ logger.warn("Unable to confirm instance {} is stopped on host {}
before expunging: {}", vm, hostId,
+ answer == null ? "no answer from host" :
answer.getDetails());
+ } catch (final AgentUnavailableException | OperationTimedoutException
e) {
+ logger.warn("Unable to confirm instance {} is stopped on host {}
before expunging.", vm, hostId, e);
+ }
+ }
+
+ /**
+ * The host reports an instance as powered on that the database considers
destroyed or expunged. It will never be
+ * managed again, and its addresses and storage have already been handed
back, so stop it on the host that
+ * reported it instead of leaving it running unmanaged.
+ *
+ * External instances are skipped, as in
ensureInstanceIsStoppedOnLastKnownHost().
+ */
+ protected void stopUnmanagedInstanceOnReportingHost(final VMInstanceVO vm)
{
+ if (HypervisorType.External.equals(vm.getHypervisorType())) {
+ return;
+ }
+ final Long powerHostId = vm.getPowerHostId();
+ if (powerHostId == null) {
+ logger.warn("Instance {} is reported powered on but no reporting
host is recorded, cannot stop it.", vm);
+ return;
+ }
+ try {
+ // checkBeforeCleanup must be false: the instance is known to be
running, and that is exactly what
+ // has to be stopped. With it set, the host would refuse and
answer "vm is still running on host".
Review Comment:
```suggestion
// do not check before cleaning, just stop it
```
or leave out the comment completely? though in this case it does add some
value, we could also add a `buildUnconditionalStopCommand(..)` wrapper.
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -5480,20 +5566,26 @@ private void
handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
return;
}
- if (PowerState.PowerOff.equals(vm.getPowerState())) {
+ // A missing report is not proof that the instance is gone, only
that the host did not list it. Stop it
+ // on the host before giving up its resources, otherwise a
still-running instance keeps its NICs and IP
+ // addresses while the database says they are free and they get
handed to another instance.
+ if (PowerState.PowerOff.equals(vm.getPowerState()) ||
PowerState.PowerReportMissing.equals(vm.getPowerState())) {
+ // force is false for a missing report. sendStop() swallows
AgentUnavailableException and
+ // OperationTimedoutException and answers success when forced,
and a host too busy to answer is
+ // exactly the condition that produced the stale report in the
first place. Backing off and letting a
+ // later report decide is better than freeing an address on no
evidence. A PowerOff report is the
+ // host stating the instance is down, so that path keeps its
previous behaviour.
Review Comment:
```suggestion
// forceStop only if the state is know to be up`n'running.
```
again, "keeps its previous behaviour" is a comment that asks a lot of
context from the user.
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -5480,20 +5566,26 @@ private void
handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
return;
}
- if (PowerState.PowerOff.equals(vm.getPowerState())) {
+ // A missing report is not proof that the instance is gone, only
that the host did not list it. Stop it
+ // on the host before giving up its resources, otherwise a
still-running instance keeps its NICs and IP
+ // addresses while the database says they are free and they get
handed to another instance.
Review Comment:
this comment is clear to people that have seen and understand the issue. it
is clutter for new viewers.
```suggestion
```
--
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]
