Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
Content-Type: multipart/alternative;
 boundary="===============8980839804020730238=="
MIME-Version: 1.0
Subject: Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk
 from
 Snapshot
From: "John Burwell" <jburwell@basho.com>
To: "edison su" <edison.su@citrix.com>, "John Burwell" <jburwell@basho.com>,
 "Brian Federle" <brian.federle@citrix.com>,
 "Mike Tutkowski" <mike.tutkowski@solidfire.com>
Cc: "Chris Suich" <chris.suich@netapp.com>,
 "cloudstack" <dev@cloudstack.apache.org>
Date: Thu, 17 Oct 2013 15:20:22 -0000
Message-ID: <20131017152022.29093.96954@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "John Burwell" <noreply@reviews.apache.org>
References: <20131016145012.7309.7277@reviews.apache.org>
In-Reply-To: <20131016145012.7309.7277@reviews.apache.org>
Reply-To: "John Burwell" <jburwell@basho.com>

--===============8980839804020730238==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


Overall, this patch is very close to ready.  I really like the way the condition logic is being simplified out with stronger error handling and detection.  Namely, it needs to add more information to the error reporting, and ensure that it is surfaced to the end user.  My concerns regarding the hypervisor vs. storage strategy selection are larger architectural issues that need to be addressed separately.

- John Burwell


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


--===============8980839804020730238==--