Skip to content

Commit c593127

Browse files
slavkapslavkap
committed
Address reviews
1 parent f92cd6b commit c593127

File tree

11 files changed

+26
-43
lines changed

11 files changed

+26
-43
lines changed

‎engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/DataStoreCapabilities.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,5 @@ public enum DataStoreCapabilities {
4444
/**
4545
* indicates that the driver supports copying snapshot between zones on pools of the same type
4646
*/
47-
CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE,
48-
/**
49-
* indicates that the storage does not need to delete the snapshot when creating a volume/template from it
50-
* and the setting `snapshot.backup.to.secondary` is enabled
51-
*/
52-
KEEP_SNAPSHOT_ON_PRIMARY_AND_BACKUP
47+
CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE
5348
}

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@
5555
import org.apache.cloudstack.engine.subsystem.api.storage.ChapInfo;
5656
import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
5757
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
58-
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities;
5958
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreDriver;
6059
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
6160
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
@@ -580,8 +579,8 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume, Snapshot snapshot, Use
580579

581580
boolean kvmSnapshotOnlyInPrimaryStorage = snapshotHelper.isKvmSnapshotOnlyInPrimaryStorage(snapshot, dataStoreRole, volume.getDataCenterId());
582581
boolean skipCopyToSecondary = false;
583-
Map<String, String> capabilities = snapInfo.getDataStore().getDriver().getCapabilities();
584-
boolean keepOnPrimary = MapUtils.isNotEmpty(capabilities) && capabilities.containsKey(DataStoreCapabilities.KEEP_SNAPSHOT_ON_PRIMARY_AND_BACKUP.toString());
582+
boolean keepOnPrimary = snapshotHelper.isStorPoolStorage(snapInfo);
583+
585584
if (kvmSnapshotOnlyInPrimaryStorage && keepOnPrimary) {
586585
skipCopyToSecondary = true;
587586
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -365,11 +365,15 @@ protected void runInContext() {
365365
SpConnectionDesc conn = StorPoolUtil.getSpConnection(storagePool.getUuid(),
366366
storagePool.getId(), storagePoolDetailsDao, storagePoolDao);
367367
JsonArray arr = StorPoolUtil.snapshotsList(conn);
368-
List<String> snapshots = snapshotsForRcovery(arr);
368+
List<String> snapshots = snapshotsForRecovery(arr);
369369
if (snapshots.isEmpty()) {
370370
continue;
371371
}
372372
for (SnapshotDetailsVO snapshot : snapshotDetails) {
373+
String[] snapshotOnRemote = snapshot.getValue().split(";");
374+
if (snapshotOnRemote.length != 2) {
375+
continue;
376+
}
373377
String name = snapshot.getValue().split(";")[0];
374378
String location = snapshot.getValue().split(";")[1];
375379
if (name == null || location == null) {
@@ -398,7 +402,7 @@ protected void runInContext() {
398402
}
399403
}
400404

401-
private static List<String> snapshotsForRcovery(JsonArray arr) {
405+
private static List<String> snapshotsForRecovery(JsonArray arr) {
402406
List<String> snapshots = new ArrayList<>();
403407
for (int i = 0; i < arr.size(); i++) {
404408
boolean recoveringFromRemote = arr.get(i).getAsJsonObject().get("recoveringFromRemote").getAsBoolean();

‎plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/driver/StorPoolPrimaryDataStoreDriver.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,6 @@ private SnapshotDataStoreVO getSnapshotImageStoreRef(long snapshotId, long zoneI
188188
public Map<String, String> getCapabilities() {
189189
Map<String, String> mapCapabilities = new HashMap<>();
190190
mapCapabilities.put(DataStoreCapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE.toString(), Boolean.TRUE.toString());
191-
mapCapabilities.put(DataStoreCapabilities.KEEP_SNAPSHOT_ON_PRIMARY_AND_BACKUP.toString(), Boolean.TRUE.toString());
192191
return mapCapabilities;
193192
}
194193

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

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717
package com.cloud.storage.snapshot;
1818

19+
import com.cloud.storage.StoragePoolStatus;
1920
import java.util.ArrayList;
2021
import java.util.Collections;
2122
import java.util.Comparator;
@@ -53,10 +54,12 @@
5354
import org.apache.cloudstack.context.CallContext;
5455
import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
5556
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
57+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities;
5658
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
5759
import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint;
5860
import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector;
5961
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine;
62+
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
6063
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
6164
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
6265
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotResult;
@@ -135,7 +138,6 @@
135138
import com.cloud.storage.Storage.StoragePoolType;
136139
import com.cloud.storage.StorageManager;
137140
import com.cloud.storage.StoragePool;
138-
import com.cloud.storage.StoragePoolStatus;
139141
import com.cloud.storage.VMTemplateStorageResourceAssoc;
140142
import com.cloud.storage.VMTemplateVO;
141143
import com.cloud.storage.Volume;
@@ -181,8 +183,6 @@
181183
import com.cloud.vm.snapshot.VMSnapshotVO;
182184
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
183185
import com.cloud.vm.snapshot.dao.VMSnapshotDetailsDao;
184-
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities;
185-
import org.apache.cloudstack.engine.subsystem.api.storage.PrimaryDataStore;
186186

187187
@Component
188188
public class SnapshotManagerImpl extends MutualExclusiveIdsManagerBase implements SnapshotManager, SnapshotApiService, Configurable {
@@ -1723,14 +1723,6 @@ private void copyNewSnapshotToZonesOnPrimary(CreateSnapshotPayload payload, Snap
17231723
private boolean copySnapshotOnPool(SnapshotInfo snapshot, SnapshotStrategy snapshotStrategy, Long storagePoolId) {
17241724
DataStore store = dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);
17251725
SnapshotInfo snapshotOnStore = (SnapshotInfo) store.create(snapshot);
1726-
// if (snapshotOnStore.getStatus() == ObjectInDataStoreStateMachine.State.Allocated) {
1727-
// snapshotOnStore.processEvent(Event.CreateOnlyRequested);
1728-
// } else if (snapshotOnStore.getStatus() == ObjectInDataStoreStateMachine.State.Ready) {
1729-
// snapshotOnStore.processEvent(Event.CopyRequested);
1730-
// } else {
1731-
// logger.info(String.format("Cannot copy snapshot to another storage in different zone. It's not in the right state %s", snapshotOnStore.getStatus()));
1732-
// return false;
1733-
// }
17341726

17351727
try {
17361728
AsyncCallFuture<SnapshotResult> future = snapshotSrv.copySnapshot(snapshot, snapshotOnStore, snapshotStrategy);
@@ -1751,10 +1743,6 @@ private boolean copySnapshotOnPool(SnapshotInfo snapshot, SnapshotStrategy snaps
17511743
} catch (ExecutionException e) {
17521744
throw new RuntimeException(e);
17531745
}
1754-
// boolean copySuccessful = snapshotStrategy.copySnapshot(snapshot, snapshotOnStore, null);
1755-
// if (!copySuccessful) {
1756-
// snapshotOnStore.processEvent(Event.OperationFailed);
1757-
// }
17581746
return true;
17591747
}
17601748

‎server/src/main/java/com/cloud/template/TemplateManagerImpl.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@
6565
import org.apache.cloudstack.context.CallContext;
6666
import org.apache.cloudstack.engine.orchestration.service.VolumeOrchestrationService;
6767
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
68-
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreCapabilities;
6968
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
7069
import org.apache.cloudstack.engine.subsystem.api.storage.EndPoint;
7170
import org.apache.cloudstack.engine.subsystem.api.storage.EndPointSelector;
@@ -1686,13 +1685,11 @@ public VirtualMachineTemplate createPrivateTemplate(CreateTemplateCmd command) t
16861685
boolean kvmIncrementalSnapshot = SnapshotManager.kvmIncrementalSnapshot.valueIn(_hostDao.findClusterIdByVolumeInfo(snapInfo.getBaseVolume()));
16871686

16881687
boolean skipCopyToSecondary = false;
1689-
1690-
Map<String, String> capabilities = snapInfo.getDataStore().getDriver().getCapabilities();
1691-
boolean keepOnPrimary = MapUtils.isNotEmpty(capabilities) && capabilities.containsKey(DataStoreCapabilities.KEEP_SNAPSHOT_ON_PRIMARY_AND_BACKUP.toString());
1688+
boolean keepOnPrimary = snapshotHelper.isStorPoolStorage(snapInfo);
16921689
if (kvmSnapshotOnlyInPrimaryStorage && keepOnPrimary) {
16931690
skipCopyToSecondary = true;
16941691
}
1695-
if (dataStoreRole == DataStoreRole.Image) {
1692+
if (dataStoreRole == DataStoreRole.Image || !skipCopyToSecondary) {
16961693
snapInfo = snapshotHelper.backupSnapshotToSecondaryStorageIfNotExists(snapInfo, dataStoreRole, snapshot, kvmSnapshotOnlyInPrimaryStorage);
16971694
_accountMgr.checkAccess(caller, null, true, snapInfo);
16981695
DataStore snapStore = snapInfo.getDataStore();

‎server/src/main/java/org/apache/cloudstack/snapshot/SnapshotHelper.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,11 @@ public void expungeTemporarySnapshot(boolean kvmSnapshotOnlyInPrimaryStorage, Sn
100100
long storeId = snapInfo.getDataStore().getId();
101101
long zoneId = dataStorageManager.getStoreZoneId(storeId, snapInfo.getDataStore().getRole());
102102

103-
Map<String, String> capabilities = snapInfo.getDataStore().getDriver().getCapabilities();
104-
boolean keepOnPrimary = MapUtils.isNotEmpty(capabilities) && capabilities.containsKey(DataStoreCapabilities.KEEP_SNAPSHOT_ON_PRIMARY_AND_BACKUP.toString());
105-
if (keepOnPrimary) {
103+
if (isStorPoolStorage(snapInfo)) {
106104
logger.debug("The primary storage does not delete the snapshots even if there is a backup on secondary");
107105
return;
108106
}
107+
109108
List<SnapshotJoinVO> snapshots = snapshotJoinDao.listBySnapshotIdAndZoneId(zoneId, snapInfo.getSnapshotId());
110109
if (kvmSnapshotOnlyInPrimaryStorage || snapshots.size() <= 1) {
111110
if (snapInfo != null) {
@@ -147,6 +146,13 @@ public void expungeTemporarySnapshot(boolean kvmSnapshotOnlyInPrimaryStorage, Sn
147146
snapshotDataStoreDao.expungeReferenceBySnapshotIdAndDataStoreRole(snapInfo.getId(), storeId, DataStoreRole.Image);
148147
}
149148

149+
public boolean isStorPoolStorage(SnapshotInfo snapInfo) {
150+
if (DataStoreRole.Primary.equals(snapInfo.getDataStore().getRole())) {
151+
StoragePoolVO storagePoolVO = primaryDataStoreDao.findById(snapInfo.getDataStore().getId());
152+
return storagePoolVO != null && StoragePoolType.StorPool.equals(storagePoolVO.getPoolType());
153+
}
154+
return false;
155+
}
150156
/**
151157
* Backup the snapshot to secondary storage if it should be backed up and was not yet or it is a temporary backup to create a volume.
152158
* @return The parameter snapInfo if the snapshot is not backupable, else backs up the snapshot to secondary storage and returns its info.

‎server/src/test/java/org/apache/cloudstack/snapshot/SnapshotHelperTest.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@
4646

4747
import java.util.ArrayList;
4848
import java.util.Arrays;
49-
import java.util.HashMap;
5049
import java.util.HashSet;
5150
import java.util.List;
5251
import java.util.Set;
@@ -108,8 +107,6 @@ public void validateExpungeTemporarySnapshotNotAKvmSnapshotOnPrimaryStorageDoNot
108107
Mockito.when(snapshotInfoMock.getDataStore()).thenReturn(store);
109108
Mockito.when(snapshotInfoMock.getDataStore().getId()).thenReturn(1L);
110109
Mockito.when(snapshotInfoMock.getSnapshotId()).thenReturn(1L);
111-
Mockito.when(snapshotInfoMock.getDataStore().getDriver()).thenReturn(storeDriver);
112-
Mockito.when(snapshotInfoMock.getDataStore().getDriver().getCapabilities()).thenReturn(new HashMap<>());
113110
snapshotHelperSpy.expungeTemporarySnapshot(false, snapshotInfoMock);
114111
Mockito.verifyNoInteractions(snapshotServiceMock, snapshotDataStoreDaoMock);
115112
}
@@ -122,8 +119,6 @@ public void validateExpungeTemporarySnapshotKvmSnapshotOnPrimaryStorageExpungesS
122119
Mockito.when(store.getRole()).thenReturn(DataStoreRole.Image);
123120
Mockito.when(store.getId()).thenReturn(1L);
124121
Mockito.when(snapshotInfoMock.getDataStore()).thenReturn(store);
125-
Mockito.when(snapshotInfoMock.getDataStore().getDriver()).thenReturn(storeDriver);
126-
Mockito.when(snapshotInfoMock.getDataStore().getDriver().getCapabilities()).thenReturn(new HashMap<>());
127122
snapshotHelperSpy.expungeTemporarySnapshot(true, snapshotInfoMock);
128123
}
129124

‎ui/src/views/storage/FormSchedule.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ export default {
359359
const listStoragePools = json.liststoragepoolsresponse.storagepool
360360
if (listStoragePools) {
361361
this.storagePools = listStoragePools
362-
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES && pool.zoneid !== this.resource.zoneid)
362+
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE && pool.zoneid !== this.resource.zoneid)
363363
}
364364
}).finally(() => {
365365
this.storagePoolsLoading = false

‎ui/src/views/storage/SnapshotZones.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,7 @@ export default {
532532
const listStoragePools = json.liststoragepoolsresponse.storagepool
533533
if (listStoragePools) {
534534
this.storagePools = listStoragePools
535-
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES && pool.zoneid !== this.resource.zoneid)
535+
this.storagePools = this.storagePools.filter(pool => pool.storagecapabilities.CAN_COPY_SNAPSHOT_BETWEEN_ZONES_AND_SAME_POOL_TYPE && pool.zoneid !== this.resource.zoneid)
536536
}
537537
}).finally(() => {
538538
this.storagePoolsLoading = false

0 commit comments

Comments
 (0)