Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3143623866
##########
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:
Removed this helper and updated the test to call the freeze/verify methods
directly.
##########
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:
The freeze and suspend cover different parts of the consistency window. The
guest-agent freeze flushes/quiesces guest filesystems, while suspend prevents
concurrent pflash/NVRAM writes while the sidecar is copied. Without the
suspend, NVRAM can change during the copy and no longer match the disk
snapshot. I added the API and UI warning for this behavior.
--
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]
