Skip to content

Commit 8608b4e

Browse files
winterhazelabh1sar
winterhazel
and
abh1sar
committed
Fix snapshot copy resource limit concurrency
Co-authored-by: Abhisar Sinha <63767682+abh1sar@users.noreply.github.com>
1 parent f333134 commit 8608b4e

File tree

2 files changed

+58
-57
lines changed

2 files changed

+58
-57
lines changed

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

Lines changed: 57 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -2088,57 +2088,61 @@ private boolean checkAndProcessSnapshotAlreadyExistInStore(long snapshotId, Data
20882088

20892089
@DB
20902090
private boolean copySnapshotToZone(SnapshotDataStoreVO snapshotDataStoreVO, DataStore srcSecStore,
2091-
DataCenterVO dstZone, DataStore dstSecStore, Account account, boolean kvmIncrementalSnapshot)
2091+
DataCenterVO dstZone, DataStore dstSecStore, Account account, boolean kvmIncrementalSnapshot, boolean shouldCheckResourceLimits)
20922092
throws ResourceAllocationException {
20932093
final long snapshotId = snapshotDataStoreVO.getSnapshotId();
20942094
final long dstZoneId = dstZone.getId();
20952095
if (checkAndProcessSnapshotAlreadyExistInStore(snapshotId, dstSecStore)) {
20962096
return true;
20972097
}
2098-
_resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
2099-
// snapshotId may refer to ID of a removed parent snapshot
2100-
SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
2101-
if (kvmIncrementalSnapshot) {
2102-
snapshotOnSecondary = snapshotHelper.convertSnapshotIfNeeded(snapshotOnSecondary);
2103-
}
2104-
String copyUrl = null;
2105-
try {
2106-
AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
2107-
CreateCmdResult result = future.get();
2108-
if (!result.isFailed()) {
2109-
copyUrl = result.getPath();
2110-
}
2111-
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
2112-
logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
2113-
}
2114-
if (StringUtils.isEmpty(copyUrl)) {
2115-
logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
2116-
return false;
2117-
}
2118-
logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
2119-
try {
2120-
AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
2121-
SnapshotResult result = future.get();
2122-
if (result.isFailed()) {
2123-
logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
2124-
return false;
2098+
// Resource limit checks are not performed here at the moment, but they were added in case this method is used
2099+
// in the future to copy a standalone snapshot
2100+
long requiredSecondaryStorageSpace = shouldCheckResourceLimits ? snapshotDataStoreVO.getSize() : 0L;
2101+
try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, requiredSecondaryStorageSpace, null, reservationDao, _resourceLimitMgr)) {
2102+
// snapshotId may refer to ID of a removed parent snapshot
2103+
SnapshotInfo snapshotOnSecondary = snapshotFactory.getSnapshot(snapshotId, srcSecStore);
2104+
if (kvmIncrementalSnapshot) {
2105+
snapshotOnSecondary = snapshotHelper.convertSnapshotIfNeeded(snapshotOnSecondary);
21252106
}
2126-
snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
2127-
_resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
2128-
if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
2129-
SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
2130-
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
2131-
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
2107+
String copyUrl = null;
2108+
try {
2109+
AsyncCallFuture<CreateCmdResult> future = snapshotSrv.queryCopySnapshot(snapshotOnSecondary);
2110+
CreateCmdResult result = future.get();
2111+
if (!result.isFailed()) {
2112+
copyUrl = result.getPath();
2113+
}
2114+
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
2115+
logger.error("Failed to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore, ex);
21322116
}
2133-
if (kvmIncrementalSnapshot) {
2134-
((ImageStoreEntity) srcSecStore).deleteExtractUrl(snapshotOnSecondary.getPath(), copyUrl, Upload.Type.SNAPSHOT);
2117+
if (StringUtils.isEmpty(copyUrl)) {
2118+
logger.error("Unable to prepare URL for copy for snapshot ID: {} on store: {}", snapshotId, srcSecStore);
2119+
return false;
21352120
}
2121+
logger.debug(String.format("Copying snapshot ID: %d to destination zones using download URL: %s", snapshotId, copyUrl));
2122+
try {
2123+
AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshotOnSecondary, copyUrl, dstSecStore);
2124+
SnapshotResult result = future.get();
2125+
if (result.isFailed()) {
2126+
logger.debug("Copy snapshot ID: {} failed for image store {}: {}", snapshotId, dstSecStore, result.getResult());
2127+
return false;
2128+
}
2129+
snapshotZoneDao.addSnapshotToZone(snapshotId, dstZoneId);
2130+
_resourceLimitMgr.incrementResourceCount(account.getId(), ResourceType.secondary_storage, snapshotDataStoreVO.getSize());
2131+
if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
2132+
SnapshotVO snapshotVO = _snapshotDao.findByIdIncludingRemoved(snapshotId);
2133+
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, account.getId(), dstZoneId, snapshotId, null, null, null, snapshotVO.getSize(),
2134+
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
2135+
}
2136+
if (kvmIncrementalSnapshot) {
2137+
((ImageStoreEntity) srcSecStore).deleteExtractUrl(snapshotOnSecondary.getPath(), copyUrl, Upload.Type.SNAPSHOT);
2138+
}
21362139

2137-
return true;
2138-
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
2139-
logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
2140+
return true;
2141+
} catch (InterruptedException | ExecutionException | ResourceUnavailableException ex) {
2142+
logger.debug("Failed to copy snapshot ID: {} to image store: {}", snapshotId, dstSecStore);
2143+
}
2144+
return false;
21402145
}
2141-
return false;
21422146
}
21432147

