Skip to content

Commit 8eca04e

Browse files
subhash yedugundlayadvr
subhash yedugundla
authored and
yadvr
committed
CLOUDSTACK-9572: Snapshot on primary storage not cleaned up after Storage migration (#1740)
Snapshot on primary storage not cleaned up after Storage migration. This happens in the following scenario: Steps To Reproduce Create an instance on the local storage on any host Create a scheduled snapshot of the volume: Wait until ACS created the snapshot. ACS is creating a snapshot on local storage and is transferring this snapshot to secondary storage. But the latest snapshot on local storage will stay there. This is as expected. Migrate the instance to another XenServer host with ACS UI and Storage Live Migration The Snapshot on the old host on local storage will not be cleaned up and is staying on local storage. So local storage will fill up with unneeded snapshots.
1 parent 053b12c commit 8eca04e

6 files changed

Lines changed: 74 additions & 30 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
@@ -64,6 +64,24 @@ public SnapshotInfo getSnapshot(DataObject obj, DataStore store) {
6464
return so;
6565
}
6666

67+
@Override
68+
public List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole role) {
69+
70+
SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByVolume(volumeId, role);
71+
if (snapshotStore == null) {
72+
return new ArrayList<>();
73+
}
74+
DataStore store = storeMgr.getDataStore(snapshotStore.getDataStoreId(), role);
75+
List<SnapshotVO> volSnapShots = snapshotDao.listByVolumeId(volumeId);
76+
List<SnapshotInfo> infos = new ArrayList<>();
77+
for(SnapshotVO snapshot: volSnapShots) {
78+
SnapshotObject info = SnapshotObject.getSnapshotObject(snapshot, store);
79+
infos.add(info);
80+
}
81+
return infos;
82+
}
83+
84+
6785
@Override
6886
public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) {
6987
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
@@ -1543,6 +1543,7 @@ protected Void migrateVolumeCallBack(AsyncCallbackDispatcher<VolumeServiceImpl,
15431543
future.complete(res);
15441544
} else {
15451545
srcVolume.processEvent(Event.OperationSuccessed);
1546+
snapshotMgr.cleanupSnapshotsByVolume(srcVolume.getId());
15461547
future.complete(res);
15471548
}
15481549
} catch (Exception e) {
@@ -1624,6 +1625,7 @@ public AsyncCallFuture<CommandResult> migrateVolumes(Map<VolumeInfo, DataStore>
16241625
} else {
16251626
for (Map.Entry<VolumeInfo, DataStore> entry : volumeToPool.entrySet()) {
16261627
VolumeInfo volume = entry.getKey();
1628+
snapshotMgr.cleanupSnapshotsByVolume(volume.getId());
16271629
volume.processEvent(Event.OperationSuccessed);
16281630
}
16291631
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
@@ -74,6 +74,8 @@ public interface SnapshotManager extends Configurable {
7474

7575
boolean canOperateOnVolume(Volume volume);
7676

77+
void cleanupSnapshotsByVolume(Long volumeId);
78+
7779
Answer sendToPool(Volume vol, Command cmd);
7880

7981
SnapshotVO getParentSnapshot(VolumeInfo volume);

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1339,6 +1339,21 @@ public boolean canOperateOnVolume(Volume volume) {
13391339
return true;
13401340
}
13411341

1342+
@Override
1343+
public void cleanupSnapshotsByVolume(Long volumeId) {
1344+
List<SnapshotInfo> infos = snapshotFactory.getSnapshots(volumeId, DataStoreRole.Primary);
1345+
for(SnapshotInfo info: infos) {
1346+
try {
1347+
if(info != null) {
1348+
snapshotSrv.deleteSnapshot(info);
1349+
}
1350+
} catch(CloudRuntimeException e) {
1351+
String msg = "Cleanup of Snapshot with uuid " + info.getUuid() + " in primary storage is failed. Ignoring";
1352+
s_logger.warn(msg);
1353+
}
1354+
}
1355+
}
1356+
13421357
@Override
13431358
public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName, Snapshot.LocationType locationType) throws ResourceAllocationException {
13441359
Account caller = CallContext.current().getCallingAccount();

‎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)