storage: allow VM snapshots without memory for KVM when global setting allows

[yadvr]

This removes the conditional logic which is recommended in comments to remove it after PR #5297 is merged, and that is certainly applicable for ACS 4.18+. Only when the global setting is enabled and memory isn't selected, VM snapshot could be allowed for VMs on KVM that have qemu-guest-agent running.

Refer:
#3724
#5297

Doc improvement PR - apache/cloudstack-documentation#353

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• 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

Testing

1. After applying this fix on a 4.18.1 env, I could take a disk-only snapshots on VM with root/data disks and having qemu-guest-agent installed (and the global setting enabled):

2. On taking a disk-only snapshot of a running VM, I see snapshot (external) files created on the storage directory on the primary storage (tested local storage, but behaviour would be same on nfs as well). In my case there was a root disk and data disk, and therefore two files seen. This is also in the snapshot_store_ref table:

*************************** 36. row ***************************
id: 352
store_id: 17
snapshot_id: 185
created: 2023-10-10 08:51:09
last_updated: NULL
job_id: NULL
store_role: Primary
size: 26843545600
physical_size: 26843545600
parent_snapshot_id: 0
install_path: /var/lib/libvirt/images/snapshots/2013cd98-17c8-4bd0-aeec-a6f97ebacb0b
state: Ready
update_count: 2
ref_cnt: 0
updated: 2023-10-10 08:51:10
volume_id: 758
*************************** 37. row ***************************
id: 353
store_id: 17
snapshot_id: 186
created: 2023-10-10 08:51:10
last_updated: NULL
job_id: NULL
store_role: Primary
size: 53687091200
physical_size: 53687091200
parent_snapshot_id: 0
install_path: /var/lib/libvirt/images/snapshots/8d287129-658f-4f90-82b5-4478b8bc7c65
state: Ready
update_count: 2
ref_cnt: 0
updated: 2023-10-10 08:51:10
volume_id: 759

3. However, creating (volume) snapshot from VM snapshot somehow failed for me.

4. Reverting the snapshot also failed as this was a disk-only snapshot and VM was running. After stopping the VM, reverting the disk-only VM snapshot worked for me.

[harikrishna-patnala]

@blueorangutan package

[blueorangutan]

@harikrishna-patnala a [SF] 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.

[yadvr]

Also worth checking why this test isn't failing (or even running) - https://github.com/apache/cloudstack/blob/main/test/integration/smoke/test_vm_snapshot_kvm.py#L198

[yadvr]

@blueorangutan package

[blueorangutan]

@rohityadavcloud a [SF] 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.

[blueorangutan]

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7297

[codecov]

Codecov Report

Merging #8062 (efadbd2) into 4.18 (29c7b31) will increase coverage by 0.04%.
Report is 68 commits behind head on 4.18.
The diff coverage is 18.30%.

@@ Coverage Diff @@## 4.18 #8062 +/- ##
============================================
+ Coverage 13.02% 13.06% +0.04% - Complexity 9032 9108 +76 
============================================
Files 2720 2720 Lines 257080 257527 +447 Branches 40088 40153 +65 ============================================
+ Hits 33476 33658 +182 - Misses 219400 219639 +239 - Partials 4204 4230 +26 

Files	Coverage Δ
...hestration/service/VolumeOrchestrationService.java	100.00% <ø> (ø)
.../main/java/com/cloud/network/IpAddressManager.java	100.00% <100.00%> (ø)
...ava/com/cloud/network/as/AutoScaleVmProfileVO.java	80.20% <100.00%> (+11.66%)	⬆️
...java/com/cloud/upgrade/DatabaseUpgradeChecker.java	40.89% <100.00%> (+0.64%)	⬆️
...va/com/cloud/upgrade/DatabaseVersionHierarchy.java	85.10% <100.00%> (+1.01%)	⬆️
.../storage/vmsnapshot/StorageVMSnapshotStrategy.java	25.39% <ø> (+0.95%)	⬆️
.../api/command/admin/ratelimit/ResetApiLimitCmd.java	0.00% <ø> (ø)
...oud/hypervisor/kvm/resource/LibvirtConnection.java	0.00% <ø> (ø)
.../hypervisor/kvm/storage/ScaleIOStorageAdaptor.java	10.44% <100.00%> (ø)
...ava/com/cloud/api/commands/StopNetScalerVMCmd.java	0.00% <ø> (ø)
... and 59 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[yadvr]

