Skip to content

Commit 246339f

Browse files
slavkapslavkap
committed
Addressed reviews
1 parent 172697b commit 246339f

File tree

10 files changed

+76
-61
lines changed

10 files changed

+76
-61
lines changed

‎api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CopySnapshotCmd.java‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.apache.cloudstack.api.response.ZoneResponse;
4040
import org.apache.cloudstack.context.CallContext;
4141
import org.apache.commons.collections.CollectionUtils;
42+
import org.apache.commons.lang3.BooleanUtils;
4243
import org.apache.logging.log4j.LogManager;
4344
import org.apache.logging.log4j.Logger;
4445

@@ -91,11 +92,12 @@ public class CopySnapshotCmd extends BaseAsyncCmd implements UserCmd {
9192
entityType = StoragePoolResponse.class,
9293
required = false,
9394
authorized = RoleType.Admin,
95+
since = "4.21.0",
9496
description = "A comma-separated list of IDs of the storage pools in other zones in which the snapshot will be made available. " +
9597
"The snapshot will always be made available in the zone in which the volume is present. Currently supported for StorPool only")
9698
protected List<Long> storagePoolIds;
9799

98-
@Parameter (name = ApiConstants.USE_STORAGE_REPLICATION, type=CommandType.BOOLEAN, required = false, description = "This parameter enables the option the snapshot to be copied to supported primary storage")
100+
@Parameter (name = ApiConstants.USE_STORAGE_REPLICATION, type=CommandType.BOOLEAN, required = false, since = "4.21.0", description = "This parameter enables the option the snapshot to be copied to supported primary storage")
99101
protected Boolean useStorageReplication;
100102

101103
/////////////////////////////////////////////////////
@@ -128,10 +130,7 @@ public List<Long> getStoragePoolIds() {
128130
}
129131

130132
public Boolean useStorageReplication() {
131-
if (useStorageReplication == null) {
132-
return false;
133-
}
134-
return useStorageReplication;
133+
return BooleanUtils.toBoolean(useStorageReplication);
135134
}
136135

137136
@Override

‎api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotCmd.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package org.apache.cloudstack.api.command.user.snapshot;
1818

19+
import java.util.ArrayList;
1920
import java.util.Collection;
2021
import java.util.HashMap;
2122
import java.util.List;
@@ -108,7 +109,7 @@ public class CreateSnapshotCmd extends BaseAsyncCreateCmd {
108109
authorized = RoleType.Admin,
109110
description = "A comma-separated list of IDs of the storage pools in other zones in which the snapshot will be made available. " +
110111
"The snapshot will always be made available in the zone in which the volume is present.",
111-
since = "4.20.0")
112+
since = "4.21.0")
112113
protected List<Long> storagePoolIds;
113114

114115
@Parameter (name = ApiConstants.USE_STORAGE_REPLICATION, type=CommandType.BOOLEAN, required = false, description = "This parameter enables the option the snapshot to be copied to supported primary storage")
@@ -177,7 +178,7 @@ public List<Long> getZoneIds() {
177178
}
178179

179180
public List<Long> getStoragePoolIds() {
180-
return storagePoolIds;
181+
return storagePoolIds == null ? new ArrayList<>() : storagePoolIds;
181182
}
182183

183184
public Boolean useStorageReplication() {

‎api/src/main/java/org/apache/cloudstack/api/command/user/snapshot/CreateSnapshotPolicyCmd.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import com.cloud.storage.Volume;
2323
import com.cloud.storage.snapshot.SnapshotPolicy;
2424
import com.cloud.user.Account;
25+
import java.util.ArrayList;
2526
import org.apache.cloudstack.acl.RoleType;
2627
import org.apache.cloudstack.api.APICommand;
2728
import org.apache.cloudstack.api.ApiCommandResourceType;
@@ -89,7 +90,7 @@ public class CreateSnapshotPolicyCmd extends BaseCmd {
8990
entityType = StoragePoolResponse.class,
9091
description = "A comma-separated list of IDs of the storage pools in other zones in which the snapshot will be made available. " +
9192
"The snapshot will always be made available in the zone in which the volume is present.",
92-
since = "4.20.0")
93+
since = "4.21.0")
9394
protected List<Long> storagePoolIds;
9495
/////////////////////////////////////////////////////
9596
/////////////////// Accessors ///////////////////////
@@ -127,7 +128,9 @@ public List<Long> getZoneIds() {
127128
return zoneIds;
128129
}
129130

130-
public List<Long> getStoragePoolIds() { return storagePoolIds; }
131+
public List<Long> getStoragePoolIds() {
132+
return storagePoolIds == null ? new ArrayList<>() : storagePoolIds;
133+
}
131134

132135
/////////////////////////////////////////////////////
133136
/////////////// API Implementation///////////////////

