JoaoJandre commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3137436179
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -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);
+ }
Review Comment:
The last host ID is supposed to be the last host that was chosen to host the
VM. It means that it is likely that it will be chosen again. I'm not sure we
should set it here when reverting the snapshot.
Could you explain the reasoning behind this?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -88,44 +103,70 @@ 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<String, Map<String, Pair<Long, String>>>
snapshotXmlAndVolumeToNewPathMap =
createSnapshotXmlAndNewVolumePathMap(volumeObjectTOS, disks, target, resource);
+ if (shouldFreezeVmFilesystemsForSnapshot(cmd)) {
+ freezeVmFilesystems(dm, vmName);
+ filesystemsFrozenByThisWrapper = true;
+ verifyVmFilesystemsFrozen(dm, vmName);
+ }
+ if (shouldSuspendVmForSnapshot(cmd)) {
Review Comment:
Do we need to freeze the filesystem if the vm is suspended?
Also, if we always need to suspend the VM to create the snapshot when it is
using UEFI, this should be explicitly stated in the API and GUI.
What happens if the VM is not suspended?
##########
PendingReleaseNotes:
##########
@@ -39,3 +39,17 @@ 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.
+
+ * 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.
Review Comment:
This should be warned in the command as well. And GUI.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -195,4 +237,186 @@ protected Pair<String, Map<String, Pair<Long, String>>>
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<VolumeObjectTO>
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 {
Review Comment:
This method is only called in the test class.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -176,6 +198,7 @@ public boolean revertVMSnapshot(VMSnapshot vmSnapshot) {
VMSnapshotVO vmSnapshotBeingReverted = (VMSnapshotVO) vmSnapshot;
Long hostId =
vmSnapshotHelper.pickRunningHost(vmSnapshotBeingReverted.getVmId());
+ validateHostSupportsUefiNvramAwareDiskOnlySnapshots(hostId, userVm,
"revert");
Review Comment:
If we need to validate the host support for restore, we should try to find a
host with this in mind in the first place.
If the VM has no lastHost set, the pickRunningHost method will return a
random host that is up and enabled.
In a scenario where half the hosts have UEFI support and half does not, we
might get an error when trying to revert, followed by a success on the
following attempt. It makes the system seem unstable.
You could either create a new method to pick hosts that have uefi enabled,
or tinker with the existing one, adding a parameter that when set will filter
this on the SQL query.
I would apply this to all processes (create, delete, revert).
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtReadyCommandWrapper.java:
##########
@@ -43,6 +43,7 @@ public final class LibvirtReadyCommandWrapper extends
CommandWrapper<ReadyComman
@Override
public Answer execute(final ReadyCommand command, final
LibvirtComputingResource libvirtComputingResource) {
Map<String, String> hostDetails = new HashMap<String, String>();
+ hostDetails.put(Host.HOST_KVM_DISK_ONLY_VM_SNAPSHOT_NVRAM,
Boolean.TRUE.toString());
Review Comment:
Isn't this supposed to be configurable?
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -368,6 +403,21 @@ private List<SnapshotVO> 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");
Review Comment:
You already call this method everytime before calling
deleteNvramSnapshotIfNeeded, do we need to validate again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
