Skip to content

Garbage snapshots are left on Primary and Secondary #3649

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

Closed
GabrielBrascher wants to merge 6 commits into from
Closed

Garbage snapshots are left on Primary and Secondary #3649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
63135f3
Fixes snapshot deletion
GabrielBrascher Oct 17, 2019
a0269b6
Remove legacy '@Component', it is not necessary in this bean/class.
GabrielBrascher Oct 21, 2019
39c1b84
Fix log message missing %d and remove snapshot on DB
GabrielBrascher Nov 25, 2019
6da9881
Remove "dummy" boolean return statement
GabrielBrascher Nov 27, 2019
c9c1888
Manage snapshot deletion for KVM + NFS (primary storage)
GabrielBrascher Mar 13, 2020
7d9d608
checkstyle trailing spaces
GabrielBrascher Mar 16, 2020
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: 1 addition & 1 deletion engine/storage/integration-test/src/test/resources/fakeDriverTestContext.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
<bean id="dataStoreProviderManagerImpl" class="org.apache.cloudstack.storage.datastore.provider.DataStoreProviderManagerImpl" />
<bean id="storageCacheManagerImpl" class="org.apache.cloudstack.storage.cache.manager.StorageCacheManagerImpl" />
<bean id="storageCacheRandomAllocator" class="org.apache.cloudstack.storage.cache.allocator.StorageCacheRandomAllocator" />
<bean id="xenserverSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
<bean id="defaultSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />
<bean id="bAREMETAL" class="org.apache.cloudstack.storage.image.format.BAREMETAL" />
<bean id="dataMotionServiceImpl" class="org.apache.cloudstack.storage.motion.DataMotionServiceImpl" />
<bean id="dataObjectManagerImpl" class="org.apache.cloudstack.storage.datastore.DataObjectManagerImpl" />
Expand Down
2 changes: 1 addition & 1 deletion engine/storage/integration-test/src/test/resources/storageContext.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
<bean id="ancientDataMotionStrategy" class="org.apache.cloudstack.storage.motion.AncientDataMotionStrategy" />
<bean id="storageCacheManagerImpl" class="org.apache.cloudstack.storage.cache.manager.StorageCacheManagerImpl" />
<bean id="storageCacheRandomAllocator" class="org.apache.cloudstack.storage.cache.allocator.StorageCacheRandomAllocator" />
<bean id="xenserverSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
<bean id="defaultSnapshotStrategy" class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />
<bean id="bAREMETAL" class="org.apache.cloudstack.storage.image.format.BAREMETAL" />
<bean id="dataMotionServiceImpl" class="org.apache.cloudstack.storage.motion.DataMotionServiceImpl" />
<bean id="dataObjectManagerImpl" class="org.apache.cloudstack.storage.datastore.DataObjectManagerImpl" />
Expand Down
125 changes: 71 additions & 54 deletions ...e/snapshot/XenserverSnapshotStrategy.java → ...age/snapshot/DefaultSnapshotStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import javax.inject.Inject;

import org.apache.log4j.Logger;
import org.springframework.stereotype.Component;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event;
Expand All @@ -34,7 +33,6 @@
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
import org.apache.cloudstack.framework.jobs.AsyncJob;
import org.apache.cloudstack.framework.jobs.dao.SyncQueueItemDao;
import org.apache.cloudstack.storage.command.CreateObjectAnswer;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
Expand Down Expand Up @@ -71,9 +69,8 @@
import com.cloud.utils.exception.CloudRuntimeException;
import com.cloud.utils.fsm.NoTransitionException;