21442148
@DB
@@ -2170,13 +2174,6 @@ private boolean copySnapshotChainToZone(SnapshotVO snapshotVO, DataStore srcSecS
21702174
if (CollectionUtils.isEmpty(snapshotChain)) {
21712175
return true;
21722176
}
2173-
try {
2174-
_resourceLimitMgr.checkResourceLimit(account, ResourceType.secondary_storage, size);
2175-
} catch (ResourceAllocationException e) {
2176-
logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
2177-
return false;
2178-
}
2179-
Collections.reverse(snapshotChain);
21802177
if (dstSecStore == null) {
21812178
// find all eligible image stores for the destination zone
21822179
List<DataStore> dstSecStores = dataStoreMgr.getImageStoresByScopeExcludingReadOnly(new ZoneScope(destZoneId));
@@ -2188,15 +2185,21 @@ private boolean copySnapshotChainToZone(SnapshotVO snapshotVO, DataStore srcSecS
21882185
throw new StorageUnavailableException("Destination zone is not ready, no image store with free capacity", DataCenter.class, destZoneId);
21892186
}
21902187
}
2191-
logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
2192-
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
2193-
if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account, kvmIncrementalSnapshot)) {
2194-
logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
2195-
snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
2196-
return false;
2188+
try (CheckedReservation secondaryStorageReservation = new CheckedReservation(account, ResourceType.secondary_storage, null, null, null, size, null, reservationDao, _resourceLimitMgr)) {
2189+
logger.debug("Copying snapshot chain for snapshot ID: {} on secondary store: {} of zone ID: {}", snapshotVO, dstSecStore, destZone);
2190+
Collections.reverse(snapshotChain);
2191+
for (SnapshotDataStoreVO snapshotDataStoreVO : snapshotChain) {
2192+
if (!copySnapshotToZone(snapshotDataStoreVO, srcSecStore, destZone, dstSecStore, account, kvmIncrementalSnapshot, false)) {
2193+
logger.error("Failed to copy snapshot: {} to zone: {} due to failure to copy snapshot ID: {} from snapshot chain",
2194+
snapshotVO, destZone, snapshotDataStoreVO.getSnapshotId());
2195+
return false;
2196+
}
21972197
}
2198+
return true;
2199+
} catch (ResourceAllocationException e) {
2200+
logger.error(String.format("Unable to allocate secondary storage resources for snapshot chain for %s with size: %d", snapshotVO, size), e);
2201+
return false;
21982202
}
2199-
return true;
22002203
}
22012204

22022205
@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
@@ -22,7 +22,6 @@
2222
import com.cloud.event.ActionEventUtils;
2323
import com.cloud.exception.InvalidParameterValueException;
2424
import com.cloud.exception.PermissionDeniedException;
25-
import com.cloud.exception.ResourceAllocationException;
2625
import com.cloud.exception.ResourceUnavailableException;
2726
import com.cloud.org.Grouping;
2827
import com.cloud.storage.DataStoreRole;
@@ -309,12 +308,11 @@ public void testCopyNewSnapshotToZones() {
309308
Mockito.when(result1.isFailed()).thenReturn(false);
310309
AsyncCallFuture<SnapshotResult> future1 = Mockito.mock(AsyncCallFuture.class);
311310
try {
312-
Mockito.doNothing().when(resourceLimitService).checkResourceLimit(Mockito.any(), Mockito.any(), Mockito.anyLong());
313311
Mockito.when(future.get()).thenReturn(result);
314312
Mockito.when(snapshotService.queryCopySnapshot(Mockito.any())).thenReturn(future);
315313
Mockito.when(future1.get()).thenReturn(result1);
316314
Mockito.when(snapshotService.copySnapshot(Mockito.any(SnapshotInfo.class), Mockito.anyString(), Mockito.any(DataStore.class))).thenReturn(future1);
317-
} catch (ResourceAllocationException | ResourceUnavailableException | ExecutionException | InterruptedException e) {
315+
} catch (ResourceUnavailableException | ExecutionException | InterruptedException e) {
318316
Assert.fail(e.getMessage());
319317
}
320318
List<Long> addedZone = new ArrayList<>();

0 commit comments

Comments
 (0)