Skip to content

Commit 41a9eca

Browse files
subhash yedugundlasubhash_y
subhash yedugundla
authored and
subhash_y
committed
CLOUDSTACK-9572 Snapshot on primary storage not cleaned up after Storage migration
1 parent 61ce75e commit 41a9eca

6 files changed

Lines changed: 74 additions & 33 deletions

File tree

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ public interface SnapshotDataFactory {
2929

3030
SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role);
3131

32+
List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole store);
33+
3234
List<SnapshotInfo> listSnapshotOnCache(long snapshotId);
3335

3436
SnapshotInfo getReadySnapshotOnCache(long snapshotId);

‎engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,24 @@ public SnapshotInfo getSnapshot(DataObject obj, DataStore store) {
6767
return so;
6868
}
6969

70+
@Override
71+
public List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole role) {
72+
73+
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByVolume(volumeId, role);
74+
if (snapshotStore == null) {
75+
return null;
76+
}
77+
DataStore store = storeMgr.getDataStore(snapshotStore.getDataStoreId(), role);
78+
List<SnapshotVO> volSnapShots = snapshotDao.listByVolumeId(volumeId);
79+
List<SnapshotInfo> infos = new ArrayList<>();
80+
for(SnapshotVO snapshot: volSnapShots) {
81+
SnapshotObject info = SnapshotObject.getSnapshotObject(snapshot, store);
82+
infos.add(info);
83+
}
84+
return infos;
85+
}
86+
87+
7088
@Override
7189
public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) {
7290
SnapshotVO snapshot = snapshotDao.findById(snapshotId);

‎engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1507,6 +1507,7 @@ protected Void migrateVolumeCallBack(AsyncCallbackDispatcher<VolumeServiceImpl,
15071507
future.complete(res);
15081508
} else {
15091509
srcVolume.processEvent(Event.OperationSuccessed);
1510+
snapshotMgr.cleanupSnapshotsByVolume(srcVolume.getId());
15101511
future.complete(res);
15111512
}
15121513
} catch (Exception e) {
@@ -1588,6 +1589,7 @@ public AsyncCallFuture<CommandResult> migrateVolumes(Map<VolumeInfo, DataStore>
15881589
} else {
15891590
for (Map.Entry<VolumeInfo, DataStore> entry : volumeToPool.entrySet()) {
15901591
VolumeInfo volume = entry.getKey();
1592+
snapshotMgr.cleanupSnapshotsByVolume(volume.getId());
15911593
volume.processEvent(Event.OperationSuccessed);
15921594
}
15931595
future.complete(res);

‎server/src/com/cloud/storage/snapshot/SnapshotManager.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ public interface SnapshotManager {
5656

5757
boolean canOperateOnVolume(Volume volume);
5858

59+
void cleanupSnapshotsByVolume(Long volumeId);
60+
5961
Answer sendToPool(Volume vol, Command cmd);
6062

6163
SnapshotVO getParentSnapshot(VolumeInfo volume);

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,6 +1246,21 @@ public boolean canOperateOnVolume(Volume volume) {
12461246
return true;
12471247
}
12481248

1249+
@Override
1250+
public void cleanupSnapshotsByVolume(Long volumeId) {
1251+
List<SnapshotInfo> infos = snapshotFactory.getSnapshots(volumeId, DataStoreRole.Primary);
1252+
for(SnapshotInfo info: infos) {
1253+
try {
1254+
if(info != null) {
1255+
snapshotSrv.deleteSnapshot(info);
1256+
}
1257+
} catch(CloudRuntimeException e) {
1258+
String msg = "Cleanup of Snapshot with uuid " + info.getUuid() + " in primary storage is failed. Ignoring";
1259+
s_logger.warn(msg);
1260+
}
1261+
}
1262+
}
1263+
12491264
@Override
12501265
public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName, Snapshot.LocationType locationType) throws ResourceAllocationException {
12511266
Account caller = CallContext.current().getCallingAccount();
@@ -1308,7 +1323,4 @@ public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName,
13081323
_resourceLimitMgr.incrementResourceCount(volume.getAccountId(), ResourceType.secondary_storage, new Long(volume.getSize()));
13091324
return snapshot;
13101325
}
1311-
1312-
1313-
13141326
}

‎server/test/com/cloud/storage/snapshot/SnapshotManagerTest.java‎

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

19+
import static org.mockito.Matchers.any;
20+
import static org.mockito.Matchers.anyLong;
21+
import static org.mockito.Mockito.doNothing;
22+
import static org.mockito.Mockito.mock;
23+
import static org.mockito.Mockito.when;
24+
25+
import java.util.List;
26+
import java.util.UUID;
27+
28+
import org.apache.cloudstack.acl.ControlledEntity;
29+
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
30+
import org.apache.cloudstack.context.CallContext;
31+
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
32+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
33+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
34+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
35+
import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
36+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
37+
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
38+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
39+
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
40+
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
41+
import org.junit.After;
42+
import org.junit.Assert;
43+
import org.junit.Before;
44+
import org.junit.Test;
45+
import org.mockito.Mock;
46+
import org.mockito.Mockito;
47+
import org.mockito.MockitoAnnotations;
48+
import org.mockito.Spy;
49+
1950
import com.cloud.configuration.Resource.ResourceType;
2051
import com.cloud.exception.InvalidParameterValueException;
2152
import com.cloud.exception.ResourceAllocationException;
@@ -44,38 +75,10 @@
4475
import com.cloud.vm.snapshot.VMSnapshot;
4576
import com.cloud.vm.snapshot.VMSnapshotVO;
4677
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
47-
import org.apache.cloudstack.acl.ControlledEntity;
48-
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
49-
import org.apache.cloudstack.context.CallContext;
50-
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
51-
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
52-
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
53-
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
54-
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
55-
import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
56-
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
57-
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
58-
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
5978
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
6079
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
61-
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
62-
import org.junit.After;
63-
import org.junit.Assert;
64-
import org.junit.Before;
65-
import org.junit.Test;
66-
import org.mockito.Mock;
67-
import org.mockito.Mockito;
68-
import org.mockito.MockitoAnnotations;
69-
import org.mockito.Spy;
80+
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;
7081

71-
import java.util.List;
72-
import java.util.UUID;
73-
74-
import static org.mockito.Matchers.any;
75-
import static org.mockito.Matchers.anyLong;
76-
import static org.mockito.Mockito.doNothing;
77-
import static org.mockito.Mockito.mock;
78-
import static org.mockito.Mockito.when;
7982

8083
public class SnapshotManagerTest {
8184
@Spy
@@ -126,6 +129,9 @@ public class SnapshotManagerTest {
126129
SnapshotDataStoreDao _snapshotStoreDao;
127130
@Mock
128131
SnapshotDataStoreVO snapshotStoreMock;
132+
@Mock
133+
SnapshotService snapshotSrv;
134+
129135

130136
private static final long TEST_SNAPSHOT_ID = 3L;
131137
private static final long TEST_VOLUME_ID = 4L;
@@ -330,5 +336,4 @@ public void testBackupSnapshotFromVmSnapshotF3() {
330336
Snapshot snapshot = _snapshotMgr.backupSnapshotFromVmSnapshot(TEST_SNAPSHOT_ID, TEST_VM_ID, TEST_VOLUME_ID, TEST_VM_SNAPSHOT_ID);
331337
Assert.assertNull(snapshot);
332338
}
333-
334339
}

0 commit comments

Comments
 (0)