Skip to content

Commit c19630f

Browse files
abh1sarabh1sar
committed
Fix snapshot copy resource limit concurrency
1 parent b497f58 commit c19630f

File tree

2 files changed

+61
-60
lines changed

2 files changed

+61
-60
lines changed

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

Lines changed: 60 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,17 +1748,17 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
17481748

17491749
try (CheckedReservation volumeSnapshotReservation = new CheckedReservation(owner, ResourceType.snapshot, null, null, 1L, reservationDao, _resourceLimitMgr);
17501750
CheckedReservation storageReservation = new CheckedReservation(owner, storeResourceType, null, null, volume.getSize(), reservationDao, _resourceLimitMgr)) {
1751-
SnapshotVO snapshotVO = new SnapshotVO(volume.getDataCenterId(), volume.getAccountId(), volume.getDomainId(), volume.getId(), volume.getDiskOfferingId(), snapshotName,
1752-
(short)snapshotType.ordinal(), snapshotType.name(), volume.getSize(), volume.getMinIops(), volume.getMaxIops(), hypervisorType, locationType);
1751+
SnapshotVO snapshotVO = new SnapshotVO(volume.getDataCenterId(), volume.getAccountId(), volume.getDomainId(), volume.getId(), volume.getDiskOfferingId(), snapshotName,
1752+
(short)snapshotType.ordinal(), snapshotType.name(), volume.getSize(), volume.getMinIops(), volume.getMaxIops(), hypervisorType, locationType);
17531753

1754-
SnapshotVO snapshot = _snapshotDao.persist(snapshotVO);
1755-
if (snapshot == null) {
1756-
throw new CloudRuntimeException(String.format("Failed to create snapshot for volume: %s", volume));
1757-
}
1758-
CallContext.current().putContextParameter(Snapshot.class, snapshot.getUuid());
1759-
_resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.snapshot);
1760-
_resourceLimitMgr.incrementResourceCount(volume.getAccountId(), storeResourceType, volume.getSize());
1761-
return snapshot;
1754+
SnapshotVO snapshot = _snapshotDao.persist(snapshotVO);
1755+
if (snapshot == null) {
1756+
throw new CloudRuntimeException(String.format("Failed to create snapshot for volume: %s", volume));
1757+
}
1758+
CallContext.current().putContextParameter(Snapshot.class, snapshot.getUuid());
1759+
_resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.snapshot);
1760+
_resourceLimitMgr.incrementResourceCount(volume.getAccountId(), storeResourceType, volume.getSize());
1761+
return snapshot;
17621762
} catch (ResourceAllocationException e) {
17631763
if (snapshotType != Type.MANUAL) {
17641764
String msg = String.format("Snapshot resource limit exceeded for account id : %s. Failed to create recurring snapshots", owner.getId());
@@ -1807,50 +1807,54 @@ private boolean checkAndProcessSnapshotAlreadyExistInStore(long snapshotId, Data
18071807

18081808
@DB
18091809
private boolean copySnapshotToZone(SnapshotDataStoreVO snapshotDataStoreVO, DataStore srcSecStore,
1810-
DataCenterVO dstZone, DataStore dstSecStore, Account account)
1810+
DataCenterVO dstZone, DataStore dstSecStore, Account account, boolean shouldCheckResourceLimits)
18111811
throws ResourceAllocationException {
18121812
final long snapshotId = snapshotDataStoreVO.getSnapshotId();
18131813
final long dstZoneId = dstZone.getId();
18141814
if (checkAndProcessSnapshotAlreadyExistInStore(snapshotId, dstSecStore)) {
18151815
return true;
18161816
}
1817-
_resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
1818-
// snapshotId may refer to ID of a removed parent snapshot
1819-
SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
1820-
String copyUrl = null;
1821-
try {
1822-
AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
1823-
CreateCmdResult result = future.get();
1824-
if (!result.isFailed()) {
1825-
copyUrl = result.getPath();
1817+
// Resource limit checks are not performed here at the moment, but they were added in case this method is used
1818+
// in the future to copy a standalone snapshot
1819+
long requiredSecondaryStorageSpace = shouldCheckResourceLimits ? snapshotDataStoreVO.getSize() : 0L;
1820+
try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, requiredSecondaryStorageSpace, null, reservationDao, _resourceLimitMgr)) {
1821+
SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
1822+
String copyUrl = null;
1823+
try {
1824+
AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
1825+
CreateCmdResult result = future.get();
1826+
if (!result.isFailed()) {
1827+
copyUrl = result.getPath();
1828+
}
1829+
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
1830+
logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
18261831
}
1827-
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
1828-
logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
1829-
}
1830-
if (StringUtils.isEmpty(copyUrl)) {
1831-
logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
1832-
return false;
1833-
}
1834-
logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
1835-
try {
1836-
AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
1837-
SnapshotResult result = future.get();
1838-
if (result.isFailed()) {
1839-
logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
1832+
if (StringUtils.isEmpty(copyUrl)) {
1833+
logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
18401834
return false;
18411835
}
1842-
snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
1843-
_resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
1844-
if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
1845-
SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
1846-
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
1847-
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
1836+
logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
1837+
try {
1838+
AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
1839+
SnapshotResult result = future.get();
1840+
if (result.isFailed()) {
1841+
logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
1842+
return false;
1843+
}
1844+
snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
1845+
_resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
1846+
if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
1847+
SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
1848+
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
1849+
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
1850+
}
1851+
1852+
return true;
1853+
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
1854+
logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
18481855
}
1849-
return true;
1850-
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
1851-
logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
1856+
return false;
18521857
}
1853-
return false;
18541858
}
18551859

18561860
@DB
@@ -1881,13 +1885,6 @@ private boolean copySnapshotChainToZone(SnapshotVO snapshotVO, DataStore srcSecS
18811885
if (CollectionUtils.isEmpty(snapshotChain)) {
18821886
return true;
18831887
}
1884-
try {
1885-
_resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, size);
1886-
} catch (ResourceAllocationException e) {
1887-
logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
1888-
return false;
1889-
}
1890-
Collections.reverse(snapshotChain);
18911888
if (dstSecStore == null) {
18921889
// find all eligible image stores for the destination zone
18931890
List<DataStore> dstSecStores = dataStoreMgr.getImageStoresByScopeExcludingReadOnly(new ZoneScope(destZoneId));
@@ -1899,15 +1896,21 @@ private boolean copySnapshotChainToZone(SnapshotVO snapshotVO, DataStore srcSecS
18991896
throw new StorageUnavailableException("Destination zone is not ready, no image store with free capacity", DataCenter.class, destZoneId);
19001897
}
19011898
}
1902-
logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
1903-
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
1904-
if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account)) {
1905-
logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
1906-
snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
1907-
return false;
1899+
try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, size, null, reservationDao, _resourceLimitMgr)) {
1900+
logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
1901+
Collections.reverse(snapshotChain);
1902+
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
1903+
if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account, false)) {
1904+
logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
1905+
snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
1906+
return false;
1907+
}
19081908
}
1909+
return true;
1910+
} catch (ResourceAllocationException e) {
1911+
logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
1912+
return false;
19091913
}
1910-
return true;
19111914
}
19121915

