> On Oct. 17, 2013, 11:18 a.m., John Burwell wrote: > > ui/scripts/storage.js, line 1963 > > <https://reviews.apache.org/r/14522/diff/3/?file=365104#file365104line1963> > > > > How will a corrupted snapshot failure be surfaced to the user? > > Chris Suich wrote: > It depends on what you mean by corrupted. If we can no longer find the > snapshot we expect from the DB, then the API will throw an exception, per > your other review, and the list of snapshots will never populate. If the file > exists but is corrupted, then that wouldn't be discovered until either the > attempt to revert the snapshot or after the revert operation and the user > attempts to use the volume.
As I mentioned earlier, the user will get the idea that something bad happened, but will be left with little to no information to pass onto the operator/administrator. We need to surface enough breadcrumb information so that the user a) understands that the snapshot metadata has become corrupted and b) provide them with enough identifying information to support the forensic work of their operator/administrator. > On Oct. 17, 2013, 11:18 a.m., John Burwell wrote: > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java, > > line 62 > > <https://reviews.apache.org/r/14522/diff/3/?file=365094#file365094line62> > > > > Add context information to error message to help operators and users > > determine what couldn't be snapshotted. > > Chris Suich wrote: > Fair enough - I just copied these existing error messages. What kind of > context would you like to see in this error message? The src & dest > datastores, files, etc.? I agree that there needs to be context, but this > would potentially be a user facing error, so I also don't want to overload > them with context that means nothing to them. Enough details so an operator/admin could look up the associated volume/snapshot and debug the issue. Likely a good start would be to include the identifying information of the srcData and destData parameters. Ask yourself, "What I need to know as an operator/administrator to debug this problem?" and put that information in the error message. > On Oct. 17, 2013, 11:18 a.m., John Burwell wrote: > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java, > > line 79 > > <https://reviews.apache.org/r/14522/diff/3/?file=365094#file365094line79> > > > > DRY out this code (e.g. extract to a private method). This rules needs > > to be consolidated + add the context information mentioned above. > > Chris Suich wrote: > The two invocations of pickStrategy() are actually calling an overloaded > function. I'm not sure how we could DRY this out since they method > calls/parameter lists are actually different. I missed the overload. The actual issue, in my mind, is that pickStrategy needs to be DRY'ed out. This likely requires a new interface which is outside the bounds of this patch. However, it is technical debt that we somehow need to surface. As such, please open a ticket to describing issue and put a TODO on pickStrategy to consolidate the implementations with a reference to the ticket. Also, place a TODO on this lines to reference the future refactoring. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14522/#review27126 ----------------------------------------------------------- On Oct. 16, 2013, 10:50 a.m., Chris Suich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14522/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2013, 10:50 a.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 62eed09 > 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/StrategyPriority.java > 81034b1 > > engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java > 3d75279 > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java > 2d31320 > > 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 > >
