Fix snapshot chaining on Xen#12597
Conversation
|
@blueorangutan package |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12597 +/- ##
=========================================
Coverage 17.60% 17.61%
- Complexity 15660 15661 +1
=========================================
Files 5917 5917
Lines 531415 531426 +11
Branches 64973 64973
=========================================
+ Hits 93566 93585 +19
+ Misses 427294 427283 -11
- Partials 10555 10558 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #12524 where the snapshot.delta.max configuration was not being respected on XenServer after the introduction of the Hidden state. The fix ensures that hidden snapshots are properly considered when calculating snapshot chain length, preventing unending snapshot chains. It also resolves a regression in snapshot deletion by using the correct NOTIN operator instead of NEQ when excluding multiple states.
Changes:
- Modified snapshot chain calculation to include Hidden state snapshots alongside Ready state snapshots
- Added new DAO method to query snapshots by multiple states using the IN operator
- Fixed snapshot deletion regression by properly using NOTIN operator for multiple state exclusion
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| DefaultSnapshotStrategy.java | Updated getSnapshotImageStoreRef to include Hidden snapshots in chain calculation by using the new multi-state query method |
| SnapshotDataStoreDaoImpl.java | Added search builders for NOTIN and IN operations, implemented new multi-state query method, fixed snapshot deletion query |
| SnapshotDataStoreDao.java | Added interface method signature for querying snapshots by multiple states |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16711 |
|
@blueorangutan package |
|
@JoaoJandre 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16712 |
|
@blueorangutan test ol9 xcpng83 |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15385)
|
|
@DaanHoogland can we rerun the tests? |
|
@blueorangutan package |
|
@JoaoJandre 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16882 |
|
@blueorangutan test ol9 xcpng83 |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests |
|
@nvazquez 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17138 |
|
@blueorangutan test ol9 xcpng83 |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15652)
|
|
@nvazquez I looked at the errors reported by Blue Orangutan but no issues were related to this PR. |
|
Thanks @JoaoJandre let me kick one more round of packaging and tests @blueorangutan package |
|
@nvazquez 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17188 |
|
@blueorangutan test ol9 xcpng83 |
|
@nvazquez a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15689)
|
vladimirpetrov
left a comment
vladimirpetrov
left a comment
There was a problem hiding this comment.
LGTM based on manial testing.
Test case:
Pre-conditions:
- CloudStack 4.22.1+ with PR #12597 applied
snapshot.delta.max= 3- XenServer/XCP-ng hypervisor configured
- Volume attached to a VM or available
Test Steps:
- Create snapshot S1 on a volume
- Verify S1 is a full snapshot (check
snapshot_store_reftable) - Create snapshot S2
- Verify S2 is an incremental snapshot linked to S1
- Create snapshot S3
- Verify S3 is an incremental snapshot linked to S2
- Delete snapshot S1
- Verify S1 state changes to
Hiddeninsnapshot_store_reftable - Create snapshot S4
- Query the database to check the snapshot chain length
- Verify that S4 is a full snapshot (new chain started)
Expected Results:
- S1 is created as a full snapshot
- S2 and S3 are incremental snapshots
- After deleting S1, it transitions to
Hiddenstate - S4 should be a full snapshot because chain length (S1-Hidden, S2, S3) = 3 which equals
snapshot.delta.max - No snapshots should remain in
Readystate if older than retention policy
|
@blueorangutan test ol9 xcpng83 |
|
@vladimirpetrov a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests |
vladimirpetrov
added
status:waiting-for-smoketest-results
status:tested
and removed
status:needs-testing
ready-for-testing
labels
|
Thanks for testing @vladimirpetrov |
|
[SF] Trillian test result (tid-15807)
|
|
@blueorangutan test ol8 xcpng83 |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + xcpng83) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-15832)
|
github-project-automation
Bot
moved this from In Progress
to Done
in Apache CloudStack 4.22.1
Source local main commit: - bb635f652f snapshot: preserve Xen snapshot chaining through hidden refs Source Apache commits: - 2a60305 Fix snapshot chaining on Xen (apache#12597) Change summary: - add a DAO method that lists snapshot-store refs by snapshot id, role, and a set of states - update DefaultSnapshotStrategy.getSnapshotImageStoreRef(...) to consider both Ready and Hidden image-store refs - align the unit test with the new DAO method and remove redundant null-path stubbing - record Record 040 and mark 8608b4e as already satisfied in the history document Functional impact: - preserves Xen incremental snapshot chain lookup even when a parent snapshot is hidden on secondary storage - reduces the chance of losing the expected parent chain and falling back to an incorrect full backup path - keeps zone-scoped image-store lookup while widening acceptable persisted states Validation: - cherry-pick from main applied cleanly on ablestack-europa with no additional manual conflict resolution - mvn/mvnw-based tests not run in this environment by request
Description
This PR fixes #12524.
After the introduction of the
Hiddenstate, the snapshot chain calculation no longer works as expected for XenServer as it does not consider hidden snapshots as part of the chain, possibly leading to unending chains. This PR fixes this issue by adding the hidden snapshots to the chain calculation.This PR also fixes a regression introduced in commit d700e2d which made snapshot deletion impossible.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
In an environment using XCP-ng, I set
snapshot.delta.maxto 3 and followed the steps below:Before the changes, the new snapshot would be part of the old chain. With the changes, the last snapshot is a full snapshot that is not part of the old chain.