Skip to content

CLOUDSTACK-9572 Snapshot on primary storage not cleaned up after Stor… #1740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yadvr merged 1 commit into from
Jan 5, 2018
Merged

CLOUDSTACK-9572 Snapshot on primary storage not cleaned up after Stor… #1740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotDataFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ public interface SnapshotDataFactory {

SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role);

List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole store);

List<SnapshotInfo> listSnapshotOnCache(long snapshotId);

SnapshotInfo getReadySnapshotOnCache(long snapshotId);
Expand Down
18 changes: 18 additions & 0 deletions .../storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotDataFactoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,24 @@ public SnapshotInfo getSnapshot(DataObject obj, DataStore store) {
return so;
}

@Override
public List<SnapshotInfo> getSnapshots(long volumeId, DataStoreRole role) {

SnapshotDataStoreVO snapshotStore = snapshotStoreDao.findByVolume(volumeId, role);
if (snapshotStore == null) {
return new ArrayList<>();
}
DataStore store = storeMgr.getDataStore(snapshotStore.getDataStoreId(), role);
List<SnapshotVO> volSnapShots = snapshotDao.listByVolumeId(volumeId);
List<SnapshotInfo> infos = new ArrayList<>();
for(SnapshotVO snapshot: volSnapShots) {
SnapshotObject info = SnapshotObject.getSnapshotObject(snapshot, store);
infos.add(info);
}
return infos;
}


@Override
public SnapshotInfo getSnapshot(long snapshotId, DataStoreRole role) {
SnapshotVO snapshot = snapshotDao.findById(snapshotId);
Expand Down
2 changes: 2 additions & 0 deletions engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,7 @@ protected Void migrateVolumeCallBack(AsyncCallbackDispatcher<VolumeServiceImpl,
future.complete(res);
} else {
srcVolume.processEvent(Event.OperationSuccessed);
snapshotMgr.cleanupSnapshotsByVolume(srcVolume.getId());
future.complete(res);
}
} catch (Exception e) {
Expand Down Expand Up @@ -1624,6 +1625,7 @@ public AsyncCallFuture<CommandResult> migrateVolumes(Map<VolumeInfo, DataStore>
} else {
for (Map.Entry<VolumeInfo, DataStore> entry : volumeToPool.entrySet()) {
VolumeInfo volume = entry.getKey();
snapshotMgr.cleanupSnapshotsByVolume(volume.getId());
volume.processEvent(Event.OperationSuccessed);
}
future.complete(res);
Expand Down
2 changes: 2 additions & 0 deletions server/src/com/cloud/storage/snapshot/SnapshotManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ public interface SnapshotManager extends Configurable {

boolean canOperateOnVolume(Volume volume);

void cleanupSnapshotsByVolume(Long volumeId);

Answer sendToPool(Volume vol, Command cmd);

SnapshotVO getParentSnapshot(VolumeInfo volume);
Expand Down
15 changes: 15 additions & 0 deletions server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,21 @@ public boolean canOperateOnVolume(Volume volume) {
return true;
}

@Override
public void cleanupSnapshotsByVolume(Long volumeId) {
List<SnapshotInfo> infos = snapshotFactory.getSnapshots(volumeId, DataStoreRole.Primary);
for(SnapshotInfo info: infos) {
try {
if(info != null) {
Copy link
Copy Markdown
Member

@rafaelweingartner rafaelweingartner Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this null check?
This only happen if one uses the set method of List object to replace a position with a Null value

snapshotSrv.deleteSnapshot(info);
}
} catch(CloudRuntimeException e) {
String msg = "Cleanup of Snapshot with uuid " + info.getUuid() + " in primary storage is failed. Ignoring";
s_logger.warn(msg);
}
}
}

@Override
public Snapshot allocSnapshot(Long volumeId, Long policyId, String snapshotName, Snapshot.LocationType locationType) throws ResourceAllocationException {
Account caller = CallContext.current().getCallingAccount();
Expand Down
65 changes: 35 additions & 30 deletions server/test/com/cloud/storage/snapshot/SnapshotManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,37 @@
// under the License.
package com.cloud.storage.snapshot;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.List;
import java.util.UUID;

import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.mockito.Spy;

import com.cloud.configuration.Resource.ResourceType;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.ResourceAllocationException;
Expand Down Expand Up @@ -44,38 +75,10 @@
import com.cloud.vm.snapshot.VMSnapshot;
import com.cloud.vm.snapshot.VMSnapshotVO;
import com.cloud.vm.snapshot.dao.VMSnapshotDao;
import org.apache.cloudstack.acl.ControlledEntity;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation;
import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.mockito.Spy;
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotService;

import java.util.List;
import java.util.UUID;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class SnapshotManagerTest {
@Spy
Expand Down Expand Up @@ -126,6 +129,9 @@ public class SnapshotManagerTest {
SnapshotDataStoreDao _snapshotStoreDao;
@Mock
SnapshotDataStoreVO snapshotStoreMock;
@Mock
SnapshotService snapshotSrv;


private static final long TEST_SNAPSHOT_ID = 3L;
private static final long TEST_VOLUME_ID = 4L;
Expand Down Expand Up @@ -330,5 +336,4 @@ public void testBackupSnapshotFromVmSnapshotF3() {
Snapshot snapshot = _snapshotMgr.backupSnapshotFromVmSnapshot(TEST_SNAPSHOT_ID, TEST_VM_ID, TEST_VOLUME_ID, TEST_VM_SNAPSHOT_ID);
Assert.assertNull(snapshot);
}

}