Skip to content

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

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

Open
sureshanaparti wants to merge 4 commits into
base: 4.20
Choose a base branch
from
+73 −23
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71884a1
Update snapshot size for the primary storage resource after snapshot …
sureshanaparti Jan 20, 2026
0659e8d
Update snapshot physical size
sureshanaparti Jan 21, 2026
5e0024f
review
sureshanaparti Jan 21, 2026
77073a3
review
sureshanaparti Jan 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ public void execute() {
}

private Snapshot.LocationType getLocationType() {

if (Snapshot.LocationType.values() == null || Snapshot.LocationType.values().length == 0 || locationType == null) {
if (locationType == null) {
return null;
}

Expand Down
14 changes: 14 additions & 0 deletions ...schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,18 @@ public interface SnapshotDataStoreDao extends GenericDao<SnapshotDataStoreVO, Lo
void updateDisplayForSnapshotStoreRole(long snapshotId, long storeId, DataStoreRole role, boolean display);

int expungeBySnapshotList(List<Long> snapshotIds, Long batchSize);

/**
* Returns the total physical size, in bytes, of all snapshots stored on primary
* storage for the specified account that have not yet been backed up to
* secondary storage.
*
* <p>If no such snapshots are found, this method returns {@code 0}.</p>
*
* @param accountId the ID of the account whose snapshots on primary storage
* should be considered
* @return the total physical size in bytes of matching snapshots on primary
* storage, or {@code 0} if none are found
*/
long getSnapshotsPhysicalSizeOnPrimaryStorageByAccountId(long accountId);
}
29 changes: 28 additions & 1 deletion ...ma/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase<SnapshotDataStoreVO
" order by created %s " +
" limit 1";

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')";
Comment on lines +81 to +88
Copy link
Collaborator

@abh1sar abh1sar Jan 21, 2026

Choose a reason for hiding this comment

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

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' ";

Copy link
Collaborator

@abh1sar abh1sar Jan 21, 2026

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

@sureshanaparti sureshanaparti Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.


@Override
public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
super.configure(name, params);
Expand Down Expand Up @@ -118,7 +127,6 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
stateSearch.and(STATE, stateSearch.entity().getState(), SearchCriteria.Op.IN);
stateSearch.done();


idStateNeqSearch = createSearchBuilder();
idStateNeqSearch.and(SNAPSHOT_ID, idStateNeqSearch.entity().getSnapshotId(), SearchCriteria.Op.EQ);
idStateNeqSearch.and(STATE, idStateNeqSearch.entity().getState(), SearchCriteria.Op.NEQ);
Expand Down Expand Up @@ -578,4 +586,23 @@ public int expungeBySnapshotList(final List<Long> snapshotIds, final Long batchS
sc.setParameters("snapshotIds", snapshotIds.toArray());
return batchExpunge(sc, batchSize);
}

@Override
public long getSnapshotsPhysicalSizeOnPrimaryStorageByAccountId(long accountId) {
long snapshotsPhysicalSize = 0;
try (TransactionLegacy transactionLegacy = TransactionLegacy.currentTxn()) {
try (PreparedStatement preparedStatement = transactionLegacy.prepareStatement(GET_PHYSICAL_SIZE_OF_SNAPSHOTS_ON_PRIMARY_BY_ACCOUNT)) {
preparedStatement.setLong(1, accountId);

try (ResultSet resultSet = preparedStatement.executeQuery()) {
if (resultSet.next()) {
snapshotsPhysicalSize = resultSet.getLong(1);
}
}
}
} catch (SQLException e) {
logger.warn("Failed to get the snapshots physical size for the account [{}] due to [{}].", accountId, e.getMessage(), e);
}
return snapshotsPhysicalSize;
}
}
10 changes: 5 additions & 5 deletions server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,7 +1171,6 @@ protected long recalculateDomainResourceCount(final long domainId, final Resourc
}

