Skip to content

Commit 167320e

Browse files
Damans227sachindoddaguniDaanHoogland
authored
Propagate StopAnswer error details up the call chain when stopping a VM (#13558)
Co-authored-by: Sachin R <32716246+sachindoddaguni@users.noreply.github.com> Co-authored-by: dahn <daan@onecht.net>
1 parent a5954f9 commit 167320e

2 files changed

Lines changed: 62 additions & 40 deletions

File tree

‎engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java‎

Lines changed: 53 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1630,9 +1630,10 @@ public void orchestrateStart(final String vmUuid, final Map<VirtualMachineProfil
16301630
}
16311631

16321632
if (answer == null || !answer.getResult()) {
1633-
logger.warn("Unable to stop {} dut to {}", vm, (answer != null ? answer.getDetails() : "no answers"));
1633+
String errorDetails = (answer != null ? answer.getDetails() : "no answers");
1634+
logger.warn("Unable to stop {} dut to {}", vm, errorDetails);
16341635
_haMgr.scheduleStop(vm, destHostId, WorkType.ForceStop);
1635-
throw new ExecutionException("Unable to stop this VM, " + vm.getUuid() + " so we are unable to retry the start operation");
1636+
throw new ExecutionException("Unable to stop this VM, " + vm.getUuid() + " so we are unable to retry the start operation due to " + errorDetails);
16361637
}
16371638
throw new ExecutionException("Unable to start VM:" + vm.getUuid() + " due to error in finalizeStart, not retrying");
16381639
}
@@ -2208,7 +2209,7 @@ private List<Map<String, String>> getVolumesToDisconnect(VirtualMachine vm) {
22082209
return volumesToDisconnect;
22092210
}
22102211

2211-
protected boolean sendStop(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final boolean force, final boolean checkBeforeCleanup) {
2212+
protected Pair<Boolean, String> sendStop(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final boolean force, final boolean checkBeforeCleanup) {
22122213
final VirtualMachine vm = profile.getVirtualMachine();
22132214
Map<String, Boolean> vlanToPersistenceMap = getVlanToPersistenceMapForVM(vm.getId());
22142215
StopCommand stpCmd = new StopCommand(vm, getExecuteInSequence(vm.getHypervisorType()), checkBeforeCleanup);
@@ -2241,7 +2242,7 @@ protected boolean sendStop(final VirtualMachineGuru guru, final VirtualMachinePr
22412242
if (!answer.getResult()) {
22422243
final String details = answer.getDetails();
22432244
logger.debug("Unable to stop VM due to {}", details);
2244-
return false;
2245+
return new Pair<>(false, details);
22452246
}
22462247

22472248
guru.finalizeStop(profile, answer);
@@ -2254,21 +2255,23 @@ protected boolean sendStop(final VirtualMachineGuru guru, final VirtualMachinePr
22542255
}
22552256
}
22562257
} else {
2257-
logger.error("Invalid answer received in response to a StopCommand for {}", vm.getInstanceName());
2258-
return false;
2258+
String errorMsg = String.format("Invalid answer received in response to a StopCommand for %s", vm.getInstanceName());
2259+
logger.error(errorMsg);
2260+
return new Pair<>(false, errorMsg);
22592261
}
22602262

22612263
} catch (final AgentUnavailableException | OperationTimedoutException e) {
2262-
logger.warn("Unable to stop {} due to [{}].", vm.toString(), e.getMessage(), e);
2264+
String errorMsg = String.format("Unable to stop %s due to [%s].", vm.toString(), e.getMessage());
2265+
logger.warn(errorMsg, e);
22632266
if (!force) {
2264-
return false;
2267+
return new Pair<>(false, errorMsg);
22652268
}
22662269
}
22672270

2268-
return true;
2271+
return new Pair<>(true, null);
22692272
}
22702273

