-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Show chain size in snapshot response for incremental snapshots #11313
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11313 +/- ##
============================================
- Coverage 16.99% 16.99% -0.01%
- Complexity 14718 14719 +1
============================================
Files 5832 5832
Lines 517562 517582 +20
Branches 62982 62986 +4
============================================
+ Hits 87985 87988 +3
- Misses 419644 419661 +17
Partials 9933 9933
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 |
|
@abh1sar 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new chain_size field to snapshot responses that displays the total disk usage of incremental snapshots including their parent snapshots. This addresses the issue where incremental snapshots show misleadingly small physical sizes when their parent snapshots have been deleted from the UI but remain on disk.
- Introduces a configurable
snapshot.show.chain.sizesetting to control chain size visibility - Adds chain size calculation logic that traverses the snapshot parent hierarchy
- Updates UI components to display the chain size field for snapshots
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/main/java/org/apache/cloudstack/api/ApiConstants.java | Adds CHAIN_SIZE constant for API response field |
| api/src/main/java/org/apache/cloudstack/api/response/SnapshotResponse.java | Adds chainSize field and setter method to snapshot response |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java | Defines configuration key for enabling chain size display |
| server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java | Registers the new configuration key |
| server/src/main/java/com/cloud/api/query/dao/SnapshotJoinDaoImpl.java | Implements chain size calculation logic and response population |
| ui/src/config/section/storage.js | Adds chainsize to snapshot details fields |
| ui/src/components/view/DetailsTab.vue | Adds UI rendering for chain size with GiB formatting |
| ui/public/locales/en.json | Adds label translation for chain size |
| tools/marvin/setup.py | Updates version from snapshot to release |
server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
|
@abh1sar a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
|
UI build: ✔️ |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14429 |
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, just a question on the co-pilots remark.
sureshanaparti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
|
@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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14471 |
JoaoJandre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLGTM, did not test it.
slavkap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, but I don't have a free env. to test it now
|
@blueorangutan test |
|
@borisstoyanov a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
borisstoyanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, manually checked, chainsize is available in the response and usage records seem to be in line with expectations set at 'keep' argument in setting up recurring snapshots.
|
[SF] Trillian test result (tid-13984)
|
Description
This PR fixes #9416
When a parent (full) snapshot of an incremental snapshot is deleted in CloudStack, it is removed from the UI/API but not from disk if it has dependent snapshots. As a result, the physical size reported for the incremental snapshot is very small, even though the actual data resides in its parent, which still consumes disk space. From the user’s perspective, there will be a mismatch between the physical size shown in CloudStack and the actual storage usage on disk.
This PR introduces
chain_sizeas a new filed in snapshot response for incremental snapshots. The chain_size represents the total disk usage of the snapshot chain, calculated as the sum of the physical size of the snapshot and all its parent snapshots. This provides a more accurate representation of actual storage consumed and improves transparency for users.This feature can be enabled by the setting
snapshot.show.chain.size.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?