You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ab...@apache.org on 2026/03/19 12:50:41 UTC
(cloudstack) branch 4.20 updated: Fix snapshot copy resource limit concurrency
This is an automated email from the ASF dual-hosted git repository.
abhisar pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new c19630f0a4f Fix snapshot copy resource limit concurrency
c19630f0a4f is described below
commit c19630f0a4f34a82a9414b9b6d16b5ec44b86860
Author: Abhisar Sinha <63...@users.noreply.github.com>
AuthorDate: Tue Mar 17 06:56:09 2026 +0530
Fix snapshot copy resource limit concurrency
---
.../storage/snapshot/SnapshotManagerImpl.java | 117 +++++++++++----------
.../storage/snapshot/SnapshotManagerImplTest.java | 4 +-
2 files changed, 61 insertions(+), 60 deletions(-)
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 4a4a7544ce7..0d6e9de509f 100755
--- a/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
+++ b/server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java
@@ -1748,17 +1748,17 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
try (CheckedReservation volumeSnapshotReservation = new CheckedReservation(owner, ResourceType.snapshot, null, null, 1L, reservationDao, _resourceLimitMgr);
CheckedReservation storageReservation = new CheckedReservation(owner, storeResourceType, null, null, volume.getSize(), reservationDao, _resourceLimitMgr)) {
- SnapshotVO snapshotVO = new SnapshotVO(volume.getDataCenterId(), volume.getAccountId(), volume.getDomainId(), volume.getId(), volume.getDiskOfferingId(), snapshotName,
- (short)snapshotType.ordinal(), snapshotType.name(), volume.getSize(), volume.getMinIops(), volume.getMaxIops(), hypervisorType, locationType);
+ SnapshotVO snapshotVO = new SnapshotVO(volume.getDataCenterId(), volume.getAccountId(), volume.getDomainId(), volume.getId(), volume.getDiskOfferingId(), snapshotName,
+ (short)snapshotType.ordinal(), snapshotType.name(), volume.getSize(), volume.getMinIops(), volume.getMaxIops(), hypervisorType, locationType);
- SnapshotVO snapshot = _snapshotDao.persist(snapshotVO);
- if (snapshot == null) {
- throw new CloudRuntimeException(String.format("Failed to create snapshot for volume: %s", volume));
- }
- CallContext.current().putContextParameter(Snapshot.class, snapshot.getUuid());
- _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.snapshot);
- _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), storeResourceType, volume.getSize());
- return snapshot;
+ SnapshotVO snapshot = _snapshotDao.persist(snapshotVO);
+ if (snapshot == null) {
+ throw new CloudRuntimeException(String.format("Failed to create snapshot for volume: %s", volume));
+ }
+ CallContext.current().putContextParameter(Snapshot.class, snapshot.getUuid());
+ _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.snapshot);
+ _resourceLimitMgr.incrementResourceCount(volume.getAccountId(), storeResourceType, volume.getSize());
+ return snapshot;
} catch (ResourceAllocationException e) {
if (snapshotType != Type.MANUAL) {
String msg = String.format("Snapshot resource limit exceeded for account id : %s. Failed to create recurring snapshots", owner.getId());
@@ -1807,50 +1807,54 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
@DB
private boolean copySnapshotToZone(SnapshotDataStoreVO snapshotDataStoreVO, DataStore srcSecStore,
- DataCenterVO dstZone, DataStore dstSecStore, Account account)
+ DataCenterVO dstZone, DataStore dstSecStore, Account account, boolean shouldCheckResourceLimits)
throws ResourceAllocationException {
final long snapshotId = snapshotDataStoreVO.getSnapshotId();
final long dstZoneId = dstZone.getId();
if (checkAndProcessSnapshotAlreadyExistInStore(snapshotId, dstSecStore)) {
return true;
}
- _resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
- // snapshotId may refer to ID of a removed parent snapshot
- SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
- String copyUrl = null;
- try {
- AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
- CreateCmdResult result = future.get();
- if (!result.isFailed()) {
- copyUrl = result.getPath();
+ // Resource limit checks are not performed here at the moment, but they were added in case this method is used
+ // in the future to copy a standalone snapshot
+ long requiredSecondaryStorageSpace = shouldCheckResourceLimits ? snapshotDataStoreVO.getSize() : 0L;
+ try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, requiredSecondaryStorageSpace, null, reservationDao, _resourceLimitMgr)) {
+ SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
+ String copyUrl = null;
+ try {
+ AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
+ CreateCmdResult result = future.get();
+ if (!result.isFailed()) {
+ copyUrl = result.getPath();
+ }
+ } catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
+ logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
}
- } catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
- logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
- }
- if (StringUtils.isEmpty(copyUrl)) {
- logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
- return false;
- }
- logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
- try {
- AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
- SnapshotResult result = future.get();
- if (result.isFailed()) {
- logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
+ if (StringUtils.isEmpty(copyUrl)) {
+ logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
return false;
}
- snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
- _resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
- if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
- SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
- UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
- snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
+ logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
+ try {
+ AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
+ SnapshotResult result = future.get();
+ if (result.isFailed()) {
+ logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
+ return false;
+ }
+ snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
+ _resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
+ if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
+ SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
+ UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
+ snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
+ }
+
+ return true;
+ } catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
+ logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
}
- return true;
- } catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
- logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
+ return false;
}
- return false;
}
@DB
@@ -1881,13 +1885,6 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
if (CollectionUtils.isEmpty(snapshotChain)) {
return true;
}
- try {
- _resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, size);
- } catch (ResourceAllocationException e) {
- logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
- return false;
- }
- Collections.reverse(snapshotChain);
if (dstSecStore == null) {
// find all eligible image stores for the destination zone
List<DataStore> dstSecStores = dataStoreMgr.getImageStoresByScopeExcludingReadOnly(new ZoneScope(destZoneId));
@@ -1899,15 +1896,21 @@ public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implement
throw new StorageUnavailableException("Destination zone is not ready, no image store with free capacity", DataCenter.class, destZoneId);
}
}
- logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
- for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
- if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account)) {
- logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
- snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
- return false;
+ try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, size, null, reservationDao, _resourceLimitMgr)) {
+ logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
+ Collections.reverse(snapshotChain);
+ for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
+ if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account, false)) {
+ logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
+ snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
+ return false;
+ }
}
+ return true;
+ } catch (ResourceAllocationException e) {
+ logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
+ return false;
}
- return true;
}
@DB
diff --git a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java
index 86fdcfecc13..32b103f39d3 100644
--- a/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java
+++ b/server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java
@@ -46,7 +46,6 @@ import com.cloud.dc.dao.DataCenterDao;
import com.cloud.event.ActionEventUtils;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.PermissionDeniedException;
-import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.org.Grouping;
import com.cloud.storage.DataStoreRole;
@@ -284,12 +283,11 @@ public class SnapshotManagerImplTest {
Mockito.when(result1.isFailed()).thenReturn(false);
AsyncCallFuture<SnapshotResult> future1 = Mockito.mock(AsyncCallFuture.class);
try {
- Mockito.doNothing().when(resourceLimitService).checkResourceLimit(Mockito.any(), Mockito.any(), Mockito.anyLong());
Mockito.when(future.get()).thenReturn(result);
Mockito.when(snapshotService.queryCopySnapshot(Mockito.any())).thenReturn(future);
Mockito.when(future1.get()).thenReturn(result1);
Mockito.when(snapshotService.copySnapshot(Mockito.any(SnapshotInfo.class), Mockito.anyString(), Mockito.any(DataStore.class))).thenReturn(future1);
- } catch (ResourceAllocationException | ResourceUnavailableException | ExecutionException | InterruptedException e) {
+ } catch (ResourceUnavailableException | ExecutionException | InterruptedException e) {
Assert.fail(e.getMessage());
}
List<Long> addedZone = new ArrayList<>();