You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "Kunalbehbud (via GitHub)" <gi...@apache.org> on 2026/04/14 11:44:05 UTC
[PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Kunalbehbud opened a new pull request, #13020:
URL: https://github.com/apache/cloudstack/pull/13020
### Description
KVM disk-only instance snapshots do not capture the active UEFI NVRAM state, which makes revert unsafe for UEFI guests.
This PR fixes the KVM disk-only instance snapshot flow to:
- plumb optional NVRAM sidecar metadata through the management and KVM agent commands
- copy the active UEFI NVRAM file during snapshot creation and restore it during revert
- clean up the NVRAM sidecar during delete and merge flows
- gate create, revert, and sidecar cleanup on host UEFI/NVRAM capabilities and clear stale capability details on reconnect
- suspend UEFI guests before copying NVRAM, freezing filesystems first when `quiescevm=true`
- preserve successful snapshot metadata if post-snapshot thaw/resume fails, surfacing the issue via warning/alert instead of discarding the snapshot
- reject reverting UEFI disk-only snapshots that do not contain NVRAM state
Older UEFI disk-only snapshots created without an NVRAM sidecar are intentionally rejected on revert.
This PR only covers `disk-only` instance snapshots for KVM UEFI VMs. `snapshotMemory=true` / internal snapshots remain out of scope.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
N/A
### How Has This Been Tested?
- `mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`
- Live-tested on a 4.22.1 KVM environment with a UEFI VM:
- created a disk-only instance snapshot
- reverted the snapshot and booted the VM successfully
- deleted a parent snapshot and verified the merge path removed the old NVRAM sidecar
- corrupted the active NVRAM after the merge, reverted again, verified the active NVRAM checksum matched the snapshot sidecar before boot, and booted successfully
#### How did you try to break this feature and the system with this change?
- tried reverting a UEFI disk-only snapshot without an NVRAM sidecar and verified the revert is rejected
- covered stale host capability details on reconnect
- covered the case where freeze verification fails after a successful freeze and verified cleanup still thaws the guest
- covered post-snapshot thaw/resume failures and verified the snapshot metadata is preserved while the warning/alert path is exercised
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3169412423
##########
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:
Just so I understand 100%. You added this to cover for the case where a part of the hosts in the environment is in a older ACS version that does not have the NVRAM flow implemented?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4362647126
<b>[SF] Trillian test result (tid-15999)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 54968 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13020-t15999-kvm-ol8.zip
Smoke tests completed. 147 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_03_ssvm_internals | `Failure` | 3.44 | test_ssvm.py
test_01_redundant_vpc_site2site_vpn | `Failure` | 407.80 | test_vpc_vpn.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5620085319
@shwstppr done. I think it is ok but I'll eventually test this to make sure it is actually ok.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4358299130
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3172806707
##########
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:
Yes, mainly for rolling upgrade cases. Older KVM agents will not know how to handle the NVRAM sidecar fields, so management should only use this flow on hosts that explicitly advertise support for it. I did not make it configurable because this is not really an operator choice that it reflects whether the agent code on that host supports the flow. The reconnect handling also removes the detail again if a host comes back without advertising it.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4243901151
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5603500996
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4358845739
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3169386737
##########
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:
Understood, thanks
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4377386702
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5566986980
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5567876734
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19119
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3169414453
##########
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:
Thanks
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4358843351
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4246020027
Rebased this branch on the latest `4.22` to resolve the merge conflict in `AgentManagerImpl` after the recent VDDK host detail changes.
Local verification passed again on top of the updated base:
`mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`
The new GitHub Actions runs for this fork push are currently in `action_required`, so they appear to be waiting for maintainer approval before re-running.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5567056245
@Kunalbehbud , I rebased your PR because of some conflicts. It looks good to me.
Just these co-pilot comments,
https://github.com/apache/cloudstack/pull/13020#discussion_r3118131199 and https://github.com/apache/cloudstack/pull/13020#discussion_r3118131272, I’d like to see a rebuttal to. The third one (https://github.com/apache/cloudstack/pull/13020#discussion_r3118131314) is rather trivial, apply or deny, but please address it.
And as I was writing this some new co-pilot comments came in, please address.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5568966280
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5566947657
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5579410951
<b>[SF] Trillian test result (tid-16915)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 63412 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13020-t16915-kvm-ol8.zip
Smoke tests completed. 144 look OK, 5 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
ContextSuite context=TestVMDeploymentPlannerStrictTags>:teardown | `Error` | 573.77 | test_vm_strict_host_tags.py
test_01_verify_ipv6_vpc | `Failure` | 67.26 | test_vpc_ipv6.py
ContextSuite context=TestVPCRedundancy>:setup | `Error` | 0.00 | test_vpc_redundant.py
ContextSuite context=TestVPCNics>:setup | `Error` | 0.00 | test_vpc_router_nics.py
ContextSuite context=TestRVPCSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVPCSite2SiteVPNMultipleOptions>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcRemoteAccessVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
ContextSuite context=TestVpcSite2SiteVpn>:setup | `Error` | 0.00 | test_vpc_vpn.py
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4281102283
Latest head is rebased cleanly on `4.22` and carries the same patchset without the GitHub `Update branch` merge commit.\n\nLocal verification passed again:\n`mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`\n\nHowever, no GitHub checks are attaching to the current head yet. Could a maintainer please re-run/approve the workflows for this PR head?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4281006900
Replaced the GitHub `Update branch` merge commit with an equivalent clean rebase on the latest `4.22` to keep the patchset linear. No functional change relative to the previous head; the rebased branch carries the same five commits. Local verification passed again on the rebased head: `mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4376665471
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17712
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4322216229
Pushed a follow-up commit for the review feedback.
The main change is that stopped UEFI disk-only snapshot creation no longer falls back away from the VM's last host. The active NVRAM file is host-local, so if the selected host is not the last host we fail early instead of risking a stale or missing NVRAM sidecar. Capability-aware host selection is still used for revert and sidecar cleanup.
I also removed the duplicate sidecar cleanup validation and the test-only freeze helper, handled QEMU guest-agent error payloads in freeze/thaw paths, and added the API/UI warning for the brief suspend used for running KVM UEFI snapshots.
Verified locally:
`mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3143622506
##########
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:
Removed the duplicate validation from deleteNvramSnapshotIfNeeded; callers validate before invoking it
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4373154768
Thanks for testing it. You were right, the validation was happening before the VM snapshot left `Allocated`, so the failure path could not move it to `Error`.
I pushed `414d9c1` to move the create transition before host capability validation. The unsupported-host case still fails before any agent command is sent, but now the VM snapshot can be marked failed instead of being left in `Allocated`. I also added a regression test for that path.
Local verification:
`mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4376439583
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3143614294
##########
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:
Kept this intentionally. For KVM UEFI guests, the active pflash/NVRAM file is host-local. Revert restores the snapshot sidecar into the active NVRAM path on the host executing the revert, so lastHostId needs to point to that host for the next start to use the restored firmware state. The update is now conditional to avoid an unnecessary DB write when it is already the same host.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3143618719
##########
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:
Agreed. Host selection now looks for the required capabilities before sending the command. One nuance is stopped UEFI snapshot creation does not fall back away from the VM's last host, because the active NVRAM file is host-local. Revert and sidecar cleanup can still use a capable fallback host.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5594488707
<b>[SF] Trillian test result (tid-16927)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 49580 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13020-t16927-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4968768816
Could a committer help get this one over the line when convenient? It's approved by @JoaoJandre and the last smoke run (tid-16018) came back 149/149 on `414d9c1`, and it's already milestoned for 4.22.2. It was briefly labeled `ready-for-merge` before going back to `needs-review`, so I think it's just waiting on a second look. The only red check is the `triage` merge-conflict check, which looks stale since GitHub still reports the branch as mergeable — happy to rebase to force a re-run if that clears it.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4376437512
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4243946936
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/13020?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.70%. Comparing base ([`d75acb6`](https://app.codecov.io/gh/apache/cloudstack/commit/d75acb6efcc2f80b878248b7d125102612eed7b2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f657823`](https://app.codecov.io/gh/apache/cloudstack/commit/f657823afb6b639c183e50b89670e16ce78b48ab?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 29 commits behind head on 4.22.
> :exclamation: There is a different number of reports uploaded between BASE (d75acb6) and HEAD (f657823). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (d75acb6) | HEAD (f657823) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.22 #13020 +/- ##
=============================================
- Coverage 17.61% 3.70% -13.91%
=============================================
Files 5919 448 -5471
Lines 532005 38052 -493953
Branches 65057 7046 -58011
=============================================
- Hits 93696 1409 -92287
+ Misses 427746 36456 -391290
+ Partials 10563 187 -10376
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/13020/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/13020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.70% <ø> (-0.01%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/13020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/13020?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4354359226
@Kunalbehbud thanks for the update, I'll try to test this manually when I can.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5568980119
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5618773442
@JoaoJandre can you please help with the forward merge of `4.22` branch into `main`? Changes from here conflict with KBOSS changes in `main`, and I don't want to break stuff
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4268025196
Addressed the Copilot comment in 142d116. `verifyVmFilesystemsFrozen` now parses the guest-agent freeze status defensively: the `JsonParser` call is wrapped in a try/catch for `RuntimeException` and rethrown as `IOException` so the outer `catch (LibvirtException | IOException)` still runs the thaw/resume path in `finally`, `{"error": {...}}` responses are treated as a freeze failure, and the `return` field is validated as a `JsonPrimitive` string before `getAsString()` is called.
Added two regression tests (`testVerifyVmFilesystemsFrozenFailsForQemuErrorResponse` and `testTakeDiskOnlyVmSnapshotOfRunningVmReturnsFailureAnswerWhenFreezeStatusJsonIsMalformed`) covering the error-payload and malformed-JSON cases. Both assert the thaw recovery still fires on the failure path.
Also updated `PendingReleaseNotes`. The fix path briefly suspends UEFI guests while the NVRAM sidecar is copied, which is a user-visible behavior change that was missing from the original entry. Non-UEFI VMs are unaffected.
Local verification on top of 142d116 passed: `mvn -pl engine/orchestration,engine/storage/snapshot,plugins/hypervisors/kvm -am -Dtest=AgentManagerImplTest,KvmFileBasedStorageVmSnapshotStrategyTest,LibvirtDiskOnlyVMSnapshotCommandWrapperTest -Dsurefire.failIfNoSpecifiedTests=false test`
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Kunalbehbud (via GitHub)" <gi...@apache.org>.
Kunalbehbud commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3143624307
##########
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:
I kept this as an advertised agent capability rather than a config option. It reflects whether the KVM agent version understands the NVRAM sidecar flow. Making it configurable could let management send these commands to an agent that does not actually support them, which is unsafe during rolling upgrades.
##########
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:
added this to the createVMSnapshot API description and to the UI confirmation message for the Instance Snapshot action.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4377384488
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4243895023
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-5618822434
> @JoaoJandre can you please help with the forward merge of `4.22` branch into `main`? Changes from here conflict with KBOSS changes in `main`, and I don't want to break stuff
@shwstppr sure, I'll do it in a moment
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4244451393
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17485
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4245906631
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4358294291
@blueorangutan package
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4358445930
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17680
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4383653430
<b>[SF] Trillian test result (tid-16018)</b>
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 50367 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr13020-t16018-kvm-ol8.zip
Smoke tests completed. 149 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#issuecomment-4372673196
I was able to do some tests. I'm not versed in how the UEFI handling works and do not use UEFI in general, so my tests were focused on the VM snapshot workflows. I'm sure there could be some UEFI related tests to be done, but I'll leave it to someone else.
| Test | Result | Expected? |
| --- | --- | --- |
| Created a VM in a host with UEFI but withtout the patch. Tried to create a VM snapshot | Error | Y |
| Migrated said VM to a host with the patch and created VM snapshots s1 and s2 | Success | Y |
| Restored to snapshot s1 and checked files | Success | Y |
| Restored to snapshot s2 and checked files | Success | Y |
| Deleted snapshots s1 and s2 | Success, and nvram files deleted | Y |
I redid the tests with a stopped VM and obtained the same results.
Regarding the first test:
If you try to take a VM snapshot of a VM with UEFI in a host that does not have the patch applied, the VM snapshot will fail (as expected), but it will be in allocated state, meaning it is impossible to remove. You should put the snapshot in the `Error` state.
To fix this, I think you could move the host selection and validation to after the `transitStateWithoutThrow(vmSnapshot, VMSnapshot.Event.CreateRequested);` line in the `takeVmSnapshotInternal` Method. The current problem is that this exception is thrown:
```
Failed to create Instance Snapshot: [VMSnapshot {"id":22,"name":"i-2-382-VM_VS_20260504134423","uuid":"5236dc5e-795f-4284-81a1-56b1102c1662","vmId":382}] due to: Unable to transition to a new state from Allocated via OperationFailed com.cloud.utils.exception.CloudRuntimeException: Unable to transition to a new state from Allocated via OperationFailed
```
Could you check this @Kunalbehbud ?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3947534807
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -53,6 +57,40 @@ public Answer execute(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingR
return new Answer(command, e);
}
}
+
+ deleteNvramSnapshotIfNeeded(command, resource, storagePoolMgr, snapshotsToDelete);
return new Answer(command, true, null);
}
+
+ protected void deleteNvramSnapshotIfNeeded(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingResource resource, KVMStoragePoolManager storagePoolMgr,
+ List<DataTO> snapshotsToDelete) {
+ if (StringUtils.isBlank(command.getNvramSnapshotPath())) {
+ return;
+ }
+
+ try {
+ KVMStoragePool storagePool;
+ if (command.getPrimaryDataStore() != null) {
+ PrimaryDataStoreTO dataStore = command.getPrimaryDataStore();
+ storagePool = storagePoolMgr.getStoragePool(dataStore.getPoolType(), dataStore.getUuid());
+ } else {
+ SnapshotObjectTO rootVolumeSnapshot = snapshotsToDelete.stream()
+ .map(SnapshotObjectTO.class::cast)
+ .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType()))
+ .findFirst()
+ .orElse(null);
+
+ if (rootVolumeSnapshot == null) {
+ logger.warn("Unable to locate the root volume snapshot while deleting NVRAM snapshot [{}].", command.getNvramSnapshotPath());
+ return;
+ }
+
+ storagePool = resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot, storagePoolMgr);
+ }
+
+ Files.deleteIfExists(Path.of(storagePool.getLocalPathFor(command.getNvramSnapshotPath())));
Review Comment:
NVRAM sidecar deletion uses storagePool.getLocalPathFor(command.getNvramSnapshotPath()) without validating that the provided path is a safe relative path. Because getLocalPathFor() is a simple string concatenation, a crafted value containing ".." or an absolute path could lead to deleting files outside the pool mount.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRevertDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -108,4 +116,88 @@ public Answer execute(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResou
return new RevertDiskOnlyVmSnapshotAnswer(cmd, volumeObjectTos);
}
+
+ protected SnapshotObjectTO getRootVolumeSnapshot(List<SnapshotObjectTO> snapshotObjectTos) {
+ return snapshotObjectTos.stream()
+ .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType()))
+ .findFirst()
+ .orElse(null);
+ }
+
+ protected void validateNvramRevertState(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot,
+ KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException {
+ String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid());
+ if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) {
+ if (cmd.isUefiEnabled()) {
+ throw new IOException(String.format("Cannot safely revert disk-only VM snapshot for UEFI VM [%s] because the snapshot does not contain NVRAM state.",
+ cmd.getVmName()));
+ }
+ return;
+ }
+
+ if (StringUtils.isBlank(activeNvramPath)) {
+ throw new IOException(String.format("Unable to determine the active UEFI NVRAM path for VM [%s].", cmd.getVmName()));
+ }
+
+ Path snapshotNvramPath = getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot, resource, storagePoolMgr);
+ if (!Files.exists(snapshotNvramPath)) {
+ throw new IOException(String.format("Unable to find the UEFI NVRAM snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName()));
+ }
+ }
+
+ protected void restoreNvramIfNeeded(RevertDiskOnlyVmSnapshotCommand cmd, LibvirtComputingResource resource, SnapshotObjectTO rootVolumeSnapshot,
+ KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException {
+ if (StringUtils.isBlank(cmd.getNvramSnapshotPath())) {
+ return;
+ }
+
+ String activeNvramPath = resource.getUefiNvramPath(cmd.getVmUuid());
+ if (StringUtils.isBlank(activeNvramPath)) {
+ throw new IOException(String.format("Unable to determine the active UEFI NVRAM path for VM [%s].", cmd.getVmName()));
+ }
+
+ Path snapshotNvramPath = getNvramSnapshotAbsolutePath(cmd.getNvramSnapshotPath(), rootVolumeSnapshot, resource, storagePoolMgr);
+ if (!Files.exists(snapshotNvramPath)) {
+ throw new IOException(String.format("Unable to find the UEFI NVRAM snapshot [%s] for VM [%s].", cmd.getNvramSnapshotPath(), cmd.getVmName()));
+ }
+
+ replaceNvramAtomically(snapshotNvramPath, Path.of(activeNvramPath));
+ }
+
+ protected void replaceNvramAtomically(Path snapshotNvramPath, Path activeNvramPath) throws IOException {
+ Path targetDirectory = activeNvramPath.getParent();
+ if (targetDirectory != null) {
+ Files.createDirectories(targetDirectory);
+ }
+
+ Path temporaryNvramPath = Files.createTempFile(targetDirectory, activeNvramPath.getFileName().toString(), ".tmp");
+ try {
+ copyNvramSnapshotToTemporaryPath(snapshotNvramPath, temporaryNvramPath);
+ moveTemporaryNvramIntoPlace(temporaryNvramPath, activeNvramPath);
+ } finally {
+ Files.deleteIfExists(temporaryNvramPath);
+ }
+ }
+
+ protected void copyNvramSnapshotToTemporaryPath(Path snapshotNvramPath, Path temporaryNvramPath) throws IOException {
+ Files.copy(snapshotNvramPath, temporaryNvramPath, StandardCopyOption.REPLACE_EXISTING);
+ }
+
+ protected void moveTemporaryNvramIntoPlace(Path temporaryNvramPath, Path activeNvramPath) throws IOException {
+ try {
+ Files.move(temporaryNvramPath, activeNvramPath, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+ } catch (AtomicMoveNotSupportedException e) {
+ Files.move(temporaryNvramPath, activeNvramPath, StandardCopyOption.REPLACE_EXISTING);
+ }
+ }
+
+ protected Path getNvramSnapshotAbsolutePath(String nvramSnapshotPath, SnapshotObjectTO rootVolumeSnapshot, LibvirtComputingResource resource,
+ KVMStoragePoolManager storagePoolMgr) throws IOException, LibvirtException {
+ if (rootVolumeSnapshot == null) {
+ throw new IOException("Unable to locate the root volume snapshot while handling the UEFI NVRAM state.");
+ }
+
+ KVMStoragePool storagePool = resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot, storagePoolMgr);
+ return Path.of(storagePool.getLocalPathFor(nvramSnapshotPath));
+ }
Review Comment:
getNvramSnapshotAbsolutePath() passes nvramSnapshotPath directly into storagePool.getLocalPathFor(). Since getLocalPathFor() does not sanitize traversal (it concatenates strings), an unexpected nvramSnapshotPath containing ".." or an absolute path could escape the pool mount and read/overwrite arbitrary files when restoring NVRAM.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtCreateDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -88,44 +103,71 @@ 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)) {
+ // The guest-agent freeze flushes guest filesystems; suspend below prevents concurrent UEFI NVRAM writes.
+ freezeVmFilesystems(dm, vmName);
+ filesystemsFrozenByThisWrapper = true;
+ verifyVmFilesystemsFrozen(dm, vmName);
+ }
+ if (shouldSuspendVmForSnapshot(cmd)) {
+ suspendedByThisWrapper = suspendVmIfNeeded(dm);
+ }
+ nvramSnapshotPath = backupNvramIfNeeded(cmd, resource);
- dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(), getFlagsToUseForRunningVmSnapshotCreation(target));
+ dm.snapshotCreateXML(snapshotXmlAndVolumeToNewPathMap.first(), getFlagsToUseForRunningVmSnapshotCreation(target, filesystemsFrozenByThisWrapper));
- return new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, snapshotXmlAndVolumeToNewPathMap.second());
- } catch (LibvirtException e) {
+ postSnapshotCleanupIssue = recoverVmAfterSnapshot(dm, vmName, suspendedByThisWrapper, filesystemsFrozenByThisWrapper, postSnapshotCleanupIssue);
+ filesystemsFrozenByThisWrapper = false;
+ suspendedByThisWrapper = false;
+
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, true, null, snapshotXmlAndVolumeToNewPathMap.second(), nvramSnapshotPath);
+ } catch (LibvirtException | IOException e) {
String errorMsg = String.format("Creation of disk-only VM snapshot for VM [%s] failed due to %s.", vmName, e.getMessage());
logger.error(errorMsg, e);
- if (e.getMessage().contains("QEMU guest agent is not connected")) {
+ cleanupNvramSnapshotIfNeeded(cmd, resource, nvramSnapshotPath);
+ if (StringUtils.contains(e.getMessage(), "QEMU guest agent is not connected")) {
errorMsg = "QEMU guest agent is not connected. If the VM has been recently started, it might connect soon. Otherwise the VM does not have the" +
" guest agent installed; thus the QuiesceVM parameter is not supported.";
- return new CreateDiskOnlyVmSnapshotAnswer(cmd, false, errorMsg, null);
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false, errorMsg, null, null);
+ } else {
+ answer = new CreateDiskOnlyVmSnapshotAnswer(cmd, false, e.getMessage(), null, null);
Review Comment:
In the non-guest-agent error path, the failure Answer uses e.getMessage() directly. This can be null and also drops the VM context that was already assembled into errorMsg, making failures harder to diagnose from the management server/UI.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -6334,6 +6336,15 @@ public String getSnapshotTemporaryPath(String diskPath, String snapshotName) {
return String.join(File.separator, diskPathSplitted);
}
+ public String getUefiNvramPath(String vmUuid) {
+ String nvramDirectory = uefiProperties.getProperty(LibvirtVMDef.GuestDef.GUEST_NVRAM_PATH);
+ if (StringUtils.isBlank(nvramDirectory) || StringUtils.isBlank(vmUuid)) {
+ return null;
+ }
+
+ return nvramDirectory + vmUuid + ".fd";
Review Comment:
getUefiNvramPath() builds the NVRAM file path via string concatenation (nvramDirectory + vmUuid + ".fd"), which silently breaks if the configured directory does not end with a path separator. Using Path joining avoids relying on trailing slashes and keeps path handling consistent.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KVM: fix UEFI disk-only instance snapshot NVRAM handling [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #13020:
URL: https://github.com/apache/cloudstack/pull/13020#discussion_r3958188774
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteDiskOnlyVMSnapshotCommandWrapper.java:
##########
@@ -53,6 +57,40 @@ public Answer execute(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingR
return new Answer(command, e);
}
}
+
+ deleteNvramSnapshotIfNeeded(command, resource, storagePoolMgr, snapshotsToDelete);
return new Answer(command, true, null);
}
+
+ protected void deleteNvramSnapshotIfNeeded(DeleteDiskOnlyVmSnapshotCommand command, LibvirtComputingResource resource, KVMStoragePoolManager storagePoolMgr,
+ List<DataTO> snapshotsToDelete) {
+ if (StringUtils.isBlank(command.getNvramSnapshotPath())) {
+ return;
+ }
+
+ try {
+ KVMStoragePool storagePool;
+ if (command.getPrimaryDataStore() != null) {
+ PrimaryDataStoreTO dataStore = command.getPrimaryDataStore();
+ storagePool = storagePoolMgr.getStoragePool(dataStore.getPoolType(), dataStore.getUuid());
+ } else {
+ SnapshotObjectTO rootVolumeSnapshot = snapshotsToDelete.stream()
+ .map(SnapshotObjectTO.class::cast)
+ .filter(snapshotObjectTO -> Volume.Type.ROOT.equals(snapshotObjectTO.getVolume().getVolumeType()))
+ .findFirst()
+ .orElse(null);
+
+ if (rootVolumeSnapshot == null) {
+ logger.warn("Unable to locate the root volume snapshot while deleting NVRAM snapshot [{}].", command.getNvramSnapshotPath());
+ return;
+ }
+
+ storagePool = resource.getLibvirtUtilitiesHelper().getPrimaryPoolFromDataTo(rootVolumeSnapshot, storagePoolMgr);
+ }
+
+ Files.deleteIfExists(Path.of(storagePool.getLocalPathFor(command.getNvramSnapshotPath())));
Review Comment:
This looks like a valid security concern, @Kunalbehbud . Can you investigate and rebute or apply?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org