Copilot commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3118131272
##########
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 {
+ 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)) {
+ throw new IOException(String.format("Failed to verify VM [%s]
filesystem freeze state before taking the disk-only VM snapshot. Result: %s",
vmName, status));
+ }
+
+ 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));
+ }
+ }
+
+ 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) {
Review Comment:
thawVmFilesystemsIfNeeded() uses the same `result.startsWith("error")`
check, so a JSON QEMU error response (e.g. `{ "error": ... }`) could be treated
as success. That can incorrectly suppress the warning/alert path and leave
guest filesystems frozen. Parse the JSON and treat responses containing an
`error` field (or lacking an expected `return`) as a thaw failure.
##########
engine/storage/snapshot/src/main/java/org/apache/cloudstack/storage/vmsnapshot/KvmFileBasedStorageVmSnapshotStrategy.java:
##########
@@ -651,6 +714,101 @@ private long getVMSnapshotRealSize(VMSnapshotVO
vmSnapshot) {
return realSize;
}
+ protected boolean isUefiVm(UserVm userVm) {
+ return vmInstanceDetailsDao.findDetail(userVm.getId(),
ApiConstants.BootType.UEFI.toString()) != null;
+ }
+
+ protected PrimaryDataStoreTO
getRootVolumePrimaryDataStore(List<VolumeObjectTO> 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<VolumeObjectTO> 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<VolumeObjectTO> 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",
Review Comment:
notifyGuestRecoveryIssueIfNeeded() hard-codes "for UEFI VM" in the alert
message, but the method is invoked unconditionally after snapshot creation. If
`answer.getDetails()` is ever populated for a non-UEFI snapshot path, the alert
text will be misleading. Consider either gating this method on
`isUefiVm(userVm)` or making the alert text generic (or include boot type
conditionally).
```suggestion
String message = String.format("Disk-only VM snapshot [%s] for VM
[%s] completed, but post-snapshot guest recovery reported: %s",
```
##########
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 {
+ 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));
+ }
Review Comment:
freezeVmFilesystems() treats QEMU guest agent failures as only `null/blank`
or `result.startsWith("error")`. qemuAgentCommand typically returns JSON like
`{ "error": ... }` on failure, which will not be caught here and may allow the
snapshot flow to continue as if the freeze succeeded. Consider parsing the JSON
response and failing when it contains an `error` object (or reuse the existing
FreezeThawVM logic of checking `guest-fsfreeze-status` after the freeze).
```suggestion
}
try {
JsonElement resultElement = new JsonParser().parse(result);
if (resultElement.isJsonObject()) {
JsonObject resultObject = resultElement.getAsJsonObject();
if (resultObject.has("error")) {
throw new IOException(String.format("Failed to freeze VM
[%s] filesystems before taking the disk-only VM snapshot. Result: %s", vmName,
result));
}
}
} catch (IOException e) {
throw e;
} catch (RuntimeException e) {
// Keep existing behavior for non-JSON successful responses
while still failing on JSON error payloads.
}
```
--
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]
