diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java index bd541b69183..078d4517f95 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java @@ -244,8 +244,7 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd { } private Snapshot.LocationType getLocationType() { - - if (Snapshot.LocationType.values() == null || Snapshot.LocationType.values().length == 0 || locationType == null) { + if (locationType == null) { return null; } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java index ef0a5d0ebff..96df4928773 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDao.java @@ -110,4 +110,18 @@ StateDao 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. + * + *

If no such snapshots are found, this method returns {@code 0}.

+ * + * @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); } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java index ba76a6b3f41..c68316dd1fe 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/storage/datastore/db/SnapshotDataStoreDaoImpl.java @@ -78,6 +78,15 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase params) throws ConfigurationException { super.configure(name, params); @@ -118,7 +127,6 @@ public class SnapshotDataStoreDaoImpl extends GenericDaoBase) status -> { - long newResourceCount = 0L; List domainIdList = childDomains.stream().map(DomainVO::getId).collect(Collectors.toList()); domainIdList.add(domainId); List accountIdList = accounts.stream().map(AccountVO::getId).collect(Collectors.toList()); @@ -1189,6 +1188,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim List resourceCounts = _resourceCountDao.lockRows(rowIdsToLock); long oldResourceCount = 0L; + long newResourceCount = 0L; ResourceCountVO domainRC = null; // calculate project count here @@ -1210,7 +1210,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim 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; @@ -1436,16 +1436,17 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim } protected long calculatePrimaryStorageForAccount(long accountId, String tag) { + long snapshotsPhysicalSizeOnPrimaryStorage = _snapshotDataStoreDao.getSnapshotsPhysicalSizeOnPrimaryStorageByAccountId(accountId); if (StringUtils.isEmpty(tag)) { List virtualRouters = _vmDao.findIdsOfAllocatedVirtualRoutersForAccount(accountId); - return _volumeDao.primaryStorageUsedForAccount(accountId, virtualRouters); + return snapshotsPhysicalSizeOnPrimaryStorage + _volumeDao.primaryStorageUsedForAccount(accountId, virtualRouters); } long storage = 0; List volumes = getVolumesWithAccountAndTag(accountId, tag); for (VolumeVO volume : volumes) { storage += volume.getSize() == null ? 0L : volume.getSize(); } - return storage; + return snapshotsPhysicalSizeOnPrimaryStorage + storage; } @Override @@ -2143,7 +2144,6 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim protected class ResourceCountCheckTask extends ManagedContextRunnable { public ResourceCountCheckTask() { - } @Override diff --git a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java index e7606572a07..19cde4da0f1 100755 --- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java +++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java @@ -276,6 +276,15 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement 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(); @@ -614,7 +623,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement _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); @@ -622,7 +631,6 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement 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); @@ -771,12 +779,11 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement _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> storeRefAndZones = getStoreRefsAndZonesForSnapshotDelete(snapshotId, zoneId); List snapshotStoreRefs = storeRefAndZones.first(); List zoneIds = storeRefAndZones.second(); @@ -1472,8 +1479,9 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement 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()); @@ -1485,15 +1493,17 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement 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; @@ -1695,11 +1705,7 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement 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()); diff --git a/server/src/test/java/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java b/server/src/test/java/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java index 34030626d22..53ccc830dd2 100644 --- a/server/src/test/java/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java +++ b/server/src/test/java/com/cloud/resourcelimit/ResourceLimitManagerImplTest.java @@ -28,6 +28,7 @@ import org.apache.cloudstack.api.response.DomainResponse; 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; @@ -118,6 +119,8 @@ public class ResourceLimitManagerImplTest extends TestCase { VolumeDao volumeDao; @Mock UserVmDao userVmDao; + @Mock + SnapshotDataStoreDao snapshotDataStoreDao; private List hostTags = List.of("htag1", "htag2", "htag3"); private List storageTags = List.of("stag1", "stag2"); @@ -840,12 +843,13 @@ public class ResourceLimitManagerImplTest extends TestCase { 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); @@ -853,7 +857,7 @@ public class ResourceLimitManagerImplTest extends TestCase { Mockito.when(vol.getSize()).thenReturn(size); List 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