2271-
protected boolean cleanup(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final ItWorkVO work, final Event event, final boolean cleanUpEvenIfUnableToStop) {
2274+
protected Pair<Boolean, String> cleanup(final VirtualMachineGuru guru, final VirtualMachineProfile profile, final ItWorkVO work, final Event event, final boolean cleanUpEvenIfUnableToStop) {
22722275
final VirtualMachine vm = profile.getVirtualMachine();
22732276
final State state = vm.getState();
22742277
logger.debug("Cleaning up resources for the vm {} in {} state", vm, state);
@@ -2277,57 +2280,63 @@ protected boolean cleanup(final VirtualMachineGuru guru, final VirtualMachinePro
22772280
if (work != null) {
22782281
final Step step = work.getStep();
22792282
if (step == Step.Starting && !cleanUpEvenIfUnableToStop) {
2280-
logger.warn("Unable to cleanup vm {}; work state is incorrect: {}", vm, step);
2281-
return false;
2283+
String errorMsg = String.format("Unable to cleanup vm %s; work state is incorrect: %s", vm, step);
2284+
logger.warn(errorMsg);
2285+
return new Pair<>(false, errorMsg);
22822286
}
22832287

22842288
if (step == Step.Started || step == Step.Starting || step == Step.Release) {
22852289
if (vm.getHostId() != null) {
2286-
if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) {
2290+
Pair<Boolean, String> result = sendStop(guru, profile, cleanUpEvenIfUnableToStop, false);
2291+
if (!result.first()) {
22872292
logger.warn("Failed to stop vm {} in {} state as a part of cleanup process", vm, State.Starting);
2288-
return false;
2293+
return result;
22892294
}
22902295
}
22912296
}
22922297

22932298
if (step != Step.Release && step != Step.Prepare && step != Step.Started && step != Step.Starting) {
22942299
logger.debug("Cleanup is not needed for vm {}; work state is incorrect: {}", vm, step);
2295-
return true;
2300+
return new Pair<>(true, null);
22962301
}
22972302
} else {
22982303
if (vm.getHostId() != null) {
2299-
if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) {
2304+
Pair<Boolean, String> result = sendStop(guru, profile, cleanUpEvenIfUnableToStop, false);
2305+
if (!result.first()) {
23002306
logger.warn("Failed to stop vm {} in {} state as a part of cleanup process", vm, State.Starting);
2301-
return false;
2307+
return result;
23022308
}
23032309
}
23042310
}
23052311

23062312
} else if (state == State.Stopping) {
23072313
if (vm.getHostId() != null) {
2308-
if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) {
2314+
Pair<Boolean, String> result = sendStop(guru, profile, cleanUpEvenIfUnableToStop, false);
2315+
if (!result.first()) {
23092316
logger.warn("Failed to stop vm {} in {} state as a part of cleanup process", vm, State.Stopping);
2310-
return false;
2317+
return result;
23112318
}
23122319
}
23132320
} else if (state == State.Migrating) {
23142321
if (vm.getHostId() != null || vm.getLastHostId() != null) {
2315-
if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) {
2322+
Pair<Boolean, String> result = sendStop(guru, profile, cleanUpEvenIfUnableToStop, false);
2323+
if (!result.first()) {
23162324
logger.warn("Failed to stop vm {} in {} state as a part of cleanup process", vm, State.Migrating);
2317-
return false;
2325+
return result;
23182326
}
23192327
}
23202328
} else if (state == State.Running) {
2321-
if (!sendStop(guru, profile, cleanUpEvenIfUnableToStop, false)) {
2329+
Pair<Boolean, String> result = sendStop(guru, profile, cleanUpEvenIfUnableToStop, false);
2330+
if (!result.first()) {
23222331
logger.warn("Failed to stop vm {} in {} state as a part of cleanup process", vm, State.Running);
2323-
return false;
2332+
return result;
23242333
}
23252334
}
23262335
} finally {
23272336
releaseVmResources(profile, cleanUpEvenIfUnableToStop);
23282337
}
23292338

2330-
return true;
2339+
return new Pair<>(true, null);
23312340
}
23322341

23332342
protected void releaseVmResources(final VirtualMachineProfile profile, final boolean forced) {
@@ -2509,7 +2518,8 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
25092518
logger.warn("Unable to transition the state but we're moving on because it's forced stop", e1);
25102519

25112520
if (doCleanup) {
2512-
if (cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.StopRequested, cleanUpEvenIfUnableToStop)) {
2521+
Pair<Boolean, String> cleanupResult = cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.StopRequested, cleanUpEvenIfUnableToStop);
2522+
if (cleanupResult.first()) {
25132523
try {
25142524
if (work != null) {
25152525
logger.debug("Updating work item to Done, id: {}", work.getId());
@@ -2524,7 +2534,8 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
25242534
}
25252535
} else {
25262536
logger.debug("Failed to cleanup VM: {}", vm);
2527-
throw new CloudRuntimeException("Failed to cleanup " + vm + " , current state " + vm.getState());
2537+
String errorDetails = cleanupResult.second() != null ? " due to " + cleanupResult.second() : "";
2538+
throw new CloudRuntimeException("Failed to cleanup " + vm + " , current state " + vm.getState() + errorDetails);
25282539
}
25292540
}
25302541
}
@@ -2545,6 +2556,7 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
25452556

25462557
boolean stopped = false;
25472558
Answer answer = null;
2559+
String agentExceptionDetail = null;
25482560
try {
25492561
answer = _agentMgr.send(vm.getHostId(), stop);
25502562
if (answer != null) {
@@ -2572,6 +2584,7 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
25722584
}
25732585

25742586
} catch (AgentUnavailableException | OperationTimedoutException e) {
2587+
agentExceptionDetail = e.getMessage();
25752588
logger.warn("Unable to stop {} due to [{}].", profile.toString(), e.toString(), e);
25762589
} finally {
25772590
if (!stopped) {
@@ -2582,7 +2595,9 @@ private void advanceStop(final VMInstanceVO vm, final boolean cleanUpEvenIfUnabl
25822595
} catch (final NoTransitionException e) {
25832596
logger.warn("Unable to transition the state " + vm, e);
25842597
}
2585-
throw new CloudRuntimeException("Unable to stop " + vm);
2598+
String errorDetail = (answer != null && answer.getDetails() != null) ? answer.getDetails() : agentExceptionDetail;
2599+
String errorDetails = errorDetail != null ? " due to " + errorDetail : "";
2600+
throw new CloudRuntimeException("Unable to stop " + vm + errorDetails);
25862601
} else {
25872602
logger.warn("Unable to actually stop {} but continue with release because it's a force stop", vm);
25882603
vmGuru.finalizeStop(profile, answer);
@@ -3261,8 +3276,9 @@ protected void migrate(final VMInstanceVO vm, final long srcHostId, final Deploy
32613276
} catch (final AgentUnavailableException e) {
32623277
logger.error("AgentUnavailableException while cleanup on source host: {}", fromHost, e);
32633278
}
3264-
cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
3265-
throw new CloudRuntimeException("Unable to complete migration for " + vm);
3279+
Pair<Boolean, String> cleanupResult = cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
3280+
String errorDetails = (cleanupResult.second() != null) ? " due to " + cleanupResult.second() : "";
3281+
throw new CloudRuntimeException("Unable to complete migration for " + vm + errorDetails);
32663282
}
32673283
} catch (final OperationTimedoutException e) {
32683284
logger.warn("Error while checking the vm {} on host {}", vm, dest.getHost(), e);
@@ -3718,8 +3734,9 @@ private void orchestrateMigrateWithStorage(final String vmUuid, final long srcHo
37183734
} catch (final AgentUnavailableException e) {
37193735
logger.error("AgentUnavailableException while cleanup on source host: {}", srcHost, e);
37203736
}
3721-
cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
3722-
throw new CloudRuntimeException("VM not found on destination host. Unable to complete migration for " + vm);
3737+
Pair<Boolean, String> cleanupResult = cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
3738+
String errorDetails = (cleanupResult.second() != null) ? " due to " + cleanupResult.second() : "";
3739+
throw new CloudRuntimeException("VM not found on destination host. Unable to complete migration for " + vm + errorDetails);
37233740
}
37243741
} catch (final OperationTimedoutException e) {
37253742
logger.error("Error while checking the vm {} is on host {}", vm, destHost, e);
@@ -5003,8 +5020,9 @@ private void orchestrateMigrateForScale(final String vmUuid, final long srcHostI
50035020
} catch (final AgentUnavailableException e) {
50045021
logger.error("Unable to cleanup source host [{}] due to [{}].", fromHost, e.getMessage(), e);
50055022
}
5006-
cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
5007-
throw new CloudRuntimeException("Unable to complete migration for " + vm);
5023+
Pair<Boolean, String> cleanupResult = cleanup(vmGuru, new VirtualMachineProfileImpl(vm), work, Event.AgentReportStopped, true);
5024+
String errorDetails = (cleanupResult.second() != null) ? " due to " + cleanupResult.second() : "";
5025+
throw new CloudRuntimeException("Unable to complete migration for " + vm + errorDetails);
50085026
}
50095027
} catch (final OperationTimedoutException e) {
50105028
logger.debug("Error while checking the {} on {}", vm, dstHost, e);
@@ -5465,7 +5483,8 @@ private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
54655483
if (PowerState.PowerOff.equals(vm.getPowerState())) {
54665484
final VirtualMachineGuru vmGuru = getVmGuru(vm);
54675485
final VirtualMachineProfile profile = new VirtualMachineProfileImpl(vm);
5468-
if (!sendStop(vmGuru, profile, true, true)) {
5486+
Pair<Boolean, String> result = sendStop(vmGuru, profile, true, true);
5487+
if (!result.first()) {
54695488
return;
54705489
} else {
54715490
// Release resources on StopCommand success

‎engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,9 @@ public void testSendStopWithOkAnswer() throws Exception {
394394
when(vm.getHostId()).thenReturn(1L);
395395
when(agentManagerMock.send(anyLong(), (Command)any())).thenReturn(answer);
396396

397-
boolean actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
397+
Pair<Boolean, String> actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
398398

399-
Assert.assertTrue(actual);
399+
Assert.assertTrue(actual.first());
400400
}
401401

402402
@Test
@@ -409,9 +409,10 @@ public void testSendStopWithFailAnswer() throws Exception {
409409
when(vm.getHostId()).thenReturn(1L);
410410
when(agentManagerMock.send(anyLong(), (Command)any())).thenReturn(answer);
411411

412-
boolean actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
412+
Pair<Boolean, String> actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
413413

414-
assertFalse(actual);
414+
assertFalse(actual.first());
415+
Assert.assertEquals("fail", actual.second());
415416
}
416417

417418
@Test
@@ -421,11 +422,13 @@ public void testSendStopWithNullAnswer() throws Exception {
421422
VirtualMachineProfile profile = mock(VirtualMachineProfile.class);
422423
when(profile.getVirtualMachine()).thenReturn(vm);
423424
when(vm.getHostId()).thenReturn(1L);
425+
when(vm.getInstanceName()).thenReturn("test-vm");
424426
when(agentManagerMock.send(anyLong(), (Command)any())).thenReturn(null);
425427

426-
boolean actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
428+
Pair<Boolean, String> actual = virtualMachineManagerImpl.sendStop(guru, profile, false, false);
427429

428-
assertFalse(actual);
430+
assertFalse(actual.first());
431+
Assert.assertNotNull(actual.second());
429432
}
430433

431434
@Test

0 commit comments

Comments
 (0)