nvazquez commented on code in PR #9478:
URL: https://github.com/apache/cloudstack/pull/9478#discussion_r2126886200
##########
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CopySnapshotCmd.java:
##########
@@ -84,6 +85,19 @@ public class CopySnapshotCmd extends BaseAsyncCmd implements
UserCmd {
"Do not specify destzoneid and destzoneids together,
however one of them is required.")
protected List<Long> destZoneIds;
+ @Parameter(name = ApiConstants.STORAGE_ID_LIST,
Review Comment:
Can maybe add the since attribute for these new parameters
##########
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CopySnapshotCmd.java:
##########
@@ -106,7 +120,18 @@ public List<Long> getDestinationZoneIds() {
destIds.add(destZoneId);
return destIds;
}
- return null;
+ return new ArrayList<>();
+ }
+
+ public List<Long> getStoragePoolIds() {
+ return storagePoolIds;
+ }
+
+ public Boolean useStorageReplication() {
+ if (useStorageReplication == null) {
+ return false;
+ }
+ return useStorageReplication;
Review Comment:
Minor one: can reduce the implementation by returning
`BooleanUtils.toBoolean(useStorageReplication)` and make the method return a
boolean instead (to avoid null checks)
##########
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java:
##########
@@ -161,6 +176,17 @@ public List<Long> getZoneIds() {
return zoneIds;
}
+ public List<Long> getStoragePoolIds() {
+ return storagePoolIds;
Review Comment:
Maybe returning an empty list when the parameter is not set?
##########
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java:
##########
@@ -99,6 +101,19 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd {
since = "4.19.0")
protected List<Long> zoneIds;
+ @Parameter(name = ApiConstants.STORAGE_ID_LIST,
+ type=CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = StoragePoolResponse.class,
+ authorized = RoleType.Admin,
+ description = "A comma-separated list of IDs of the storage pools
in other zones in which the snapshot will be made available. " +
+ "The snapshot will always be made available in the zone in
which the volume is present.",
+ since = "4.20.0")
Review Comment:
Now moved to 4.21 :)
##########
api/src/main/java/org/apache/cloudstack/api/response/SnapshotPolicyResponse.java:
##########
@@ -62,9 +61,14 @@ public class SnapshotPolicyResponse extends
BaseResponseWithTagInformation {
@Param(description = "The list of zones in which snapshot backup is
scheduled", responseObject = ZoneResponse.class, since = "4.19.0")
protected Set<ZoneResponse> zones;
+ @SerializedName(ApiConstants.STORAGE)
+ @Param(description = "The list of pools in which snapshot backup is
scheduled", responseObject = StoragePoolResponse.class, since = "4.20.0")
Review Comment:
Also here
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -591,7 +595,7 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume,
Snapshot snapshot, Use
}
// don't try to perform a sync if the DataStoreRole of the snapshot is
equal to DataStoreRole.Primary
- if (!DataStoreRole.Primary.equals(dataStoreRole) ||
kvmSnapshotOnlyInPrimaryStorage) {
+ if (!DataStoreRole.Primary.equals(dataStoreRole) || !keepOnPrimary) {
Review Comment:
Maybe I'm not following the logic but the names of these variables look
similar, but have opposite meanings. Would it be fair to rename the one named
`keepOnPrimary` to `storageSupportSnapshotToTemplateEnabled`?
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -2155,6 +2289,73 @@ public Snapshot copySnapshot(CopySnapshotCmd cmd) throws
StorageUnavailableExcep
}
}
+ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
+ List<Long> poolsToBeRemoved = new ArrayList<>();
+ for (Long poolId : poolIds) {
+ PrimaryDataStore dataStore = (PrimaryDataStore)
dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+ if (isObjectNull(dataStore == null, poolsToBeRemoved, poolId))
continue;
+
+ SnapshotInfo snapshotInfo =
snapshotFactory.getSnapshot(snapshot.getId(), poolId, DataStoreRole.Primary);
+ if (isSnapshotExistsOnPool(snapshot, dataStore, snapshotInfo))
continue;
+
+ VolumeVO volume = _volsDao.findById(snapshot.getVolumeId());
+ if (isObjectNull(volume == null, poolsToBeRemoved, poolId))
continue;
+ doesStorageSupportCopySnapshot(poolsToBeRemoved, poolId,
dataStore, volume);
+ }
+ poolIds.removeAll(poolsToBeRemoved);
+ if (CollectionUtils.isEmpty(poolIds)) {
+ return false;
+ }
+ return true;
+ }
+
+ private void doesStorageSupportCopySnapshot(List<Long> poolsToBeRemoved,
Long poolId, PrimaryDataStore dataStore, VolumeVO volume) {
+ if (!dataStore.getDriver().getCapabilities()
+
.containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString())
+ && dataStore.getPoolType() != volume.getPoolType()) {
+ poolsToBeRemoved.add(poolId);
+ logger.debug(String.format("The %s does not support copy to %s
between zones", dataStore.getPoolType(), volume.getPoolType()));
+ }
+ }
+
+ private boolean isSnapshotExistsOnPool(Snapshot snapshot, PrimaryDataStore
dataStore, SnapshotInfo snapshotInfo) {
+ if (snapshotInfo != null) {
+ logger.debug(String.format("Snapshot [%s] already exist on pool
[%s]", snapshot.getUuid(), dataStore.getName()));
+ return true;
+ }
+ return false;
+ }
+
+ private static boolean isObjectNull(boolean object, List<Long>
poolsToBeRemoved, Long poolId) {
Review Comment:
Can this method be renamed? By its name it would be assumed its just a null
check but its doing more than that
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -2155,6 +2289,73 @@ public Snapshot copySnapshot(CopySnapshotCmd cmd) throws
StorageUnavailableExcep
}
}
+ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
+ List<Long> poolsToBeRemoved = new ArrayList<>();
+ for (Long poolId : poolIds) {
+ PrimaryDataStore dataStore = (PrimaryDataStore)
dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
+ if (isObjectNull(dataStore == null, poolsToBeRemoved, poolId))
continue;
+
+ SnapshotInfo snapshotInfo =
snapshotFactory.getSnapshot(snapshot.getId(), poolId, DataStoreRole.Primary);
+ if (isSnapshotExistsOnPool(snapshot, dataStore, snapshotInfo))
continue;
+
+ VolumeVO volume = _volsDao.findById(snapshot.getVolumeId());
+ if (isObjectNull(volume == null, poolsToBeRemoved, poolId))
continue;
+ doesStorageSupportCopySnapshot(poolsToBeRemoved, poolId,
dataStore, volume);
+ }
+ poolIds.removeAll(poolsToBeRemoved);
+ if (CollectionUtils.isEmpty(poolIds)) {
+ return false;
+ }
+ return true;
+ }
+
+ private void doesStorageSupportCopySnapshot(List<Long> poolsToBeRemoved,
Long poolId, PrimaryDataStore dataStore, VolumeVO volume) {
+ if (!dataStore.getDriver().getCapabilities()
Review Comment:
To avoid NPEs, can you check `dataStore.getDriver() != null` and
`MapUtils.isNotEmpty(dataStore.getDriver().getCapabilities())`?
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -1650,10 +1709,47 @@ public boolean
isHypervisorKvmAndFileBasedStorage(VolumeInfo volumeInfo, Storage
return volumeInfo.getHypervisorType() == HypervisorType.KVM &&
fileBasedStores.contains(storagePool.getPoolType());
}
+ private void copyNewSnapshotToZonesOnPrimary(CreateSnapshotPayload
payload, SnapshotInfo snapshot) {
+ SnapshotStrategy snapshotStrategy;
+ snapshotStrategy =
_storageStrategyFactory.getSnapshotStrategy(snapshot, SnapshotOperation.COPY);
+ if (snapshotStrategy != null) {
+ for (Long storagePoolId : payload.getStoragePoolIds()) {
+ copySnapshotOnPool(snapshot, snapshotStrategy, storagePoolId);
+ }
+ } else {
+ logger.info("Unable to find snapshot strategy to handle the copy
of a snapshot with id " + snapshot.getUuid());
+ }
+ }
+
+ private boolean copySnapshotOnPool(SnapshotInfo snapshot, SnapshotStrategy
snapshotStrategy, Long storagePoolId) {
+ DataStore store = dataStoreMgr.getDataStore(storagePoolId,
DataStoreRole.Primary);
+ SnapshotInfo snapshotOnStore = (SnapshotInfo) store.create(snapshot);
+
+ try {
+ AsyncCallFuture<SnapshotResult> future =
snapshotSrv.copySnapshot(snapshot, snapshotOnStore, snapshotStrategy);
+ SnapshotResult result = future.get();
+ if (result.isFailed()) {
+ logger.debug(String.format("Copy snapshot ID: %d failed for
primary storage %s: %s", snapshot.getSnapshotId(), storagePoolId,
result.getResult()));
+ return false;
+ }
+ snapshotZoneDao.addSnapshotToZone(snapshot.getId(),
snapshotOnStore.getDataCenterId());
+
_resourceLimitMgr.incrementResourceCount(CallContext.current().getCallingUserId(),
ResourceType.primary_storage, snapshot.getSize());
+ if (CallContext.current().getCallingUserId() !=
Account.ACCOUNT_ID_SYSTEM) {
+ SnapshotVO snapshotVO =
_snapshotDao.findByIdIncludingRemoved(snapshot.getSnapshotId());
+
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY,
CallContext.current().getCallingUserId(), snapshotOnStore.getDataCenterId(),
snapshotVO.getId(), null, null, null, snapshotVO.getSize(),
+ snapshotVO.getSize(), snapshotVO.getClass().getName(),
snapshotVO.getUuid());
+ }
+ } catch (InterruptedException e) {
Review Comment:
Maybe can collapse both on a single catch block and log the exception as
well?
##########
api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotPolicyCmd.java:
##########
@@ -83,6 +83,14 @@ public class CreateSnapshotPolicyCmd extends BaseCmd {
"The snapshots will always be made available in the zone
in which the volume is present.")
protected List<Long> zoneIds;
+ @Parameter(name = ApiConstants.STORAGE_ID_LIST,
+ type=CommandType.LIST,
+ collectionType = CommandType.UUID,
+ entityType = StoragePoolResponse.class,
+ description = "A comma-separated list of IDs of the storage pools
in other zones in which the snapshot will be made available. " +
+ "The snapshot will always be made available in the zone in
which the volume is present.",
+ since = "4.20.0")
Review Comment:
Also here
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java:
##########
@@ -2122,16 +2226,42 @@ protected DataCenterVO
getCheckedDestinationZoneForSnapshotCopy(long zoneId, boo
return dstZone;
}
+ protected StoragePoolVO getCheckedDestinationStorageForSnapshotCopy(long
poolId, boolean isRootAdmin) {
+ StoragePoolVO destPool = _storagePoolDao.findById(poolId);
+ if (destPool == null) {
+ throw new InvalidParameterValueException("Please specify a valid
destination zone.");
+ }
+ if (!StoragePoolStatus.Up.equals(destPool.getStatus()) &&
!isRootAdmin) {
+ throw new PermissionDeniedException("Cannot perform this
operation, the storage pool is not in Up state: " + destPool.getName());
Review Comment:
The message is missing something like: "or not the ROOT admin"
##########
server/src/main/java/com/cloud/api/ApiDBUtils.java:
##########
@@ -1707,6 +1706,19 @@ public static List<DataCenterVO>
findSnapshotPolicyZones(SnapshotPolicy policy,
return s_zoneDao.listByIds(zoneIds);
}
+ public static List<StoragePoolVO> findSnapshotPolicyPools(SnapshotPolicy
policy, Volume volume) {
+ List<SnapshotPolicyDetailVO> poolDetails =
s_snapshotPolicyDetailsDao.findDetails(policy.getId(), ApiConstants.STORAGE_ID);
+ List<Long> poolIds = new ArrayList<>();
+ for (SnapshotPolicyDetailVO detail : poolDetails) {
+ try {
+ poolIds.add(Long.valueOf(detail.getValue()));
+ } catch (NumberFormatException ignored) {}
Review Comment:
I think this should be logged in case there is an exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
