Copilot commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3084379042
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -195,4 +235,166 @@ 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 {
+ 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));
+ }
Review Comment:
`verifyVmFilesystemsFrozen` assumes the guest-agent response is valid JSON
with a string `return` field. `JsonParser.parse(...)`, `.get("return")`, and
`.getAsString()` can throw unchecked exceptions (e.g.,
JsonSyntaxException/IllegalStateException/NullPointerException) on
malformed/unexpected responses, which will bypass the surrounding `catch
(LibvirtException | IOException)` and can cause the agent command to fail
without returning a proper Answer (and potentially leave the guest frozen until
the `finally` recovery runs). Consider parsing defensively (e.g., catch runtime
JSON parsing exceptions and rethrow as `IOException`), validating the
presence/type of `return`, and treating JSON `error` responses explicitly as
failures.
--
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]
