Skip to content

Commit 142d116

Browse files
author
kunal.behbudzade
committed
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 <kunal.behbudzade@btsgrp.com>
1 parent 43e9809 commit 142d116

File tree

3 files changed

+103
-2
lines changed

3 files changed

+103
-2
lines changed

PendingReleaseNotes

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,8 @@ example.ver.1 > example.ver.2:
4848
* UEFI disk-only instance snapshots taken before this change do not contain an
4949
NVRAM sidecar and cannot be safely reverted. Take a new snapshot after
5050
upgrading before relying on revert for UEFI VMs.
51+
52+
* Taking a disk-only instance snapshot for KVM UEFI VMs now briefly suspends
53+
the guest while the NVRAM sidecar is copied, so that the captured firmware
54+
state is consistent with the disk snapshot. Non-UEFI VMs are unaffected and
55+
continue to snapshot live.

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
import org.libvirt.Connect;
4242
import org.libvirt.Domain;
4343
import org.libvirt.LibvirtException;
44+
import com.google.gson.JsonElement;
45+
import com.google.gson.JsonObject;
4446
import com.google.gson.JsonParser;
4547

4648
import java.io.File;
@@ -315,11 +317,31 @@ protected void freezeVmFilesystems(Domain domain, String vmName) throws LibvirtE
315317

316318
protected void verifyVmFilesystemsFrozen(Domain domain, String vmName) throws LibvirtException, IOException {
317319
String status = getResultOfQemuCommand(FreezeThawVMCommand.STATUS, domain);
318-
if (StringUtils.isBlank(status) || !new JsonParser().parse(status).isJsonObject()) {
320+
if (StringUtils.isBlank(status)) {
319321
throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status));
320322
}
321323

322-
String statusResult = new JsonParser().parse(status).getAsJsonObject().get("return").getAsString();
324+
JsonObject statusObject;
325+
try {
326+
JsonElement statusElement = new JsonParser().parse(status);
327+
if (!statusElement.isJsonObject()) {
328+
throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status));
329+
}
330+
statusObject = statusElement.getAsJsonObject();
331+
} catch (RuntimeException e) {
332+
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);
333+
}
334+
335+
if (statusObject.has("error")) {
336+
throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status));
337+
}
338+
339+
JsonElement returnElement = statusObject.get("return");
340+
if (returnElement == null || !returnElement.isJsonPrimitive() || !returnElement.getAsJsonPrimitive().isString()) {
341+
throw new IOException(String.format("Failed to verify VM [%s] filesystem freeze state before taking the disk-only VM snapshot. Result: %s", vmName, status));
342+
}
343+
344+
String statusResult = returnElement.getAsString();
323345
if (!FreezeThawVMCommand.FREEZE.equals(statusResult)) {
324346
throw new IOException(String.format("Failed to freeze VM [%s] filesystems before taking the disk-only VM snapshot. Status: %s", vmName, statusResult));
325347
}

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDiskOnlyVMSnapshotCommandWrapperTest.java

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,80 @@ protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) {
469469
assertTrue("Thaw must be attempted after a successful freeze followed by verification failure", thawCalled[0]);
470470
}
471471

472+
@Test
473+
public void testVerifyVmFilesystemsFrozenFailsForQemuErrorResponse() throws Exception {
474+
LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() {
475+
@Override
476+
protected String getResultOfQemuCommand(String cmd, Domain domain) {
477+
return "{\"error\":{\"class\":\"GenericError\",\"desc\":\"guest agent failure\"}}";
478+
}
479+
};
480+
481+
try {
482+
wrapper.verifyVmFilesystemsFrozen(mock(Domain.class), "vm-name");
483+
fail("QEMU guest agent error responses must be treated as IO failures.");
484+
} catch (IOException e) {
485+
assertTrue(e.getMessage().contains("Failed to verify VM [vm-name] filesystem freeze state"));
486+
}
487+
}
488+
489+
@Test
490+
public void testTakeDiskOnlyVmSnapshotOfRunningVmReturnsFailureAnswerWhenFreezeStatusJsonIsMalformed() throws Exception {
491+
final boolean[] thawCalled = {false};
492+
LibvirtCreateDiskOnlyVMSnapshotCommandWrapper wrapper = new LibvirtCreateDiskOnlyVMSnapshotCommandWrapper() {
493+
@Override
494+
protected Pair<String, java.util.Map<String, Pair<Long, String>>> createSnapshotXmlAndNewVolumePathMap(List<VolumeObjectTO> volumeObjectTOS,
495+
List<com.cloud.hypervisor.kvm.resource.LibvirtVMDef.DiskDef> disks, VMSnapshotTO target, LibvirtComputingResource resource) {
496+
return new Pair<>("<domainsnapshot/>", Collections.emptyMap());
497+
}
498+
499+
@Override
500+
protected boolean shouldSuspendVmForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) {
501+
return false;
502+
}
503+
504+
@Override
505+
protected boolean shouldFreezeVmFilesystemsForSnapshot(CreateDiskOnlyVmSnapshotCommand cmd) {
506+
return true;
507+
}
508+
509+
@Override
510+
protected void freezeVmFilesystems(Domain domain, String vmName) {
511+
}
512+
513+
@Override
514+
protected String getResultOfQemuCommand(String cmd, Domain domain) {
515+
return "not-json";
516+
}
517+
518+
@Override
519+
protected boolean thawVmFilesystemsIfNeeded(Domain domain, String vmName) {
520+
thawCalled[0] = true;
521+
return true;
522+
}
523+
};
524+
LibvirtComputingResource resource = mock(LibvirtComputingResource.class);
525+
LibvirtUtilitiesHelper libvirtUtilitiesHelper = mock(LibvirtUtilitiesHelper.class);
526+
org.libvirt.Connect connect = mock(org.libvirt.Connect.class);
527+
Domain domain = mock(Domain.class);
528+
CreateDiskOnlyVmSnapshotCommand command = mock(CreateDiskOnlyVmSnapshotCommand.class);
529+
VMSnapshotTO target = mock(VMSnapshotTO.class);
530+
531+
when(command.getVmName()).thenReturn("vm-name");
532+
when(command.getVolumeTOs()).thenReturn(List.of());
533+
when(command.getTarget()).thenReturn(target);
534+
when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper);
535+
when(libvirtUtilitiesHelper.getConnection()).thenReturn(connect);
536+
when(resource.getDisks(connect, "vm-name")).thenReturn(List.of());
537+
when(resource.getDomain(connect, "vm-name")).thenReturn(domain);
538+
539+
CreateDiskOnlyVmSnapshotAnswer answer = (CreateDiskOnlyVmSnapshotAnswer) wrapper.takeDiskOnlyVmSnapshotOfRunningVm(command, resource);
540+
541+
assertFalse(answer.getResult());
542+
assertTrue(answer.getDetails().contains("Failed to verify VM [vm-name] filesystem freeze state"));
543+
assertTrue("Thaw must be attempted when freeze verification fails on malformed JSON", thawCalled[0]);
544+
}
545+
472546
@Test
473547
public void testTakeDiskOnlyVmSnapshotOfRunningVmSuspendsBeforeNvramCopyForQuiescedUefiSnapshots() throws Exception {
474548
List<String> operations = new ArrayList<>();

0 commit comments

Comments
 (0)