@blueorangutan test matrix

[blueorangutan]

@rohityadavcloud a [SF] Trillian-Jenkins matrix job (centos7 mgmt + xenserver71, rocky8 mgmt + vmware67u3, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

[GutoVeronezi]

The proposal LGTM, we would only have to validate why creating volume snapshot from VM snapshot is not working.

[blueorangutan]

[SF] Trillian test result (tid-7900)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42722 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8062-t7900-xenserver-71.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_upgrade_kubernetes_cluster	Failure	610.49	test_kubernetes_clusters.py

[blueorangutan]

[SF] Trillian test result (tid-7902)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46428 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8062-t7902-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_08_upgrade_kubernetes_ha_cluster	Failure	3649.83	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	Failure	0.05	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	Error	316.49	test_kubernetes_clusters.py

[blueorangutan]

[SF] Trillian test result (tid-7901)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server r8
Total time taken: 46439 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8062-t7901-vmware-67u3.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[DaanHoogland]

code looks ok, but not sure about the consequences.
also I don't get this description line @rohityadavcloud :

This removes the conditional logic where comment notest to remove it after PR #5297 is merged that is applicable for ACS 4.18+.

especially the where comment notest to remove I suppose there is a typo in that line, but I can't firgure it out.

The proposal LGTM, we would only have to validate why creating volume snapshot from VM snapshot is not working.

@GutoVeronezi , is that pertinent to this change?

[slavkap]

Code LGTM, tested take/revert/delete VM snapshot without memory

[yadvr]

Thanks @slavkap for reviewing and testing

@GutoVeronezi thanks, it might be an env issue in my case, welcome any testing/feedback or changes to this PR

@DaanHoogland yup, my bad - sentence structure was mixed and made some tyP0$; fixed now :)

[yadvr]

I'll go ahead and merge this, and open an issue to track the case of creating (volume) snapshot from VM snapshot issue.
#8078

[andrijapanicsb]

@slavkap do you mind noting here in the PR what is the equivalent of this feature (this is basically enabling your PR #3724 for NFS and Local storage) - if you would be executing it manually via virsh utility?
Greatly appreciated if you can do it.

[andrijapanicsb]

Also @slavkap how do you determine if the Qemu agent is running inside a VM?
Is there a cwiki page for this maybe? Thanks

[slavkap]

Hi @andrijapanicsb, I think @GutoVeronezi probably could help with virsh steps because the NFS/Local storage snapshots come from #5297

From what I got from the code, I think those are the steps for creating a snapshot:

The XML file contains this:

<domainsnapshot><name>%s</name><disks><disk name='%s' snapshot='external'><source file='%s'/></disk>%s</disks>"
+ "</domainsnapshot>"

snapshot-create --domain [domain] --xmlfile [xml] --disk-only

Merging the snapshot:
virsh blockcommit [VM name] [disk label] --base [base file] --active --wait --delete --pivot

About the qemu agent, I've added only this in the docs - Storage-based VM snapshots. There aren't any checks that the qemu-guest-agent for sure works

[andrijapanicsb]

@slavkap thx for the info - however if the Qemu agent is not installed, the snapshot fails gracefully (tested by Rohit) - so some checks seems to be performed... ?

[slavkap]

Yes, the check is only if the guest agent fails to freeze/thaw the VM, but there aren't any special checks if there is an installed guest agent or if it's working.