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