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.

Sign up for GitHub

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

External snapshot deletion failure modification! #5177

Merged
merged 1 commit into from
Jan 11, 2024
Merged

External snapshot deletion failure modification! #5177

merged 1 commit into from
Jan 11, 2024

Conversation

Anushree-Mathur
Copy link
Contributor

In libvirt version lesser than 9.0.0 external snapshot deletion was unsupported, but the testcase is failing now with the error message as "Snapshot delete succeed but expect fail" because the deletion is supported now with external disk.
For reference : https://libvirt.org/news.html#v9-0-0-2023-01-16
Signed-off-by: Anushree Mathur [email protected]

@Anushree-Mathur
Copy link
Contributor Author

LOGS for Testcase runs:
Before patch

 (2/4) type_specific.io-github-autotest-libvirt.virsh.snapshot_disk.delete_test.negative_test.no_pool.attach_img_qcow2.snapshot_from_xml.disk_external.sys_checkpoint.default.memory_external: STARTED
 (2/4) type_specific.io-github-autotest-libvirt.virsh.snapshot_disk.delete_test.negative_test.no_pool.attach_img_qcow2.snapshot_from_xml.disk_external.sys_checkpoint.default.memory_external: FAIL: Snapshot delete succeed but expect fail. (106.01 s)

After Patch :

 (2/4) type_specific.io-github-autotest-libvirt.virsh.snapshot_disk.delete_test.negative_test.no_pool.attach_img_qcow2.snapshot_from_xml.disk_external.sys_checkpoint.default.memory_external: PASS (87.28 s)

@Anushree-Mathur
External snapshot deletion failure modification!
575cb64 
In libvirt version lesser than 9.0.0 external snapshot deletion was
unsupported, but the testcase is failing now with the error message as
"Snapshot delete succeed but expect fail" because the deletion is
supported now with external disk.
For reference : https://libvirt.org/news.html#v9-0-0-2023-01-16
Signed-off-by: Anushree Mathur <[email protected]>
sacsant
sacsant suggested changes Sep 13, 2023
View reviewed changes
libvirt/tests/src/virsh_cmd/snapshot/virsh_snapshot_disk.py
@@ -439,7 +439,7 @@ def run(test, params, env):
test.fail("Snapshot xml file %s missing"
% snap_xml_path)
else:
if status_error:
if (not libvirt_version.version_compare(9, 0, 0)) and status_error:
Copy link

Choose a reason for hiding this comment

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

Will this check also work for libvirt 9.1.0? Can we make it generic?

if libvirt_version < 9.0.0 and status error
fail(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sacsant It will work for all the libvirt_version because the version compare will take the libvirt version and then check whether it is greater than to the specified libvirt version or not then will return true or false accordingly!

Copy link

Choose a reason for hiding this comment

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

Thanks, then we are good with the proposed changes.

bskjois
bskjois approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@bskjois bskjois left a comment

Choose a reason for hiding this comment

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

LGTM

@Anushree-Mathur
Copy link
Contributor Author

@chunfuwen @dzhengfy Could you review this PR!

@Anushree-Mathur
Copy link
Contributor Author

@chunfuwen @dzhengfy Could you review this PR!
Thanks in advance!

@Anushree-Mathur
Copy link
Contributor Author

@Yingshun @luckyh @yalzhang @chunfuwen @dzhengfy Could you review and merge this PR!
Thanks in advance!

@Anushree-Mathur
Copy link
Contributor Author

@Yingshun @luckyh @yalzhang @chunfuwen @dzhengfy Could you review and merge this PR! I have already got 2 LGTMs
Thanks in advance!

1 similar comment
@Anushree-Mathur
Copy link
Contributor Author

@Yingshun @luckyh @yalzhang @chunfuwen @dzhengfy Could you review and merge this PR! I have already got 2 LGTMs
Thanks in advance!

@qiankehan
Copy link
Contributor

A notification for libvirt external snapshot deletion: there is an issue with deleting the external snapshot with multiple children snapshots: https://gitlab.com/libvirt/libvirt/-/issues/534. It is fixed in libvirt v9.10.0

yalzhang
yalzhang approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@yalzhang yalzhang left a comment

Choose a reason for hiding this comment

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

no impact for downstream testing since we have added it into skiplist

@Anushree-Mathur
Copy link
Contributor Author

@chunfuwen @dzhengfy I have got 4 approvals. Could you please review and merge this PR!

chunfuwen
chunfuwen approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@chunfuwen chunfuwen left a comment

Choose a reason for hiding this comment

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

lgtm

@chunfuwen chunfuwen merged commit e92ae68 into autotest:master Jan 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sacsant sacsant sacsant requested changes

@bskjois bskjois bskjois approved these changes

@yalzhang yalzhang yalzhang approved these changes

@chunfuwen chunfuwen chunfuwen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Anushree-Mathur @qiankehan @sacsant @chunfuwen @yalzhang @bskjois