From 29329d8ddd3e19a375b3a4af80a40fc2ce1faeb9 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Tue, 14 Apr 2026 14:25:04 +0300 Subject: [PATCH 1/7] KVM: add UEFI NVRAM plumbing for disk-only instance snapshots Add the command payload, answer metadata, and host capability plumbing required for KVM disk-only instance snapshots to carry UEFI NVRAM state between management and the KVM agent. Also synchronize host capability booleans on reconnect so stale UEFI/NVRAM support details are removed when an older agent reconnects. --- api/src/main/java/com/cloud/host/Host.java | 1 + .../CreateDiskOnlyVmSnapshotAnswer.java | 11 ++++++ .../CreateDiskOnlyVmSnapshotCommand.java | 17 +++++++++ .../DeleteDiskOnlyVmSnapshotCommand.java | 25 ++++++++++-- .../RevertDiskOnlyVmSnapshotCommand.java | 22 +++++++++++ .../cloud/agent/manager/AgentManagerImpl.java | 38 +++++++++++++++++-- .../resource/LibvirtComputingResource.java | 11 ++++++ .../wrapper/LibvirtReadyCommandWrapper.java | 1 + 8 files changed, 119 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/com/cloud/host/Host.java b/api/src/main/java/com/cloud/host/Host.java index b52348201516..a7b89b8a2b85 100644 --- a/api/src/main/java/com/cloud/host/Host.java +++ b/api/src/main/java/com/cloud/host/Host.java @@ -55,6 +55,7 @@ public static String[] toStrings(Host.Type... types) { } String HOST_UEFI_ENABLE = "host.uefi.enable"; + String HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM = "host.kvm.diskonlyvmsnapshot.nvram"; String HOST_VOLUME_ENCRYPTION = "host.volume.encryption"; String HOST_INSTANCE_CONVERSION = "host.instance.conversion"; String HOST_VDDK_SUPPORT = "host.vddk.support"; diff --git a/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotAnswer.java b/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotAnswer.java index 4d61249c7cbc..ffa4eaff2963 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotAnswer.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotAnswer.java @@ -27,13 +27,24 @@ public class CreateDiskOnlyVmSnapshotAnswer extends Answer { protected Map> mapVolumeToSnapshotSizeAndNewVolumePath; + private String nvramSnapshotPath; public CreateDiskOnlyVmSnapshotAnswer(Command command, boolean success, String details, Map> mapVolumeToSnapshotSizeAndNewVolumePath) { + this(command, success, details, mapVolumeToSnapshotSizeAndNewVolumePath, null); + } + + public CreateDiskOnlyVmSnapshotAnswer(Command command, boolean success, String details, Map> mapVolumeToSnapshotSizeAndNewVolumePath, + String nvramSnapshotPath) { super(command, success, details); this.mapVolumeToSnapshotSizeAndNewVolumePath = mapVolumeToSnapshotSizeAndNewVolumePath; + this.nvramSnapshotPath = nvramSnapshotPath; } public Map> getMapVolumeToSnapshotSizeAndNewVolumePath() { return mapVolumeToSnapshotSizeAndNewVolumePath; } + + public String getNvramSnapshotPath() { + return nvramSnapshotPath; + } } diff --git a/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotCommand.java b/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotCommand.java index 952bf0c971de..7f328bab81db 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotCommand.java +++ b/core/src/main/java/com/cloud/agent/api/storage/CreateDiskOnlyVmSnapshotCommand.java @@ -29,13 +29,30 @@ public class CreateDiskOnlyVmSnapshotCommand extends VMSnapshotBaseCommand { protected VirtualMachine.State vmState; + private final String vmUuid; + private final boolean uefiEnabled; public CreateDiskOnlyVmSnapshotCommand(String vmName, VMSnapshotTO snapshot, List volumeTOs, String guestOSType, VirtualMachine.State vmState) { + this(vmName, null, snapshot, volumeTOs, guestOSType, vmState, false); + } + + public CreateDiskOnlyVmSnapshotCommand(String vmName, String vmUuid, VMSnapshotTO snapshot, List volumeTOs, String guestOSType, + VirtualMachine.State vmState, boolean uefiEnabled) { super(vmName, snapshot, volumeTOs, guestOSType); + this.vmUuid = vmUuid; this.vmState = vmState; + this.uefiEnabled = uefiEnabled; } public VirtualMachine.State getVmState() { return vmState; } + + public String getVmUuid() { + return vmUuid; + } + + public boolean isUefiEnabled() { + return uefiEnabled; + } } diff --git a/core/src/main/java/com/cloud/agent/api/storage/DeleteDiskOnlyVmSnapshotCommand.java b/core/src/main/java/com/cloud/agent/api/storage/DeleteDiskOnlyVmSnapshotCommand.java index bf7bdd597360..1ac0a0ff719b 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/DeleteDiskOnlyVmSnapshotCommand.java +++ b/core/src/main/java/com/cloud/agent/api/storage/DeleteDiskOnlyVmSnapshotCommand.java @@ -19,24 +19,43 @@ package com.cloud.agent.api.storage; import com.cloud.agent.api.Command; - import com.cloud.agent.api.to.DataTO; - +import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import java.util.List; public class DeleteDiskOnlyVmSnapshotCommand extends Command { - List snapshots; + private final List snapshots; + private final String nvramSnapshotPath; + private final PrimaryDataStoreTO primaryDataStore; public DeleteDiskOnlyVmSnapshotCommand(List snapshots) { + this(snapshots, null); + } + + public DeleteDiskOnlyVmSnapshotCommand(List snapshots, String nvramSnapshotPath) { + this(snapshots, nvramSnapshotPath, null); + } + + public DeleteDiskOnlyVmSnapshotCommand(List snapshots, String nvramSnapshotPath, PrimaryDataStoreTO primaryDataStore) { this.snapshots = snapshots; + this.nvramSnapshotPath = nvramSnapshotPath; + this.primaryDataStore = primaryDataStore; } public List getSnapshots() { return snapshots; } + public String getNvramSnapshotPath() { + return nvramSnapshotPath; + } + + public PrimaryDataStoreTO getPrimaryDataStore() { + return primaryDataStore; + } + @Override public boolean executeInSequence() { return false; diff --git a/core/src/main/java/com/cloud/agent/api/storage/RevertDiskOnlyVmSnapshotCommand.java b/core/src/main/java/com/cloud/agent/api/storage/RevertDiskOnlyVmSnapshotCommand.java index 72bb92bcb10d..3c9859aa44af 100644 --- a/core/src/main/java/com/cloud/agent/api/storage/RevertDiskOnlyVmSnapshotCommand.java +++ b/core/src/main/java/com/cloud/agent/api/storage/RevertDiskOnlyVmSnapshotCommand.java @@ -27,11 +27,21 @@ public class RevertDiskOnlyVmSnapshotCommand extends Command { private List snapshotObjectTos; private String vmName; + private final String vmUuid; + private final boolean uefiEnabled; + private final String nvramSnapshotPath; public RevertDiskOnlyVmSnapshotCommand(List snapshotObjectTos, String vmName) { + this(snapshotObjectTos, vmName, null, false, null); + } + + public RevertDiskOnlyVmSnapshotCommand(List snapshotObjectTos, String vmName, String vmUuid, boolean uefiEnabled, String nvramSnapshotPath) { super(); this.snapshotObjectTos = snapshotObjectTos; this.vmName = vmName; + this.vmUuid = vmUuid; + this.uefiEnabled = uefiEnabled; + this.nvramSnapshotPath = nvramSnapshotPath; } public List getSnapshotObjectTos() { @@ -42,6 +52,18 @@ public String getVmName() { return vmName; } + public String getVmUuid() { + return vmUuid; + } + + public boolean isUefiEnabled() { + return uefiEnabled; + } + + public String getNvramSnapshotPath() { + return nvramSnapshotPath; + } + @Override public boolean executeInSequence() { return false; diff --git a/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java index 8c69dcdc4828..ecb789a15bd5 100644 --- a/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/agent/manager/AgentManagerImpl.java @@ -108,10 +108,12 @@ import com.cloud.exception.UnsupportedVersionException; import com.cloud.ha.HighAvailabilityManager; import com.cloud.host.Host; +import com.cloud.host.DetailVO; import com.cloud.host.HostVO; import com.cloud.host.Status; import com.cloud.host.Status.Event; import com.cloud.host.dao.HostDao; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.hypervisor.HypervisorGuruManager; import com.cloud.org.Cluster; @@ -167,6 +169,8 @@ public class AgentManagerImpl extends ManagerBase implements AgentManager, Handl @Inject protected HostDao _hostDao = null; @Inject + protected HostDetailsDao _hostDetailsDao = null; + @Inject private ManagementServerHostDao _mshostDao; @Inject protected OutOfBandManagementDao outOfBandManagementDao; @@ -802,18 +806,24 @@ protected AgentAttache notifyMonitorsOfConnection(final AgentAttache attache, fi ReadyAnswer readyAnswer = (ReadyAnswer)answer; Map detailsMap = readyAnswer.getDetailsMap(); if (detailsMap != null) { + _hostDao.loadDetails(host); + if (host.getDetails() == null) { + host.setDetails(new HashMap<>()); + } String uefiEnabled = detailsMap.get(Host.HOST_UEFI_ENABLE); + String diskOnlyVmSnapshotNvramSupport = detailsMap.get(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM); String virtv2vVersion = detailsMap.get(Host.HOST_VIRTV2V_VERSION); String ovftoolVersion = detailsMap.get(Host.HOST_OVFTOOL_VERSION); String vddkSupport = detailsMap.get(Host.HOST_VDDK_SUPPORT); String vddkLibDir = detailsMap.get(Host.HOST_VDDK_LIB_DIR); String vddkVersion = detailsMap.get(Host.HOST_VDDK_VERSION); logger.debug("Got HOST_UEFI_ENABLE [{}] for host [{}]:", uefiEnabled, host); - if (ObjectUtils.anyNotNull(uefiEnabled, virtv2vVersion, ovftoolVersion, vddkSupport, vddkLibDir, vddkVersion)) { - _hostDao.loadDetails(host); + if (ObjectUtils.anyNotNull(uefiEnabled, diskOnlyVmSnapshotNvramSupport, virtv2vVersion, ovftoolVersion, vddkSupport, vddkLibDir, vddkVersion)) { boolean updateNeeded = false; - if (StringUtils.isNotBlank(uefiEnabled) && !uefiEnabled.equals(host.getDetails().get(Host.HOST_UEFI_ENABLE))) { - host.getDetails().put(Host.HOST_UEFI_ENABLE, uefiEnabled); + if (syncBooleanHostCapability(host, Host.HOST_UEFI_ENABLE, uefiEnabled)) { + updateNeeded = true; + } + if (syncBooleanHostCapability(host, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, diskOnlyVmSnapshotNvramSupport)) { updateNeeded = true; } if (StringUtils.isNotBlank(virtv2vVersion) && !virtv2vVersion.equals(host.getDetails().get(Host.HOST_VIRTV2V_VERSION))) { @@ -856,6 +866,26 @@ protected AgentAttache notifyMonitorsOfConnection(final AgentAttache attache, fi return attache; } + protected boolean syncBooleanHostCapability(HostVO host, String capabilityName, String advertisedValue) { + if (StringUtils.isNotBlank(advertisedValue)) { + if (!advertisedValue.equals(host.getDetails().get(capabilityName))) { + host.getDetails().put(capabilityName, advertisedValue); + return true; + } + return false; + } + + if (host.getDetails().containsKey(capabilityName)) { + host.getDetails().remove(capabilityName); + DetailVO hostDetail = _hostDetailsDao.findDetail(host.getId(), capabilityName); + if (hostDetail != null) { + _hostDetailsDao.remove(hostDetail.getId()); + } + return true; + } + return false; + } + @Override public boolean start() { ManagementServerHostVO msHost = _mshostDao.findByMsid(_nodeId); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index 25162ca9b929..bfc216327fd1 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -17,6 +17,7 @@ package com.cloud.hypervisor.kvm.resource; import static com.cloud.host.Host.HOST_INSTANCE_CONVERSION; +import static com.cloud.host.Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM; import static com.cloud.host.Host.HOST_OVFTOOL_VERSION; import static com.cloud.host.Host.HOST_VDDK_LIB_DIR; import static com.cloud.host.Host.HOST_VDDK_SUPPORT; @@ -4265,6 +4266,7 @@ public StartupCommand[] initialize() { privateIp = cmd.getPrivateIpAddress(); cmd.getHostDetails().putAll(getVersionStrings()); cmd.getHostDetails().put(KeyStoreUtils.SECURED, String.valueOf(isHostSecured()).toLowerCase()); + cmd.getHostDetails().put(HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString()); cmd.setPool(pool); cmd.setCluster(clusterId); cmd.setGatewayIpAddress(localGateway); @@ -6334,6 +6336,15 @@ public String getSnapshotTemporaryPath(String diskPath, String snapshotName) { return String.join(File.separator, diskPathSplitted); } + public String getUefiNvramPath(String vmUuid) { + String nvramDirectory = uefiProperties.getProperty(LibvirtVMDef.GuestDef.GUEST_NVRAM_PATH); + if (StringUtils.isBlank(nvramDirectory) || StringUtils.isBlank(vmUuid)) { + return null; + } + + return nvramDirectory + vmUuid + ".fd"; + } + public static String generateSecretUUIDFromString(String seed) { return UuidUtils.nameUUIDFromBytes(seed.getBytes()).toString(); } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java index 5a7d6d2c203a..1ff6d7851f20 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java @@ -43,6 +43,7 @@ public final class LibvirtReadyCommandWrapper extends CommandWrapper hostDetails = new HashMap(); + hostDetails.put(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString()); if (hostSupportsUefi(libvirtComputingResource.isUbuntuOrDebianHost()) && libvirtComputingResource.isUefiPropertiesFileLoaded()) { hostDetails.put(Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString()); From 2fc4621bee174a6400bf2d83dd4cbf88f90f1ed2 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Tue, 14 Apr 2026 14:25:10 +0300 Subject: [PATCH 2/7] KVM: capture UEFI NVRAM in disk-only instance snapshots Copy the active UEFI NVRAM file as a sidecar during disk-only instance snapshot creation, restore it on revert, and clean it up during delete and merge flows. Also tighten host capability checks, preserve successful snapshot metadata when post-snapshot thaw or resume fails, and reject reverting UEFI disk-only snapshots that do not contain NVRAM state. --- PendingReleaseNotes | 9 + ...KvmFileBasedStorageVmSnapshotStrategy.java | 166 ++++++++++++- ...reateDiskOnlyVMSnapshotCommandWrapper.java | 224 +++++++++++++++++- ...eleteDiskOnlyVMSnapshotCommandWrapper.java | 38 +++ ...evertDiskOnlyVMSnapshotCommandWrapper.java | 94 +++++++- 5 files changed, 515 insertions(+), 16 deletions(-) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 9670b6e7c13a..fb382e4e8478 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -39,3 +39,12 @@ example.ver.1 > example.ver.2: which can now be attached to Instances. This is to prevent the Secondary Storage to grow to enormous sizes as Linux Distributions keep growing in size while a stripped down Linux should fit on a 2.88MB floppy. + +4.22.0.0 > 4.22.0.1: + * Disk-only instance snapshots for KVM UEFI VMs now include a sidecar copy of + the active NVRAM state so revert operations restore both disk and firmware + boot state consistently. + + * UEFI disk-only instance snapshots taken before this change do not contain an + NVRAM sidecar and cannot be safely reverted. Take a new snapshot after + upgrading before relying on revert for UEFI VMs. diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java index 003065e394f5..ad2be6f4fb84 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java @@ -28,9 +28,13 @@ import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotCommand; import com.cloud.agent.api.storage.SnapshotMergeTreeTO; import com.cloud.agent.api.to.DataTO; +import com.cloud.alert.AlertManager; import com.cloud.configuration.Resource; import com.cloud.event.EventTypes; import com.cloud.event.UsageEventUtils; +import com.cloud.host.DetailVO; +import com.cloud.host.Host; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.DataStoreRole; import com.cloud.storage.Snapshot; @@ -46,9 +50,11 @@ import com.cloud.utils.fsm.NoTransitionException; import com.cloud.vm.UserVmVO; import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.VMInstanceDetailsDao; import com.cloud.vm.snapshot.VMSnapshot; import com.cloud.vm.snapshot.VMSnapshotDetailsVO; import com.cloud.vm.snapshot.VMSnapshotVO; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.backup.BackupOfferingVO; import org.apache.cloudstack.backup.dao.BackupOfferingDao; import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine; @@ -59,10 +65,12 @@ import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; +import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.snapshot.SnapshotObject; import org.apache.cloudstack.storage.to.SnapshotObjectTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang3.StringUtils; import javax.inject.Inject; import java.util.ArrayList; @@ -76,6 +84,7 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStrategy { private static final List supportedStoragePoolTypes = List.of(Storage.StoragePoolType.Filesystem, Storage.StoragePoolType.NetworkFilesystem, Storage.StoragePoolType.SharedMountPoint); + private static final String KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM = "kvmFileBasedStorageSnapshotNvram"; @Inject protected SnapshotDataStoreDao snapshotDataStoreDao; @@ -86,6 +95,15 @@ public class KvmFileBasedStorageVmSnapshotStrategy extends StorageVMSnapshotStra @Inject protected BackupOfferingDao backupOfferingDao; + @Inject + protected VMInstanceDetailsDao vmInstanceDetailsDao; + + @Inject + protected HostDetailsDao hostDetailsDao; + + @Inject + protected AlertManager alertManager; + @Override public VMSnapshot takeVMSnapshot(VMSnapshot vmSnapshot) { Map volumeInfoToSnapshotObjectMap = new HashMap<>(); @@ -117,6 +135,7 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot; Long hostId = vmSnapshotHelper.pickRunningHost(vmSnapshotBeingDeleted.getVmId()); + validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete"); long virtualSize = 0; boolean isCurrent = vmSnapshotBeingDeleted.getCurrent(); @@ -124,6 +143,7 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { List volumeTOs = vmSnapshotHelper.getVolumeTOList(vmSnapshotBeingDeleted.getVmId()); List snapshotChildren = vmSnapshotDao.listByParentAndStateIn(vmSnapshotBeingDeleted.getId(), VMSnapshot.State.Ready, VMSnapshot.State.Hidden); + PrimaryDataStoreTO nvramPrimaryDataStore = getPrimaryDataStoreForNvramCleanup(vmSnapshotBeingDeleted, volumeTOs); long realSize = getVMSnapshotRealSize(vmSnapshotBeingDeleted); int numberOfChildren = snapshotChildren.size(); @@ -157,6 +177,8 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { return true; } + deleteNvramSnapshotIfNeeded(vmSnapshotBeingDeleted, hostId, nvramPrimaryDataStore); + transitStateWithoutThrow(vmSnapshotBeingDeleted, VMSnapshot.Event.OperationSucceeded); vmSnapshotDetailsDao.removeDetails(vmSnapshotBeingDeleted.getId()); @@ -176,6 +198,7 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) { VMSnapshotVO vmSnapshotBeingReverted = (VMSnapshotVO) vmSnapshot; Long hostId = vmSnapshotHelper.pickRunningHost(vmSnapshotBeingReverted.getVmId()); + validateHostSupportsUefiNvramAwareDiskOnlySnapshots(hostId, userVm, "revert"); transitStateWithoutThrow(vmSnapshotBeingReverted, VMSnapshot.Event.RevertRequested); @@ -184,7 +207,9 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) { .map(snapshot -> (SnapshotObjectTO) snapshotDataFactory.getSnapshot(snapshot.getSnapshotId(), snapshot.getDataStoreId(), DataStoreRole.Primary).getTO()) .collect(Collectors.toList()); - RevertDiskOnlyVmSnapshotCommand revertDiskOnlyVMSnapshotCommand = new RevertDiskOnlyVmSnapshotCommand(volumeSnapshotTos, userVm.getName()); + RevertDiskOnlyVmSnapshotCommand revertDiskOnlyVMSnapshotCommand = + new RevertDiskOnlyVmSnapshotCommand(volumeSnapshotTos, userVm.getName(), userVm.getUuid(), isUefiVm(userVm), + getNvramSnapshotPath(vmSnapshotBeingReverted)); Answer answer = agentMgr.easySend(hostId, revertDiskOnlyVMSnapshotCommand); if (answer == null || !answer.getResult()) { @@ -204,6 +229,11 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) { publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_REVERT, vmSnapshotBeingReverted, userVm, volumeObjectTo); } + if (isUefiVm(userVm)) { + userVm.setLastHostId(hostId); + userVmDao.update(userVm.getId(), userVm); + } + transitStateWithoutThrow(vmSnapshotBeingReverted, VMSnapshot.Event.OperationSucceeded); VMSnapshotVO currentVmSnapshot = vmSnapshotDao.findCurrentSnapshotByVmId(userVm.getId()); @@ -248,6 +278,8 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen return; } + validateHostSupportsNvramSidecarCleanup(oldParent, hostId, "clean up"); + PrimaryDataStoreTO nvramPrimaryDataStore = getPrimaryDataStoreForNvramCleanup(oldParent, volumeTOs); List snapshotVos; if (oldParent.getCurrent()) { @@ -276,6 +308,8 @@ private void mergeOldSiblingWithOldParentIfOldParentIsDead(VMSnapshotVO oldParen snapshotDao.update(snapshotVO.getId(), snapshotVO); } + deleteNvramSnapshotIfNeeded(oldParent, hostId, nvramPrimaryDataStore); + vmSnapshotDetailsDao.removeDetails(oldParent.getId()); oldParent.setRemoved(DateUtil.now()); @@ -347,12 +381,13 @@ public StrategyPriority canHandle(Long vmId, Long rootPoolId, boolean snapshotMe } private List deleteSnapshot(VMSnapshotVO vmSnapshotVO, Long hostId) { + validateHostSupportsNvramSidecarCleanup(vmSnapshotVO, hostId, "delete"); List volumeSnapshots = getVolumeSnapshotsAssociatedWithVmSnapshot(vmSnapshotVO); List volumeSnapshotTOList = volumeSnapshots.stream() .map(snapshotDataStoreVO -> snapshotDataFactory.getSnapshot(snapshotDataStoreVO.getSnapshotId(), snapshotDataStoreVO.getDataStoreId(), DataStoreRole.Primary).getTO()) .collect(Collectors.toList()); - DeleteDiskOnlyVmSnapshotCommand deleteSnapshotCommand = new DeleteDiskOnlyVmSnapshotCommand(volumeSnapshotTOList); + DeleteDiskOnlyVmSnapshotCommand deleteSnapshotCommand = new DeleteDiskOnlyVmSnapshotCommand(volumeSnapshotTOList, getNvramSnapshotPath(vmSnapshotVO)); Answer answer = agentMgr.easySend(hostId, deleteSnapshotCommand); if (answer == null || !answer.getResult()) { logger.error("Failed to delete VM snapshot [{}] due to {}.", vmSnapshotVO.getUuid(), answer != null ? answer.getDetails() : "Communication failure"); @@ -368,6 +403,21 @@ private List deleteSnapshot(VMSnapshotVO vmSnapshotVO, Long hostId) return snapshotVOList; } + protected void deleteNvramSnapshotIfNeeded(VMSnapshotVO vmSnapshotVO, Long hostId, PrimaryDataStoreTO primaryDataStore) { + String nvramSnapshotPath = getNvramSnapshotPath(vmSnapshotVO); + if (StringUtils.isBlank(nvramSnapshotPath) || primaryDataStore == null) { + return; + } + + validateHostSupportsNvramSidecarCleanup(vmSnapshotVO, hostId, "delete"); + DeleteDiskOnlyVmSnapshotCommand deleteSnapshotCommand = new DeleteDiskOnlyVmSnapshotCommand(List.of(), nvramSnapshotPath, primaryDataStore); + Answer answer = agentMgr.easySend(hostId, deleteSnapshotCommand); + if (answer == null || !answer.getResult()) { + logger.warn("Failed to delete the NVRAM sidecar of VM snapshot [{}] due to {}.", vmSnapshotVO.getUuid(), + answer != null ? answer.getDetails() : "communication failure"); + } + } + private List mergeSnapshots(VMSnapshotVO vmSnapshotVO, VMSnapshotVO childSnapshot, UserVmVO userVm, List volumeObjectTOS, Long hostId) { logger.debug("Merging VM snapshot [{}] with its child [{}].", vmSnapshotVO.getUuid(), childSnapshot.getUuid()); @@ -471,6 +521,7 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map volumeTOs = vmSnapshotHelper.getVolumeTOList(userVm.getId()); @@ -493,14 +544,18 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map volumeTOs) { + return (PrimaryDataStoreTO) volumeTOs.stream() + .filter(volumeObjectTO -> Volume.Type.ROOT.equals(volumeObjectTO.getVolumeType())) + .findFirst() + .orElseThrow(() -> new CloudRuntimeException("Failed to locate the root volume while handling the VM snapshot.")) + .getDataStore(); + } + + protected PrimaryDataStoreTO getRootVolumePrimaryDataStoreForCleanup(VMSnapshotVO vmSnapshot, List volumeTOs) { + try { + return getRootVolumePrimaryDataStore(volumeTOs); + } catch (CloudRuntimeException e) { + logger.warn("Failed to locate the root volume while cleaning up the NVRAM sidecar for VM snapshot [{}].", vmSnapshot.getUuid(), e); + return null; + } + } + + protected PrimaryDataStoreTO getPrimaryDataStoreForNvramCleanup(VMSnapshotVO vmSnapshot, List volumeTOs) { + PrimaryDataStoreTO rootSnapshotPrimaryDataStore = getRootSnapshotPrimaryDataStoreForCleanup(vmSnapshot); + return rootSnapshotPrimaryDataStore != null ? rootSnapshotPrimaryDataStore : getRootVolumePrimaryDataStoreForCleanup(vmSnapshot, volumeTOs); + } + + protected PrimaryDataStoreTO getRootSnapshotPrimaryDataStoreForCleanup(VMSnapshotVO vmSnapshot) { + try { + return (PrimaryDataStoreTO) getVolumeSnapshotsAssociatedWithVmSnapshot(vmSnapshot).stream() + .map(snapshotDataStoreVO -> (SnapshotObjectTO) snapshotDataFactory.getSnapshot(snapshotDataStoreVO.getSnapshotId(), + snapshotDataStoreVO.getDataStoreId(), DataStoreRole.Primary).getTO()) + .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType())) + .findFirst() + .orElseThrow(() -> new CloudRuntimeException("Failed to locate the root volume snapshot while handling the VM snapshot.")) + .getDataStore(); + } catch (CloudRuntimeException e) { + logger.warn("Failed to locate the root volume snapshot while cleaning up the NVRAM sidecar for VM snapshot [{}].", vmSnapshot.getUuid(), e); + return null; + } + } + + protected String getNvramSnapshotPath(VMSnapshotVO vmSnapshot) { + VMSnapshotDetailsVO nvramDetail = vmSnapshotDetailsDao.findDetail(vmSnapshot.getId(), KVM_FILE_BASED_STORAGE_SNAPSHOT_NVRAM); + return nvramDetail != null ? nvramDetail.getValue() : null; + } + + protected void validateHostSupportsUefiNvramAwareDiskOnlySnapshots(Long hostId, UserVm userVm, String operation) { + if (!isUefiVm(userVm)) { + return; + } + + if (!isHostCapabilityEnabled(hostId, Host.HOST_UEFI_ENABLE)) { + throw new CloudRuntimeException(String.format("Cannot %s a disk-only snapshot for UEFI VM [%s] on host [%s] because the host does not advertise " + + "UEFI support. Ensure the host is configured with UEFI support and retry.", operation, userVm.getUuid(), hostId)); + } + + if (!isHostCapabilityEnabled(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) { + throw new CloudRuntimeException(String.format("Cannot %s a disk-only snapshot for UEFI VM [%s] on host [%s] because the KVM agent does not advertise " + + "NVRAM-aware disk-only snapshot support. Upgrade the host and retry.", operation, userVm.getUuid(), hostId)); + } + } + + protected boolean isHostCapabilityEnabled(Long hostId, String capabilityName) { + DetailVO hostCapability = hostDetailsDao.findDetail(hostId, capabilityName); + return hostCapability != null && Boolean.parseBoolean(hostCapability.getValue()); + } + + protected void validateHostSupportsNvramSidecarCleanup(VMSnapshotVO vmSnapshotVO, Long hostId, String operation) { + if (StringUtils.isBlank(getNvramSnapshotPath(vmSnapshotVO))) { + return; + } + + if (!isHostCapabilityEnabled(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) { + throw new CloudRuntimeException(String.format("Cannot %s VM snapshot [%s] on host [%s] because the KVM agent does not advertise " + + "NVRAM-aware disk-only snapshot support and the snapshot has an NVRAM sidecar that must be cleaned up. Upgrade the host and retry.", + operation, vmSnapshotVO.getUuid(), hostId)); + } + } + + protected void notifyGuestRecoveryIssueIfNeeded(CreateDiskOnlyVmSnapshotAnswer answer, UserVm userVm, VMSnapshotVO vmSnapshot) { + if (StringUtils.isBlank(answer.getDetails())) { + return; + } + + String subject = String.format("Disk-only VM snapshot [%s] completed with guest recovery warnings", vmSnapshot.getUuid()); + String message = String.format("Disk-only VM snapshot [%s] for UEFI VM [%s] completed, but post-snapshot guest recovery reported: %s", + vmSnapshot.getUuid(), userVm.getUuid(), answer.getDetails()); + logger.error(message); + try { + alertManager.sendAlert(AlertManager.AlertType.ALERT_TYPE_VM_SNAPSHOT, userVm.getDataCenterId(), userVm.getPodIdToDeployIn(), subject, message); + } catch (Exception e) { + logger.warn("Failed to send post-snapshot guest recovery alert for VM snapshot [{}].", vmSnapshot.getUuid(), e); + } + } + /** * Given a list of VM snapshots, will remove any that are part of the current direct backing chain (all the direct ancestors of the current vm snapshot). * This is done because, when using virDomainBlockCommit}, Libvirt will maintain diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java index 84d17a1a1161..4a549b747f85 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java @@ -19,6 +19,7 @@ package com.cloud.hypervisor.kvm.resource.wrapper; import com.cloud.agent.api.Answer; +import com.cloud.agent.api.FreezeThawVMCommand; import com.cloud.agent.api.VMSnapshotTO; import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotAnswer; import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotCommand; @@ -32,24 +33,31 @@ import com.cloud.vm.VirtualMachine; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.apache.cloudstack.utils.qemu.QemuCommand; import org.apache.cloudstack.utils.qemu.QemuImg; import org.apache.cloudstack.utils.qemu.QemuImgException; import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.commons.lang3.StringUtils; import org.libvirt.Connect; import org.libvirt.Domain; import org.libvirt.LibvirtException; +import com.google.gson.JsonParser; import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.UUID; +import com.cloud.storage.Volume; + @ResourceWrapper(handles = CreateDiskOnlyVmSnapshotCommand.class) public class LibvirtCreateDiskOnlyVMSnapshotCommandWrapper extends CommandWrapper { + protected static final String NVRAM_SNAPSHOT_DIR = ".cloudstack-vm-snapshot-nvram"; private static final String SNAPSHOT_XML = "\n" + "%s\n" + @@ -79,6 +87,11 @@ protected Answer takeDiskOnlyVmSnapshotOfRunningVm(CreateDiskOnlyVmSnapshotComma logger.info("Taking disk-only VM snapshot of running VM [{}].", vmName); Domain dm = null; + String nvramSnapshotPath = null; + boolean suspendedByThisWrapper = false; + boolean filesystemsFrozenByThisWrapper = false; + CreateDiskOnlyVmSnapshotAnswer answer = null; + String postSnapshotCleanupIssue = null; try { LibvirtUtilitiesHelper libvirtUtilitiesHelper = resource.getLibvirtUtilitiesHelper(); Connect conn = libvirtUtilitiesHelper.getConnection(); @@ -88,26 +101,44 @@ protected Answer takeDiskOnlyVmSnapshotOfRunningVm(CreateDiskOnlyVmSnapshotComma dm = resource.getDomain(conn, vmName); if (dm == null) { - return new CreateDiskOnlyVmSnapshotAnswer(cmd, false, String.format("Creation of disk-only VM Snapshot failed as we could not find the VM [%s].", vmName), null); + answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false, + String.format("Creation of disk-only VM Snapshot failed as we could not find the VM [%s].", vmName), null, null); + return answer; } VMSnapshotTO target = cmd.getTarget(); Pair>> snapshotXmlAndVolumeToNewPathMap = createSnapshotXmlAndNewVolumePathMap(volumeObjectTOS, disks, target, resource); + if (shouldFreezeVmFilesystemsForSnapshot(cmd)) { + freezeVmFilesystems(dm, vmName); + filesystemsFrozenByThisWrapper = true; + verifyVmFilesystemsFrozen(dm, vmName); + } + if (shouldSuspendVmForSnapshot(cmd)) { + suspendedByThisWrapper = suspendVmIfNeeded(dm); + } + nvramSnapshotPath = backupNvramIfNeeded(cmd, resource); - dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(), getFlagsToUseForRunningVmSnapshotCreation(target)); + dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(), getFlagsToUseForRunningVmSnapshotCreation(target, filesystemsFrozenByThisWrapper)); - return new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, snapshotXmlAndVolumeToNewPathMap.second()); - } catch (LibvirtException e) { + postSnapshotCleanupIssue = recoverVmAfterSnapshot(dm, vmName, suspendedByThisWrapper, filesystemsFrozenByThisWrapper, postSnapshotCleanupIssue); + filesystemsFrozenByThisWrapper = false; + suspendedByThisWrapper = false; + + answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, snapshotXmlAndVolumeToNewPathMap.second(), nvramSnapshotPath); + } catch (LibvirtException | IOException e) { String errorMsg = String.format("Creation of disk-only VM snapshot for VM [%s] failed due to %s.", vmName, e.getMessage()); logger.error(errorMsg, e); - if (e.getMessage().contains("QEMU guest agent is not connected")) { + cleanupNvramSnapshotIfNeeded(cmd, resource, nvramSnapshotPath); + if (StringUtils.contains(e.getMessage(), "QEMU guest agent is not connected")) { errorMsg = "QEMU guest agent is not connected. If the VM has been recently started, it might connect soon. Otherwise the VM does not have the" + " guest agent installed; thus the QuiesceVM parameter is not supported."; - return new CreateDiskOnlyVmSnapshotAnswer(cmd, false, errorMsg, null); + answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false, errorMsg, null, null); + } else { + answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false, e.getMessage(), null, null); } - return new CreateDiskOnlyVmSnapshotAnswer(cmd, false, e.getMessage(), null); } finally { if (dm != null) { + postSnapshotCleanupIssue = recoverVmAfterSnapshot(dm, vmName, suspendedByThisWrapper, filesystemsFrozenByThisWrapper, postSnapshotCleanupIssue); try { dm.free(); } catch (LibvirtException l) { @@ -115,6 +146,12 @@ protected Answer takeDiskOnlyVmSnapshotOfRunningVm(CreateDiskOnlyVmSnapshotComma } } } + + if (answer != null && StringUtils.isNotBlank(postSnapshotCleanupIssue)) { + answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, answer.getResult(), + appendSnapshotOperationIssue(answer.getDetails(), postSnapshotCleanupIssue), answer.getMapVolumeToSnapshotSizeAndNewVolumePath(), answer.getNvramSnapshotPath()); + } + return answer; } protected Answer takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) { @@ -122,10 +159,12 @@ protected Answer takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotComma logger.info("Taking disk-only VM snapshot of stopped VM [{}].", vmName); Map> mapVolumeToSnapshotSizeAndNewVolumePath = new HashMap<>(); + String nvramSnapshotPath = null; List volumeObjectTos = cmd.getVolumeTOs(); KVMStoragePoolManager storagePoolMgr = resource.getStoragePoolMgr(); try { + nvramSnapshotPath = backupNvramIfNeeded(cmd, resource); for (VolumeObjectTO volumeObjectTO : volumeObjectTos) { PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) volumeObjectTO.getDataStore(); KVMStoragePool kvmStoragePool = storagePoolMgr.getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid()); @@ -144,7 +183,7 @@ protected Answer takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotComma mapVolumeToSnapshotSizeAndNewVolumePath.put(volumeObjectTO.getUuid(), new Pair<>(getFileSize(currentDeltaFullPath), snapshotPath)); } - } catch (LibvirtException | QemuImgException e) { + } catch (LibvirtException | QemuImgException | IOException e) { logger.error("Exception while creating disk-only VM snapshot for VM [{}]. Deleting leftover deltas.", vmName, e); for (VolumeObjectTO volumeObjectTO : volumeObjectTos) { Pair volSizeAndNewPath = mapVolumeToSnapshotSizeAndNewVolumePath.get(volumeObjectTO.getUuid()); @@ -160,14 +199,15 @@ protected Answer takeDiskOnlyVmSnapshotOfStoppedVm(CreateDiskOnlyVmSnapshotComma logger.warn("Tried to delete leftover snapshot at [{}] failed.", volSizeAndNewPath.second(), ex); } } + cleanupNvramSnapshotIfNeeded(cmd, resource, nvramSnapshotPath); return new Answer(cmd, e); } - return new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, mapVolumeToSnapshotSizeAndNewVolumePath); + return new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, mapVolumeToSnapshotSizeAndNewVolumePath, nvramSnapshotPath); } - protected int getFlagsToUseForRunningVmSnapshotCreation(VMSnapshotTO target) { - int flags = target.getQuiescevm() ? Domain.SnapshotCreateFlags.QUIESCE : 0; + protected int getFlagsToUseForRunningVmSnapshotCreation(VMSnapshotTO target, boolean filesystemsFrozenByThisWrapper) { + int flags = target.getQuiescevm() && !filesystemsFrozenByThisWrapper ? Domain.SnapshotCreateFlags.QUIESCE : 0; flags += Domain.SnapshotCreateFlags.DISK_ONLY + Domain.SnapshotCreateFlags.ATOMIC + Domain.SnapshotCreateFlags.NO_METADATA; @@ -195,4 +235,166 @@ protected Pair>> createSnapshotXmlAndNewV protected long getFileSize(String path) { return new File(path).length(); } + + protected String backupNvramIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws IOException, LibvirtException { + if (!cmd.isUefiEnabled()) { + return null; + } + + String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid()); + if (StringUtils.isBlank(activeNvramPath) || !Files.exists(Path.of(activeNvramPath))) { + throw new IOException(String.format("Unable to find the active UEFI NVRAM file for VM [%s].", cmd.getVmName())); + } + + VolumeObjectTO rootVolume = getRootVolume(cmd.getVolumeTOs()); + PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) rootVolume.getDataStore(); + KVMStoragePool storagePool = resource.getStoragePoolMgr().getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid()); + + String nvramSnapshotPath = getNvramSnapshotRelativePath(cmd.getTarget().getId()); + Path targetPath = Path.of(storagePool.getLocalPathFor(nvramSnapshotPath)); + Files.createDirectories(targetPath.getParent()); + Files.copy(Path.of(activeNvramPath), targetPath, StandardCopyOption.REPLACE_EXISTING); + return nvramSnapshotPath; + } + + protected void cleanupNvramSnapshotIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, String nvramSnapshotPath) { + if (StringUtils.isBlank(nvramSnapshotPath)) { + return; + } + + try { + VolumeObjectTO rootVolume = getRootVolume(cmd.getVolumeTOs()); + PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) rootVolume.getDataStore(); + KVMStoragePool storagePool = resource.getStoragePoolMgr().getStoragePool(primaryDataStoreTO.getPoolType(), primaryDataStoreTO.getUuid()); + Files.deleteIfExists(Path.of(storagePool.getLocalPathFor(nvramSnapshotPath))); + } catch (Exception e) { + logger.warn("Failed to clean up temporary NVRAM snapshot [{}] for VM [{}].", nvramSnapshotPath, cmd.getVmName(), e); + } + } + + protected String getNvramSnapshotRelativePath(Long vmSnapshotId) { + return String.format("%s/%s.fd", NVRAM_SNAPSHOT_DIR, vmSnapshotId); + } + + protected VolumeObjectTO getRootVolume(List volumeObjectTos) { + return volumeObjectTos.stream() + .filter(volumeObjectTO -> Volume.Type.ROOT.equals(volumeObjectTO.getVolumeType())) + .findFirst() + .orElseThrow(() -> new IllegalStateException("Unable to locate the root volume while handling the VM snapshot.")); + } + + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return cmd.isUefiEnabled(); + } + + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return cmd.isUefiEnabled() && cmd.getTarget().getQuiescevm(); + } + + protected boolean suspendVmIfNeeded(Domain domain) throws LibvirtException { + if (domain.getInfo().state == org.libvirt.DomainInfo.DomainState.VIR_DOMAIN_PAUSED) { + return false; + } + + domain.suspend(); + return true; + } + + protected boolean freezeVmFilesystemsIfNeeded(Domain domain, String vmName) throws LibvirtException, IOException { + freezeVmFilesystems(domain, vmName); + verifyVmFilesystemsFrozen(domain, vmName); + return true; + } + + protected void freezeVmFilesystems(Domain domain, String vmName) throws LibvirtException, IOException { + String result = getResultOfQemuCommand(FreezeThawVMCommand.FREEZE, domain); + if (StringUtils.isBlank(result) || result.startsWith("error")) { + throw new IOException(String.format("Failed to freeze VM [%s] filesystems before taking the disk-only VM snapshot. Result: %s", vmName, result)); + } + } + + protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) throws LibvirtException, IOException { + String status = getResultOfQemuCommand(FreezeThawVMCommand.STATUS, domain); + if (StringUtils.isBlank(status) || !new JsonParser().parse(status).isJsonObject()) { + throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status)); + } + + String statusResult = new JsonParser().parse(status).getAsJsonObject().get("return").getAsString(); + if (!FreezeThawVMCommand.FREEZE.equals(statusResult)) { + throw new IOException(String.format("Failed to freeze VM [%s] filesystems before taking the disk-only VM snapshot. Status: %s", vmName, statusResult)); + } + } + + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName, boolean filesystemsFrozenByThisWrapper) { + if (!filesystemsFrozenByThisWrapper) { + return true; + } + return thawVmFilesystemsIfNeeded(domain, vmName); + } + + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { + try { + String result = getResultOfQemuCommand(FreezeThawVMCommand.THAW, domain); + if (StringUtils.isBlank(result) || result.startsWith("error")) { + logger.warn("Failed to thaw VM [{}] filesystems after taking the disk-only VM snapshot. Result: {}", vmName, result); + return false; + } + return true; + } catch (LibvirtException e) { + logger.warn("Failed to thaw VM [{}] filesystems after taking the disk-only VM snapshot.", vmName, e); + return false; + } + } + + protected String getResultOfQemuCommand(String cmd, Domain domain) throws LibvirtException { + if (cmd.equals(FreezeThawVMCommand.FREEZE)) { + return domain.qemuAgentCommand(QemuCommand.buildQemuCommand(QemuCommand.AGENT_FREEZE, null), 10, 0); + } else if (cmd.equals(FreezeThawVMCommand.THAW)) { + return domain.qemuAgentCommand(QemuCommand.buildQemuCommand(QemuCommand.AGENT_THAW, null), 10, 0); + } else if (cmd.equals(FreezeThawVMCommand.STATUS)) { + return domain.qemuAgentCommand(QemuCommand.buildQemuCommand(QemuCommand.AGENT_FREEZE_STATUS, null), 10, 0); + } + return null; + } + + protected boolean resumeVmIfNeeded(Domain domain, String vmName, boolean suspendedByThisWrapper) { + if (!suspendedByThisWrapper) { + return true; + } + return resumeVmIfNeeded(domain, vmName); + } + + protected boolean resumeVmIfNeeded(Domain domain, String vmName) { + try { + if (domain.getInfo().state == org.libvirt.DomainInfo.DomainState.VIR_DOMAIN_PAUSED) { + domain.resume(); + } + return true; + } catch (LibvirtException e) { + logger.warn("Failed to resume VM [{}] after taking the disk-only VM snapshot.", vmName, e); + return false; + } + } + + protected String recoverVmAfterSnapshot(Domain domain, String vmName, boolean suspendedByThisWrapper, boolean filesystemsFrozenByThisWrapper, String currentIssue) { + if (suspendedByThisWrapper && !resumeVmIfNeeded(domain, vmName)) { + currentIssue = appendSnapshotOperationIssue(currentIssue, + String.format("VM [%s] could not be resumed after taking the disk-only snapshot. Guest may still be paused.", vmName)); + } + if (filesystemsFrozenByThisWrapper && !thawVmFilesystemsIfNeeded(domain, vmName)) { + currentIssue = appendSnapshotOperationIssue(currentIssue, + String.format("VM [%s] filesystems could not be thawed after taking the disk-only snapshot. Guest may still be frozen.", vmName)); + } + return currentIssue; + } + + protected String appendSnapshotOperationIssue(String currentIssue, String newIssue) { + if (StringUtils.isBlank(newIssue)) { + return currentIssue; + } + if (StringUtils.isBlank(currentIssue)) { + return newIssue; + } + return currentIssue + " " + newIssue; + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java index 15df8627a8a7..62c7c5013374 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java @@ -27,12 +27,16 @@ import com.cloud.resource.CommandWrapper; import com.cloud.resource.ResourceWrapper; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; +import org.apache.cloudstack.storage.to.SnapshotObjectTO; +import org.apache.commons.lang3.StringUtils; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; +import com.cloud.storage.Volume; + @ResourceWrapper(handles = DeleteDiskOnlyVmSnapshotCommand.class) public class LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper extends CommandWrapper { @@ -53,6 +57,40 @@ public Answer execute(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingR return new Answer(command, e); } } + + deleteNvramSnapshotIfNeeded(command, resource, storagePoolMgr, snapshotsToDelete); return new Answer(command, true, null); } + + protected void deleteNvramSnapshotIfNeeded(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingResource resource, KVMStoragePoolManager storagePoolMgr, + List snapshotsToDelete) { + if (StringUtils.isBlank(command.getNvramSnapshotPath())) { + return; + } + + try { + KVMStoragePool storagePool; + if (command.getPrimaryDataStore() != null) { + PrimaryDataStoreTO dataStore = command.getPrimaryDataStore(); + storagePool = storagePoolMgr.getStoragePool(dataStore.getPoolType(), dataStore.getUuid()); + } else { + SnapshotObjectTO rootVolumeSnapshot = snapshotsToDelete.stream() + .map(SnapshotObjectTO.class::cast) + .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType())) + .findFirst() + .orElse(null); + + if (rootVolumeSnapshot == null) { + logger.warn("Unable to locate the root volume snapshot while deleting NVRAM snapshot [{}].", command.getNvramSnapshotPath()); + return; + } + + storagePool = resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot, storagePoolMgr); + } + + Files.deleteIfExists(Path.of(storagePool.getLocalPathFor(command.getNvramSnapshotPath()))); + } catch (Exception e) { + logger.warn("Failed to delete the UEFI NVRAM snapshot [{}]. It will be left behind on storage.", command.getNvramSnapshotPath(), e); + } + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java index 1aa79d48eec2..e4cd527331bd 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java @@ -31,15 +31,20 @@ import org.apache.cloudstack.utils.qemu.QemuImg; import org.apache.cloudstack.utils.qemu.QemuImgException; import org.apache.cloudstack.utils.qemu.QemuImgFile; +import org.apache.commons.lang3.StringUtils; import org.libvirt.LibvirtException; import java.io.IOException; +import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.HashMap; import java.util.List; +import com.cloud.storage.Volume; + @ResourceWrapper(handles = RevertDiskOnlyVmSnapshotCommand.class) public class LibvirtRevertDiskOnlyVMSnapshotCommandWrapper extends CommandWrapper { @@ -55,6 +60,8 @@ public Answer execute(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResou HashMap snapshotToNewDeltaPath = new HashMap<>(); try { + SnapshotObjectTO rootVolumeSnapshot = getRootVolumeSnapshot(snapshotObjectTos); + validateNvramRevertState(cmd, resource, rootVolumeSnapshot, storagePoolMgr); for (SnapshotObjectTO snapshotObjectTo : snapshotObjectTos) { KVMStoragePool kvmStoragePool = libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(snapshotObjectTo, storagePoolMgr); @@ -71,7 +78,8 @@ public Answer execute(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResou qemuImg.create(newDelta, currentDelta); snapshotToNewDeltaPath.put(snapshotObjectTo, deltaPath); } - } catch (LibvirtException | QemuImgException e) { + restoreNvramIfNeeded(cmd, resource, rootVolumeSnapshot, storagePoolMgr); + } catch (LibvirtException | QemuImgException | IOException e) { logger.error("Exception while reverting disk-only VM snapshot for VM [{}]. Deleting leftover deltas.", vmName, e); for (SnapshotObjectTO snapshotObjectTo : snapshotObjectTos) { String newPath = snapshotToNewDeltaPath.get(snapshotObjectTo); @@ -108,4 +116,88 @@ public Answer execute(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResou return new RevertDiskOnlyVmSnapshotAnswer(cmd, volumeObjectTos); } + + protected SnapshotObjectTO getRootVolumeSnapshot(List snapshotObjectTos) { + return snapshotObjectTos.stream() + .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType())) + .findFirst() + .orElse(null); + } + + protected void validateNvramRevertState(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot, + KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException { + String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid()); + if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) { + if (cmd.isUefiEnabled()) { + throw new IOException(String.format("Cannot safely revert disk-only VM snapshot for UEFI VM [%s] because the snapshot does not contain NVRAM state.", + cmd.getVmName())); + } + return; + } + + if (StringUtils.isBlank(activeNvramPath)) { + throw new IOException(String.format("Unable to determine the active UEFI NVRAM path for VM [%s].", cmd.getVmName())); + } + + Path snapshotNvramPath = getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot, resource, storagePoolMgr); + if (!Files.exists(snapshotNvramPath)) { + throw new IOException(String.format("Unable to find the UEFI NVRAM snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName())); + } + } + + protected void restoreNvramIfNeeded(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot, + KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException { + if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) { + return; + } + + String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid()); + if (StringUtils.isBlank(activeNvramPath)) { + throw new IOException(String.format("Unable to determine the active UEFI NVRAM path for VM [%s].", cmd.getVmName())); + } + + Path snapshotNvramPath = getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot, resource, storagePoolMgr); + if (!Files.exists(snapshotNvramPath)) { + throw new IOException(String.format("Unable to find the UEFI NVRAM snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName())); + } + + replaceNvramAtomically(snapshotNvramPath, Path.of(activeNvramPath)); + } + + protected void replaceNvramAtomically(Path snapshotNvramPath, Path activeNvramPath) throws IOException { + Path targetDirectory = activeNvramPath.getParent(); + if (targetDirectory != null) { + Files.createDirectories(targetDirectory); + } + + Path temporaryNvramPath = Files.createTempFile(targetDirectory, activeNvramPath.getFileName().toString(), ".tmp"); + try { + copyNvramSnapshotToTemporaryPath(snapshotNvramPath, temporaryNvramPath); + moveTemporaryNvramIntoPlace(temporaryNvramPath, activeNvramPath); + } finally { + Files.deleteIfExists(temporaryNvramPath); + } + } + + protected void copyNvramSnapshotToTemporaryPath(Path snapshotNvramPath, Path temporaryNvramPath) throws IOException { + Files.copy(snapshotNvramPath, temporaryNvramPath, StandardCopyOption.REPLACE_EXISTING); + } + + protected void moveTemporaryNvramIntoPlace(Path temporaryNvramPath, Path activeNvramPath) throws IOException { + try { + Files.move(temporaryNvramPath, activeNvramPath, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); + } catch (AtomicMoveNotSupportedException e) { + Files.move(temporaryNvramPath, activeNvramPath, StandardCopyOption.REPLACE_EXISTING); + } + } + + protected Path getNvramSnapshotAbsolutePath(String nvramSnapshotPath, SnapshotObjectTO rootVolumeSnapshot, LibvirtComputingResource resource, + KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException { + if (rootVolumeSnapshot == null) { + throw new IOException("Unable to locate the root volume snapshot while handling the UEFI NVRAM state."); + } + + KVMStoragePool storagePool = resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot, storagePoolMgr); + return Path.of(storagePool.getLocalPathFor(nvramSnapshotPath)); + } } From a8bd7d4bb463f95b020971aa8ca8a1962a8890c2 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Tue, 14 Apr 2026 14:25:16 +0300 Subject: [PATCH 3/7] KVM: add regression tests for UEFI disk-only instance snapshots Cover host capability synchronization, UEFI NVRAM sidecar handling across create/revert/delete flows, and the running-VM recovery paths for disk-only instance snapshots. --- .../agent/manager/AgentManagerImplTest.java | 37 +- ...ileBasedStorageVmSnapshotStrategyTest.java | 413 ++++++++++ ...tDiskOnlyVMSnapshotCommandWrapperTest.java | 711 ++++++++++++++++++ 3 files changed, 1160 insertions(+), 1 deletion(-) create mode 100644 engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java diff --git a/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java b/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java index 43d83a672c0f..2377bbaefbc8 100644 --- a/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java +++ b/engine/orchestration/src/test/java/com/cloud/agent/manager/AgentManagerImplTest.java @@ -18,14 +18,17 @@ import com.cloud.agent.Listener; import com.cloud.agent.api.Answer; +import com.cloud.agent.api.ReadyAnswer; import com.cloud.agent.api.ReadyCommand; import com.cloud.agent.api.StartupCommand; import com.cloud.agent.api.StartupRoutingCommand; import com.cloud.exception.ConnectionException; +import com.cloud.host.DetailVO; import com.cloud.host.Host; import com.cloud.host.HostVO; import com.cloud.host.Status; import com.cloud.host.dao.HostDao; +import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.utils.Pair; import org.junit.Assert; @@ -34,10 +37,13 @@ import org.mockito.Mockito; import java.util.ArrayList; +import java.util.HashMap; +import java.util.Map; public class AgentManagerImplTest { private HostDao hostDao; + private HostDetailsDao hostDetailsDao; private Listener storagePoolMonitor; private AgentAttache attache; private AgentManagerImpl mgr = Mockito.spy(new AgentManagerImpl()); @@ -46,15 +52,18 @@ public class AgentManagerImplTest { @Before public void setUp() throws Exception { - host = new HostVO("some-Uuid"); + host = Mockito.spy(new HostVO("some-Uuid")); + Mockito.when(host.getId()).thenReturn(1L); host.setDataCenterId(1L); cmds = new StartupCommand[]{new StartupRoutingCommand()}; attache = new ConnectedAgentAttache(null, 1L, "uuid", "kvm-attache", Hypervisor.HypervisorType.KVM, null, false); hostDao = Mockito.mock(HostDao.class); + hostDetailsDao = Mockito.mock(HostDetailsDao.class); storagePoolMonitor = Mockito.mock(Listener.class); mgr._hostDao = hostDao; + mgr._hostDetailsDao = hostDetailsDao; mgr._hostMonitors = new ArrayList<>(); mgr._hostMonitors.add(new Pair<>(0, storagePoolMonitor)); } @@ -86,6 +95,32 @@ public void testNotifyMonitorsOfConnectionWhenStoragePoolConnectionHostFailure() Mockito.verify(mgr, Mockito.times(1)).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.eq(Status.Event.AgentDisconnected), Mockito.eq(true), Mockito.eq(true)); } + @Test + public void testNotifyMonitorsOfConnectionClearsStaleNvramCapabilityOnReconnect() throws ConnectionException { + DetailVO staleNvramCapability = Mockito.mock(DetailVO.class); + ReadyAnswer readyAnswer = Mockito.mock(ReadyAnswer.class); + host.setDetails(new HashMap<>(Map.of(Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString(), + Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString()))); + + Mockito.when(staleNvramCapability.getId()).thenReturn(11L); + Mockito.when(hostDao.findById(Mockito.anyLong())).thenReturn(host); + Mockito.doNothing().when(hostDao).loadDetails(host); + Mockito.when(hostDetailsDao.findDetail(host.getId(), Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(staleNvramCapability); + Mockito.doNothing().when(storagePoolMonitor).processConnect(Mockito.eq(host), Mockito.eq(cmds[0]), Mockito.eq(false)); + Mockito.doReturn(true).when(mgr).handleDisconnectWithoutInvestigation(Mockito.any(attache.getClass()), Mockito.any(Status.Event.class), Mockito.anyBoolean(), Mockito.anyBoolean()); + Mockito.when(readyAnswer.getResult()).thenReturn(true); + Mockito.when(readyAnswer.getDetailsMap()).thenReturn(Map.of(Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString())); + Mockito.doReturn(readyAnswer).when(mgr).easySend(Mockito.anyLong(), Mockito.any(ReadyCommand.class)); + Mockito.doReturn(true).when(mgr).agentStatusTransitTo(Mockito.eq(host), Mockito.eq(Status.Event.Ready), Mockito.anyLong()); + + final AgentAttache agentAttache = mgr.notifyMonitorsOfConnection(attache, cmds, false); + + Assert.assertTrue(agentAttache.isReady()); + Assert.assertFalse(host.getDetails().containsKey(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)); + Mockito.verify(hostDetailsDao).remove(11L); + Mockito.verify(hostDao).saveDetails(host); + } + @Test public void testGetTimeoutWithPositiveTimeout() { Commands commands = Mockito.mock(Commands.class); diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java new file mode 100644 index 000000000000..aeed1ab91974 --- /dev/null +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java @@ -0,0 +1,413 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.cloudstack.storage.vmsnapshot; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.List; + +import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory; +import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; +import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; +import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; +import org.apache.cloudstack.storage.to.SnapshotObjectTO; +import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.apache.cloudstack.api.ApiConstants; +import org.junit.Before; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotAnswer; +import com.cloud.agent.api.storage.DeleteDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotAnswer; +import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotCommand; +import com.cloud.alert.AlertManager; +import com.cloud.host.DetailVO; +import com.cloud.host.Host; +import com.cloud.host.dao.HostDetailsDao; +import com.cloud.storage.DataStoreRole; +import com.cloud.storage.Volume; +import com.cloud.storage.dao.VolumeDao; +import com.cloud.user.ResourceLimitService; +import com.cloud.uservm.UserVm; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.VMInstanceDetailVO; +import com.cloud.vm.UserVmVO; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.VMInstanceDetailsDao; +import com.cloud.vm.dao.UserVmDao; +import com.cloud.vm.snapshot.VMSnapshot; +import com.cloud.vm.snapshot.VMSnapshotDetailsVO; +import com.cloud.vm.snapshot.VMSnapshotVO; +import com.cloud.vm.snapshot.dao.VMSnapshotDao; +import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao; + +public class KvmFileBasedStorageVmSnapshotStrategyTest { + + private KvmFileBasedStorageVmSnapshotStrategy strategy; + private VMSnapshotDetailsDao vmSnapshotDetailsDao; + private VMSnapshotDao vmSnapshotDao; + private VMSnapshotHelper vmSnapshotHelper; + private AgentManager agentMgr; + private SnapshotDataStoreDao snapshotDataStoreDao; + private HostDetailsDao hostDetailsDao; + + @Before + public void setup() { + strategy = Mockito.spy(new KvmFileBasedStorageVmSnapshotStrategy()); + vmSnapshotDetailsDao = mock(VMSnapshotDetailsDao.class); + vmSnapshotDao = mock(VMSnapshotDao.class); + vmSnapshotHelper = mock(VMSnapshotHelper.class); + agentMgr = mock(AgentManager.class); + snapshotDataStoreDao = mock(SnapshotDataStoreDao.class); + hostDetailsDao = mock(HostDetailsDao.class); + + strategy.vmSnapshotDetailsDao = vmSnapshotDetailsDao; + strategy.vmSnapshotDao = vmSnapshotDao; + strategy.vmSnapshotHelper = vmSnapshotHelper; + strategy.agentMgr = agentMgr; + strategy.snapshotDataStoreDao = snapshotDataStoreDao; + strategy.resourceLimitManager = mock(ResourceLimitService.class); + strategy.snapshotDataFactory = mock(SnapshotDataFactory.class); + strategy.userVmDao = mock(UserVmDao.class); + strategy.volumeDao = mock(VolumeDao.class); + strategy.vmInstanceDetailsDao = mock(VMInstanceDetailsDao.class); + strategy.hostDetailsDao = hostDetailsDao; + strategy.alertManager = mock(AlertManager.class); + doNothing().when(strategy).publishUsageEvent(anyString(), any(VMSnapshot.class), any(UserVm.class), anyLong(), anyLong()); + doNothing().when(strategy).publishUsageEvent(anyString(), any(VMSnapshot.class), any(UserVm.class), any(VolumeObjectTO.class)); + } + + @Test + public void testProcessCreateVmSnapshotAnswerPersistsNvramPath() throws Exception { + VMSnapshot vmSnapshot = mock(VMSnapshot.class); + CreateDiskOnlyVmSnapshotAnswer answer = mock(CreateDiskOnlyVmSnapshotAnswer.class); + UserVm userVm = mock(UserVm.class); + VMSnapshotVO vmSnapshotVO = mock(VMSnapshotVO.class); + + when(vmSnapshot.getId()).thenReturn(42L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(answer.getMapVolumeToSnapshotSizeAndNewVolumePath()).thenReturn(Collections.emptyMap()); + when(answer.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + + Method method = KvmFileBasedStorageVmSnapshotStrategy.class.getDeclaredMethod("processCreateVmSnapshotAnswer", VMSnapshot.class, java.util.Map.class, + CreateDiskOnlyVmSnapshotAnswer.class, UserVm.class, VMSnapshotVO.class, long.class, VMSnapshotVO.class); + method.setAccessible(true); + method.invoke(strategy, vmSnapshot, Collections.emptyMap(), answer, userVm, vmSnapshotVO, 0L, null); + + verify(vmSnapshotDetailsDao).addDetail(42L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + } + + @Test + public void testRevertVMSnapshotPassesNvramPathToAgentCommand() { + long vmId = 10L; + long snapshotId = 20L; + long dataStoreId = 30L; + long hostId = 40L; + + UserVmVO userVm = mock(UserVmVO.class); + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VMSnapshotVO currentVmSnapshot = mock(VMSnapshotVO.class); + SnapshotDataStoreVO snapshotDataStoreVO = mock(SnapshotDataStoreVO.class); + SnapshotInfo snapshotInfo = mock(SnapshotInfo.class); + SnapshotObjectTO snapshotObjectTO = mock(SnapshotObjectTO.class); + VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(snapshotId, "kvmFileBasedStorageSnapshot", String.valueOf(snapshotId), true); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(snapshotId, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + + when(vmSnapshot.getVmId()).thenReturn(vmId); + when(vmSnapshot.getId()).thenReturn(snapshotId); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(userVm.getState()).thenReturn(VirtualMachine.State.Stopped); + when(userVm.getName()).thenReturn("i-10-VM"); + when(userVm.getUuid()).thenReturn("vm-uuid"); + when(userVm.getId()).thenReturn(vmId); + when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); + when(strategy.vmInstanceDetailsDao.findDetail(vmId, ApiConstants.BootType.UEFI.toString())) + .thenReturn(new VMInstanceDetailVO(vmId, ApiConstants.BootType.UEFI.toString(), "SECURE", true)); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); + 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(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + when(vmSnapshotDetailsDao.findDetails(snapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of(volumeSnapshotDetail)); + when(vmSnapshotDetailsDao.findDetail(snapshotId, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(snapshotDataStoreDao.findOneBySnapshotAndDatastoreRole(snapshotId, DataStoreRole.Primary)).thenReturn(snapshotDataStoreVO); + when(snapshotDataStoreVO.getSnapshotId()).thenReturn(snapshotId); + when(snapshotDataStoreVO.getDataStoreId()).thenReturn(dataStoreId); + when(strategy.snapshotDataFactory.getSnapshot(snapshotId, dataStoreId, DataStoreRole.Primary)).thenReturn(snapshotInfo); + when(snapshotInfo.getTO()).thenReturn(snapshotObjectTO); + when(vmSnapshotDao.findCurrentSnapshotByVmId(vmId)).thenReturn(currentVmSnapshot); + when(agentMgr.easySend(eq(hostId), any())).thenAnswer(invocation -> + new RevertDiskOnlyVmSnapshotAnswer((RevertDiskOnlyVmSnapshotCommand) invocation.getArgument(1), Collections.emptyList())); + + ArgumentCaptor commandCaptor = ArgumentCaptor.forClass(RevertDiskOnlyVmSnapshotCommand.class); + + strategy.revertVMSnapshot(vmSnapshot); + + verify(agentMgr).easySend(eq(hostId), commandCaptor.capture()); + verify(userVm).setLastHostId(hostId); + verify(strategy.userVmDao).update(vmId, userVm); + assertEquals("vm-uuid", commandCaptor.getValue().getVmUuid()); + assertEquals(true, commandCaptor.getValue().isUefiEnabled()); + assertEquals("nvram/42.fd", commandCaptor.getValue().getNvramSnapshotPath()); + } + + @Test + public void testDeleteNvramSnapshotIfNeededPassesPrimaryDataStoreToAgentCommand() { + long hostId = 40L; + + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + PrimaryDataStoreTO primaryDataStore = mock(PrimaryDataStoreTO.class); + + when(vmSnapshot.getId()).thenReturn(20L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + when(agentMgr.easySend(eq(hostId), any())).thenReturn(new Answer(null, true, null)); + + strategy.deleteNvramSnapshotIfNeeded(vmSnapshot, hostId, primaryDataStore); + + verify(agentMgr).easySend(eq(hostId), argThat(command -> { + if (!(command instanceof DeleteDiskOnlyVmSnapshotCommand)) { + return false; + } + + DeleteDiskOnlyVmSnapshotCommand deleteCommand = (DeleteDiskOnlyVmSnapshotCommand) command; + return deleteCommand.getSnapshots().isEmpty() + && "nvram/42.fd".equals(deleteCommand.getNvramSnapshotPath()) + && deleteCommand.getPrimaryDataStore() == primaryDataStore; + })); + } + + @Test(expected = CloudRuntimeException.class) + public void testDeleteNvramSnapshotIfNeededFailsWhenHostLacksNvramAwareCleanupCapability() { + long hostId = 40L; + + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + PrimaryDataStoreTO primaryDataStore = mock(PrimaryDataStoreTO.class); + + when(vmSnapshot.getId()).thenReturn(20L); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshotDetailsDao.findDetail(20L, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); + + try { + strategy.deleteNvramSnapshotIfNeeded(vmSnapshot, hostId, primaryDataStore); + } finally { + verify(agentMgr, never()).easySend(eq(hostId), any()); + } + } + + @Test + public void testGetRootVolumePrimaryDataStoreForCleanupReturnsNullWhenRootVolumeIsMissing() { + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VolumeObjectTO dataVolume = mock(VolumeObjectTO.class); + + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(dataVolume.getVolumeType()).thenReturn(Volume.Type.DATADISK); + + PrimaryDataStoreTO primaryDataStore = strategy.getRootVolumePrimaryDataStoreForCleanup(vmSnapshot, List.of(dataVolume)); + + assertNull(primaryDataStore); + } + + @Test + public void testGetPrimaryDataStoreForNvramCleanupPrefersRootSnapshotPrimaryDataStore() { + long vmSnapshotId = 20L; + long rootSnapshotId = 30L; + long dataStoreId = 40L; + + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VolumeObjectTO rootVolume = mock(VolumeObjectTO.class); + PrimaryDataStoreTO liveRootVolumePrimaryDataStore = mock(PrimaryDataStoreTO.class); + PrimaryDataStoreTO rootSnapshotPrimaryDataStore = mock(PrimaryDataStoreTO.class); + SnapshotDataStoreVO rootSnapshotDataStore = mock(SnapshotDataStoreVO.class); + SnapshotInfo rootSnapshotInfo = mock(SnapshotInfo.class); + SnapshotObjectTO rootSnapshotObjectTo = mock(SnapshotObjectTO.class); + VolumeObjectTO rootSnapshotVolume = mock(VolumeObjectTO.class); + VMSnapshotDetailsVO volumeSnapshotDetail = new VMSnapshotDetailsVO(vmSnapshotId, "kvmFileBasedStorageSnapshot", String.valueOf(rootSnapshotId), true); + + when(vmSnapshot.getId()).thenReturn(vmSnapshotId); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(vmSnapshotDetailsDao.findDetails(vmSnapshotId, "kvmFileBasedStorageSnapshot")).thenReturn(List.of(volumeSnapshotDetail)); + when(snapshotDataStoreDao.findOneBySnapshotAndDatastoreRole(rootSnapshotId, DataStoreRole.Primary)).thenReturn(rootSnapshotDataStore); + when(rootSnapshotDataStore.getSnapshotId()).thenReturn(rootSnapshotId); + when(rootSnapshotDataStore.getDataStoreId()).thenReturn(dataStoreId); + when(strategy.snapshotDataFactory.getSnapshot(rootSnapshotId, dataStoreId, DataStoreRole.Primary)).thenReturn(rootSnapshotInfo); + when(rootSnapshotInfo.getTO()).thenReturn(rootSnapshotObjectTo); + when(rootSnapshotObjectTo.getVolume()).thenReturn(rootSnapshotVolume); + when(rootSnapshotVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootSnapshotObjectTo.getDataStore()).thenReturn(rootSnapshotPrimaryDataStore); + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootVolume.getDataStore()).thenReturn(liveRootVolumePrimaryDataStore); + + PrimaryDataStoreTO primaryDataStore = strategy.getPrimaryDataStoreForNvramCleanup(vmSnapshot, List.of(rootVolume)); + + assertSame(rootSnapshotPrimaryDataStore, primaryDataStore); + } + + @Test(expected = CloudRuntimeException.class) + public void testTakeVmSnapshotInternalFailsWhenHostLacksNvramAwareSnapshotCapabilityForUefiVm() 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(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); + + strategy.takeVmSnapshotInternal(vmSnapshot, Collections.emptyMap()); + } + + @Test(expected = CloudRuntimeException.class) + public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityForSidecarSnapshot() { + long vmId = 10L; + long vmSnapshotId = 20L; + long hostId = 40L; + + UserVmVO userVm = mock(UserVmVO.class); + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(vmSnapshotId, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); + + when(vmSnapshot.getVmId()).thenReturn(vmId); + when(vmSnapshot.getId()).thenReturn(vmSnapshotId); + when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); + when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); + + strategy.deleteVMSnapshot(vmSnapshot); + } + + @Test(expected = CloudRuntimeException.class) + public void testTakeVmSnapshotInternalFailsWhenHostLacksUefiCapabilityForUefiVm() 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(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(null); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + + strategy.takeVmSnapshotInternal(vmSnapshot, Collections.emptyMap()); + } + + @Test(expected = CloudRuntimeException.class) + public void testRevertVMSnapshotFailsWhenHostLacksNvramAwareSnapshotCapabilityForUefiVm() { + 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.Stopped); + 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); + + strategy.revertVMSnapshot(vmSnapshot); + } + + @Test(expected = CloudRuntimeException.class) + public void testRevertVMSnapshotFailsWhenHostLacksUefiCapabilityForUefiVm() { + 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.Stopped); + 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(null); + when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + + strategy.revertVMSnapshot(vmSnapshot); + } + + @Test + public void testNotifyGuestRecoveryIssueIfNeededSendsAlert() { + CreateDiskOnlyVmSnapshotAnswer answer = mock(CreateDiskOnlyVmSnapshotAnswer.class); + UserVm userVm = mock(UserVm.class); + VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); + + when(answer.getDetails()).thenReturn("VM could not be thawed"); + when(userVm.getUuid()).thenReturn("vm-uuid"); + when(userVm.getDataCenterId()).thenReturn(1L); + when(userVm.getPodIdToDeployIn()).thenReturn(2L); + when(vmSnapshot.getUuid()).thenReturn("snapshot-uuid"); + + strategy.notifyGuestRecoveryIssueIfNeeded(answer, userVm, vmSnapshot); + + verify(strategy.alertManager).sendAlert(eq(AlertManager.AlertType.ALERT_TYPE_VM_SNAPSHOT), eq(1L), eq(2L), anyString(), anyString()); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java new file mode 100644 index 000000000000..361761f28d41 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java @@ -0,0 +1,711 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.cloud.hypervisor.kvm.resource.wrapper; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.UUID; + +import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; +import org.apache.cloudstack.storage.to.SnapshotObjectTO; +import org.apache.cloudstack.storage.to.VolumeObjectTO; +import org.junit.Test; +import org.libvirt.Domain; +import org.libvirt.DomainInfo; + +import com.cloud.agent.api.VMSnapshotTO; +import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.CreateDiskOnlyVmSnapshotAnswer; +import com.cloud.agent.api.storage.DeleteDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.storage.RevertDiskOnlyVmSnapshotCommand; +import com.cloud.agent.api.to.DataTO; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.hypervisor.kvm.storage.KVMStoragePool; +import com.cloud.hypervisor.kvm.storage.KVMStoragePoolManager; +import com.cloud.storage.Storage; +import com.cloud.storage.Volume; +import com.cloud.utils.Pair; + +public class LibvirtDiskOnlyVMSnapshotCommandWrapperTest { + + @Test + public void testBackupNvramIfNeededCopiesActiveNvram() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO vmSnapshotTO = mock(VMSnapshotTO.class); + PrimaryDataStoreTO dataStoreTO = mock(PrimaryDataStoreTO.class); + VolumeObjectTO rootVolume = mock(VolumeObjectTO.class); + + Path activeNvram = Files.createTempFile("active-", ".fd"); + Files.writeString(activeNvram, "snapshot-nvram"); + Path poolDirectory = Files.createTempDirectory("pool-"); + + when(command.isUefiEnabled()).thenReturn(true); + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.getTarget()).thenReturn(vmSnapshotTO); + when(vmSnapshotTO.getId()).thenReturn(42L); + when(command.getVolumeTOs()).thenReturn(List.of(rootVolume)); + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootVolume.getDataStore()).thenReturn(dataStoreTO); + when(dataStoreTO.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(dataStoreTO.getUuid()).thenReturn("pool-uuid"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(activeNvram.toString()); + when(resource.getStoragePoolMgr()).thenReturn(storagePoolManager); + when(storagePoolManager.getStoragePool(Storage.StoragePoolType.NetworkFilesystem, "pool-uuid")).thenReturn(storagePool); + when(storagePool.getLocalPathFor(anyString())).thenAnswer(invocation -> poolDirectory.resolve(invocation.getArgument(0, String.class)).toString()); + + String nvramSnapshotPath = wrapper.backupNvramIfNeeded(command, resource); + + assertEquals(".cloudstack-vm-snapshot-nvram/42.fd", nvramSnapshotPath); + assertEquals("snapshot-nvram", Files.readString(poolDirectory.resolve(nvramSnapshotPath))); + } + + @Test(expected = IOException.class) + public void testBackupNvramIfNeededFailsWhenUefiNvramIsMissing() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + + when(command.isUefiEnabled()).thenReturn(true); + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn("/tmp/" + UUID.randomUUID() + ".fd"); + + wrapper.backupNvramIfNeeded(command, resource); + } + + @Test(expected = IOException.class) + public void testValidateNvramRevertStateFailsForLegacySnapshotsOnUefiVms() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + + Path activeNvram = Files.createTempFile("active-", ".fd"); + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.isUefiEnabled()).thenReturn(true); + when(command.getNvramSnapshotPath()).thenReturn(null); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(activeNvram.toString()); + + wrapper.validateNvramRevertState(command, resource, null, mock(KVMStoragePoolManager.class)); + } + + @Test(expected = IOException.class) + public void testValidateNvramRevertStateFailsForLegacySnapshotsOnFallbackHostsForUefiVms() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.isUefiEnabled()).thenReturn(true); + when(command.getNvramSnapshotPath()).thenReturn(null); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(Path.of("/tmp", UUID.randomUUID() + ".fd").toString()); + + wrapper.validateNvramRevertState(command, resource, null, mock(KVMStoragePoolManager.class)); + } + + @Test + public void testValidateNvramRevertStateAllowsFallbackHostsWithoutLocalNvram() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + SnapshotObjectTO rootSnapshot = mock(SnapshotObjectTO.class); + + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.isUefiEnabled()).thenReturn(true); + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(poolDirectory.resolve("missing").resolve("vm-uuid.fd").toString()); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(rootSnapshot, storagePoolManager)).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + wrapper.validateNvramRevertState(command, resource, rootSnapshot, storagePoolManager); + } + + @Test + public void testRestoreNvramIfNeededRestoresSnapshotBytes() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + SnapshotObjectTO rootSnapshot = mock(SnapshotObjectTO.class); + + Path activeNvram = Files.createTempFile("active-", ".fd"); + Files.writeString(activeNvram, "current"); + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(activeNvram.toString()); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(rootSnapshot, storagePoolManager)).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + wrapper.restoreNvramIfNeeded(command, resource, rootSnapshot, storagePoolManager); + + assertEquals("snapshot", Files.readString(activeNvram)); + } + + @Test + public void testRestoreNvramIfNeededCreatesMissingActiveNvramFile() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + SnapshotObjectTO rootSnapshot = mock(SnapshotObjectTO.class); + + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Path activeNvram = poolDirectory.resolve("target").resolve("vm-uuid.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(activeNvram.toString()); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(rootSnapshot, storagePoolManager)).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + wrapper.restoreNvramIfNeeded(command, resource, rootSnapshot, storagePoolManager); + + assertEquals("snapshot", Files.readString(activeNvram)); + } + + @Test + public void testRestoreNvramIfNeededPreservesActiveNvramWhenCopyFails() throws Exception { + LibvirtRevertDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtRevertDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected void copyNvramSnapshotToTemporaryPath(Path snapshotNvramPath, Path temporaryNvramPath) throws IOException { + Files.writeString(temporaryNvramPath, "partial"); + throw new IOException("copy failed"); + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + RevertDiskOnlyVmSnapshotCommand command = mock(RevertDiskOnlyVmSnapshotCommand.class); + SnapshotObjectTO rootSnapshot = mock(SnapshotObjectTO.class); + + Path activeNvram = Files.createTempFile("active-", ".fd"); + Files.writeString(activeNvram, "current"); + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getVmUuid()).thenReturn("vm-uuid"); + when(command.getVmName()).thenReturn("vm-name"); + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(resource.getUefiNvramPath("vm-uuid")).thenReturn(activeNvram.toString()); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(rootSnapshot, storagePoolManager)).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + try { + wrapper.restoreNvramIfNeeded(command, resource, rootSnapshot, storagePoolManager); + fail("Expected restore to fail when the snapshot copy fails."); + } catch (IOException expected) { + assertEquals("current", Files.readString(activeNvram)); + } + } + + @Test + public void testDeleteNvramSnapshotIfNeededDeletesSidecar() throws Exception { + LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + DeleteDiskOnlyVmSnapshotCommand command = mock(DeleteDiskOnlyVmSnapshotCommand.class); + SnapshotObjectTO rootSnapshot = mock(SnapshotObjectTO.class); + VolumeObjectTO rootVolume = mock(VolumeObjectTO.class); + + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(rootSnapshot.getVolume()).thenReturn(rootVolume); + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getPrimaryPoolFromDataTo(rootSnapshot, storagePoolManager)).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + wrapper.deleteNvramSnapshotIfNeeded(command, resource, storagePoolManager, List.of(rootSnapshot)); + + assertFalse(Files.exists(snapshotNvram)); + assertTrue(Files.exists(poolDirectory)); + } + + @Test + public void testDeleteNvramSnapshotIfNeededDeletesSidecarUsingPrimaryDataStore() throws Exception { + LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper(); + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + KVMStoragePoolManager storagePoolManager = mock(KVMStoragePoolManager.class); + KVMStoragePool storagePool = mock(KVMStoragePool.class); + DeleteDiskOnlyVmSnapshotCommand command = mock(DeleteDiskOnlyVmSnapshotCommand.class); + PrimaryDataStoreTO primaryDataStoreTO = mock(PrimaryDataStoreTO.class); + + Path poolDirectory = Files.createTempDirectory("pool-"); + Path snapshotNvram = poolDirectory.resolve("nvram/42.fd"); + Files.createDirectories(snapshotNvram.getParent()); + Files.writeString(snapshotNvram, "snapshot"); + + when(command.getNvramSnapshotPath()).thenReturn("nvram/42.fd"); + when(command.getPrimaryDataStore()).thenReturn(primaryDataStoreTO); + when(primaryDataStoreTO.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + when(primaryDataStoreTO.getUuid()).thenReturn("pool-uuid"); + when(storagePoolManager.getStoragePool(Storage.StoragePoolType.NetworkFilesystem, "pool-uuid")).thenReturn(storagePool); + when(storagePool.getLocalPathFor("nvram/42.fd")).thenReturn(snapshotNvram.toString()); + + wrapper.deleteNvramSnapshotIfNeeded(command, resource, storagePoolManager, List.of()); + + assertFalse(Files.exists(snapshotNvram)); + } + + @Test + public void testResumeVmIfNeededOnlyResumesWhenWrapperSuspendedVm() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + Domain domain = mock(Domain.class); + + wrapper.resumeVmIfNeeded(domain, "vm-name", false); + + verify(domain, never()).resume(); + verify(domain, never()).getInfo(); + } + + @Test + public void testSuspendVmIfNeededSkipsAlreadyPausedVm() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + Domain domain = mock(Domain.class); + DomainInfo domainInfo = new DomainInfo(); + domainInfo.state = DomainInfo.DomainState.VIR_DOMAIN_PAUSED; + when(domain.getInfo()).thenReturn(domainInfo); + + assertFalse(wrapper.suspendVmIfNeeded(domain)); + verify(domain, never()).suspend(); + } + + @Test + public void testShouldSuspendVmForSnapshotWhenUefiAndNotQuiesced() { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.isUefiEnabled()).thenReturn(true); + when(command.getTarget()).thenReturn(target); + when(target.getQuiescevm()).thenReturn(false); + + assertTrue(wrapper.shouldSuspendVmForSnapshot(command)); + } + + @Test + public void testShouldSuspendVmForSnapshotWhenQuiesceIsRequested() { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.isUefiEnabled()).thenReturn(true); + when(command.getTarget()).thenReturn(target); + when(target.getQuiescevm()).thenReturn(true); + + assertTrue(wrapper.shouldSuspendVmForSnapshot(command)); + } + + @Test + public void testShouldFreezeVmFilesystemsForSnapshotWhenQuiesceIsRequested() { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.isUefiEnabled()).thenReturn(true); + when(command.getTarget()).thenReturn(target); + when(target.getQuiescevm()).thenReturn(true); + + assertTrue(wrapper.shouldFreezeVmFilesystemsForSnapshot(command)); + } + + @Test + public void testGetFlagsToUseForRunningVmSnapshotCreationOmitsLibvirtQuiesceWhenAlreadyFrozen() { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper(); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(target.getQuiescevm()).thenReturn(true); + + int flags = wrapper.getFlagsToUseForRunningVmSnapshotCreation(target, true); + + assertEquals(0, flags & Domain.SnapshotCreateFlags.QUIESCE); + assertTrue((flags & Domain.SnapshotCreateFlags.DISK_ONLY) != 0); + assertTrue((flags & Domain.SnapshotCreateFlags.ATOMIC) != 0); + assertTrue((flags & Domain.SnapshotCreateFlags.NO_METADATA) != 0); + } + + @Test + public void testFreezeVmFilesystemsIfNeededSucceedsWhenGuestAgentReportsFrozen() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected String getResultOfQemuCommand(String cmd, Domain domain) { + if ("status".equals(cmd)) { + return "{\"return\":\"frozen\"}"; + } + return "{\"return\":0}"; + } + }; + + assertTrue(wrapper.freezeVmFilesystemsIfNeeded(mock(Domain.class), "vm-name")); + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmThawsWhenFreezeVerificationFails() throws Exception { + final boolean[] thawCalled = {false}; + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return true; + } + + @Override + protected void freezeVmFilesystems(Domain domain, String vmName) { + } + + @Override + protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) throws IOException { + throw new IOException("status verification failed"); + } + + @Override + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { + thawCalled[0] = true; + return true; + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertFalse(answer.getResult()); + assertTrue("Thaw must be attempted after a successful freeze followed by verification failure", thawCalled[0]); + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmSuspendsBeforeNvramCopyForQuiescedUefiSnapshots() throws Exception { + List operations = new ArrayList<>(); + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected void freezeVmFilesystems(Domain domain, String vmName) { + operations.add("freeze"); + } + + @Override + protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) { + operations.add("verify"); + } + + @Override + protected boolean suspendVmIfNeeded(Domain domain) { + operations.add("suspend"); + return true; + } + + @Override + protected String backupNvramIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) { + operations.add("backup"); + return "nvram/42.fd"; + } + + @Override + protected void cleanupNvramSnapshotIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, String nvramSnapshotPath) { + } + + @Override + protected boolean resumeVmIfNeeded(Domain domain, String vmName) { + operations.add("resume"); + return true; + } + + @Override + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { + operations.add("thaw"); + return true; + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(command.isUefiEnabled()).thenReturn(true); + when(target.getQuiescevm()).thenReturn(true); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + doThrow(mock(org.libvirt.LibvirtException.class)).when(domain).snapshotCreateXML(anyString(), anyInt()); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertFalse(answer.getResult()); + assertEquals(List.of("freeze", "verify", "suspend", "backup", "resume", "thaw"), operations); + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmReturnsSuccessWithWarningWhenThawFails() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return true; + } + + @Override + protected void freezeVmFilesystems(Domain domain, String vmName) { + } + + @Override + protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) { + } + + @Override + protected String backupNvramIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) { + return null; + } + + @Override + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { + return false; + } + + @Override + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName, boolean frozen) { + return false; + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertTrue("Snapshot metadata must still be returned when thaw fails after snapshot creation", answer.getResult()); + assertTrue(answer.getDetails().contains("could not be thawed")); + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmReturnsSuccessWithWarningWhenResumeFails() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return true; + } + + @Override + protected boolean suspendVmIfNeeded(Domain domain) { + return true; + } + + @Override + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected String backupNvramIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) { + return null; + } + + @Override + protected boolean resumeVmIfNeeded(Domain domain, String vmName) { + return false; + } + + @Override + protected boolean resumeVmIfNeeded(Domain domain, String vmName, boolean suspended) { + return false; + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertTrue("Snapshot metadata must still be returned when resume fails after snapshot creation", answer.getResult()); + assertTrue(answer.getDetails().contains("could not be resumed")); + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmHandlesNullErrorMessage() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected String backupNvramIfNeeded(CreateDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource) throws IOException { + throw new IOException(); + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(command.isUefiEnabled()).thenReturn(true); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertFalse(answer.getResult()); + } +} From 6a81730b0c539326adadd1b24bfb4a6e4fad0cd4 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Tue, 14 Apr 2026 21:08:16 +0300 Subject: [PATCH 4/7] build: fix pre-commit issues after 4.22 rebase --- agent/conf/agent.properties | 1 - .../resource/wrapper/LibvirtConvertInstanceCommandWrapper.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties index 013c2e5bf2af..ba4a3874664a 100644 --- a/agent/conf/agent.properties +++ b/agent/conf/agent.properties @@ -472,4 +472,3 @@ iscsi.session.cleanup.enabled=false # Optional vCenter SHA1 thumbprint for VMware to KVM conversion via VDDK, passed as # -io vddk-thumbprint=. If unset, CloudStack computes it on the KVM host via openssl. #vddk.thumbprint= - diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java index eaa8b7904397..f28b5eda4a63 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtConvertInstanceCommandWrapper.java @@ -107,7 +107,7 @@ public Answer execute(ConvertInstanceCommand cmd, LibvirtComputingResource serve String vddkLibDir = resolveVddkSetting(cmd.getVddkLibDir(), serverResource.getVddkLibDir()); if (StringUtils.isBlank(vddkLibDir)) { String err = String.format("VDDK lib dir is not configured on the host. " + - "Set '%s' in agent.properties or in details parameter of the import api calll to use VDDK-based conversion.", "vddk.lib.dir"); + "Set '%s' in agent.properties or in details parameter of the import api call to use VDDK-based conversion.", "vddk.lib.dir"); logger.error("({}) {}", originalVMName, err); return new Answer(cmd, false, err); } From 75bf21521284db9bd9b7b8028c98ced4c1989340 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Fri, 17 Apr 2026 14:41:02 +0300 Subject: [PATCH 5/7] kvm: harden NVRAM freeze status parsing and document UEFI snapshot suspend Address Copilot's review comment on #13020. `JsonParser.parse(...)`, `.get("return")` and `.getAsString()` in `verifyVmFilesystemsFrozen` can throw unchecked exceptions (`JsonSyntaxException`, `IllegalStateException`, `NullPointerException`) on malformed or unexpected QEMU guest-agent responses, bypassing the surrounding `catch (LibvirtException | IOException)` and potentially leaving the guest frozen until the `finally` recovery runs. Parse defensively, validate that `return` is a JSON string, and explicitly treat `{"error": {...}}` guest-agent responses as an IO failure so the outer handler thaws/resumes the guest and surfaces a proper failure answer. Also document in PendingReleaseNotes that taking a disk-only instance snapshot for KVM UEFI VMs now briefly suspends the guest while the NVRAM sidecar is copied. Non-UEFI VMs are unaffected. Add regression tests covering the guest-agent error response and the malformed-JSON path, including verification that the thaw recovery is invoked when freeze verification fails. Signed-off-by: kunal.behbudzade --- PendingReleaseNotes | 5 ++ ...reateDiskOnlyVMSnapshotCommandWrapper.java | 26 ++++++- ...tDiskOnlyVMSnapshotCommandWrapperTest.java | 74 +++++++++++++++++++ 3 files changed, 103 insertions(+), 2 deletions(-) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index fb382e4e8478..02d63811e36e 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -48,3 +48,8 @@ example.ver.1 > example.ver.2: * UEFI disk-only instance snapshots taken before this change do not contain an NVRAM sidecar and cannot be safely reverted. Take a new snapshot after upgrading before relying on revert for UEFI VMs. + + * Taking a disk-only instance snapshot for KVM UEFI VMs now briefly suspends + the guest while the NVRAM sidecar is copied, so that the captured firmware + state is consistent with the disk snapshot. Non-UEFI VMs are unaffected and + continue to snapshot live. diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java index 4a549b747f85..ceedd5caf5ae 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java @@ -41,6 +41,8 @@ import org.libvirt.Connect; import org.libvirt.Domain; import org.libvirt.LibvirtException; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; import com.google.gson.JsonParser; import java.io.File; @@ -315,11 +317,31 @@ protected void freezeVmFilesystems(Domain domain, String vmName) throws LibvirtE protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) throws LibvirtException, IOException { String status = getResultOfQemuCommand(FreezeThawVMCommand.STATUS, domain); - if (StringUtils.isBlank(status) || !new JsonParser().parse(status).isJsonObject()) { + if (StringUtils.isBlank(status)) { throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status)); } - String statusResult = new JsonParser().parse(status).getAsJsonObject().get("return").getAsString(); + JsonObject statusObject; + try { + JsonElement statusElement = new JsonParser().parse(status); + if (!statusElement.isJsonObject()) { + throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status)); + } + statusObject = statusElement.getAsJsonObject(); + } catch (RuntimeException e) { + throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status), e); + } + + if (statusObject.has("error")) { + throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status)); + } + + JsonElement returnElement = statusObject.get("return"); + if (returnElement == null || !returnElement.isJsonPrimitive() || !returnElement.getAsJsonPrimitive().isString()) { + throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status)); + } + + String statusResult = returnElement.getAsString(); if (!FreezeThawVMCommand.FREEZE.equals(statusResult)) { throw new IOException(String.format("Failed to freeze VM [%s] filesystems before taking the disk-only VM snapshot. Status: %s", vmName, statusResult)); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java index 361761f28d41..b37bb137c643 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java @@ -469,6 +469,80 @@ protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { assertTrue("Thaw must be attempted after a successful freeze followed by verification failure", thawCalled[0]); } + @Test + public void testVerifyVmFilesystemsFrozenFailsForQemuErrorResponse() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected String getResultOfQemuCommand(String cmd, Domain domain) { + return "{\"error\":{\"class\":\"GenericError\",\"desc\":\"guest agent failure\"}}"; + } + }; + + try { + wrapper.verifyVmFilesystemsFrozen(mock(Domain.class), "vm-name"); + fail("QEMU guest agent error responses must be treated as IO failures."); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Failed to verify VM [vm-name] filesystem freeze state")); + } + } + + @Test + public void testTakeDiskOnlyVmSnapshotOfRunningVmReturnsFailureAnswerWhenFreezeStatusJsonIsMalformed() throws Exception { + final boolean[] thawCalled = {false}; + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected Pair>> createSnapshotXmlAndNewVolumePathMap(List volumeObjectTOS, + List disks, VMSnapshotTO target, LibvirtComputingResource resource) { + return new Pair<>("", Collections.emptyMap()); + } + + @Override + protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return false; + } + + @Override + protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) { + return true; + } + + @Override + protected void freezeVmFilesystems(Domain domain, String vmName) { + } + + @Override + protected String getResultOfQemuCommand(String cmd, Domain domain) { + return "not-json"; + } + + @Override + protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { + thawCalled[0] = true; + return true; + } + }; + LibvirtComputingResource resource = mock(LibvirtComputingResource.class); + LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class); + org.libvirt.Connect connect = mock(org.libvirt.Connect.class); + Domain domain = mock(Domain.class); + CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class); + VMSnapshotTO target = mock(VMSnapshotTO.class); + + when(command.getVmName()).thenReturn("vm-name"); + when(command.getVolumeTOs()).thenReturn(List.of()); + when(command.getTarget()).thenReturn(target); + when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect); + when(resource.getDisks(connect, "vm-name")).thenReturn(List.of()); + when(resource.getDomain(connect, "vm-name")).thenReturn(domain); + + CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource); + + assertFalse(answer.getResult()); + assertTrue(answer.getDetails().contains("Failed to verify VM [vm-name] filesystem freeze state")); + assertTrue("Thaw must be attempted when freeze verification fails on malformed JSON", thawCalled[0]); + } + @Test public void testTakeDiskOnlyVmSnapshotOfRunningVmSuspendsBeforeNvramCopyForQuiescedUefiSnapshots() throws Exception { List operations = new ArrayList<>(); From a4d9825252c8d6bb563100a4c1afec81f9618e84 Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Sun, 26 Apr 2026 17:11:33 +0300 Subject: [PATCH 6/7] KVM: tighten UEFI disk-only snapshot handling Keep stopped UEFI snapshot creation tied to the last host so the agent copies the active host-local NVRAM file, while still allowing capable-host selection for revert and NVRAM sidecar cleanup. Also handle QEMU guest-agent error payloads consistently and add the API/UI warning for the brief suspend used when snapshotting running UEFI guests. Signed-off-by: kunal.behbudzade --- .../user/vmsnapshot/CreateVMSnapshotCmd.java | 3 +- ...KvmFileBasedStorageVmSnapshotStrategy.java | 103 +++++++++++++++- ...ileBasedStorageVmSnapshotStrategyTest.java | 111 +++++++++++++++++- ...reateDiskOnlyVMSnapshotCommandWrapper.java | 24 ++-- ...tDiskOnlyVMSnapshotCommandWrapperTest.java | 35 +++++- ui/public/locales/en.json | 1 + ui/src/config/section/compute.js | 1 + ui/src/config/section/network.js | 1 + 8 files changed, 260 insertions(+), 19 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vmsnapshot/CreateVMSnapshotCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vmsnapshot/CreateVMSnapshotCmd.java index 6e1a7daf4c23..f2decf21a1db 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vmsnapshot/CreateVMSnapshotCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vmsnapshot/CreateVMSnapshotCmd.java @@ -37,7 +37,8 @@ import com.cloud.vm.VirtualMachine; import com.cloud.vm.snapshot.VMSnapshot; -@APICommand(name = "createVMSnapshot", description = "Creates Snapshot for an Instance.", responseObject = VMSnapshotResponse.class, since = "4.2.0", entityType = {VMSnapshot.class}, +@APICommand(name = "createVMSnapshot", description = "Creates Snapshot for an Instance. Running KVM UEFI disk-only snapshots briefly suspend the Instance while copying NVRAM state.", + responseObject = VMSnapshotResponse.class, since = "4.2.0", entityType = {VMSnapshot.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) public class CreateVMSnapshotCmd extends BaseAsyncCreateCmd { diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java index ad2be6f4fb84..e946598d8ae0 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java @@ -34,6 +34,7 @@ import com.cloud.event.UsageEventUtils; import com.cloud.host.DetailVO; import com.cloud.host.Host; +import com.cloud.host.HostVO; import com.cloud.host.dao.HostDetailsDao; import com.cloud.hypervisor.Hypervisor; import com.cloud.storage.DataStoreRole; @@ -134,7 +135,7 @@ public boolean deleteVMSnapshot(VMSnapshot vmSnapshot) { logger.info("Starting VM snapshot delete process for snapshot [{}].", vmSnapshot.getUuid()); UserVmVO userVm = userVmDao.findById(vmSnapshot.getVmId()); VMSnapshotVO vmSnapshotBeingDeleted = (VMSnapshotVO) vmSnapshot; - Long hostId = vmSnapshotHelper.pickRunningHost(vmSnapshotBeingDeleted.getVmId()); + Long hostId = pickHostForNvramSidecarCleanup(vmSnapshotBeingDeleted, userVm, "delete"); validateHostSupportsNvramSidecarCleanup(vmSnapshotBeingDeleted, hostId, "delete"); long virtualSize = 0; boolean isCurrent = vmSnapshotBeingDeleted.getCurrent(); @@ -197,7 +198,7 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) { } VMSnapshotVO vmSnapshotBeingReverted = (VMSnapshotVO) vmSnapshot; - Long hostId = vmSnapshotHelper.pickRunningHost(vmSnapshotBeingReverted.getVmId()); + Long hostId = pickHostForUefiNvramAwareDiskOnlySnapshot(userVm, "revert"); validateHostSupportsUefiNvramAwareDiskOnlySnapshots(hostId, userVm, "revert"); transitStateWithoutThrow(vmSnapshotBeingReverted, VMSnapshot.Event.RevertRequested); @@ -229,7 +230,9 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) { publishUsageEvent(EventTypes.EVENT_VM_SNAPSHOT_REVERT, vmSnapshotBeingReverted, userVm, volumeObjectTo); } - if (isUefiVm(userVm)) { + if (isUefiVm(userVm) && !Objects.equals(userVm.getLastHostId(), hostId)) { + logger.debug("Updating last host of UEFI VM [{}] to [{}] after disk-only snapshot revert because the NVRAM state was restored on that host.", + userVm.getUuid(), hostId); userVm.setLastHostId(hostId); userVmDao.update(userVm.getId(), userVm); } @@ -409,7 +412,6 @@ protected void deleteNvramSnapshotIfNeeded(VMSnapshotVO vmSnapshotVO, Long hostI return; } - validateHostSupportsNvramSidecarCleanup(vmSnapshotVO, hostId, "delete"); DeleteDiskOnlyVmSnapshotCommand deleteSnapshotCommand = new DeleteDiskOnlyVmSnapshotCommand(List.of(), nvramSnapshotPath, primaryDataStore); Answer answer = agentMgr.easySend(hostId, deleteSnapshotCommand); if (answer == null || !answer.getResult()) { @@ -520,7 +522,7 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map volumeTOs = vmSnapshotHelper.getVolumeTOList(userVm.getId()); @@ -760,6 +762,97 @@ protected String getNvramSnapshotPath(VMSnapshotVO vmSnapshot) { return nvramDetail != null ? nvramDetail.getValue() : null; } + protected Long pickHostForUefiNvramAwareDiskOnlySnapshot(UserVm userVm, String operation) { + Long selectedHostId = vmSnapshotHelper.pickRunningHost(userVm.getId()); + if (!isUefiVm(userVm)) { + return selectedHostId; + } + + boolean isCreate = "create".equals(operation); + if (isCreate) { + validateUefiSnapshotCreateHostOwnsActiveNvram(userVm, selectedHostId); + } + + return pickHostWithRequiredCapabilities(userVm, selectedHostId, operation, !isCreate, + List.of(Host.HOST_UEFI_ENABLE, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)); + } + + protected void validateUefiSnapshotCreateHostOwnsActiveNvram(UserVm userVm, Long selectedHostId) { + if (VirtualMachine.State.Running.equals(userVm.getState())) { + return; + } + + Long lastHostId = userVm.getLastHostId(); + if (lastHostId == null || !Objects.equals(lastHostId, selectedHostId)) { + throw new CloudRuntimeException(String.format("Cannot create a disk-only snapshot for stopped UEFI VM [%s] on host [%s] because the active NVRAM " + + "state is expected on last host [%s]. Make the last host available or start the VM on a UEFI-capable KVM host before retrying.", + userVm.getUuid(), selectedHostId, lastHostId)); + } + } + + protected Long pickHostForNvramSidecarCleanup(VMSnapshotVO vmSnapshotVO, UserVm userVm, String operation) { + Long selectedHostId = vmSnapshotHelper.pickRunningHost(vmSnapshotVO.getVmId()); + if (StringUtils.isBlank(getNvramSnapshotPath(vmSnapshotVO))) { + return selectedHostId; + } + + return pickHostWithRequiredCapabilities(userVm, selectedHostId, operation, true, List.of(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)); + } + + protected Long pickHostWithRequiredCapabilities(UserVm userVm, Long selectedHostId, String operation, boolean canFallbackFromSelectedHost, + List requiredCapabilities) { + if (hostSupportsCapabilities(selectedHostId, requiredCapabilities)) { + return selectedHostId; + } + + if (VirtualMachine.State.Running.equals(userVm.getState()) || !canFallbackFromSelectedHost) { + return selectedHostId; + } + + return listCandidateHostsForVmSnapshot(userVm).stream() + .filter(host -> hostSupportsCapabilities(host.getId(), requiredCapabilities)) + .findFirst() + .map(HostVO::getId) + .orElseThrow(() -> new CloudRuntimeException(String.format("Cannot %s disk-only snapshot state for VM [%s] because no Up and Enabled host in the " + + "VM storage scope advertises [%s].", operation, userVm.getUuid(), String.join(", ", requiredCapabilities)))); + } + + protected List listCandidateHostsForVmSnapshot(UserVm userVm) { + List volumes = volumeDao.findByInstance(userVm.getId()); + if (CollectionUtils.isEmpty(volumes)) { + throw new CloudRuntimeException(String.format("Cannot find a host for VM snapshot operation because VM [%s] has no volumes.", userVm.getUuid())); + } + + VolumeVO volume = volumes.stream() + .filter(volumeVO -> Volume.Type.ROOT.equals(volumeVO.getVolumeType())) + .findFirst() + .orElse(volumes.get(0)); + Long poolId = volume.getPoolId(); + if (poolId == null) { + throw new CloudRuntimeException(String.format("Cannot find a host for VM snapshot operation because volume [%s] has no pool.", volume.getUuid())); + } + + StoragePoolVO storagePoolVO = storagePool.findById(poolId); + if (storagePoolVO == null) { + throw new CloudRuntimeException(String.format("Cannot find a host for VM snapshot operation because storage pool [%s] was not found.", poolId)); + } + + List hosts = hostDao.listAllUpAndEnabledNonHAHosts(Host.Type.Routing, storagePoolVO.getClusterId(), storagePoolVO.getPodId(), + storagePoolVO.getDataCenterId(), null); + if (CollectionUtils.isEmpty(hosts)) { + throw new CloudRuntimeException(String.format("Cannot find a host for VM snapshot operation because no Up and Enabled host was found in storage pool [%s] scope.", + storagePoolVO.getUuid())); + } + return hosts; + } + + protected boolean hostSupportsCapabilities(Long hostId, List requiredCapabilities) { + if (hostId == null || CollectionUtils.isEmpty(requiredCapabilities)) { + return false; + } + return requiredCapabilities.stream().allMatch(capability -> isHostCapabilityEnabled(hostId, capability)); + } + protected void validateHostSupportsUefiNvramAwareDiskOnlySnapshots(Long hostId, UserVm userVm, String operation) { if (!isUefiVm(userVm)) { return; diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java index aeed1ab91974..053fff844aa1 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java +++ b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java @@ -38,8 +38,10 @@ import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo; +import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao; import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO; +import org.apache.cloudstack.storage.datastore.db.StoragePoolVO; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; import org.apache.cloudstack.storage.to.SnapshotObjectTO; import org.apache.cloudstack.storage.to.VolumeObjectTO; @@ -58,9 +60,12 @@ import com.cloud.alert.AlertManager; import com.cloud.host.DetailVO; import com.cloud.host.Host; +import com.cloud.host.HostVO; +import com.cloud.host.dao.HostDao; import com.cloud.host.dao.HostDetailsDao; import com.cloud.storage.DataStoreRole; import com.cloud.storage.Volume; +import com.cloud.storage.VolumeVO; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.ResourceLimitService; import com.cloud.uservm.UserVm; @@ -85,6 +90,8 @@ public class KvmFileBasedStorageVmSnapshotStrategyTest { private AgentManager agentMgr; private SnapshotDataStoreDao snapshotDataStoreDao; private HostDetailsDao hostDetailsDao; + private HostDao hostDao; + private PrimaryDataStoreDao storagePoolDao; @Before public void setup() { @@ -95,12 +102,16 @@ public void setup() { agentMgr = mock(AgentManager.class); snapshotDataStoreDao = mock(SnapshotDataStoreDao.class); hostDetailsDao = mock(HostDetailsDao.class); + hostDao = mock(HostDao.class); + storagePoolDao = mock(PrimaryDataStoreDao.class); strategy.vmSnapshotDetailsDao = vmSnapshotDetailsDao; strategy.vmSnapshotDao = vmSnapshotDao; strategy.vmSnapshotHelper = vmSnapshotHelper; strategy.agentMgr = agentMgr; strategy.snapshotDataStoreDao = snapshotDataStoreDao; + strategy.hostDao = hostDao; + strategy.storagePool = storagePoolDao; strategy.resourceLimitManager = mock(ResourceLimitService.class); strategy.snapshotDataFactory = mock(SnapshotDataFactory.class); strategy.userVmDao = mock(UserVmDao.class); @@ -155,6 +166,7 @@ public void testRevertVMSnapshotPassesNvramPathToAgentCommand() { when(userVm.getName()).thenReturn("i-10-VM"); when(userVm.getUuid()).thenReturn("vm-uuid"); when(userVm.getId()).thenReturn(vmId); + when(userVm.getLastHostId()).thenReturn(39L); when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); when(strategy.vmInstanceDetailsDao.findDetail(vmId, ApiConstants.BootType.UEFI.toString())) .thenReturn(new VMInstanceDetailVO(vmId, ApiConstants.BootType.UEFI.toString(), "SECURE", true)); @@ -186,6 +198,74 @@ public void testRevertVMSnapshotPassesNvramPathToAgentCommand() { assertEquals("nvram/42.fd", commandCaptor.getValue().getNvramSnapshotPath()); } + @Test + public void testPickHostForUefiNvramAwareDiskOnlySnapshotUsesCapableCandidateForRevertWhenDefaultHostLacksSupport() { + long vmId = 10L; + long defaultHostId = 40L; + long capableHostId = 41L; + long poolId = 50L; + long clusterId = 60L; + long podId = 70L; + long dataCenterId = 80L; + + UserVmVO userVm = mock(UserVmVO.class); + VolumeVO rootVolume = mock(VolumeVO.class); + StoragePoolVO storagePool = mock(StoragePoolVO.class); + HostVO defaultHost = mock(HostVO.class); + HostVO capableHost = mock(HostVO.class); + + when(userVm.getId()).thenReturn(vmId); + when(userVm.getUuid()).thenReturn("vm-uuid"); + when(userVm.getState()).thenReturn(VirtualMachine.State.Stopped); + when(strategy.vmInstanceDetailsDao.findDetail(vmId, ApiConstants.BootType.UEFI.toString())) + .thenReturn(new VMInstanceDetailVO(vmId, ApiConstants.BootType.UEFI.toString(), "SECURE", true)); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(defaultHostId); + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootVolume.getPoolId()).thenReturn(poolId); + when(strategy.volumeDao.findByInstance(vmId)).thenReturn(List.of(rootVolume)); + when(storagePoolDao.findById(poolId)).thenReturn(storagePool); + when(storagePool.getClusterId()).thenReturn(clusterId); + when(storagePool.getPodId()).thenReturn(podId); + when(storagePool.getDataCenterId()).thenReturn(dataCenterId); + when(defaultHost.getId()).thenReturn(defaultHostId); + when(capableHost.getId()).thenReturn(capableHostId); + when(hostDao.listAllUpAndEnabledNonHAHosts(Host.Type.Routing, clusterId, podId, dataCenterId, null)) + .thenReturn(List.of(defaultHost, capableHost)); + when(hostDetailsDao.findDetail(defaultHostId, Host.HOST_UEFI_ENABLE)).thenReturn(null); + when(hostDetailsDao.findDetail(defaultHostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); + when(hostDetailsDao.findDetail(capableHostId, Host.HOST_UEFI_ENABLE)) + .thenReturn(new DetailVO(capableHostId, Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString())); + when(hostDetailsDao.findDetail(capableHostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(capableHostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + + Long hostId = strategy.pickHostForUefiNvramAwareDiskOnlySnapshot(userVm, "revert"); + + assertEquals(Long.valueOf(capableHostId), hostId); + } + + @Test(expected = CloudRuntimeException.class) + public void testPickHostForUefiNvramAwareDiskOnlySnapshotFailsCreateWhenSelectedHostIsNotLastHost() { + long vmId = 10L; + long lastHostId = 39L; + long selectedHostId = 40L; + + UserVmVO userVm = mock(UserVmVO.class); + + when(userVm.getId()).thenReturn(vmId); + when(userVm.getUuid()).thenReturn("vm-uuid"); + when(userVm.getState()).thenReturn(VirtualMachine.State.Stopped); + when(userVm.getLastHostId()).thenReturn(lastHostId); + when(strategy.vmInstanceDetailsDao.findDetail(vmId, ApiConstants.BootType.UEFI.toString())) + .thenReturn(new VMInstanceDetailVO(vmId, ApiConstants.BootType.UEFI.toString(), "SECURE", true)); + when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(selectedHostId); + when(hostDetailsDao.findDetail(selectedHostId, Host.HOST_UEFI_ENABLE)) + .thenReturn(new DetailVO(selectedHostId, Host.HOST_UEFI_ENABLE, Boolean.TRUE.toString())); + when(hostDetailsDao.findDetail(selectedHostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)) + .thenReturn(new DetailVO(selectedHostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM, Boolean.TRUE.toString())); + + strategy.pickHostForUefiNvramAwareDiskOnlySnapshot(userVm, "create"); + } + @Test public void testDeleteNvramSnapshotIfNeededPassesPrimaryDataStoreToAgentCommand() { long hostId = 40L; @@ -216,12 +296,11 @@ public void testDeleteNvramSnapshotIfNeededPassesPrimaryDataStoreToAgentCommand( } @Test(expected = CloudRuntimeException.class) - public void testDeleteNvramSnapshotIfNeededFailsWhenHostLacksNvramAwareCleanupCapability() { + public void testValidateHostSupportsNvramSidecarCleanupFailsWhenHostLacksNvramAwareCleanupCapability() { long hostId = 40L; VMSnapshotVO vmSnapshot = mock(VMSnapshotVO.class); VMSnapshotDetailsVO nvramDetail = new VMSnapshotDetailsVO(20L, "kvmFileBasedStorageSnapshotNvram", "nvram/42.fd", false); - PrimaryDataStoreTO primaryDataStore = mock(PrimaryDataStoreTO.class); when(vmSnapshot.getId()).thenReturn(20L); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); @@ -229,7 +308,7 @@ public void testDeleteNvramSnapshotIfNeededFailsWhenHostLacksNvramAwareCleanupCa when(hostDetailsDao.findDetail(hostId, Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM)).thenReturn(null); try { - strategy.deleteNvramSnapshotIfNeeded(vmSnapshot, hostId, primaryDataStore); + strategy.validateHostSupportsNvramSidecarCleanup(vmSnapshot, hostId, "delete"); } finally { verify(agentMgr, never()).easySend(eq(hostId), any()); } @@ -294,6 +373,7 @@ public void testTakeVmSnapshotInternalFailsWhenHostLacksNvramAwareSnapshotCapabi 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())) @@ -318,6 +398,7 @@ public void testDeleteVMSnapshotFailsWhenHostLacksNvramAwareCleanupCapabilityFor when(vmSnapshot.getVmId()).thenReturn(vmId); when(vmSnapshot.getId()).thenReturn(vmSnapshotId); when(vmSnapshot.getUuid()).thenReturn("vm-snapshot"); + when(userVm.getState()).thenReturn(VirtualMachine.State.Running); when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); when(vmSnapshotDetailsDao.findDetail(vmSnapshotId, "kvmFileBasedStorageSnapshotNvram")).thenReturn(nvramDetail); @@ -337,6 +418,7 @@ public void testTakeVmSnapshotInternalFailsWhenHostLacksUefiCapabilityForUefiVm( 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())) @@ -362,6 +444,7 @@ public void testRevertVMSnapshotFailsWhenHostLacksNvramAwareSnapshotCapabilityFo when(userVm.getState()).thenReturn(VirtualMachine.State.Stopped); when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); + mockCandidateHostScope(vmId, 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)) @@ -385,6 +468,7 @@ public void testRevertVMSnapshotFailsWhenHostLacksUefiCapabilityForUefiVm() { when(userVm.getState()).thenReturn(VirtualMachine.State.Stopped); when(strategy.userVmDao.findById(vmId)).thenReturn(userVm); when(vmSnapshotHelper.pickRunningHost(vmId)).thenReturn(hostId); + mockCandidateHostScope(vmId, 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(null); @@ -410,4 +494,25 @@ public void testNotifyGuestRecoveryIssueIfNeededSendsAlert() { verify(strategy.alertManager).sendAlert(eq(AlertManager.AlertType.ALERT_TYPE_VM_SNAPSHOT), eq(1L), eq(2L), anyString(), anyString()); } + + private void mockCandidateHostScope(long vmId, long hostId) { + long poolId = 50L; + long clusterId = 60L; + long podId = 70L; + long dataCenterId = 80L; + + VolumeVO rootVolume = mock(VolumeVO.class); + StoragePoolVO storagePool = mock(StoragePoolVO.class); + HostVO host = mock(HostVO.class); + + when(rootVolume.getVolumeType()).thenReturn(Volume.Type.ROOT); + when(rootVolume.getPoolId()).thenReturn(poolId); + when(strategy.volumeDao.findByInstance(vmId)).thenReturn(List.of(rootVolume)); + when(storagePoolDao.findById(poolId)).thenReturn(storagePool); + when(storagePool.getClusterId()).thenReturn(clusterId); + when(storagePool.getPodId()).thenReturn(podId); + when(storagePool.getDataCenterId()).thenReturn(dataCenterId); + when(host.getId()).thenReturn(hostId); + when(hostDao.listAllUpAndEnabledNonHAHosts(Host.Type.Routing, clusterId, podId, dataCenterId, null)).thenReturn(List.of(host)); + } } diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java index ceedd5caf5ae..46a7da70c61f 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java @@ -111,6 +111,7 @@ protected Answer takeDiskOnlyVmSnapshotOfRunningVm(CreateDiskOnlyVmSnapshotComma VMSnapshotTO target = cmd.getTarget(); Pair>> snapshotXmlAndVolumeToNewPathMap = createSnapshotXmlAndNewVolumePathMap(volumeObjectTOS, disks, target, resource); if (shouldFreezeVmFilesystemsForSnapshot(cmd)) { + // The guest-agent freeze flushes guest filesystems; suspend below prevents concurrent UEFI NVRAM writes. freezeVmFilesystems(dm, vmName); filesystemsFrozenByThisWrapper = true; verifyVmFilesystemsFrozen(dm, vmName); @@ -302,15 +303,9 @@ protected boolean suspendVmIfNeeded(Domain domain) throws LibvirtException { return true; } - protected boolean freezeVmFilesystemsIfNeeded(Domain domain, String vmName) throws LibvirtException, IOException { - freezeVmFilesystems(domain, vmName); - verifyVmFilesystemsFrozen(domain, vmName); - return true; - } - protected void freezeVmFilesystems(Domain domain, String vmName) throws LibvirtException, IOException { String result = getResultOfQemuCommand(FreezeThawVMCommand.FREEZE, domain); - if (StringUtils.isBlank(result) || result.startsWith("error")) { + if (isQemuAgentErrorResponse(result)) { throw new IOException(String.format("Failed to freeze VM [%s] filesystems before taking the disk-only VM snapshot. Result: %s", vmName, result)); } } @@ -357,7 +352,7 @@ protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName, boolea protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { try { String result = getResultOfQemuCommand(FreezeThawVMCommand.THAW, domain); - if (StringUtils.isBlank(result) || result.startsWith("error")) { + if (isQemuAgentErrorResponse(result)) { logger.warn("Failed to thaw VM [{}] filesystems after taking the disk-only VM snapshot. Result: {}", vmName, result); return false; } @@ -368,6 +363,19 @@ protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) { } } + protected boolean isQemuAgentErrorResponse(String result) { + if (StringUtils.isBlank(result) || result.startsWith("error")) { + return true; + } + + try { + JsonElement resultElement = new JsonParser().parse(result); + return resultElement.isJsonObject() && resultElement.getAsJsonObject().has("error"); + } catch (RuntimeException e) { + return false; + } + } + protected String getResultOfQemuCommand(String cmd, Domain domain) throws LibvirtException { if (cmd.equals(FreezeThawVMCommand.FREEZE)) { return domain.qemuAgentCommand(QemuCommand.buildQemuCommand(QemuCommand.AGENT_FREEZE, null), 10, 0); diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java index b37bb137c643..e447e6f6cfdb 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java @@ -399,7 +399,7 @@ public void testGetFlagsToUseForRunningVmSnapshotCreationOmitsLibvirtQuiesceWhen } @Test - public void testFreezeVmFilesystemsIfNeededSucceedsWhenGuestAgentReportsFrozen() throws Exception { + public void testFreezeAndVerifyVmFilesystemsSucceedsWhenGuestAgentReportsFrozen() throws Exception { LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { @Override protected String getResultOfQemuCommand(String cmd, Domain domain) { @@ -410,7 +410,38 @@ protected String getResultOfQemuCommand(String cmd, Domain domain) { } }; - assertTrue(wrapper.freezeVmFilesystemsIfNeeded(mock(Domain.class), "vm-name")); + Domain domain = mock(Domain.class); + wrapper.freezeVmFilesystems(domain, "vm-name"); + wrapper.verifyVmFilesystemsFrozen(domain, "vm-name"); + } + + @Test + public void testFreezeVmFilesystemsFailsForQemuErrorResponse() throws Exception { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected String getResultOfQemuCommand(String cmd, Domain domain) { + return "{\"error\":{\"class\":\"GenericError\",\"desc\":\"guest agent failure\"}}"; + } + }; + + try { + wrapper.freezeVmFilesystems(mock(Domain.class), "vm-name"); + fail("QEMU guest agent error responses must be treated as freeze failures."); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Failed to freeze VM [vm-name] filesystems")); + } + } + + @Test + public void testThawVmFilesystemsFailsForQemuErrorResponse() { + LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() { + @Override + protected String getResultOfQemuCommand(String cmd, Domain domain) { + return "{\"error\":{\"class\":\"GenericError\",\"desc\":\"guest agent failure\"}}"; + } + }; + + assertFalse(wrapper.thawVmFilesystemsIfNeeded(mock(Domain.class), "vm-name")); } @Test diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index f5b7cc647dc9..2dd997ce272c 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -2914,6 +2914,7 @@ "message.action.cancel.maintenance": "Your host has been successfully canceled for maintenance. This process can take up to several minutes.", "message.action.cancel.maintenance.mode": "Please confirm that you want to cancel this maintenance.", "message.action.create.snapshot.from.vmsnapshot": "Please confirm that you want to create Snapshot from Instance Snapshot", +"message.action.vmsnapshot.create": "Please confirm that you want to create an Instance Snapshot.
Running KVM UEFI disk-only snapshots briefly suspend the Instance while NVRAM state is copied.", "message.action.create.instance.from.backup": "Please confirm that you want to create a new Instance from the given Backup.
Click on configure to edit the parameters for the new Instance before creation.", "message.create.instance.from.backup.different.zone": "Creating Instance from Backup on a different Zone. Please ensure that the backup repository is accessible in the selected Zone.", "message.csv.empty": "Empty CSV File", diff --git a/ui/src/config/section/compute.js b/ui/src/config/section/compute.js index 63d0e365db92..ab4a58bb8649 100644 --- a/ui/src/config/section/compute.js +++ b/ui/src/config/section/compute.js @@ -190,6 +190,7 @@ export default { api: 'createVMSnapshot', icon: 'camera-outlined', label: 'label.action.vmsnapshot.create', + message: 'message.action.vmsnapshot.create', docHelp: 'adminguide/virtual_machines.html#virtual-machine-snapshots', dataView: true, args: (record, store) => { diff --git a/ui/src/config/section/network.js b/ui/src/config/section/network.js index acc7424c9f0c..863403f14ae0 100644 --- a/ui/src/config/section/network.js +++ b/ui/src/config/section/network.js @@ -503,6 +503,7 @@ export default { api: 'createVMSnapshot', icon: 'camera-outlined', label: 'label.action.vmsnapshot.create', + message: 'message.action.vmsnapshot.create', docHelp: 'adminguide/virtual_machines.html#virtual-machine-snapshots', dataView: true, args: ['virtualmachineid', 'name', 'description', 'snapshotmemory', 'quiescevm'], From 414d9c16aba8382842f6eff6b55f2e773d9fd1fa Mon Sep 17 00:00:00 2001 From: "kunal.behbudzade" Date: Mon, 4 May 2026 20:41:12 +0300 Subject: [PATCH 7/7] 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. --- ...KvmFileBasedStorageVmSnapshotStrategy.java | 4 +-- ...ileBasedStorageVmSnapshotStrategyTest.java | 32 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java index e946598d8ae0..79e2a9722039 100644 --- a/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java +++ b/engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java @@ -522,13 +522,13 @@ protected VMSnapshot takeVmSnapshotInternal(VMSnapshot vmSnapshot, Map volumeTOs = vmSnapshotHelper.getVolumeTOList(userVm.getId()); - transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.CreateRequested); - VMSnapshotTO parentSnapshotTo = null; VMSnapshotVO parentSnapshotVo = vmSnapshotDao.findCurrentSnapshotByVmId(userVm.getId()); if (parentSnapshotVo != null) { diff --git a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java b/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java index 053fff844aa1..35b1f9ff65bf 100644 --- a/engine/storage/snapshot/src/test/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategyTest.java +++ b/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;