Skip to content

Commit 81529e4

Browse files
slavkapslavkap
committed
Addressed reviews
1 parent f33b615 commit 81529e4

File tree

10 files changed

+82
-67
lines changed

10 files changed

+82
-67
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
@@ -583,10 +583,10 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
583583
SnapshotInfo snapInfo = snapshotFactory.getSnapshotWithRoleAndZone(snapshot.getId(), dataStoreRole, zoneId);
584584

585585
boolean kvmSnapshotOnlyInPrimaryStorage = snapshotHelper.isKvmSnapshotOnlyInPrimaryStorage(snapshot, dataStoreRole, volume.getDataCenterId());
586-
boolean keepOnPrimary = snapshotHelper.isStorageSupportSnapshotToTemplate(snapInfo);
586+
boolean storageSupportSnapshotToTemplateEnabled = snapshotHelper.isStorageSupportSnapshotToTemplate(snapInfo);
587587

588588
try {
589-
if (!keepOnPrimary) {
589+
if (!storageSupportSnapshotToTemplateEnabled) {
590590
snapInfo = snapshotHelper.backupSnapshotToSecondaryStorageIfNotExists(snapInfo, dataStoreRole, snapshot, kvmSnapshotOnlyInPrimaryStorage);
591591
}
592592
} catch (CloudRuntimeException e) {
@@ -600,7 +600,7 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
600600
}
601601

602602
// don't try to perform a sync if the DataStoreRole of the snapshot is equal to DataStoreRole.Primary
603-
if (!DataStoreRole.Primary.equals(dataStoreRole) || !keepOnPrimary) {
603+
if (!DataStoreRole.Primary.equals(dataStoreRole) || !storageSupportSnapshotToTemplateEnabled) {
604604
try {
605605
// sync snapshot to region store if necessary
606606
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
@@ -360,7 +360,11 @@
360360
import com.cloud.vm.snapshot.VMSnapshot;
361361
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
362362

363+
import org.apache.logging.log4j.LogManager;
364+
import org.apache.logging.log4j.Logger;
365+
363366
public class ApiDBUtils {
367+
private static final Logger log = LogManager.getLogger(ApiDBUtils.class);
364368
private static ManagementServer s_ms;
365369
static AsyncJobManager s_asyncMgr;
366370
static SecurityGroupManager s_securityGroupMgr;
@@ -1723,7 +1727,9 @@ public static List<StoragePoolVO> findSnapshotPolicyPools(SnapshotPolicy policy,
17231727
for (SnapshotPolicyDetailVO detail : poolDetails) {
17241728
try {
17251729
poolIds.add(Long.valueOf(detail.getValue()));
1726-
} catch (NumberFormatException ignored) {}
1730+
} catch (NumberFormatException ignored) {
1731+
log.debug(String.format("Could not parse the storage ID value of %s", detail.getValue()), ignored);
1732+
}
17271733
}
17281734
if (volume != null && !poolIds.contains(volume.getPoolId())) {
17291735
poolIds.add(0, volume.getPoolId());

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

Lines changed: 8 additions & 8 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;
@@ -39,6 +37,14 @@
3937

4038
import javax.inject.Inject;
4139

40+
import com.cloud.dc.Pod;
41+
import com.cloud.dc.dao.DataCenterDao;
42+
import com.cloud.dc.dao.HostPodDao;
43+
import com.cloud.org.Cluster;
44+
import com.cloud.server.ManagementService;
45+
import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
46+
import com.cloud.cluster.ManagementServerHostPeerJoinVO;
47+
4248
import org.apache.cloudstack.acl.ControlledEntity;
4349
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
4450
import org.apache.cloudstack.acl.SecurityChecker;
@@ -236,11 +242,8 @@
236242
import com.cloud.dc.ClusterVO;
237243
import com.cloud.dc.DataCenter;
238244
import com.cloud.dc.DedicatedResourceVO;
239-
import com.cloud.dc.Pod;
240245
import com.cloud.dc.dao.ClusterDao;
241-
import com.cloud.dc.dao.DataCenterDao;
242246
import com.cloud.dc.dao.DedicatedResourceDao;
243-
import com.cloud.dc.dao.HostPodDao;
244247
import com.cloud.domain.Domain;
245248
import com.cloud.domain.DomainVO;
246249
import com.cloud.domain.dao.DomainDao;
@@ -278,7 +281,6 @@
278281
import com.cloud.network.vo.PublicIpQuarantineVO;
279282
import com.cloud.offering.DiskOffering;
280283
import com.cloud.offering.ServiceOffering;
281-
import com.cloud.org.Cluster;
282284
import com.cloud.org.Grouping;
283285
import com.cloud.projects.Project;
284286
import com.cloud.projects.Project.ListProjectResourcesCriteria;
@@ -290,7 +292,6 @@
290292
import com.cloud.projects.dao.ProjectInvitationDao;
291293
import com.cloud.resource.ResourceManager;
292294
import com.cloud.resource.icon.dao.ResourceIconDao;
293-
import com.cloud.server.ManagementService;
294295
import com.cloud.server.ResourceManagerUtil;
295296
import com.cloud.server.ResourceMetaDataService;
296297
import com.cloud.server.ResourceTag;
@@ -320,7 +321,6 @@
320321
import com.cloud.storage.dao.BucketDao;
321322
import com.cloud.storage.dao.DiskOfferingDao;
322323
import com.cloud.storage.dao.GuestOSDao;
323-
import com.cloud.storage.dao.StoragePoolAndAccessGroupMapDao;
324324
import com.cloud.storage.dao.StoragePoolHostDao;
325325
import com.cloud.storage.dao.StoragePoolTagsDao;
326326
import com.cloud.storage.dao.VMTemplateDao;

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

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1741,10 +1741,8 @@ private boolean copySnapshotOnPool(SnapshotInfo snapshot, SnapshotStrategy snaps
17411741
UsageEventUtils.publishUsageEvent(EventTypes.EVENT_SNAPSHOT_COPY, CallContext.current().getCallingUserId(), snapshotOnStore.getDataCenterId(), snapshotVO.getId(), null, null, null, snapshotVO.getSize(),
17421742
snapshotVO.getSize(), snapshotVO.getClass().getName(), snapshotVO.getUuid());
17431743
}
1744-
} catch (InterruptedException e) {
1745-
throw new RuntimeException(e);
1746-
} catch (ExecutionException e) {
1747-
throw new RuntimeException(e);
1744+
} catch (InterruptedException | ExecutionException e) {
1745+
throw new RuntimeException("Could not copy the snapshot to another pool", e);
17481746
}
17491747
return true;
17501748
}
@@ -2234,7 +2232,7 @@ protected StoragePoolVO getCheckedDestinationStorageForSnapshotCopy(long poolId,
22342232
throw new InvalidParameterValueException("Please specify a valid destination zone.");
22352233
}
22362234
if (!StoragePoolStatus.Up.equals(destPool.getStatus()) && !isRootAdmin) {
2237-
throw new PermissionDeniedException("Cannot perform this operation, the storage pool is not in Up state: " + destPool.getName());
2235+
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());
22382236
}
22392237
DataCenterVO destZone = dataCenterDao.findById(destPool.getDataCenterId());
22402238
if (DataCenter.Type.Edge.equals(destZone.getType())) {
@@ -2295,13 +2293,13 @@ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
22952293
List<Long> poolsToBeRemoved = new ArrayList<>();
22962294
for (Long poolId : poolIds) {
22972295
PrimaryDataStore dataStore = (PrimaryDataStore) dataStoreMgr.getDataStore(poolId, DataStoreRole.Primary);
2298-
if (isObjectNull(dataStore == null, poolsToBeRemoved, poolId)) continue;
2296+
if (isDataStoreNull(dataStore == null, poolsToBeRemoved, poolId)) continue;
22992297

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

23032301
VolumeVO volume = _volsDao.findById(snapshot.getVolumeId());
2304-
if (isObjectNull(volume == null, poolsToBeRemoved, poolId)) continue;
2302+
if (isDataStoreNull(volume == null, poolsToBeRemoved, poolId)) continue;
23052303
doesStorageSupportCopySnapshot(poolsToBeRemoved, poolId, dataStore, volume);
23062304
}
23072305
poolIds.removeAll(poolsToBeRemoved);
@@ -2312,8 +2310,9 @@ private boolean canCopyOnPrimary(List<Long> poolIds, Snapshot snapshot) {
23122310
}
23132311

23142312
private void doesStorageSupportCopySnapshot(List<Long> poolsToBeRemoved, Long poolId, PrimaryDataStore dataStore, VolumeVO volume) {
2315-
if (!dataStore.getDriver().getCapabilities()
2316-
.containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString())
2313+
if (dataStore.getDriver() != null
2314+
&& MapUtils.isNotEmpty(dataStore.getDriver().getCapabilities())
2315+
&& !dataStore.getDriver().getCapabilities().containsKey(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString())
23172316
&& dataStore.getPoolType() != volume.getPoolType()) {
23182317
poolsToBeRemoved.add(poolId);
23192318
logger.debug(String.format("The %s does not support copy to %s between zones", dataStore.getPoolType(), volume.getPoolType()));
@@ -2328,7 +2327,7 @@ private boolean isSnapshotExistsOnPool(Snapshot snapshot, PrimaryDataStore dataS
23282327
return false;
23292328
}
23302329

2331-
private static boolean isObjectNull(boolean object, List<Long> poolsToBeRemoved, Long poolId) {
2330+
private static boolean isDataStoreNull(boolean object, List<Long> poolsToBeRemoved, Long poolId) {
23322331
if (object) {
23332332
poolsToBeRemoved.add(poolId);
23342333
return true;

0 commit comments

Comments
 (0)