Skip to content

Mark VMs in error state when expunge fails during destroy operation #12749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sureshanaparti wants to merge 1 commit into
base: 4.22
Choose a base branch
from
+84 −2
Open

Mark VMs in error state when expunge fails during destroy operation #12749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/src/main/java/com/cloud/vm/VirtualMachine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ public static StateMachine2<State, VirtualMachine.Event, VirtualMachine> getStat
s_fsm.addTransition(new Transition<State, Event>(State.Stopping, VirtualMachine.Event.StopRequested, State.Stopping, null));
s_fsm.addTransition(new Transition<State, Event>(State.Stopping, VirtualMachine.Event.AgentReportShutdowned, State.Stopped, Arrays.asList(new Impact[]{Impact.USAGE})));
s_fsm.addTransition(new Transition<State, Event>(State.Expunging, VirtualMachine.Event.OperationFailed, State.Expunging,null));
// Note: In addition to the Stopped -> Error transition for failed VM creation,
// a VM can also transition from Expunging to Error on OperationFailedToError.
s_fsm.addTransition(new Transition<State, Event>(State.Expunging, VirtualMachine.Event.OperationFailedToError, State.Error, null));
s_fsm.addTransition(new Transition<State, Event>(State.Expunging, VirtualMachine.Event.ExpungeOperation, State.Expunging,null));
s_fsm.addTransition(new Transition<State, Event>(State.Error, VirtualMachine.Event.DestroyRequested, State.Expunging, null));
s_fsm.addTransition(new Transition<State, Event>(State.Error, VirtualMachine.Event.ExpungeOperation, State.Expunging, null));
Expand Down
31 changes: 29 additions & 2 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}

/**
* Release network resources, it was done on vm stop previously.
* @param id vm id
Expand Down Expand Up @@ -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);
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
throw new CloudRuntimeException("Failed to expunge VM " + destroyedVm);
throw new CloudRuntimeException("Failed to expunge VM " + vm.getUuid());

Copilot uses AI. Check for mistakes.
}
Comment on lines +3580 to +3592
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

autoScaleManager.removeVmFromVmGroup(vmId);
Expand Down
52 changes: 52 additions & 0 deletions server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import java.util.UUID;

import com.cloud.storage.dao.SnapshotPolicyDao;
import com.cloud.utils.fsm.NoTransitionException;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker;
import org.apache.cloudstack.api.ApiCommandResourceType;
Expand Down Expand Up @@ -4177,4 +4178,55 @@ public void testUnmanageUserVMSuccess() {
verify(userVmDao, times(1)).releaseFromLockTable(vmId);
}

@Test
public void testTransitionExpungingToErrorVmInExpungingState() throws Exception {
UserVmVO vm = mock(UserVmVO.class);
when(vm.getState()).thenReturn(VirtualMachine.State.Expunging);
when(vm.getUuid()).thenReturn("test-uuid");
when(userVmDao.findById(vmId)).thenReturn(vm);
when(virtualMachineManager.stateTransitTo(eq(vm), eq(VirtualMachine.Event.OperationFailedToError), eq(null))).thenReturn(true);

java.lang.reflect.Method method = UserVmManagerImpl.class.getDeclaredMethod("transitionExpungingToError", long.class);
method.setAccessible(true);
method.invoke(userVmManagerImpl, vmId);

Mockito.verify(virtualMachineManager).stateTransitTo(vm, VirtualMachine.Event.OperationFailedToError, null);
}

@Test
public void testTransitionExpungingToErrorVmNotInExpungingState() throws Exception {
UserVmVO vm = mock(UserVmVO.class);
when(vm.getState()).thenReturn(VirtualMachine.State.Stopped);
when(userVmDao.findById(vmId)).thenReturn(vm);

java.lang.reflect.Method method = UserVmManagerImpl.class.getDeclaredMethod("transitionExpungingToError", long.class);
method.setAccessible(true);
method.invoke(userVmManagerImpl, vmId);

Mockito.verify(virtualMachineManager, Mockito.never()).stateTransitTo(any(VirtualMachine.class), any(VirtualMachine.Event.class), any());
}

@Test
public void testTransitionExpungingToErrorVmNotFound() throws Exception {
when(userVmDao.findById(vmId)).thenReturn(null);

java.lang.reflect.Method method = UserVmManagerImpl.class.getDeclaredMethod("transitionExpungingToError", long.class);
method.setAccessible(true);
method.invoke(userVmManagerImpl, vmId);

Mockito.verify(virtualMachineManager, Mockito.never()).stateTransitTo(any(VirtualMachine.class), any(VirtualMachine.Event.class), any());
}

@Test
public void testTransitionExpungingToErrorHandlesNoTransitionException() throws Exception {
UserVmVO vm = mock(UserVmVO.class);
when(vm.getState()).thenReturn(VirtualMachine.State.Expunging);
when(userVmDao.findById(vmId)).thenReturn(vm);
when(virtualMachineManager.stateTransitTo(eq(vm), eq(VirtualMachine.Event.OperationFailedToError), eq(null)))
.thenThrow(new NoTransitionException("no transition"));

java.lang.reflect.Method method = UserVmManagerImpl.class.getDeclaredMethod("transitionExpungingToError", long.class);
method.setAccessible(true);
method.invoke(userVmManagerImpl, vmId);
}
}
Loading