Skip to content

Commit f33b4a1

Browse files
swillswill
committed
Merge pull request #1441 from mike-tutkowski/4.7
CLOUDSTACK-9297 - Reworked logic in StorageSystemSnapshotStrategy and XenserverSnapshotStrategyThe ticket this PR fixes was opened because KVM-specific code had been added to the StorageSystemSnapshotStrategy class and that class' canHandle method was only prepared to handle managed storage being used with XenServer (and a case was hit for KVM that triggered a CloudRuntimeException to be thrown). To solve the problem, I moved the KVM logic to the default snapshot strategy class, which is (unfortunately) named XenserverSnapshotStrategy. I plan to rename XenserverSnapshotStrategy to something like DefaultSnapshotStrategy in 4.9. My guess is that when XenserverSnapshotStrategy was originally written, it was written only for XenServer, but has since that time had its usage increased to support other hypervisors (with non-managed storage). * pr/1441: CLOUDSTACK-9297: delete snapshot without id is failing with Unable to determine the storage pool of the snapshot Signed-off-by: Will Stevens <williamstevens@gmail.com>
2 parents ef115ab + 158d196 commit f33b4a1

2 files changed

Lines changed: 70 additions & 68 deletions

File tree

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

Lines changed: 13 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,10 @@
3535
import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority;
3636
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
3737
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeService;
38-
import org.apache.cloudstack.engine.subsystem.api.storage.ObjectInDataStoreStateMachine.Event;
3938
import org.apache.cloudstack.storage.command.SnapshotAndCopyAnswer;
4039
import org.apache.cloudstack.storage.command.SnapshotAndCopyCommand;
4140
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
4241
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreDao;
43-
import org.apache.cloudstack.storage.datastore.db.SnapshotDataStoreVO;
4442
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
4543