return Transaction.execute((TransactionCallback<Long>) status -> {
long newResourceCount = 0L;
List<Long> domainIdList = childDomains.stream().map(DomainVO::getId).collect(Collectors.toList());
domainIdList.add(domainId);
List<Long> accountIdList = accounts.stream().map(AccountVO::getId).collect(Collectors.toList());
Expand All @@ -1189,6 +1188,7 @@ protected long recalculateDomainResourceCount(final long domainId, final Resourc
List<ResourceCountVO> resourceCounts = _resourceCountDao.lockRows(rowIdsToLock);

long oldResourceCount = 0L;
long newResourceCount = 0L;
ResourceCountVO domainRC = null;

// calculate project count here
Expand All @@ -1210,7 +1210,7 @@ protected long recalculateDomainResourceCount(final long domainId, final Resourc
if (oldResourceCount != newResourceCount) {
domainRC.setCount(newResourceCount);
_resourceCountDao.update(domainRC.getId(), domainRC);
logger.warn("Discrepency in the resource count has been detected (original count = {} correct count = {}) for Type = {} for Domain ID = {} is fixed during resource count recalculation.",
logger.warn("Discrepancy in the resource count has been detected (original count = {} correct count = {}) for Type = {} for Domain ID = {} is fixed during resource count recalculation.",
oldResourceCount, newResourceCount, type, domainId);
}
return newResourceCount;
Expand Down Expand Up @@ -1436,16 +1436,17 @@ private long calculatePublicIpForAccount(long accountId) {
}

protected long calculatePrimaryStorageForAccount(long accountId, String tag) {
long snapshotsPhysicalSizeOnPrimaryStorage = _snapshotDataStoreDao.getSnapshotsPhysicalSizeOnPrimaryStorageByAccountId(accountId);
if (StringUtils.isEmpty(tag)) {
List<Long> virtualRouters = _vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId);
return _volumeDao.primaryStorageUsedForAccount(accountId, virtualRouters);
return snapshotsPhysicalSizeOnPrimaryStorage + _volumeDao.primaryStorageUsedForAccount(accountId, virtualRouters);
}
long storage = 0;
List<VolumeVO> volumes = getVolumesWithAccountAndTag(accountId, tag);
for (VolumeVO volume : volumes) {
storage += volume.getSize() == null ? 0L : volume.getSize();
}
return storage;
return snapshotsPhysicalSizeOnPrimaryStorage + storage;
}

@Override
Expand Down Expand Up @@ -2143,7 +2144,6 @@ public ConfigKey<?>[] getConfigKeys() {

protected class ResourceCountCheckTask extends ManagedContextRunnable {
public ResourceCountCheckTask() {

}

@Override
Expand Down
30 changes: 18 additions & 12 deletions server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,15 @@ protected boolean isBackupSnapshotToSecondaryForZone(long zoneId) {
return !DataCenter.Type.Edge.equals(zone.getType());
}

private ResourceType getStoreResourceType(long dataCenterId, Snapshot.LocationType locationType) {
ResourceType storeResourceType = ResourceType.secondary_storage;
if (!isBackupSnapshotToSecondaryForZone(dataCenterId) ||
Snapshot.LocationType.PRIMARY.equals(locationType)) {
storeResourceType = ResourceType.primary_storage;
}
return storeResourceType;
}

@Override
public String getConfigComponentName() {
return SnapshotManager.class.getSimpleName();
Expand Down Expand Up @@ -614,15 +623,14 @@ public Snapshot backupSnapshotFromVmSnapshot(Long snapshotId, Long vmId, Long vo
_snapshotDao.update(snapshot.getId(), snapshot);
snapshotInfo = this.snapshotFactory.getSnapshot(snapshotId, store);

Long snapshotOwnerId = vm.getAccountId();
long snapshotOwnerId = vm.getAccountId();

try {
SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.BACKUP);
if (snapshotStrategy == null) {
throw new CloudRuntimeException(String.format("Unable to find Snapshot strategy to handle Snapshot [%s]", snapshot));
}
snapshotInfo = snapshotStrategy.backupSnapshot(snapshotInfo);

} catch (Exception e) {
logger.debug("Failed to backup Snapshot from Instance Snapshot", e);
_resourceLimitMgr.decrementResourceCount(snapshotOwnerId, ResourceType.snapshot);
Expand Down Expand Up @@ -771,12 +779,11 @@ public boolean deleteSnapshot(long snapshotId, Long zoneId) {
_accountMgr.checkAccess(caller, null, true, snapshotCheck);

SnapshotStrategy snapshotStrategy = _storageStrategyFactory.getSnapshotStrategy(snapshotCheck, zoneId, SnapshotOperation.DELETE);

if (snapshotStrategy == null) {
logger.error("Unable to find snapshot strategy to handle snapshot [{}]", snapshotCheck);

return false;
}

Pair<List<SnapshotDataStoreVO>, List<Long>> storeRefAndZones = getStoreRefsAndZonesForSnapshotDelete(snapshotId, zoneId);
List<SnapshotDataStoreVO> snapshotStoreRefs = storeRefAndZones.first();
List<Long> zoneIds = storeRefAndZones.second();
Expand Down Expand Up @@ -1472,8 +1479,9 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_CREATE, snapshot.getAccountId(), snapshot.getDataCenterId(), snapshotId, snapshot.getName(), null, null,
snapshotStoreRef.getPhysicalSize(), volume.getSize(), snapshot.getClass().getName(), snapshot.getUuid());

ResourceType storeResourceType = dataStoreRole == DataStoreRole.Image ? ResourceType.secondary_storage : ResourceType.primary_storage;
// Correct the resource count of snapshot in case of delta snapshots.
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), ResourceType.secondary_storage, new Long(volume.getSize() - snapshotStoreRef.getPhysicalSize()));
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), storeResourceType, new Long(volume.getSize() - snapshotStoreRef.getPhysicalSize()));

if (!payload.getAsyncBackup() && backupSnapToSecondary) {
copyNewSnapshotToZones(snapshotId, snapshot.getDataCenterId(), payload.getZoneIds());
Expand All @@ -1485,15 +1493,17 @@ public SnapshotInfo takeSnapshot(VolumeInfo volume) throws ResourceAllocationExc
if (logger.isDebugEnabled()) {
logger.debug("Failed to create snapshot" + cre.getLocalizedMessage());
}
ResourceType storeResourceType = getStoreResourceType(volume.getDataCenterId(), payload.getLocationType());
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), ResourceType.snapshot);
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), ResourceType.secondary_storage, new Long(volume.getSize()));
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), storeResourceType, new Long(volume.getSize()));
throw cre;
} catch (Exception e) {
if (logger.isDebugEnabled()) {
logger.debug("Failed to create snapshot", e);
}
ResourceType storeResourceType = getStoreResourceType(volume.getDataCenterId(), payload.getLocationType());
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), ResourceType.snapshot);
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), ResourceType.secondary_storage, new Long(volume.getSize()));
_resourceLimitMgr.decrementResourceCount(snapshotOwner.getId(), storeResourceType, new Long(volume.getSize()));
throw new CloudRuntimeException("Failed to create snapshot", e);
}
return snapshot;
Expand Down Expand Up @@ -1695,11 +1705,7 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
Type snapshotType = getSnapshotType(policyId);
Account owner = _accountMgr.getAccount(volume.getAccountId());

