> 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 > >