4644
import com.cloud.agent.AgentManager;
@@ -57,8 +55,6 @@
5755
import com.cloud.storage.Snapshot;
5856
import com.cloud.storage.SnapshotVO;
5957
import com.cloud.storage.Storage.ImageFormat;
60-
import com.cloud.storage.StoragePool;
61-
import com.cloud.storage.StoragePoolStatus;
6258
import com.cloud.storage.Volume;
6359
import com.cloud.storage.VolumeVO;
6460
import com.cloud.storage.dao.SnapshotDao;
@@ -157,43 +153,7 @@ public boolean deleteSnapshot(Long snapshotId) {
157153

158154
@Override
159155
public boolean revertSnapshot(SnapshotInfo snapshot) {
160-
if (canHandle(snapshot,SnapshotOperation.REVERT) == StrategyPriority.CANT_HANDLE) {
161-
throw new UnsupportedOperationException("Reverting not supported. Create a template or volume based on the snapshot instead.");
162-
}
163-
164-
SnapshotVO snapshotVO = _snapshotDao.acquireInLockTable(snapshot.getId());
165-
if (snapshotVO == null) {
166-
throw new CloudRuntimeException("Failed to get lock on snapshot:" + snapshot.getId());
167-
}
168-
169-
try {
170-
VolumeInfo volumeInfo = snapshot.getBaseVolume();
171-
StoragePool store = (StoragePool)volumeInfo.getDataStore();
172-
if (store != null && store.getStatus() != StoragePoolStatus.Up) {
173-
snapshot.processEvent(Event.OperationFailed);
174-
throw new CloudRuntimeException("store is not in up state");
175-
}
176-
volumeInfo.stateTransit(Volume.Event.RevertSnapshotRequested);
177-
boolean result = false;
178-
try {
179-
result = snapshotSvr.revertSnapshot(snapshot);
180-
if (! result) {
181-
s_logger.debug("Failed to revert snapshot: " + snapshot.getId());
182-
throw new CloudRuntimeException("Failed to revert snapshot: " + snapshot.getId());
183-
}
184-
} finally {
185-
if (result) {
186-
volumeInfo.stateTransit(Volume.Event.OperationSucceeded);
187-
} else {
188-
volumeInfo.stateTransit(Volume.Event.OperationFailed);
189-
}
190-
}
191-
return result;
192-
} finally {
193-
if (snapshotVO != null) {
194-
_snapshotDao.releaseFromLockTable(snapshot.getId());
195-
}
196-
}
156+
throw new UnsupportedOperationException("Reverting not supported. Create a template or volume based on the snapshot instead.");
197157
}
198158

199159
@Override
@@ -440,41 +400,28 @@ private void markAsBackedUp(SnapshotObject snapshotObj) {
440400

441401
@Override
442402
public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
443-
long volumeId = snapshot.getVolumeId();
444-
VolumeVO volumeVO = _volumeDao.findById(volumeId);
445403
if (SnapshotOperation.REVERT.equals(op)) {
446-
if (volumeVO != null && ImageFormat.QCOW2.equals(volumeVO.getFormat()))
447-
return StrategyPriority.DEFAULT;
448-
else
449-
return StrategyPriority.CANT_HANDLE;
404+
return StrategyPriority.CANT_HANDLE;
450405
}
451406

452-
long storagePoolId;
407+
long volumeId = snapshot.getVolumeId();
453408

454-
if (volumeVO == null) {
455-
SnapshotDataStoreVO snapshotStore = _snapshotStoreDao.findBySnapshot(snapshot.getId(), DataStoreRole.Primary);
409+
VolumeVO volumeVO = _volumeDao.findByIdIncludingRemoved(volumeId);
456410

457-
if (snapshotStore != null) {
458-
storagePoolId = snapshotStore.getDataStoreId();
459-
}
460-
else {
461-
throw new CloudRuntimeException("Unable to determine the storage pool of the snapshot");
462-
}
463-
}
464-
else {
465-
storagePoolId = volumeVO.getPoolId();
466-
}
411+
long storagePoolId = volumeVO.getPoolId();
467412

468413
DataStore dataStore = _dataStoreMgr.getDataStore(storagePoolId, DataStoreRole.Primary);
469414

470-
Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();
415+
if (dataStore != null) {
416+
Map<String, String> mapCapabilities = dataStore.getDriver().getCapabilities();
471417

472-
if (mapCapabilities != null) {
473-
String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString());
474-
Boolean supportsStorageSystemSnapshots = new Boolean(value);
418+
if (mapCapabilities != null) {
419+
String value = mapCapabilities.get(DataStoreCapabilities.STORAGE_SYSTEM_SNAPSHOT.toString());
420+
Boolean supportsStorageSystemSnapshots = new Boolean(value);
475421

476-
if (supportsStorageSystemSnapshots) {
477-
return StrategyPriority.HIGHEST;
422+
if (supportsStorageSystemSnapshots) {
423+
return StrategyPriority.HIGHEST;
424+
}
478425
}
479426
}
480427

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

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,14 @@
4545
import com.cloud.storage.DataStoreRole;
4646
import com.cloud.storage.Snapshot;
4747
import com.cloud.storage.SnapshotVO;
48+
import com.cloud.storage.StoragePool;
49+
import com.cloud.storage.StoragePoolStatus;
4850
import com.cloud.storage.Volume;
4951
import com.cloud.storage.VolumeVO;
5052
import com.cloud.storage.dao.SnapshotDao;
5153
import com.cloud.storage.dao.VolumeDao;
5254
import com.cloud.storage.snapshot.SnapshotManager;
55+
import com.cloud.storage.Storage.ImageFormat;
5356
import com.cloud.utils.NumbersUtil;
5457
import com.cloud.utils.db.DB;
5558
import com.cloud.utils.exception.CloudRuntimeException;
@@ -279,7 +282,52 @@ public boolean deleteSnapshot(Long snapshotId) {
279282

280283
@Override
281284
public boolean revertSnapshot(SnapshotInfo snapshot) {
282-
throw new CloudRuntimeException("revert Snapshot is not supported");
285+
if (canHandle(snapshot,SnapshotOperation.REVERT) == StrategyPriority.CANT_HANDLE) {
286+
throw new UnsupportedOperationException("Reverting not supported. Create a template or volume based on the snapshot instead.");
287+
}
288+
289+
SnapshotVO snapshotVO = snapshotDao.acquireInLockTable(snapshot.getId());
290+
291+
if (snapshotVO == null) {
292+
throw new CloudRuntimeException("Failed to get lock on snapshot:" + snapshot.getId());
293+
}
294+
295+
try {
296+
VolumeInfo volumeInfo = snapshot.getBaseVolume();
297+
StoragePool store = (StoragePool)volumeInfo.getDataStore();
298+
299+
if (store != null && store.getStatus() != StoragePoolStatus.Up) {
300+
snapshot.processEvent(Event.OperationFailed);
301+
302+
throw new CloudRuntimeException("store is not in up state");
303+
}
304+
305+
volumeInfo.stateTransit(Volume.Event.RevertSnapshotRequested);
306+
307+
boolean result = false;
308+
309+
try {
310+
result = snapshotSvr.revertSnapshot(snapshot);
311+
312+
if (!result) {
313+
s_logger.debug("Failed to revert snapshot: " + snapshot.getId());
314+
315+
throw new CloudRuntimeException("Failed to revert snapshot: " + snapshot.getId());
316+
}
317+
} finally {
318+
if (result) {
319+
volumeInfo.stateTransit(Volume.Event.OperationSucceeded);
320+
} else {
321+
volumeInfo.stateTransit(Volume.Event.OperationFailed);
322+
}
323+
}
324+
325+
return result;
326+
} finally {
327+
if (snapshotVO != null) {
328+
snapshotDao.releaseFromLockTable(snapshot.getId());
329+
}
330+
}
283331
}
284332

285333
@Override
@@ -353,7 +401,14 @@ public SnapshotInfo takeSnapshot(SnapshotInfo snapshot) {
353401

354402
@Override
355403
public StrategyPriority canHandle(Snapshot snapshot, SnapshotOperation op) {
356-
if (op == SnapshotOperation.REVERT) {
404+
if (SnapshotOperation.REVERT.equals(op)) {
405+
long volumeId = snapshot.getVolumeId();
406+
VolumeVO volumeVO = volumeDao.findById(volumeId);
407+
408+
if (volumeVO != null && ImageFormat.QCOW2.equals(volumeVO.getFormat())) {
409+
return StrategyPriority.DEFAULT;
410+
}
411+
357412
return StrategyPriority.CANT_HANDLE;
358413
}
359414

0 commit comments

Comments
 (0)