Update the snapshot physical size for the primary storage resource after snapshot creation and during resource count recalculation #12481

sureshanaparti · 2026-01-21T07:16:03Z

Description

This PR updates the snapshot physical size for the primary storage resource after snapshot creation and during resource count recalculation.

The snapshot size incremented for the primary storage resource in the resource count while allocating snapshot (here: #11558), is reverted during resource count recalculation for primary storage resource. Also, post snapshot creation, the decrement resource count not performed for primary storage resource when snapshot created on primary.

Fixes #11441

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
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Created snapshots with user, admin accounts for some volumes and checked the resource count of 'primary_storage' resource.

How did you try to break this feature and the system with this change?

…creation and during resource count recalculation

codecov · 2026-01-21T07:23:30Z

Codecov Report

❌ Patch coverage is 29.41176% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.24%. Comparing base (42f1e19) to head (77073a3).
⚠️ Report is 6 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...storage/datastore/db/SnapshotDataStoreDaoImpl.java	0.00%	13 Missing ⚠️
...om/cloud/storage/snapshot/SnapshotManagerImpl.java	46.66%	6 Missing and 2 partials ⚠️
.../cloud/resourcelimit/ResourceLimitManagerImpl.java	60.00%	2 Missing ⚠️
...k/api/command/user/snapshot/CreateSnapshotCmd.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12481   +/-   ##
=========================================
  Coverage     16.23%   16.24%           
- Complexity    13380    13394   +14     
=========================================
  Files          5657     5657           
  Lines        499039   499128   +89     
  Branches      60567    60577   +10     
=========================================
+ Hits          81024    81074   +50     
- Misses       408978   409017   +39     
  Partials       9037     9037

Flag	Coverage Δ
uitests	`4.03% <ø> (ø)`
unittests	`17.09% <29.41%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR fixes an issue where snapshot physical size for primary storage resources was not being properly tracked during snapshot creation and resource count recalculation. The changes ensure that snapshots stored on primary storage are correctly accounted for in resource limits.

Changes:

Added a new DAO method to calculate the total physical size of snapshots on primary storage for an account
Updated resource count calculation to include snapshot sizes on primary storage
Refactored snapshot resource type determination logic to use a common helper method
Fixed resource cleanup in error scenarios to use the correct storage resource type

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
SnapshotDataStoreDao.java	Added interface method to get snapshots physical size on primary storage by account ID
SnapshotDataStoreDaoImpl.java	Implemented SQL query to calculate total physical size of snapshots on primary storage, excluding those backed up to secondary storage
ResourceLimitManagerImpl.java	Updated primary storage calculation to include snapshot sizes; fixed spelling error in log message
SnapshotManagerImpl.java	Added helper method getStoreResourceType() and updated resource cleanup logic to use correct resource type in success and failure paths
ResourceLimitManagerImplTest.java	Updated test expectations to include snapshot sizes in primary storage calculations
CreateSnapshotCmd.java	Removed redundant null check on enum values() method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

...ne/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java

sureshanaparti · 2026-01-21T08:04:46Z

@blueorangutan package

blueorangutan · 2026-01-21T08:06:34Z

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

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

blueorangutan · 2026-01-21T09:08:05Z

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

sureshanaparti · 2026-01-21T09:46:46Z

@blueorangutan package

blueorangutan · 2026-01-21T09:48:38Z

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

abh1sar · 2026-01-21T10:22:58Z

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java

+    private static final String GET_PHYSICAL_SIZE_OF_SNAPSHOTS_ON_PRIMARY_BY_ACCOUNT = "SELECT SUM(s.physical_size) " +
+            "FROM cloud.snapshot_store_ref s " +
+            "LEFT JOIN cloud.snapshots ON s.snapshot_id = snapshots.id " +
+            "WHERE snapshots.account_id = ? " +
+            "AND snapshots.removed IS NULL " +
+            "AND s.state = 'Ready' " +
+            "AND s.store_role = 'Primary' " +
+            "AND NOT EXISTS (SELECT 1 FROM cloud.snapshot_store_ref i WHERE i.snapshot_id = s.snapshot_id AND i.store_role = 'Image')";


Shouldn't we include the size of the snapshot in primary which is present on secondary as well?

Suggested change

private static final String GET_PHYSICAL_SIZE_OF_SNAPSHOTS_ON_PRIMARY_BY_ACCOUNT = "SELECT SUM(s.physical_size) " +

"FROM cloud.snapshot_store_ref s " +

"LEFT JOIN cloud.snapshots ON s.snapshot_id = snapshots.id " +

"WHERE snapshots.account_id = ? " +

"AND snapshots.removed IS NULL " +

"AND s.state = 'Ready' " +

"AND s.store_role = 'Primary' " +

"AND NOT EXISTS (SELECT 1 FROM cloud.snapshot_store_ref i WHERE i.snapshot_id = s.snapshot_id AND i.store_role = 'Image')";

private static final String GET_PHYSICAL_SIZE_OF_SNAPSHOTS_ON_PRIMARY_BY_ACCOUNT = "SELECT SUM(s.physical_size) " +

"FROM cloud.snapshot_store_ref s " +

"LEFT JOIN cloud.snapshots ON s.snapshot_id = snapshots.id " +

"WHERE snapshots.account_id = ? " +

"AND snapshots.removed IS NULL " +

"AND s.state = 'Ready' " +

"AND s.store_role = 'Primary' ";

Actually, if we are going this way we can just use snapshotSizeSearch in ResourceLimitManagerImpl directly similar to calculateSecondaryStorageForAccount()

SearchCriteria<SumCount> sc2 = snapshotSizeSearch.create(); sc2.setParameters("state", ObjectInDataStoreStateMachine.State.Ready); sc2.setParameters("storeRole", DataStoreRole.Primary); sc2.setJoinParameters("snapshots", "accountId", accountId); List<SumCount> snapshots = _snapshotDataStoreDao.customSearch(sc2, null);

Shouldn't we include the size of the snapshot in primary which is present on secondary as well?

@abh1sar no, the snapshots in primary are deleted when they are backed up on secondary, but the store_ref record still exists (with 'Ready' state as I noticed - this record needs to removed or state should be updated to 'Destroyed'. Also, location_type column in snapshots table is not populated, which should indicate location of the snapshot - 'Primary' or 'Image'. - these need more testing and can break any existing func, so I'll raise separate PR with these fixes/improvements).

the sql would consider the snapshots in primary and not exists in secondary storage.

blueorangutan · 2026-01-21T10:54:46Z

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

sureshanaparti added 2 commits January 20, 2026 18:27

Update snapshot size for the primary storage resource after snapshot …

71884a1

…creation and during resource count recalculation

Update snapshot physical size

0659e8d

boring-cyborg bot added component:api component:storage labels Jan 21, 2026

sureshanaparti linked an issue Jan 21, 2026 that may be closed by this pull request

Snapshot Size Not Counted in Primary Storage Quota #11441

Open

sureshanaparti added this to the 4.20.3 milestone Jan 21, 2026

sureshanaparti added the component:snapshot label Jan 21, 2026

sureshanaparti requested a review from Copilot January 21, 2026 07:17

Copilot started reviewing on behalf of sureshanaparti January 21, 2026 07:17 View session

sureshanaparti requested review from DaanHoogland, abh1sar and shwstppr January 21, 2026 07:18

Copilot AI reviewed Jan 21, 2026

View reviewed changes

review

5e0024f

DaanHoogland reviewed Jan 21, 2026

View reviewed changes

...chema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java Outdated Show resolved Hide resolved

review

77073a3

abh1sar suggested changes Jan 21, 2026

View reviewed changes

Update the snapshot physical size for the primary storage resource after snapshot creation and during resource count recalculation #12481

Are you sure you want to change the base?

Update the snapshot physical size for the primary storage resource after snapshot creation and during resource count recalculation #12481

Uh oh!

Conversation

sureshanaparti commented Jan 21, 2026

Description

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?

Uh oh!

codecov bot commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sureshanaparti commented Jan 21, 2026

Uh oh!

blueorangutan commented Jan 21, 2026

Uh oh!

Uh oh!

blueorangutan commented Jan 21, 2026

Uh oh!

sureshanaparti commented Jan 21, 2026

Uh oh!

blueorangutan commented Jan 21, 2026

Uh oh!

abh1sar Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

abh1sar Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jan 21, 2026 •

edited

Loading

sureshanaparti Jan 21, 2026 •

edited

Loading