19131916
@DB

‎server/src/test/java/com/cloud/storage/snapshot/SnapshotManagerImplTest.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646
import com.cloud.event.ActionEventUtils;
4747
import com.cloud.exception.InvalidParameterValueException;
4848
import com.cloud.exception.PermissionDeniedException;
49-
import com.cloud.exception.ResourceAllocationException;
5049
import com.cloud.exception.ResourceUnavailableException;
5150
import com.cloud.org.Grouping;
5251
import com.cloud.storage.DataStoreRole;
@@ -284,12 +283,11 @@ public void testCopyNewSnapshotToZones() {
284283
Mockito.when(result1.isFailed()).thenReturn(false);
285284
AsyncCallFuture<SnapshotResult> future1 = Mockito.mock(AsyncCallFuture.class);
286285
try {
287-
Mockito.doNothing().when(resourceLimitService).checkResourceLimit(Mockito.any(), Mockito.any(), Mockito.anyLong());
288286
Mockito.when(future.get()).thenReturn(result);
289287
Mockito.when(snapshotService.queryCopySnapshot(Mockito.any())).thenReturn(future);
290288
Mockito.when(future1.get()).thenReturn(result1);
291289
Mockito.when(snapshotService.copySnapshot(Mockito.any(SnapshotInfo.class), Mockito.anyString(), Mockito.any(DataStore.class))).thenReturn(future1);
292-
} catch (ResourceAllocationException | ResourceUnavailableException | ExecutionException | InterruptedException e) {
290+
} catch (ResourceUnavailableException | ExecutionException | InterruptedException e) {
293291
Assert.fail(e.getMessage());
294292
}
295293
List<Long> addedZone = new ArrayList<>();

0 commit comments

Comments
 (0)