storage: allow VM snapshots without memory for KVM when global setting allows by yadvr · Pull Request #8062 · apache/cloudstack

yadvr · Oct 10, 2023

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

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):

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

However, creating (volume) snapshot from VM snapshot somehow failed for me.
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.

…allows This removes the conditional logic where comment notest to remove it after PR apache#5297 is merged that is 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. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

harikrishna-patnala · Oct 10, 2023

@blueorangutan package

blueorangutan · Oct 10, 2023

@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 · Oct 10, 2023

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 · Oct 10, 2023

@blueorangutan package

blueorangutan · Oct 10, 2023

@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 · Oct 10, 2023

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

codecov · Oct 10, 2023

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 · Oct 10, 2023

@blueorangutan test matrix

blueorangutan · Oct 10, 2023

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

GutoVeronezi · Oct 10, 2023

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

blueorangutan · Oct 10, 2023

[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 · Oct 10, 2023

[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 · Oct 10, 2023

[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 · Oct 11, 2023

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 · Oct 11, 2023

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 · Oct 11, 2023

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 · Oct 12, 2023

@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 · Oct 12, 2023

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 · Oct 12, 2023

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 · Oct 12, 2023

@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 · Oct 12, 2023

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.

…g allows (apache#8062) This removes the conditional logic where comment notest to remove it after PR apache#5297 is merged that is 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. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

…g allows (apache#8062) This removes the conditional logic where comment notest to remove it after PR apache#5297 is merged that is 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. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> (cherry picked from commit 8350ce5) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

yadvr added component:kvm Severity:Major component:storage labels Oct 10, 2023

yadvr added this to the 4.18.2.0 milestone Oct 10, 2023

yadvr requested review from DaanHoogland, GutoVeronezi, andrijapanicsb, harikrishna-patnala, slavkap and weizhouapache October 10, 2023 08:20

Remove unused import

efadbd2

yadvr changed the title ~~storage: allow VM snapshots without disk for KVM when global setting allows~~ storage: allow VM snapshots without memory for KVM when global setting allows Oct 10, 2023

slavkap approved these changes Oct 11, 2023

View reviewed changes

DaanHoogland approved these changes Oct 11, 2023

View reviewed changes

yadvr merged commit 8350ce5 into apache:4.18 Oct 11, 2023

yadvr deleted the vmsnapshot-fix-418 branch October 11, 2023 15:26

slavkap mentioned this pull request Aug 22, 2024

KVM Disk-only VM Snapshots #9524

Closed

Search code, repositories, users, issues, pull requests...

Conversation

yadvr commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Testing

Uh oh!

harikrishna-patnala commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

yadvr commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

codecov bot commented Oct 10, 2023

Codecov Report

Uh oh!

yadvr commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

GutoVeronezi commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

blueorangutan commented Oct 10, 2023

Uh oh!

DaanHoogland commented Oct 11, 2023

Uh oh!

slavkap left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Oct 11, 2023

Uh oh!

yadvr commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrijapanicsb commented Oct 12, 2023

Uh oh!

andrijapanicsb commented Oct 12, 2023

Uh oh!

slavkap commented Oct 12, 2023

Uh oh!

andrijapanicsb commented Oct 12, 2023

Uh oh!

slavkap commented Oct 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yadvr commented Oct 10, 2023 •

edited

Loading

yadvr commented Oct 10, 2023 •

edited

Loading

yadvr commented Oct 11, 2023 •

edited

Loading