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 Thu, 17 Oct 2013 15:33:00 GMT


> On Oct. 17, 2013, 3:18 p.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.

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.


> On Oct. 17, 2013, 3:18 p.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.

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.


> On Oct. 17, 2013, 3:18 p.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?

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.


- Chris


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14522/#review27126
-----------------------------------------------------------


On Oct. 16, 2013, 2:50 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2013, 2: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 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message