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 Wed, 16 Oct 2013 02:06:45 GMT


> On Oct. 15, 2013, 2:41 p.m., John Burwell wrote:
> > engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java,
line 437
> > <https://reviews.apache.org/r/14522/diff/2/?file=364644#file364644line437>
> >
> >     *This comment may be more editorial at this point because of the nature of the
existing code*
> >     I would prefer to see an exception thrown here rather than the complexity of
the canHandle.  I am also concerned that the canHandle method only checks that the underlying
device can take a snapshot without accounting for whether or not the volume is in a state
to take a consistent snapshot.  I am also concerned that we don't have a semantic for handling
errors that might result from the actual snapshot operation.
> 
> Chris Suich wrote:
>     That is actually the purpose of the canHandle() method - to ONLY check whether the
underlying device can handle the operation, not whether the storage is in the right state.
For example, if my driver can handle snapshotting, but I am unable to actually perform the
snapshot, I still want to be the one asked to take the snapshot so that I am the one to cause
the API to fail.
>     
>     Additionally, I'm not sure how we could exclude other strategies from attempting
to take my storage and try to snapshot. If there are two snapshot strategies and they are
not sorted by canHandle(), then some other strategy may be given the opportunity to take the
snapshot and simply fail the API because it cannot actually handle it.
> 
> John Burwell wrote:
>     This logic seems overly complicated.  There can be a number of reasons why a snapshot
operation will not succeed for a device.  Among those reasons is that the device is being
asked to perform an operation it does not support.  As I understand the operation, a snapshot
is a operation directed to a particular volume, Volume A.  When the storage layer is asked
to snapshot Volume A, the work will eventually be performed by the associated device driver,
Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot
operation fails.  There are not other options in this scenario.  Provided my understanding
is correct, I don't see the need for an additional canHandle method.  Simply call the method,
and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current
interface seems to only concern itself with whether or not the driver can do something to
the exclusion of whether or not the device is in a state to properly p
 erform the operation.
> 
> Chris Suich wrote:
>     Ok, I think I see what you are saying. Once the strategies are sorted (by calling
canHandle()), then we no longer need to check whether the strategy can handle the operation
and should simply execute the operation on the top sorted strategy.
>     
>     Is that what you are saying?
> 
> John Burwell wrote:
>     Essentially, yes.  I also concerned with the atomicity of the operation once we properly
consider the state of the device.  By calling canHandle, and then performing the operation,
we creates a race condition.  Combining the check with the operation allows the system to
atomicity check and perform the operation -- eliminating races with other threads performing
the same operation.
> 
> Chris Suich wrote:
>     How can we determine which strategy to use if we do not check whether the strategy
can handle an operation? The canHandle method should not be doing some dynamic check of the
state of a the storage to determine whether it can handle an operation. It should simply indicate
whether a strategy supports the static operation. If there is a better way to approach this,
then that is fine, but the strategies should be able to indicate that. Again, this has nothing
to do with the underlying storage. Storage drivers that want to be baked into the default
snapshotting workflow can implement the driver level takeSnapshot() and revertSnapshot() method.

I forgot to mention - the reason I don't like having a completely static response to whether
the strategy supports an operation (like the getCapabilities() posted above) is that there
is some runtime logic that can determine whether a plugin strategy can handle an operation.
My use case, for example, is that our strategy only works on VMs on our storage. A non-storage
use case may be something like having a plugin snapshot strategy that is only used for VMs
using a certain disk offering. In cases like this, the snapshot strategy needs to run some
check against the VM/volume the request is for, not just a compile time declaration of what
it can do.


- 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