> 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.
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. > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java, > > line 46 > > <https://reviews.apache.org/r/14522/diff/2/?file=364641#file364641line46> > > > > How does the result of the canHandle method drive the comparison of two > > Snapshots? Why not just compare the ops to each other other? The 'op' something I introduced to give strategies more flexibility in their canHandle() method. The purpose of sorting the strategies by canHandle() is to ensure that strategies that cannot handle the 'op' are at the end of the list while plugins are at the front of the list (with hypervisors and default strategies in between). > 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. 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. > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java, > > line 135 > > <https://reviews.apache.org/r/14522/diff/2/?file=364645#file364645line135> > > > > What is the purpose of sorting the strategies? The purpose of sorting the strategies is to ensure that plugin strategies get priority over the hypervisor implementations and default implementations. Additionally, it will also make sure that plugin strategies that cannot handle the operation are at the end of the list, to ensure that hypervisor or default strategies would be given the operation instead of the wrong plugin. > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > server/src/com/cloud/api/ApiResponseHelper.java, line 461 > > <https://reviews.apache.org/r/14522/diff/2/?file=364650#file364650line461> > > > > If the snapshot was instance of SnapshotInfo and no SnapshotInfo was > > found, an exception should thrown. > > > > Provided my assumption is correct, I suggest refactoring to a structure > > such as the following: > > > > final SnapshotInfo secondaryInfo = (snapshot instanceof SnapshotInfo) ? > > (SnapshotInfo) snapshot : snapshotfactory.getSnapshot(snapshot.getId(), > > DataStoreRole.Image); > > > > if (secondaryInfo == null) { > > throw new CloudRuntimException(/* Appropriate error message */ ); > > } > > > > snapshotResponse.setCanRevert(secondaryInfo.canRevert); Is this how we do things in other APIs? The reason I didn't thrown an exception is that I'd be worried failing an entire API call just because a single snapshot appears to have a broken DB-file on disk relationship. There is probably a better way to go about cleaning it up, but is that worth failing the API over? > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > api/src/org/apache/cloudstack/api/response/SnapshotResponse.java, line 194 > > <https://reviews.apache.org/r/14522/diff/2/?file=364637#file364637line194> > > > > CanRevert is an awkward/non-standard property name. Prefer an adverb > > such as revertable (e.g. isRevertable/setRevertable with an attribute named > > revertableFlag). Agreed. I can fix that. > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java, > > line 36 > > <https://reviews.apache.org/r/14522/diff/2/?file=364639#file364639line36> > > > > Prefer an adverb property name such as "revertable". Since this is a method, shouldn't it be isRevertable() as well? > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java, > > line 48 > > <https://reviews.apache.org/r/14522/diff/2/?file=364641#file364641line48> > > > > Very poor performance as it circumvents the primitive pool. Change to > > Integer.valueOf(i1).compareTo(Integer.valueOf(i2)). Will do. > On Oct. 15, 2013, 2:41 p.m., John Burwell wrote: > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java, > > line 315 > > <https://reviews.apache.org/r/14522/diff/2/?file=364646#file364646line315> > > > > Coding convention: Add curly braces Do we have an ACS eclipse profile to handle these coding conventions? - 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 > >
