KVM: fail unsupported disk-only snapshots cleanly

Move the VM snapshot create transition before host capability validation so validation failures can be marked failed instead of leaving allocated snapshots behind.

Add regression coverage for the NVRAM capability validation path and verify that no agent command is sent after the early failure.

Author: kunal.behbudzade

engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java

@@ -522,13 +522,13 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map<VolumeInf
 
         logger.info("Starting disk-only VM snapshot process for VM [{}].", userVm.getUuid());
 
+        transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.CreateRequested);
+
         Long hostId = pickHostForUefiNvramAwareDiskOnlySnapshot(userVm, "create");
         validateHostSupportsUefiNvramAwareDiskOnlySnapshots(hostId, userVm, "create");
         VMSnapshotVO vmSnapshotVO = (VMSnapshotVO) vmSnapshot;
         List<VolumeObjectTO> volumeTOs = vmSnapshotHelper.getVolumeTOList(userVm.getId());
 
-        transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.CreateRequested);
-
         VMSnapshotTO parentSnapshotTo = null;
         VMSnapshotVO parentSnapshotVo = vmSnapshotDao.findCurrentSnapshotByVmId(userVm.getId());
         if (parentSnapshotVo != null) {

engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java

@@ -27,6 +27,7 @@
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
@@ -49,6 +50,7 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
 import org.mockito.Mockito;
 
 import com.cloud.agent.AgentManager;
@@ -385,6 +387,36 @@ public void testTakeVmSnapshotInternalFailsWhenHostLacksNvramAwareSnapshotCapabi
         strategy.takeVmSnapshotInternal(vmSnapshot, Collections.emptyMap());
     }
 
+    @Test(expected = CloudRuntimeException.class)
+    public void testTakeVMSnapshotMarksSnapshotFailedWhenHostCapabilityValidationFails() throws Exception {
+        long vmId = 10L;
+        long hostId = 40L;
+
+        UserVmVO userVm = mock(UserVmVO.class);
+        VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class);
+
+        when(vmSnapshot.getVmId()).thenReturn(vmId);
+        when(userVm.getId()).thenReturn(vmId);
+        when(userVm.getUuid()).thenReturn("vm-uuid");
+        when(userVm.getState()).thenReturn(VirtualMachine.State.Running);
+        when(strategy.userVmDao.findById(vmId)).thenReturn(userVm);
+        when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId);
+        when(strategy.vmInstanceDetailsDao.findDetail(vmId, ApiConstants.BootType.UEFI.toString()))
+                .thenReturn(new VMInstanceDetailVO(vmId, ApiConstants.BootType.UEFI.toString(), "SECURE", true));
+        when(hostDetailsDao.findDetail(hostId, Host.HOST_UEFI_ENABLE))
+                .thenReturn(new DetailVO(hostId, Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString()));
+        when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null);
+
+        try {
+            strategy.takeVMSnapshot(vmSnapshot);
+        } finally {
+            InOrder inOrder = inOrder(vmSnapshotHelper);
+            inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.CreateRequested);
+            inOrder.verify(vmSnapshotHelper).vmSnapshotStateTransitTo(vmSnapshot, VMSnapshot.Event.OperationFailed);
+            verify(agentMgr, never()).easySend(eq(hostId), any());
+        }
+    }
+
     @Test(expected = CloudRuntimeException.class)
     public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityForSidecarSnapshot() {
         long vmId = 10L;