cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Suich" <chris.su...@netapp.com>
Subject Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot
Date Tue, 15 Oct 2013 16:08:44 GMT


> On Oct. 15, 2013, 2:41 p.m., John Burwell wrote:
> > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java,
line 28
> > <https://reviews.apache.org/r/14522/diff/2/?file=364638#file364638line28>
> >
> >     I don't know that a flag is necessary to represent this capability since we
already have a revertSnapshot method.  Personally, I dislike the complexity of exposing methods
on an interface that are guarded by another.  I would prefer to see revertSnapshot enhanced
to throw an exception when a snapshot can't be reverted.  In addition to be a simpler approach,
it covers both the scenario when a device does not support reverting snapshots and, as well
as, the device or the snapshot not being a revertable state.
> 
> Chris Suich wrote:
>     The purpose of canRevertSnapshot() is not to guard revertSnapshot(), but to determine/query
whether the driver supports reverting so that the UI can appropriately show/hide the revert
snapshot action. I guess in my mind, canRevertSnapshot() should not check things like the
state of the snapshot, but things like, is the base volume still on the associated storage
(since the base volume could have been moved to a new primary), etc.
> 
> John Burwell wrote:
>     I think we need to find a more generic way to advertise driver capabilities.  Reviewing
the rest of the patch, it appears that this method is being used for more than simple driver
capability checking.  I need to think through the best approach, but as a strawman, we could
consider the following:
>     
>         public interface SnapshotCapability { ... }
>         
>         public enum DefaultSnapshotCapabilities implements StorageCapability {
>             /* other capabilities */
>             REVERT;
>         }
>     
>     This approach would include extracting all of the snapshot operations to a separate
Snapshotable interface that would define all of the snapshot operations plus Set<SnapshotCapability>
getSnapshotCapabilities().  Drivers that support snapshotting would implement the Snapshotable
interface in addition to the PrimaryDataStoreDriver interface.  A simple instanceof check
determines whether or not a device can be snapshotted and a query of the getSnapshotCapabilities
method to answer more fine grained questions.  The API layer can then translate those capabilities
into specific boolean values or we can pass the whole set back to UI for decision making.
>     
>     Additionally, does this method refer to hypervisor or storage-level snapshots?  If
it refers to hypervisor snapshots, why is the determination about the ability to revert being
made by a storage driver?
> 
> Chris Suich wrote:
>     Are there any other capabilities that would be included in this (other than take
snapshot)?
>     
>     This refers only to storage-level snapshots. However, a snapshot strategy overrides
the hypervisor snapshot capabilities. So, hypervisor snapshots can be replaced with storage-level
snapshots if the storage driver supports it.

At this level, though, we're not talking about driver capabilities. We're talking about strategy
capabilities. So having the storage driver implement Snapshotable won't do much, because the
snapshot strategies are the ones that need to be asked to handle operations.


- Chris


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14522/#review27021
-----------------------------------------------------------


On Oct. 14, 2013, 10:50 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2013, 10:50 p.m.)
> 
> 
> Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> After the last batch of work to the revertSnapshot API, SnapshotServiceImpl was not tied
into the workflow to be used by storage providers. I have added the logic in a similar fashion
to takeSnapshot(), backupSnapshot() and deleteSnapshot().
> 
> I have also added a 'Revert to Snapshot' action to the volume snapshots list in the UI.
> 
> 
> Diffs
> -----
> 
>   api/src/org/apache/cloudstack/api/ApiConstants.java f85784b 
>   api/src/org/apache/cloudstack/api/response/SnapshotResponse.java e9cb109 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java
b124d83 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
8d6b760 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java
e4cecb6 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
8bd04f5 
>   engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java
329b99f 
>   engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java
810afd1 
>   engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
81f77d6 
>   engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
3f35e1d 
>   engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
6a874d6 
>   plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java
3eaeb1f 
>   plugins/storage/volume/sample/src/org/apache/cloudstack/storage/datastore/driver/SamplePrimaryDataStoreDriverImpl.java
ece7b26 
>   plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java
8046b6c 
>   server/src/com/cloud/api/ApiResponseHelper.java f4ca112 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 
>   ui/scripts/storage.js b16f4d4 
> 
> Diff: https://reviews.apache.org/r/14522/diff/
> 
> 
> Testing
> -------
> 
> I have tested all of this locally with a custom storage provider.
> 
> Unfortunately, I'm still in the middle of figuring out how to properly unit test this
type of code. If anyone has any recommendations, please let me know.
> 
> 
> Thanks,
> 
> Chris Suich
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message