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 <[email protected]>
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<>();