‎api/src/main/java/org/apache/cloudstack/api/response/SnapshotPolicyResponse.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public class SnapshotPolicyResponse extends BaseResponseWithTagInformation {
6262
protected Set<ZoneResponse> zones;
6363

6464
@SerializedName(ApiConstants.STORAGE)
65-
@Param(description = "The list of pools in which snapshot backup is scheduled", responseObject = StoragePoolResponse.class, since = "4.20.0")
65+
@Param(description = "The list of pools in which snapshot backup is scheduled", responseObject = StoragePoolResponse.class, since = "4.21.0")
6666
protected Set<StoragePoolResponse> storagePools;
6767

6868
public SnapshotPolicyResponse() {

‎engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -578,10 +578,10 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
578578
SnapshotInfo snapInfo = snapshotFactory.getSnapshotWithRoleAndZone(snapshot.getId(), dataStoreRole, zoneId);
579579

580580
boolean kvmSnapshotOnlyInPrimaryStorage = snapshotHelper.isKvmSnapshotOnlyInPrimaryStorage(snapshot, dataStoreRole, volume.getDataCenterId());
581-
boolean keepOnPrimary = snapshotHelper.isStorageSupportSnapshotToTemplate(snapInfo);
581+
boolean storageSupportSnapshotToTemplateEnabled = snapshotHelper.isStorageSupportSnapshotToTemplate(snapInfo);
582582

583583
try {
584-
if (!keepOnPrimary) {
584+
if (!storageSupportSnapshotToTemplateEnabled) {
585585
snapInfo = snapshotHelper.backupSnapshotToSecondaryStorageIfNotExists(snapInfo, dataStoreRole, snapshot, kvmSnapshotOnlyInPrimaryStorage);
586586
}
587587
} catch (CloudRuntimeException e) {
@@ -595,7 +595,7 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
595595
}
596596

597597
// don't try to perform a sync if the DataStoreRole of the snapshot is equal to DataStoreRole.Primary
598-
if (!DataStoreRole.Primary.equals(dataStoreRole) || !keepOnPrimary) {
598+
if (!DataStoreRole.Primary.equals(dataStoreRole) || !storageSupportSnapshotToTemplateEnabled) {
599599
try {
600600
// sync snapshot to region store if necessary
601601
DataStore snapStore = snapInfo.getDataStore();

‎plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/collector/StorPoolAbandonObjectsCollector.java‎

Lines changed: 42 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -364,45 +364,53 @@ protected void runInContext() {
364364
}
365365
List<Long> recoveredSnapshots = new ArrayList<>();
366366
for (StoragePoolVO storagePool : onePoolforZone.values()) {
367-
try {
368-
logger.debug(String.format("Checking StorPool recovered snapshots for zone [%s]",
369-
storagePool.getDataCenterId()));
370-
SpConnectionDesc conn = StorPoolUtil.getSpConnection(storagePool.getUuid(),
371-
storagePool.getId(), storagePoolDetailsDao, storagePoolDao);
372-
JsonArray arr = StorPoolUtil.snapshotsList(conn);
373-
List<String> snapshots = snapshotsForRecovery(arr);
374-
if (snapshots.isEmpty()) {
367+
collectRecoveredSnapshotAfterExport(snapshotDetails, recoveredSnapshots, storagePool);
368+
}
369+
for (Long recoveredSnapshot : recoveredSnapshots) {
370+
snapshotDetailsDao.remove(recoveredSnapshot);
371+
}
372+
}
373+
374+
private void collectRecoveredSnapshotAfterExport(List<SnapshotDetailsVO> snapshotDetails, List<Long> recoveredSnapshots, StoragePoolVO storagePool) {
375+
try {
376+
logger.debug(String.format("Checking StorPool recovered snapshots for zone [%s]",
377+
storagePool.getDataCenterId()));
378+
SpConnectionDesc conn = StorPoolUtil.getSpConnection(storagePool.getUuid(),
379+
storagePool.getId(), storagePoolDetailsDao, storagePoolDao);
380+
JsonArray arr = StorPoolUtil.snapshotsList(conn);
381+
List<String> snapshots = snapshotsForRecovery(arr);
382+
if (snapshots.isEmpty()) {
383+
return;
384+
}
385+
for (SnapshotDetailsVO snapshot : snapshotDetails) {
386+
String[] snapshotOnRemote = snapshot.getValue().split(";");
387+
if (snapshotOnRemote.length != 2) {
375388
continue;
376389
}
377-
for (SnapshotDetailsVO snapshot : snapshotDetails) {
378-
String[] snapshotOnRemote = snapshot.getValue().split(";");
379-
if (snapshotOnRemote.length != 2) {
380-
continue;
381-
}
382-
String name = snapshot.getValue().split(";")[0];
383-
String location = snapshot.getValue().split(";")[1];
384-
if (name == null || location == null) {
385-
StorPoolUtil.spLog("Could not find name or location for the snapshot %s", snapshot.getValue());
386-
continue;
387-
}
388-
if (snapshots.contains(name)) {
389-
Long clusterId = StorPoolHelper.findClusterIdByGlobalId(StorPoolUtil.getSnapshotClusterId(name, conn), clusterDao);
390-
conn = StorPoolHelper.getSpConnectionDesc(conn, clusterId);
391-
SpApiResponse resp = StorPoolUtil.snapshotUnexport(name, location, conn);
392-
if (resp.getError() == null) {
393-
StorPoolUtil.spLog("Unexport of snapshot %s was successful", name);
394-
recoveredSnapshots.add(snapshot.getId());
395-
} else {
396-
StorPoolUtil.spLog("Could not recover StorPool snapshot %s", resp.getError());
397-
}
398-
}
390+
String name = snapshot.getValue().split(";")[0];
391+
String location = snapshot.getValue().split(";")[1];
392+
if (name == null || location == null) {
393+
StorPoolUtil.spLog("Could not find name or location for the snapshot %s", snapshot.getValue());
394+
continue;
395+
}
396+
if (snapshots.contains(name)) {
397+
findRecoveredSnapshots(recoveredSnapshots, conn, snapshot, name, location);
399398
}
400-
} catch (Exception e) {
401-
logger.debug(String.format("Could not collect StorPool recovered snapshots %s", e.getMessage()));
402399
}
400+
} catch (Exception e) {
401+
logger.debug(String.format("Could not collect StorPool recovered snapshots %s", e.getMessage()));
403402
}
404-
for (Long recoveredSnapshot : recoveredSnapshots) {
405-
snapshotDetailsDao.remove(recoveredSnapshot);
403+
}
404+
405+
private void findRecoveredSnapshots(List<Long> recoveredSnapshots, SpConnectionDesc conn, SnapshotDetailsVO snapshot, String name, String location) {
406+
Long clusterId = StorPoolHelper.findClusterIdByGlobalId(StorPoolUtil.getSnapshotClusterId(name, conn), clusterDao);
407+
conn = StorPoolHelper.getSpConnectionDesc(conn, clusterId);
408+
SpApiResponse resp = StorPoolUtil.snapshotUnexport(name, location, conn);
409+
if (resp.getError() == null) {
410+
StorPoolUtil.spLog("Unexport of snapshot %s was successful", name);
411+
recoveredSnapshots.add(snapshot.getId());
412+
} else {
413+
StorPoolUtil.spLog("Could not recover StorPool snapshot %s", resp.getError());
406414
}
407415
}
408416
}

‎plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/snapshot/StorPoolSnapshotStrategy.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,7 +409,6 @@ public void postSnapshotCreation(SnapshotInfo snapshot) {
409409

410410
@Override
411411
public void copySnapshot(DataObject snapshot, DataObject snapshotDest, AsyncCompletionCallback<CreateCmdResult> callback) {
412-
413412
// export snapshot on remote
414413
StoragePoolVO storagePoolVO = _primaryDataStoreDao.findById(snapshotDest.getDataStore().getId());
415414
String location = StorPoolConfigurationManager.StorPoolClusterLocation.valueIn(snapshotDest.getDataStore().getId());

‎server/src/main/java/com/cloud/api/ApiDBUtils.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -361,7 +361,11 @@
361361
import java.util.function.Function;
362362
import java.util.stream.Collectors;
363363

364+
import org.apache.logging.log4j.LogManager;
365+
import org.apache.logging.log4j.Logger;
366+
364367
public class ApiDBUtils {
368+
private static final Logger log = LogManager.getLogger(ApiDBUtils.class);
365369
private static ManagementServer s_ms;
366370
static AsyncJobManager s_asyncMgr;
367371
static SecurityGroupManager s_securityGroupMgr;
@@ -1712,7 +1716,9 @@ public static List<StoragePoolVO> findSnapshotPolicyPools(SnapshotPolicy policy,
17121716
for (SnapshotPolicyDetailVO detail : poolDetails) {
17131717
try {
17141718
poolIds.add(Long.valueOf(detail.getValue()));
1715-
} catch (NumberFormatException ignored) {}
1719+
} catch (NumberFormatException ignored) {
1720+
log.debug(String.format("Could not parse the storage ID value of %s", detail.getValue()), ignored);
1721+
}
17161722
}
17171723
if (volume != null && !poolIds.contains(volume.getPoolId())) {
17181724
poolIds.add(0, volume.getPoolId());

‎server/src/main/java/com/cloud/api/query/QueryManagerImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@
1818

1919
import static com.cloud.vm.VmDetailConstants.SSH_PUBLIC_KEY;
2020

21-
22-
import com.cloud.cluster.ManagementServerHostPeerJoinVO;
2321
import java.lang.reflect.InvocationTargetException;
2422
import java.lang.reflect.Method;
2523
import java.util.ArrayList;
@@ -44,6 +42,8 @@
4442
import com.cloud.org.Cluster;
4543
import com.cloud.server.ManagementService;
4644
import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
45+
import com.cloud.cluster.ManagementServerHostPeerJoinVO;
46+
4747
import org.apache.cloudstack.acl.ControlledEntity;
4848
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
4949
import org.apache.cloudstack.acl.SecurityChecker;

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1739,10 +1739,8 @@ private boolean copySnapshotOnPool(SnapshotInfo snapshot, SnapshotStrategy snaps
17391739
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, CallContext.current().getCallingUserId(), snapshotOnStore.getDataCenterId(), snapshotVO.getId(), null, null, null, snapshotVO.getSize(),
17401740
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
17411741
}
1742-
} catch (InterruptedException e) {
1743-
throw new RuntimeException(e);
1744-
} catch (ExecutionException e) {
1745-
throw new RuntimeException(e);
1742+
} catch (InterruptedException | ExecutionException e) {
1743+
throw new RuntimeException("Could not copy the snapshot to another pool", e);
17461744
}
17471745
return true;
17481746
}
@@ -2232,7 +2230,7 @@ protected StoragePoolVO getCheckedDestinationStorageForSnapshotCopy(long poolId,
22322230
throw new InvalidParameterValueException("Please specify a valid destination zone.");
22332231
}
22342232
if (!StoragePoolStatus.Up.equals(destPool.getStatus()) && !isRootAdmin) {
2235-
throw new PermissionDeniedException("Cannot perform this operation, the storage pool is not in Up state: " + destPool.getName());
2233+
throw new PermissionDeniedException("Cannot perform this operation, the storage pool is not in Up state or the user is not the Root Admin " + destPool.getName());
22362234
}
22372235
DataCenterVO destZone = dataCenterDao.findById(destPool.getDataCenterId());
22382236
if (DataCenter.Type.Edge.equals(destZone.getType())) {
@@ -2293,13 +2291,13 @@ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
22932291
List<Long> poolsToBeRemoved = new ArrayList<>();
22942292
for (Long poolId : poolIds) {
22952293
PrimaryDataStore dataStore = (PrimaryDataStore) dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
2296-
if (isObjectNull(dataStore == null, poolsToBeRemoved, poolId)) continue;
2294+
if (isDataStoreNull(dataStore == null, poolsToBeRemoved, poolId)) continue;
22972295

22982296
SnapshotInfo snapshotInfo = snapshotFactory.getSnapshot(snapshot.getId(), poolId, DataStoreRole.Primary);
22992297
if (isSnapshotExistsOnPool(snapshot, dataStore, snapshotInfo)) continue;
23002298

23012299
VolumeVO volume = _volsDao.findById(snapshot.getVolumeId());
2302-
if (isObjectNull(volume == null, poolsToBeRemoved, poolId)) continue;
2300+
if (isDataStoreNull(volume == null, poolsToBeRemoved, poolId)) continue;
23032301
doesStorageSupportCopySnapshot(poolsToBeRemoved, poolId, dataStore, volume);
23042302
}
23052303
poolIds.removeAll(poolsToBeRemoved);
@@ -2310,8 +2308,9 @@ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
23102308
}
23112309

23122310
private void doesStorageSupportCopySnapshot(List<Long> poolsToBeRemoved, Long poolId, PrimaryDataStore dataStore, VolumeVO volume) {
2313-
if (!dataStore.getDriver().getCapabilities()
2314-
.containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString())
2311+
if (dataStore.getDriver() != null
2312+
&& MapUtils.isNotEmpty(dataStore.getDriver().getCapabilities())
2313+
&& !dataStore.getDriver().getCapabilities().containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString())
23152314
&& dataStore.getPoolType() != volume.getPoolType()) {
23162315
poolsToBeRemoved.add(poolId);
23172316
logger.debug(String.format("The %s does not support copy to %s between zones", dataStore.getPoolType(), volume.getPoolType()));
@@ -2326,7 +2325,7 @@ private boolean isSnapshotExistsOnPool(Snapshot snapshot, PrimaryDataStore dataS
23262325
return false;
23272326
}
23282327

2329-
private static boolean isObjectNull(boolean object, List<Long> poolsToBeRemoved, Long poolId) {
2328+
private static boolean isDataStoreNull(boolean object, List<Long> poolsToBeRemoved, Long poolId) {
23302329
if (object) {
23312330
poolsToBeRemoved.add(poolId);
23322331
return true;

0 commit comments

Comments
 (0)