Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix extract snapshot from vm snapshot on kvm #6422

Merged
merged 2 commits into from May 31, 2022
Merged

Fix extract snapshot from vm snapshot on kvm #6422

merged 2 commits into from May 31, 2022

Conversation

GutoVeronezi
Copy link
Contributor

Description

PR #5297 introduced the disk-only snapshot feature for KVM, which changed the whole snapshot workflow for KVM. While taking a volume snapshot and the global setting snapshot.backup.to.secondary is true, ACS will backup the snapshot from the primary storage to the secondary storage. As the disk-only snapshot process generates an external file, the backup process only copies it to the secondary storage.
The extract snapshot from VM snapshot feature uses the same backup workflow to extract the snapshot from the VM directly to the secondary storage. However, as the process was changed to only copy the file, the following error occurs when trying to extract a snapshot from a VM snapshot:

2022-05-30 11:55:19,152 DEBUG [kvm.storage.KVMStorageProcessor] (agentRequest-Handler-2:null) (logid:565d35b1) Executing: /usr/share/cloudstack-common/scripts/storage/qcow2/managesnapshot.sh -b i-2-5-VM_VS_20220530114135 -n i-2-5-VM_VS_20220530114135 -p /mnt/3ac1f06a-3f50-3e03-b53e-92faead36a85/snapshots/2/5 -t 248e458e-f2c5-48c3-86bc-5201104a5832 
2022-05-30 11:55:19,155 DEBUG [kvm.storage.KVMStorageProcessor] (agentRequest-Handler-2:null) (logid:565d35b1) Executing while with timeout : 21600000
2022-05-30 11:55:19,167 DEBUG [kvm.storage.KVMStorageProcessor] (agentRequest-Handler-2:null) (logid:565d35b1) Exit value is 3
2022-05-30 11:55:19,168 DEBUG [kvm.storage.KVMStorageProcessor] (agentRequest-Handler-2:null) (logid:565d35b1) ***Failed to backup snapshot i-2-5-VM_VS_20220530114135, undefined type i-2-5-VM_VS_20220530114135

As the VM snapshot is internal, the backup workflow has to be different. This PR address the fix for the situation, by using the previous backup workflow when a snapshot is being extracted from a VM snapshot.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

How Has This Been Tested?

In a local lab I created a VM and took a VM snapshot. With the changes I could extract the VM snapshot and do other operations with it (create volume, create template and so on).
I also took a volume snapshot and tested the operations (create volume, create template and so on).

@nvazquez nvazquez added this to the 4.17.0.0 milestone May 30, 2022
@nvazquez
Copy link
Contributor

@blueorangutan package

@nvazquez nvazquez linked an issue May 30, 2022 that may be closed by this pull request
KVM: Create snapshot from VM snapshot is not working on 4.17 RC3 #6419
Closed
@nvazquez
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@nvazquez a 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.

weizhouapache
weizhouapache reviewed May 30, 2022
View reviewed changes
scripts/storage/qcow2/managesnapshot.sh
fi
else
# Backup VM snapshot
qemuimg_ret=$($qemu_img snapshot $forceShareFlag -l $disk 2>&1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GutoVeronezi
can this be replaced with qemu-img -V ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weizhouapache, as this is a fix of PR #5297 (specifically this commit: f5180dd), and I'm only returning the removed code to VM snapshots, I'd rather not refactoring anything right now.

In other hand, this entire script could be removed and the workflow implemented via Java, with documentation and unit tests. We could create an issue to do this in the future. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GutoVeronezi
looks good to me. let's fix the issue now and refactor in next releases.

@sonarcloud
Copy link

sonarcloud bot commented May 30, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@blueorangutan
Copy link

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3502

@nvazquez
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

nvazquez
nvazquez approved these changes May 30, 2022
View reviewed changes
Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - manually tested on KVM+CentOS7

weizhouapache
weizhouapache approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@GutoVeronezi GutoVeronezi mentioned this pull request May 30, 2022
Open
@blueorangutan
Copy link

Trillian test result (tid-4265)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45985 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6422-t4265-kvm-centos7.zip
Smoke tests completed. 96 look OK, 1 have errors
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_1_create_iso_with_checksum_sha1_negative Error 133.71 test_iso.py
test_01_create_iso_with_checksum_sha1 Error 66.39 test_iso.py
test_02_create_iso_with_checksum_sha256 Error 66.43 test_iso.py
test_03_create_iso_with_checksum_md5 Error 66.43 test_iso.py
test_04_create_iso_with_no_checksum Error 66.45 test_iso.py
test_01_create_iso Failure 1513.49 test_iso.py
ContextSuite context=TestISO>:setup Error 3025.85 test_iso.py

slavkap
slavkap approved these changes May 31, 2022
View reviewed changes
Copy link
Contributor

@slavkap slavkap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, manually tested on Rocky

@nvazquez nvazquez merged commit 81b7e6e into apache:main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvazquez nvazquez nvazquez approved these changes

@slavkap slavkap slavkap approved these changes

@weizhouapache weizhouapache weizhouapache approved these changes

@Pearl1594 Pearl1594 Awaiting requested review from Pearl1594

@vladimirpetrov vladimirpetrov Awaiting requested review from vladimirpetrov

Assignees
No one assigned
Labels
component:kvm component:storage type:bug type:regression
Projects
No open projects
Apache CloudStack 4.17.0
Status: Done
Milestone
4.17.0.0
Development

Successfully merging this pull request may close these issues.

KVM: Create snapshot from VM snapshot is not working on 4.17 RC3
5 participants
@GutoVeronezi @nvazquez @blueorangutan @slavkap @weizhouapache