Copilot commented on code in PR #12749:
URL: https://github.com/apache/cloudstack/pull/12749#discussion_r2890248192
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -3561,8 +3577,19 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws
ResourceUnavailableException, C
detachVolumesFromVm(vm, dataVols);
UserVm destroyedVm = destroyVm(vmId, expunge);
- if (expunge && !expunge(vm)) {
- throw new CloudRuntimeException("Failed to expunge vm " +
destroyedVm);
+ if (expunge) {
+ boolean expunged;
+ try {
+ expunged = expunge(vm);
+ } catch (RuntimeException e) {
+ logger.error("Failed to expunge VM [{}] due to: {}", vm,
e.getMessage(), e);
+ transitionExpungingToError(vm.getId());
+ throw new CloudRuntimeException("Failed to expunge VM " +
vm.getUuid() + " due to: " + e.getMessage(), e);
+ }
+ if (!expunged) {
+ transitionExpungingToError(vm.getId());
+ throw new CloudRuntimeException("Failed to expunge VM " +
destroyedVm);
Review Comment:
The error message on line 3591 uses `destroyedVm` (the `UserVm` object's
`toString()` representation, e.g. something like "VirtualMachineVO[id=N]"),
whereas the error message at line 3587 uses `vm.getUuid()` to clearly identify
the VM. Using `destroyedVm` in the error message is inconsistent within the
same block and may produce a less informative or unpredictable output for
consumers of this exception. The message should consistently use `vm.getUuid()`
or similar identifier to clearly identify the VM.
```suggestion
throw new CloudRuntimeException("Failed to expunge VM " +
vm.getUuid());
```
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -2578,6 +2578,22 @@ public boolean expunge(UserVmVO vm) {
}
}
+ private void transitionExpungingToError(long vmId) {
+ try {
+ UserVmVO vm = _vmDao.findById(vmId);
+ if (vm != null && vm.getState() == State.Expunging) {
+ boolean transitioned = _itMgr.stateTransitTo(vm,
VirtualMachine.Event.OperationFailedToError, null);
+ if (transitioned) {
+ logger.info("Transitioned VM [{}] from Expunging to Error
after failed expunge", vm.getUuid());
+ } else {
+ logger.warn("Failed to persist transition of VM [{}] from
Expunging to Error after failed expunge, possibly due to concurrent update",
vm.getUuid());
+ }
+ }
+ } catch (NoTransitionException e) {
+ logger.warn("Failed to transition VM to Error state: {}",
e.getMessage());
Review Comment:
The warning log message at line 2593 doesn't include the VM's UUID or ID to
help identify which VM failed to transition. Since this code is inside a `try`
block that starts at line 2582 and the `vm` variable is in scope when the
`NoTransitionException` is thrown (it can only be thrown from within the `if
(vm != null && ...)` check on line 2584), the log message should include
`vm.getUuid()` to make troubleshooting easier. However, `vm` is declared inside
the `try` block and would be accessible in the `catch` block because the
exception can only occur inside the `if` block where `vm != null`.
##########
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##########
@@ -3561,8 +3577,19 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws
ResourceUnavailableException, C
detachVolumesFromVm(vm, dataVols);
UserVm destroyedVm = destroyVm(vmId, expunge);
- if (expunge && !expunge(vm)) {
- throw new CloudRuntimeException("Failed to expunge vm " +
destroyedVm);
+ if (expunge) {
+ boolean expunged;
+ try {
+ expunged = expunge(vm);
+ } catch (RuntimeException e) {
+ logger.error("Failed to expunge VM [{}] due to: {}", vm,
e.getMessage(), e);
+ transitionExpungingToError(vm.getId());
+ throw new CloudRuntimeException("Failed to expunge VM " +
vm.getUuid() + " due to: " + e.getMessage(), e);
+ }
+ if (!expunged) {
+ transitionExpungingToError(vm.getId());
+ throw new CloudRuntimeException("Failed to expunge VM " +
destroyedVm);
+ }
Review Comment:
The updated `destroyVm(DestroyVMCmd)` method now has two new failure paths:
one where `expunge(vm)` returns `false` and one where it throws a
`RuntimeException`. Both paths call `transitionExpungingToError()` and then
throw a `CloudRuntimeException`. However, there are no tests in this PR
covering these new code paths in `destroyVm`. The existing `testDestroyVm` test
at line 3628 only covers the success case (where `expunge` returns `true`).
Tests verifying that `transitionExpungingToError` is called and a
`CloudRuntimeException` is thrown in both failure cases would improve
reliability.
--
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]