@Component
public class XenserverSnapshotStrategy extends SnapshotStrategyBase {
private static final Logger s_logger = Logger.getLogger(XenserverSnapshotStrategy.class);
public class DefaultSnapshotStrategy extends SnapshotStrategyBase {
private static final Logger s_logger = Logger.getLogger(DefaultSnapshotStrategy.class);

@Inject
SnapshotService snapshotSvr;
Expand All @@ -90,12 +87,8 @@ public class XenserverSnapshotStrategy extends SnapshotStrategyBase {
@Inject
SnapshotDataFactory snapshotDataFactory;
@Inject
private SnapshotDao _snapshotDao;
@Inject
private SnapshotDetailsDao _snapshotDetailsDao;
@Inject
private SyncQueueItemDao _syncQueueItemDao;
@Inject
VolumeDetailsDao _volumeDetailsDaoImpl;

@Override
Expand Down Expand Up @@ -269,63 +262,87 @@ public boolean deleteSnapshot(Long snapshotId) {
throw new InvalidParameterValueException("Can't delete snapshotshot " + snapshotId + " due to it is in " + snapshotVO.getState() + " Status");
}

// first mark the snapshot as destroyed, so that ui can't see it, but we
// may not destroy the snapshot on the storage, as other snapshots may
// depend on it.
SnapshotInfo snapshotOnImage = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Image);
if (snapshotOnImage == null) {
s_logger.debug("Can't find snapshot on backup storage, delete it in db");
snapshotDao.remove(snapshotId);
return true;
}

SnapshotObject obj = (SnapshotObject)snapshotOnImage;
try {
obj.processEvent(Snapshot.Event.DestroyRequested);
List<VolumeDetailVO> volumesFromSnapshot;
volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);

if (volumesFromSnapshot.size() > 0) {
try {
obj.processEvent(Snapshot.Event.OperationFailed);
} catch (NoTransitionException e1) {
s_logger.debug("Failed to change snapshot state: " + e1.toString());
boolean deletedOnSecondary = false;
if (snapshotOnImage == null) {
s_logger.debug(String.format("Can't find snapshot [snapshot id: %d] on backup storage", snapshotId));
} else {
SnapshotObject obj = (SnapshotObject)snapshotOnImage;
try {
deletedOnSecondary = deleteSnapshotOnSecundaryStorage(snapshotId, snapshotOnImage, obj);
if (!deletedOnSecondary) {
s_logger.debug(
String.format("Failed to find/delete snapshot (id: %d) on secondary storage. Still necessary to check and delete snapshot on primary storage.",
snapshotId));
} else {
s_logger.debug(String.format("Snapshot (id: %d) has been deleted on secondary storage.", snapshotId));
}
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
} catch (NoTransitionException e) {
s_logger.debug("Failed to set the state to destroying: ", e);
return false;
}
} catch (NoTransitionException e) {
s_logger.debug("Failed to set the state to destroying: ", e);
return false;
}

try {
boolean result = deleteSnapshotChain(snapshotOnImage);
obj.processEvent(Snapshot.Event.OperationSucceeded);
if (result) {
//snapshot is deleted on backup storage, need to delete it on primary storage
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
if (snapshotOnPrimary != null) {
SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
long volumeId = snapshotOnPrimary.getVolumeId();
VolumeVO volumeVO = volumeDao.findById(volumeId);
if (((PrimaryDataStoreImpl)snapshotOnPrimaryInfo.getDataStore()).getPoolType() == StoragePoolType.RBD && volumeVO != null) {
snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo);
}
snapshotOnPrimary.setState(State.Destroyed);
snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
}
}
} catch (Exception e) {
s_logger.debug("Failed to delete snapshot: ", e);
boolean deletedOnPrimary = deleteSnapshotOnPrimary(snapshotId);
if (deletedOnPrimary) {
s_logger.debug(String.format("Successfully deleted snapshot (id: %d) on primary storage.", snapshotId));
} else if (deletedOnSecondary) {
s_logger.debug(String.format("The snapshot was deleted on secondary storage. Could not find/delete snapshot (id: %d) on primary storage.", snapshotId));
}
return deletedOnSecondary || deletedOnPrimary;
}

/**
* Deletes the snapshot on secondary storage.
* It can return false when the snapshot was stored on primary storage and not backed up on secondary; therefore, the snapshot should also be deleted on primary storage even when this method returns false.
*/
private boolean deleteSnapshotOnSecundaryStorage(Long snapshotId, SnapshotInfo snapshotOnImage, SnapshotObject obj) throws NoTransitionException {
Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Feb 7, 2020

Choose a reason for hiding this comment

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

@GabrielBrascher typo here. it should be secondary, not secundary :)

obj.processEvent(Snapshot.Event.DestroyRequested);
List<VolumeDetailVO> volumesFromSnapshot;
volumesFromSnapshot = _volumeDetailsDaoImpl.findDetails("SNAPSHOT_ID", String.valueOf(snapshotId), null);

if (volumesFromSnapshot.size() > 0) {
try {
obj.processEvent(Snapshot.Event.OperationFailed);
} catch (NoTransitionException e1) {
s_logger.debug("Failed to change snapshot state: " + e.toString());
s_logger.debug("Failed to change snapshot state: " + e1.toString());
}
return false;
throw new InvalidParameterValueException("Unable to perform delete operation, Snapshot with id: " + snapshotId + " is in use ");
}

return true;
boolean result = deleteSnapshotChain(snapshotOnImage);
obj.processEvent(Snapshot.Event.OperationSucceeded);
return result;
}

/**
* Deletes the snapshot on primary storage. It can return false when the snapshot was not stored on primary storage; however this does not means that it failed to delete the snapshot. </br>
* In case of failure, it will throw one of the following exceptions: CloudRuntimeException, InterruptedException, or ExecutionException. </br>
*/
private boolean deleteSnapshotOnPrimary(Long snapshotId) {
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
SnapshotInfo snapshotOnPrimaryInfo = snapshotDataFactory.getSnapshot(snapshotId, DataStoreRole.Primary);
if (isSnapshotOnPrimaryStorage(snapshotId) && snapshotSvr.deleteSnapshot(snapshotOnPrimaryInfo)) {
snapshotOnPrimary.setState(State.Destroyed);
snapshotStoreDao.update(snapshotOnPrimary.getId(), snapshotOnPrimary);
snapshotDao.remove(snapshotId);
return true;
}
return false;
}

/**
* Returns true if the snapshot volume is on primary storage.
*/
private boolean isSnapshotOnPrimaryStorage(long snapshotId) {
SnapshotDataStoreVO snapshotOnPrimary = snapshotStoreDao.findBySnapshot(snapshotId, DataStoreRole.Primary);
if (snapshotOnPrimary != null) {
long volumeId = snapshotOnPrimary.getVolumeId();
VolumeVO volumeVO = volumeDao.findById(volumeId);
return volumeVO != null && volumeVO.getRemoved() == null;
}
return false;
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions .../resources/META-INF/cloudstack/storage/spring-engine-storage-snapshot-storage-context.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
http://www.springframework.org/schema/context/spring-context.xsd"
>

<bean id="xenserverSnapshotStrategy"
class="org.apache.cloudstack.storage.snapshot.XenserverSnapshotStrategy" />
<bean id="defaultSnapshotStrategy"
class="org.apache.cloudstack.storage.snapshot.DefaultSnapshotStrategy" />

<bean id="storageSystemSnapshotStrategy"
class="org.apache.cloudstack.storage.snapshot.StorageSystemSnapshotStrategy" />
Expand Down
62 changes: 38 additions & 24 deletions ...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ public class KVMStorageProcessor implements StorageProcessor {
private String _manageSnapshotPath;
private int _cmdsTimeout;

private static final String MANAGE_SNAPSTHOT_CREATE = "-c";
private static final String MANAGE_SNAPSTHOT_DESTROY = "-d";
private static final String NAME = "-n";
private static final String CEPH_MON_HOST = "mon_host";
private static final String CEPH_AUTH_KEY = "key";
private static final String CEPH_CLIENT_MOUNT_TIMEOUT = "client_mount_timeout";
private static final String CEPH_DEFAULT_MOUNT_TIMEOUT = "30";

public KVMStorageProcessor(final KVMStoragePoolManager storagePoolMgr, final LibvirtComputingResource resource) {
this.storagePoolMgr = storagePoolMgr;
this.resource = resource;
Expand Down Expand Up @@ -563,7 +571,7 @@ public Answer createTemplateFromVolume(final CopyCommand cmd) {
final Script command = new Script(_createTmplPath, wait, s_logger);
command.add("-f", disk.getPath());
command.add("-t", tmpltPath);
command.add("-n", templateName + ".qcow2");
command.add(NAME, templateName + ".qcow2");

final String result = command.execute();

Expand Down Expand Up @@ -949,7 +957,7 @@ public Answer backupSnapshot(final CopyCommand cmd) {
} else {
final Script command = new Script(_manageSnapshotPath, cmd.getWaitInMillSeconds(), s_logger);
command.add("-b", snapshotDisk.getPath());
command.add("-n", snapshotName);
command.add(NAME, snapshotName);
command.add("-p", snapshotDestPath);
if (isCreatedFromVmSnapshot) {
descName = UUID.randomUUID().toString();
Expand Down Expand Up @@ -1010,14 +1018,7 @@ public Answer backupSnapshot(final CopyCommand cmd) {
}
} else {
if (primaryPool.getType() != StoragePoolType.RBD) {
final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger);
command.add("-d", snapshotDisk.getPath());
command.add("-n", snapshotName);
final String result = command.execute();
if (result != null) {
s_logger.debug("Failed to delete snapshot on primary: " + result);
// return new CopyCmdAnswer("Failed to backup snapshot: " + result);
}
deleteSnapshotViaManageSnapshotScript(snapshotName, snapshotDisk);
}
}
} catch (final Exception ex) {
Expand All @@ -1035,6 +1036,16 @@ public Answer backupSnapshot(final CopyCommand cmd) {
}
}

private void deleteSnapshotViaManageSnapshotScript(final String snapshotName, KVMPhysicalDisk snapshotDisk) {
final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger);
command.add(MANAGE_SNAPSTHOT_DESTROY, snapshotDisk.getPath());
command.add(NAME, snapshotName);
final String result = command.execute();
if (result != null) {
s_logger.debug("Failed to delete snapshot on primary: " + result);
}
}

protected synchronized String attachOrDetachISO(final Connect conn, final String vmName, String isoPath, final boolean isAttach) throws LibvirtException, URISyntaxException,
InternalErrorException {
String isoXml = null;
Expand Down Expand Up @@ -1489,12 +1500,7 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
*/
if (primaryPool.getType() == StoragePoolType.RBD) {
try {
final Rados r = new Rados(primaryPool.getAuthUserName());
r.confSet("mon_host", primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort());
r.confSet("key", primaryPool.getAuthSecret());
r.confSet("client_mount_timeout", "30");
r.connect();
s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet("mon_host"));
Rados r = radosConnect(primaryPool);

final IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
final Rbd rbd = new Rbd(io);
Expand All @@ -1511,8 +1517,8 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
} else {
/* VM is not running, create a snapshot by ourself */
final Script command = new Script(_manageSnapshotPath, _cmdsTimeout, s_logger);
command.add("-c", disk.getPath());
command.add("-n", snapshotName);
command.add(MANAGE_SNAPSTHOT_CREATE, disk.getPath());
command.add(NAME, snapshotName);
final String result = command.execute();
if (result != null) {
s_logger.debug("Failed to manage snapshot: " + result);
Expand All @@ -1531,6 +1537,16 @@ public Answer createSnapshot(final CreateObjectCommand cmd) {
}
}

private Rados radosConnect(final KVMStoragePool primaryPool) throws RadosException {
Rados r = new Rados(primaryPool.getAuthUserName());
r.confSet(CEPH_MON_HOST, primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort());
r.confSet(CEPH_AUTH_KEY, primaryPool.getAuthSecret());
r.confSet(CEPH_CLIENT_MOUNT_TIMEOUT, CEPH_DEFAULT_MOUNT_TIMEOUT);
r.connect();
s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet(CEPH_MON_HOST));
return r;
}

@Override
public Answer deleteVolume(final DeleteCommand cmd) {
final VolumeObjectTO vol = (VolumeObjectTO)cmd.getData();
Expand Down Expand Up @@ -1619,12 +1635,7 @@ public Answer deleteSnapshot(final DeleteCommand cmd) {
String snapshotName = snapshotFullPath.substring(snapshotFullPath.lastIndexOf("/") + 1);
snap_full_name = disk.getName() + "@" + snapshotName;
if (primaryPool.getType() == StoragePoolType.RBD) {
Rados r = new Rados(primaryPool.getAuthUserName());
r.confSet("mon_host", primaryPool.getSourceHost() + ":" + primaryPool.getSourcePort());
r.confSet("key", primaryPool.getAuthSecret());
r.confSet("client_mount_timeout", "30");
r.connect();
s_logger.debug("Succesfully connected to Ceph cluster at " + r.confGet("mon_host"));
Rados r = radosConnect(primaryPool);
IoCTX io = r.ioCtxCreate(primaryPool.getSourceDir());
Rbd rbd = new Rbd(io);
RbdImage image = rbd.open(disk.getName());
Expand All @@ -1644,6 +1655,9 @@ public Answer deleteSnapshot(final DeleteCommand cmd) {
rbd.close(image);
r.ioCtxDestroy(io);
}
} else if (primaryPool.getType() == StoragePoolType.NetworkFilesystem) {
s_logger.info(String.format("Attempting to remove snapshot on primary storage (id=%s, snapshot=%s, storage type=%s)", snapshotTO.getId(), snap_full_name, primaryPool.getType()));
deleteSnapshotViaManageSnapshotScript(snapshotName, disk);
} else {
s_logger.warn("Operation not implemented for storage pool type of " + primaryPool.getType().toString());
throw new InternalErrorException("Operation not implemented for storage pool type of " + primaryPool.getType().toString());
Expand Down