ResourceType storeResourceType = ResourceType.secondary_storage;
if (!isBackupSnapshotToSecondaryForZone(volume.getDataCenterId()) ||
Snapshot.LocationType.PRIMARY.equals(locationType)) {
storeResourceType = ResourceType.primary_storage;
}
ResourceType storeResourceType = getStoreResourceType(volume.getDataCenterId(), locationType);
try {
_resourceLimitMgr.checkResourceLimit(owner, ResourceType.snapshot);
_resourceLimitMgr.checkResourceLimit(owner, storeResourceType, volume.getSize());
Expand Down
10 changes: 7 additions & 3 deletions server/src/test/java/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.cloudstack.api.response.TaggedResourceLimitAndCountResponse;
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.reservation.dao.ReservationDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
Expand Down Expand Up @@ -118,6 +119,8 @@ public class ResourceLimitManagerImplTest extends TestCase {
VolumeDao volumeDao;
@Mock
UserVmDao userVmDao;
@Mock
SnapshotDataStoreDao snapshotDataStoreDao;

private List<String> hostTags = List.of("htag1", "htag2", "htag3");
private List<String> storageTags = List.of("stag1", "stag2");
Expand Down Expand Up @@ -840,20 +843,21 @@ public void testCalculatePrimaryStorageForAccount() {
String tag = null;
Mockito.when(vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId))
.thenReturn(List.of(1L));
Mockito.when(snapshotDataStoreDao.getSnapshotsPhysicalSizeOnPrimaryStorageByAccountId(accountId)).thenReturn(100L);
Mockito.when(volumeDao.primaryStorageUsedForAccount(Mockito.eq(accountId), Mockito.anyList())).thenReturn(100L);
Assert.assertEquals(100L, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));
Assert.assertEquals(200L, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));

tag = "";
Mockito.when(volumeDao.primaryStorageUsedForAccount(Mockito.eq(accountId), Mockito.anyList())).thenReturn(200L);
Assert.assertEquals(200L, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));
Assert.assertEquals(300L, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));

tag = "tag";
VolumeVO vol = Mockito.mock(VolumeVO.class);
long size = 1024;
Mockito.when(vol.getSize()).thenReturn(size);
List<VolumeVO> vols = List.of(vol, vol);
Mockito.doReturn(vols).when(resourceLimitManager).getVolumesWithAccountAndTag(accountId, tag);
Assert.assertEquals(vols.size() * size, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));
Assert.assertEquals((vols.size() * size) + 100L, resourceLimitManager.calculatePrimaryStorageForAccount(accountId, tag));
}

@Test
Expand Down
Loading