You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@cloudstack.apache.org by Chris Suich <ch...@netapp.com> on 2013/10/07 22:26:55 UTC

Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

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

Review request for cloudstack, Brian Federle and edison su.


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/command/user/snapshot/RevertSnapshotCmd.java 946eebd 
  client/WEB-INF/classes/resources/messages.properties f92b85a 
  client/tomcatconf/commands.properties.in 58c770d 
  engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java c09adca 
  server/src/com/cloud/server/ManagementServerImpl.java 0a0fcdc 
  server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 0b53cfd 
  ui/dictionary.jsp f93f9dc 
  ui/scripts/storage.js 88fb9f2 

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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Mike Tutkowski <mi...@solidfire.com>.

Just sent you an e-mail chain under the subject: [DISCUSS/PROPOSAL]
Upgrading Driver Model


On Wed, Oct 9, 2013 at 2:17 PM, SuichII, Christopher <Chris.Suich@netapp.com
> wrote:

> Well then, I think sending back a list of supported operations with
> volumes would be a good start. Eventually, this could be extended to have
> supported fields as well. While it does cost some overhead up front to load
> the supported operations from storage providers when listing volumes, I
> think it is simpler overall than introducing new APIs for querying for that
> information.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 9, 2013, at 3:45 PM, Mike Tutkowski <mi...@solidfire.com>
> wrote:
>
> > "Has there been any thoughts to allow storage providers to indicate which
> > features they support?"
> >
> > We talked about this for a while at the CloudStack Collaboration
> Conference
> > in Santa Clara.
> >
> > Right now, this is not supported and that's a serious problem.
> >
> > This kind of ties in with Storage Tagging and how that is problematic, as
> > well.
> >
> > With Storage Tagging, there is no indication of what storage provider
> > supports the Compute or Disk Offering in question and, as such, we don't
> > know what fields to show to or hide from users.
> >
> >
> > On Wed, Oct 9, 2013 at 1:32 PM, SuichII, Christopher <
> Chris.Suich@netapp.com
> >> wrote:
> >
> >> Just bumping this since there haven't been any responses.
> >>
> >> Does anyone have any thoughts on this? I'm ready and prepared to do the
> >> work, but I don't want to move on if people have concerns with this
> >> approach or can think of a better solution.
> >>
> >> -Chris
> >> --
> >> Chris Suich
> >> chris.suich@netapp.com<ma...@netapp.com>
> >> NetApp Software Engineer
> >> Data Center Platforms – Cloud Solutions
> >> Citrix, Cisco & Red Hat
> >>
> >> On Oct 8, 2013, at 4:53 PM, Chris Suich <Chris.Suich@netapp.com<mailto:
> >> Chris.Suich@netapp.com>> wrote:
> >>
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/14522/
> >>
> >>
> >> On October 8th, 2013, 8:18 p.m. UTC, edison su wrote:
> >>
> >> ui/scripts/storage.js<
> >>
> https://reviews.apache.org/r/14522/diff/1/?file=362033#file362033line1763>
> >> (Diff revision 1)
> >>
> >> getActionFilter: function() {
> >>
> >>
> >>        1763
> >>
> >>                            revertSnapshot: {
> >>
> >>
> >> The ui change here, is there way to disable it from ui, if the storage
> >> provider is not NetApp? Or move the ui change into your plugin?
> >>
> >> This raises the question of whether people expect to see the revert
> >> snapshot functionality for hypervisors or just storage providers. I
> figured
> >> that the hypervisor functionality would be desired, but it sounds like
> that
> >> may not be the case for all hypervisors.
> >>
> >> Has there been any thoughts to allow storage providers to indicate which
> >> features they support? Maybe part of the VolumeResponse can be a set of
> >> flags for which operations are supported (take snapshot, revert
> snapshot,
> >> etc.). This way, the UI can dynamically show/hide supported actions
> without
> >> knowing who the volume's storage provider actually is. This should be a
> >> fairly straight forward UI change, but would require adding methods to
> the
> >> storage provider interface. If we don't want to always load this
> >> information just for the VolumeResponse, we could expose new APIs to
> query
> >> which operations are supported for a given volume, but we may not want
> to
> >> go exposing APIs for this.
> >>
> >> Any thoughts?
> >>
> >>
> >> - Chris
> >>
> >>
> >> On October 7th, 2013, 8:26 p.m. UTC, Chris Suich wrote:
> >>
> >> Review request for cloudstack, Brian Federle and edison su.
> >> By Chris Suich.
> >>
> >> Updated Oct. 7, 2013, 8:26 p.m.
> >>
> >> 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.
> >>
> >>
> >> 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.
> >>
> >>
> >> Diffs
> >>
> >>  *
> >>
> api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java
> >> (946eebd)
> >>  *   client/WEB-INF/classes/resources/messages.properties (f92b85a)
> >>  *   client/tomcatconf/commands.properties.in (58c770d)
> >>  *
> >>
> engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
> >> (c09adca)
> >>  *   server/src/com/cloud/server/ManagementServerImpl.java (0a0fcdc)
> >>  *   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
> >> (0b53cfd)
> >>  *   ui/dictionary.jsp (f93f9dc)
> >>  *   ui/scripts/storage.js (88fb9f2)
> >>
> >> View Diff<https://reviews.apache.org/r/14522/diff/>
> >>
> >>
> >>
> >
> >
> > --
> > *Mike Tutkowski*
> > *Senior CloudStack Developer, SolidFire Inc.*
> > e: mike.tutkowski@solidfire.com
> > o: 303.746.7302
> > Advancing the way the world uses the
> > cloud<http://solidfire.com/solution/overview/?video=play>
> > *™*
>
>


-- 
*Mike Tutkowski*
*Senior CloudStack Developer, SolidFire Inc.*
e: mike.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by "SuichII, Christopher" <Ch...@netapp.com>.

Well then, I think sending back a list of supported operations with volumes would be a good start. Eventually, this could be extended to have supported fields as well. While it does cost some overhead up front to load the supported operations from storage providers when listing volumes, I think it is simpler overall than introducing new APIs for querying for that information.

-- 
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat

On Oct 9, 2013, at 3:45 PM, Mike Tutkowski <mi...@solidfire.com> wrote:

> "Has there been any thoughts to allow storage providers to indicate which
> features they support?"
> 
> We talked about this for a while at the CloudStack Collaboration Conference
> in Santa Clara.
> 
> Right now, this is not supported and that's a serious problem.
> 
> This kind of ties in with Storage Tagging and how that is problematic, as
> well.
> 
> With Storage Tagging, there is no indication of what storage provider
> supports the Compute or Disk Offering in question and, as such, we don't
> know what fields to show to or hide from users.
> 
> 
> On Wed, Oct 9, 2013 at 1:32 PM, SuichII, Christopher <Chris.Suich@netapp.com
>> wrote:
> 
>> Just bumping this since there haven't been any responses.
>> 
>> Does anyone have any thoughts on this? I'm ready and prepared to do the
>> work, but I don't want to move on if people have concerns with this
>> approach or can think of a better solution.
>> 
>> -Chris
>> --
>> Chris Suich
>> chris.suich@netapp.com<ma...@netapp.com>
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>> 
>> On Oct 8, 2013, at 4:53 PM, Chris Suich <Chris.Suich@netapp.com<mailto:
>> Chris.Suich@netapp.com>> wrote:
>> 
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/14522/
>> 
>> 
>> On October 8th, 2013, 8:18 p.m. UTC, edison su wrote:
>> 
>> ui/scripts/storage.js<
>> https://reviews.apache.org/r/14522/diff/1/?file=362033#file362033line1763>
>> (Diff revision 1)
>> 
>> getActionFilter: function() {
>> 
>> 
>>        1763
>> 
>>                            revertSnapshot: {
>> 
>> 
>> The ui change here, is there way to disable it from ui, if the storage
>> provider is not NetApp? Or move the ui change into your plugin?
>> 
>> This raises the question of whether people expect to see the revert
>> snapshot functionality for hypervisors or just storage providers. I figured
>> that the hypervisor functionality would be desired, but it sounds like that
>> may not be the case for all hypervisors.
>> 
>> Has there been any thoughts to allow storage providers to indicate which
>> features they support? Maybe part of the VolumeResponse can be a set of
>> flags for which operations are supported (take snapshot, revert snapshot,
>> etc.). This way, the UI can dynamically show/hide supported actions without
>> knowing who the volume's storage provider actually is. This should be a
>> fairly straight forward UI change, but would require adding methods to the
>> storage provider interface. If we don't want to always load this
>> information just for the VolumeResponse, we could expose new APIs to query
>> which operations are supported for a given volume, but we may not want to
>> go exposing APIs for this.
>> 
>> Any thoughts?
>> 
>> 
>> - Chris
>> 
>> 
>> On October 7th, 2013, 8:26 p.m. UTC, Chris Suich wrote:
>> 
>> Review request for cloudstack, Brian Federle and edison su.
>> By Chris Suich.
>> 
>> Updated Oct. 7, 2013, 8:26 p.m.
>> 
>> 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.
>> 
>> 
>> 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.
>> 
>> 
>> Diffs
>> 
>>  *
>> api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java
>> (946eebd)
>>  *   client/WEB-INF/classes/resources/messages.properties (f92b85a)
>>  *   client/tomcatconf/commands.properties.in (58c770d)
>>  *
>> engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
>> (c09adca)
>>  *   server/src/com/cloud/server/ManagementServerImpl.java (0a0fcdc)
>>  *   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
>> (0b53cfd)
>>  *   ui/dictionary.jsp (f93f9dc)
>>  *   ui/scripts/storage.js (88fb9f2)
>> 
>> View Diff<https://reviews.apache.org/r/14522/diff/>
>> 
>> 
>> 
> 
> 
> -- 
> *Mike Tutkowski*
> *Senior CloudStack Developer, SolidFire Inc.*
> e: mike.tutkowski@solidfire.com
> o: 303.746.7302
> Advancing the way the world uses the
> cloud<http://solidfire.com/solution/overview/?video=play>
> *™*

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Mike Tutkowski <mi...@solidfire.com>.

"Has there been any thoughts to allow storage providers to indicate which
features they support?"

We talked about this for a while at the CloudStack Collaboration Conference
in Santa Clara.

Right now, this is not supported and that's a serious problem.

This kind of ties in with Storage Tagging and how that is problematic, as
well.

With Storage Tagging, there is no indication of what storage provider
supports the Compute or Disk Offering in question and, as such, we don't
know what fields to show to or hide from users.


On Wed, Oct 9, 2013 at 1:32 PM, SuichII, Christopher <Chris.Suich@netapp.com
> wrote:

> Just bumping this since there haven't been any responses.
>
> Does anyone have any thoughts on this? I'm ready and prepared to do the
> work, but I don't want to move on if people have concerns with this
> approach or can think of a better solution.
>
> -Chris
> --
> Chris Suich
> chris.suich@netapp.com<ma...@netapp.com>
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 8, 2013, at 4:53 PM, Chris Suich <Chris.Suich@netapp.com<mailto:
> Chris.Suich@netapp.com>> wrote:
>
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
>
>
> On October 8th, 2013, 8:18 p.m. UTC, edison su wrote:
>
> ui/scripts/storage.js<
> https://reviews.apache.org/r/14522/diff/1/?file=362033#file362033line1763>
> (Diff revision 1)
>
> getActionFilter: function() {
>
>
>         1763
>
>                             revertSnapshot: {
>
>
> The ui change here, is there way to disable it from ui, if the storage
> provider is not NetApp? Or move the ui change into your plugin?
>
> This raises the question of whether people expect to see the revert
> snapshot functionality for hypervisors or just storage providers. I figured
> that the hypervisor functionality would be desired, but it sounds like that
> may not be the case for all hypervisors.
>
> Has there been any thoughts to allow storage providers to indicate which
> features they support? Maybe part of the VolumeResponse can be a set of
> flags for which operations are supported (take snapshot, revert snapshot,
> etc.). This way, the UI can dynamically show/hide supported actions without
> knowing who the volume's storage provider actually is. This should be a
> fairly straight forward UI change, but would require adding methods to the
> storage provider interface. If we don't want to always load this
> information just for the VolumeResponse, we could expose new APIs to query
> which operations are supported for a given volume, but we may not want to
> go exposing APIs for this.
>
> Any thoughts?
>
>
> - Chris
>
>
> On October 7th, 2013, 8:26 p.m. UTC, Chris Suich wrote:
>
> Review request for cloudstack, Brian Federle and edison su.
> By Chris Suich.
>
> Updated Oct. 7, 2013, 8:26 p.m.
>
> 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.
>
>
> 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.
>
>
> Diffs
>
>   *
> api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java
> (946eebd)
>   *   client/WEB-INF/classes/resources/messages.properties (f92b85a)
>   *   client/tomcatconf/commands.properties.in (58c770d)
>   *
> engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java
> (c09adca)
>   *   server/src/com/cloud/server/ManagementServerImpl.java (0a0fcdc)
>   *   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
> (0b53cfd)
>   *   ui/dictionary.jsp (f93f9dc)
>   *   ui/scripts/storage.js (88fb9f2)
>
> View Diff<https://reviews.apache.org/r/14522/diff/>
>
>
>


-- 
*Mike Tutkowski*
*Senior CloudStack Developer, SolidFire Inc.*
e: mike.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by "SuichII, Christopher" <Ch...@netapp.com>.

Just bumping this since there haven't been any responses.

Does anyone have any thoughts on this? I'm ready and prepared to do the work, but I don't want to move on if people have concerns with this approach or can think of a better solution.

-Chris
--
Chris Suich
chris.suich@netapp.com<ma...@netapp.com>
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat

On Oct 8, 2013, at 4:53 PM, Chris Suich <Ch...@netapp.com>> wrote:

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

On October 8th, 2013, 8:18 p.m. UTC, edison su wrote:

ui/scripts/storage.js<https://reviews.apache.org/r/14522/diff/1/?file=362033#file362033line1763> (Diff revision 1)

getActionFilter: function() {

1763

revertSnapshot: {

The ui change here, is there way to disable it from ui, if the storage provider is not NetApp? Or move the ui change into your plugin?

This raises the question of whether people expect to see the revert snapshot functionality for hypervisors or just storage providers. I figured that the hypervisor functionality would be desired, but it sounds like that may not be the case for all hypervisors.

Has there been any thoughts to allow storage providers to indicate which features they support? Maybe part of the VolumeResponse can be a set of flags for which operations are supported (take snapshot, revert snapshot, etc.). This way, the UI can dynamically show/hide supported actions without knowing who the volume's storage provider actually is. This should be a fairly straight forward UI change, but would require adding methods to the storage provider interface. If we don't want to always load this information just for the VolumeResponse, we could expose new APIs to query which operations are supported for a given volume, but we may not want to go exposing APIs for this.

Any thoughts?

- Chris

On October 7th, 2013, 8:26 p.m. UTC, Chris Suich wrote:

Review request for cloudstack, Brian Federle and edison su.
By Chris Suich.

Updated Oct. 7, 2013, 8:26 p.m.

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.

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.

Diffs

* api/src/org/apache/cloudstack/api/command/user/snapshot/RevertSnapshotCmd.java (946eebd)
* client/WEB-INF/classes/resources/messages.properties (f92b85a)
* client/tomcatconf/commands.properties.in (58c770d)
* engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java (c09adca)
* server/src/com/cloud/server/ManagementServerImpl.java (0a0fcdc)
* server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java (0b53cfd)
* ui/dictionary.jsp (f93f9dc)
* ui/scripts/storage.js (88fb9f2)

View Diff<https://reviews.apache.org/r/14522/diff/>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> On Oct. 8, 2013, 8:18 p.m., edison su wrote:
> > ui/scripts/storage.js, line 1763
> > <https://reviews.apache.org/r/14522/diff/1/?file=362033#file362033line1763>
> >
> >     The ui change here, is there way to disable it from ui, if the storage provider is not NetApp? Or move the ui change into your plugin?

This raises the question of whether people expect to see the revert snapshot functionality for hypervisors or just storage providers. I figured that the hypervisor functionality would be desired, but it sounds like that may not be the case for all hypervisors.

Has there been any thoughts to allow storage providers to indicate which features they support? Maybe part of the VolumeResponse can be a set of flags for which operations are supported (take snapshot, revert snapshot, etc.). This way, the UI can dynamically show/hide supported actions without knowing who the volume's storage provider actually is. This should be a fairly straight forward UI change, but would require adding methods to the storage provider interface. If we don't want to always load this information just for the VolumeResponse, we could expose new APIs to query which operations are supported for a given volume, but we may not want to go exposing APIs for this.

Any thoughts?


- Chris


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


On Oct. 7, 2013, 8:26 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2013, 8:26 p.m.)
> 
> 
> Review request for cloudstack, Brian Federle and edison su.
> 
> 
> 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/command/user/snapshot/RevertSnapshotCmd.java 946eebd 
>   client/WEB-INF/classes/resources/messages.properties f92b85a 
>   client/tomcatconf/commands.properties.in 58c770d 
>   engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java c09adca 
>   server/src/com/cloud/server/ManagementServerImpl.java 0a0fcdc 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 0b53cfd 
>   ui/dictionary.jsp f93f9dc 
>   ui/scripts/storage.js 88fb9f2 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by edison su <ed...@citrix.com>.

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



ui/scripts/storage.js
<https://reviews.apache.org/r/14522/#comment52129>

    The ui change here, is there way to disable it from ui, if the storage provider is not NetApp? Or move the ui change into your plugin?


- edison su


On Oct. 7, 2013, 8:26 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2013, 8:26 p.m.)
> 
> 
> Review request for cloudstack, Brian Federle and edison su.
> 
> 
> 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/command/user/snapshot/RevertSnapshotCmd.java 946eebd 
>   client/WEB-INF/classes/resources/messages.properties f92b85a 
>   client/tomcatconf/commands.properties.in 58c770d 
>   engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java c09adca 
>   server/src/com/cloud/server/ManagementServerImpl.java 0a0fcdc 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 0b53cfd 
>   ui/dictionary.jsp f93f9dc 
>   ui/scripts/storage.js 88fb9f2 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> On Oct. 22, 2013, 1:32 p.m., John Burwell wrote:
> > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java, line 81
> > <https://reviews.apache.org/r/14522/diff/4/?file=366001#file366001line81>
> >
> >     Convert these lines to a StringUtils.join.  It should reduce the number of lines to one ...

We want to join the items in the key set based on getUUID(). However, by simply string joining the objects, it will call toString() on them. Is there some way around this?


- Chris


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


On Oct. 17, 2013, 6:46 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2013, 6:46 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
> -----
> 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java 81034b1 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java f4ca112 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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



engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
<https://reviews.apache.org/r/14522/#comment53128>

    Convert these lines to a StringUtils.join.  It should reduce the number of lines to one ...



server/src/com/cloud/api/ApiResponseHelper.java
<https://reviews.apache.org/r/14522/#comment53129>

    I recommend simplifying this block to the following:
    
    if (secondaryInfo == null) {
       new CloudRuntimeException("Unable to find info for image store snapshot with uuid '"+snapshot.getUuid()+"'");
    }
    
    snapshotResponse.setRevertable(secondaryInfo.isRevertable());
    
    // The rest of the method ...


- John Burwell


On Oct. 17, 2013, 2:46 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2013, 2:46 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
> -----
> 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java 81034b1 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java f4ca112 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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

Ship it!


Ship It!

- John Burwell


On Oct. 22, 2013, 10:48 a.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 10:48 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
> -----
> 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> On Oct. 22, 2013, 4:04 p.m., John Burwell wrote:
> > Merge of the patch failed.  Please rebase your local branch, test, and re-generate the patch.

I've rebased and added the 4 patches. Please give those a try.


- Chris


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


On Oct. 22, 2013, 2:48 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 2:48 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
> -----
> 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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


Merge of the patch failed.  Please rebase your local branch, test, and re-generate the patch.

- John Burwell


On Oct. 22, 2013, 10:48 a.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 10:48 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
> -----
> 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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

(Updated Oct. 23, 2013, 7:19 p.m.)

Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.

Changes
-------

Updating the patch after that merge (because someone went and change SnapshotPriority before I could commit my changes).

Repository: cloudstack-git

Description
-------

I have also added a 'Revert to Snapshot' action to the volume snapshots list in the UI.

Diffs
-----

engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320
server/src/com/cloud/api/ApiResponseHelper.java 6978c9b

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.

File Attachments (updated)
----------------

https://reviews.apache.org/media/uploaded/files/2013/10/23/0001-Squashed-merged-commit-of-the-following.patch

Thanks,

Chris Suich

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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

(Updated Oct. 22, 2013, 9:53 p.m.)


Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.


Changes
-------

Created squashed merge & commit.


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

  engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
  server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 

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.


File Attachments (updated)
----------------


  https://reviews.apache.org/media/uploaded/files/2013/10/22/0001-Squashed-commits-from-review-revisions.patch


Thanks,

Chris Suich

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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


Please use the --squash option when creating the patch to keep the merge history clean.

- John Burwell


On Oct. 22, 2013, 5:37 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 5:37 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
> -----
> 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 
> 
> 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.
> 
> 
> File Attachments
> ----------------
> 
> 
>   https://reviews.apache.org/media/uploaded/files/2013/10/22/commits.patch
> 
> 
> Thanks,
> 
> Chris Suich
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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

(Updated Oct. 22, 2013, 9:37 p.m.)


Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.


Changes
-------

Rebased and created a single patch file with all 4 commits.


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

  engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
  server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 

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.


File Attachments (updated)
----------------


  https://reviews.apache.org/media/uploaded/files/2013/10/22/commits.patch


Thanks,

Chris Suich

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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


Please rebase and provide a squashed patch.

- John Burwell


On Oct. 22, 2013, 12:42 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 22, 2013, 12:42 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
> -----
> 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 
> 
> 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.
> 
> 
> File Attachments
> ----------------
> 
> 
>   https://reviews.apache.org/media/uploaded/files/2013/10/22/0001-Added-volume-snapshot-revert-capability-to-SnapshotR.patch
> 
>   https://reviews.apache.org/media/uploaded/files/2013/10/22/0002-Updated-inefficient-strategy-sorting-selection.patch
> 
>   https://reviews.apache.org/media/uploaded/files/2013/10/22/0003-Added-context-to-strategy-sorting-error-responses.patch
> 
>   https://reviews.apache.org/media/uploaded/files/2013/10/22/0004-Updated-DataMotionServiceImpl-and-ApiResponseHelper-.patch
> 
> 
> Thanks,
> 
> Chris Suich
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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

(Updated Oct. 22, 2013, 4:42 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
-----

  engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
  server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 

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.


File Attachments (updated)
----------------


  https://reviews.apache.org/media/uploaded/files/2013/10/22/0001-Added-volume-snapshot-revert-capability-to-SnapshotR.patch

  https://reviews.apache.org/media/uploaded/files/2013/10/22/0002-Updated-inefficient-strategy-sorting-selection.patch

  https://reviews.apache.org/media/uploaded/files/2013/10/22/0003-Added-context-to-strategy-sorting-error-responses.patch

  https://reviews.apache.org/media/uploaded/files/2013/10/22/0004-Updated-DataMotionServiceImpl-and-ApiResponseHelper-.patch


Thanks,

Chris Suich

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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

(Updated Oct. 22, 2013, 2:48 p.m.)


Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.


Changes
-------

Refactoring based on review.
Also fixed an issue causing take snapshot to report as failed even though it finished. This was fixed by the:
} else {
  snapshotInfo = (SnapshotInfo) snapshot;
}


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 (updated)
-----

  engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
  server/src/com/cloud/api/ApiResponseHelper.java 6978c9b 

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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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



engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
<https://reviews.apache.org/r/14522/#comment53131>

    Missed that part.  Convert to a proper for each loop:
    
    for (final VolumeInfo aVolumeInfo : volumeMap.keySet().iterator()) {
        volumeIds.add(aVolumeInfo.getUuid());
    }


- John Burwell


On Oct. 17, 2013, 2:46 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2013, 2:46 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
> -----
> 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java 81034b1 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java f4ca112 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

Chris,

I am in-transit to CloudConnect. I will try to review the latest patch by
COB today (21 Oct 2013).

Thanks,
-John

On Oct 21, 2013, at 9:31 AM, "SuichII, Christopher" <Ch...@netapp.com>
wrote:

John,

Will you be able to take a look at this revision soon? It is a small
change to address your comments so it should not take very long.

Thanks,
Chris
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat

On Oct 17, 2013, at 2:46 PM, Chris Suich <Ch...@netapp.com> wrote:

This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14522/
Review request for cloudstack, Brian Federle, edison su, John Burwell,
and Mike Tutkowski.
By Chris Suich.

*Updated Oct. 17, 2013, 6:46 p.m.*
Changes

-Added context to the error messages for not finding
DataMotionStrategies. However, it appears that the place where
pickStrategy() is called for SnapshotStrategies is not able to
actually throw error messages. I will open a separate issue for this.
-Added TODOs for DRYing out the overloaded pickStrategy() methods.
-Added context to the error message for failing to find the image
store snapshot info for a snapshot response. However, this also is
unable to throw a meaning full error message as the exception would be
caught and replaced with a generic error. I will create a separate
issue for this as well.

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

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.

Diffs (updated)

- engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
(81034b1)
- engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
(2d31320)
- server/src/com/cloud/api/ApiResponseHelper.java (f4ca112)
- server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
(dade983)

View Diff <https://reviews.apache.org/r/14522/diff/>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by "SuichII, Christopher" <Ch...@netapp.com>.

John,

Will you be able to take a look at this revision soon? It is a small change to address your comments so it should not take very long.

Thanks,
Chris
--
Chris Suich
chris.suich@netapp.com<ma...@netapp.com>
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat

On Oct 17, 2013, at 2:46 PM, Chris Suich <Ch...@netapp.com>> wrote:

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

Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.
By Chris Suich.

Updated Oct. 17, 2013, 6:46 p.m.

Changes

-Added context to the error messages for not finding DataMotionStrategies. However, it appears that the place where pickStrategy() is called for SnapshotStrategies is not able to actually throw error messages. I will open a separate issue for this.
-Added TODOs for DRYing out the overloaded pickStrategy() methods.
-Added context to the error message for failing to find the image store snapshot info for a snapshot response. However, this also is unable to throw a meaning full error message as the exception would be caught and replaced with a generic error. I will create a separate issue for this as well.

Repository: cloudstack-git
Description

I have also added a 'Revert to Snapshot' action to the volume snapshots list in the UI.

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.

Diffs (updated)

* engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java (81034b1)
* engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java (2d31320)
* server/src/com/cloud/api/ApiResponseHelper.java (f4ca112)
* server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java (dade983)

View Diff<https://reviews.apache.org/r/14522/diff/>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

(Updated Oct. 17, 2013, 6:46 p.m.)

Review request for cloudstack, Brian Federle, edison su, John Burwell, and Mike Tutkowski.

Changes
-------

-Added context to the error messages for not finding DataMotionStrategies. However, it appears that the place where pickStrategy() is called for SnapshotStrategies is not able to actually throw error messages. I will open a separate issue for this.
-Added TODOs for DRYing out the overloaded pickStrategy() methods.
-Added context to the error message for failing to find the image store snapshot info for a snapshot response. However, this also is unable to throw a meaning full error message as the exception would be caught and replaced with a generic error. I will create a separate issue for this as well.

Repository: cloudstack-git

Description
-------

I have also added a 'Revert to Snapshot' action to the volume snapshots list in the UI.

Diffs (updated)
-----

engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java 81034b1
engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320
server/src/com/cloud/api/ApiResponseHelper.java f4ca112
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983

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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> 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.
> 
> 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.
> 
> John Burwell wrote:
>     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, 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.
> 
> 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.
> 
> John Burwell wrote:
>     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.


- Chris


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


On Oct. 17, 2013, 6:46 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2013, 6:46 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
> -----
> 
>   engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java 81034b1 
>   engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java 2d31320 
>   server/src/com/cloud/api/ApiResponseHelper.java f4ca112 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 
> 
> 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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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



engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
<https://reviews.apache.org/r/14522/#comment52773>

    Add context information to error message to help operators and users determine what couldn't be snapshotted.



engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java
<https://reviews.apache.org/r/14522/#comment52774>

    DRY out this code (e.g. extract to a private method).  This rules needs to be consolidated + add the context information mentioned above.



ui/scripts/storage.js
<https://reviews.apache.org/r/14522/#comment52775>

    How will a corrupted snapshot failure be surfaced to the user?


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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


Changes
-------

In this new revision, I have:

-Renamed some of the members and methods
-Dropped the changes to the PrimaryDataStoreDriver
-Changed from sorting the strategies to simply choosing one of the highest priorities
-Started throwing an exception if there was an error determining whether the snapshot can be reverted

After these changes, it appears there is only one remaining issue as the others have either been addressed or the associated code has been dropped.


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 (updated)
-----

  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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.


> On Oct. 15, 2013, 10:41 a.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.
> 
> Chris Suich wrote:
>     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.

Based on your comments, I think we need to rethink the approach a bit.  The storage layer should *not* be concerned with the optimal snapshot strategy for a hypervisor/virtual machine instance/volume etc.  It should simply expose a snapshot capability and let the hypervisor layer determine the best approach.  Therefore, I think the whole notion of sorting strategies in the storage layer needs to pushed up to the hypervisor layer where this determination should be made.

In terms of capability advertisement, the snapshot capability of a storage device will net down to the capabilities of the underlying driver.  Therefore, there must be some consider of the driver model when discussing higher-level storage operations.


> On Oct. 15, 2013, 10:41 a.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?
> 
> Chris Suich wrote:
>     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).
> 
> John Burwell wrote:
>     While a valid approach for UI, it does not seem necessary in the bowels of the driver code.  Given the number of drivers, it would be quicker to simply spin the list once than sort and iterate.  As a knock-on, it will also simplify the method.
> 
> Chris Suich wrote:
>     The issue with this is that the response may not be the same for every snapshot for every driver. For example, if I have the same driver applied to multiple primary storages and there are snapshots for the same volume on each primary storage, then the only snapshots which are revertable are those on the primary storage where the volume currently resides. Some storage drivers may support reverting snapshots from other primary storages, some may not.

See my comments above.  Now that I understand the purpose of this comparator, it feels like something that is better placed in the hypervisor layaer rather the storage layer.


> On Oct. 15, 2013, 10:41 a.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.
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This logic seems overly complicated.  There can be a number of reasons why a snapshot operation will not succeed for a device.  Among those reasons is that the device is being asked to perform an operation it does not support.  As I understand the operation, a snapshot is a operation directed to a particular volume, Volume A.  When the storage layer is asked to snapshot Volume A, the work will eventually be performed by the associated device driver, Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot operation fails.  There are not other options in this scenario.  Provided my understanding is correct, I don't see the need for an additional canHandle method.  Simply call the method, and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current interface seems to only concern itself with whether or not the driver can do something to the exclusion of whether or not the device is in a state to properly p
 erform the operation.
> 
> Chris Suich wrote:
>     Ok, I think I see what you are saying. Once the strategies are sorted (by calling canHandle()), then we no longer need to check whether the strategy can handle the operation and should simply execute the operation on the top sorted strategy.
>     
>     Is that what you are saying?

Essentially, yes.  I also concerned with the atomicity of the operation once we properly consider the state of the device.  By calling canHandle, and then performing the operation, we creates a race condition.  Combining the check with the operation allows the system to atomicity check and perform the operation -- eliminating races with other threads performing the same operation.


> On Oct. 15, 2013, 10:41 a.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?
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This priority sorting should be completely encapsulated.  Requiring clients to "know" leaks an important implementation detail that will lead to inconsistent behavior (i.e. bugs).  One approach would be to use a SortedSet with a Comparator implementation that encapsulates the logic.
> 
> Chris Suich wrote:
>     Encapsulated where? The list of strategies needs to be sorted based on the object and operation.

My concern was that properly using a strategy list required every consumer of the list to "know" that it needed to be sorted.  I want to see this type of logic encapsulated such that consumers of the list can safely assume it is ordered properly.

Based on my evolving understanding of the sort operation's purpose, I think we need to revisit this approach.


> On Oct. 15, 2013, 10:41 a.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);
> 
> Chris Suich wrote:
>     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?
> 
> John Burwell wrote:
>     It's a question of fail fast or fail early.  If the snapshot data is corrupt then the operation should not continue.  Failing quickly will provide the operator with a clear explanation of the failure rather than appearing to be successful, but getting a corrupt result.  From a development perspective, we get earlier, more accurate feedback about database corruption.
> 
> Chris Suich wrote:
>     Agreed - that is appropriate for an operator/admin. But what about the end user? If I have a VM and all of a sudden my list of snapshots simply won't load, what can I do about it? Instead, the list could load successfully, but not return that result, mark it as not revertable, etc.

Failing slowly hurts the end user the most because their backup data is corrupt.  Therefore, we should surface an error message to the end user that causes them their operator/admin.  This type of error should not occur when the system is operating.  In the rare event it does happen, we need to inform the end user as quickly as possible to prevent further corrupt of the snapshot chain.


- John


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


On Oct. 14, 2013, 6: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, 6: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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.


> On Oct. 15, 2013, 10:41 a.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.

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?


> On Oct. 15, 2013, 10:41 a.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".
> 
> Chris Suich wrote:
>     Since this is a method, shouldn't it be isRevertable() as well?

I apologize for the confusion.  In JavaBeans property parlance, the name of the property is revertable which could have an accessor method named isRevertable and a mutator named setRevertable.  I simply referred to the property name instead of being specific about the accessor name.


> On Oct. 15, 2013, 10:41 a.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?
> 
> Chris Suich wrote:
>     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).

While a valid approach for UI, it does not seem necessary in the bowels of the driver code.  Given the number of drivers, it would be quicker to simply spin the list once than sort and iterate.  As a knock-on, it will also simplify the method.


> On Oct. 15, 2013, 10:41 a.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.
> 
> Chris Suich wrote:
>     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.

This logic seems overly complicated.  There can be a number of reasons why a snapshot operation will not succeed for a device.  Among those reasons is that the device is being asked to perform an operation it does not support.  As I understand the operation, a snapshot is a operation directed to a particular volume, Volume A.  When the storage layer is asked to snapshot Volume A, the work will eventually be performed by the associated device driver, Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot operation fails.  There are not other options in this scenario.  Provided my understanding is correct, I don't see the need for an additional canHandle method.  Simply call the method, and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current interface seems to only concern itself with whether or not the driver can do something to the exclusion of whether or not the device is in a state to properly perform
  the operation.


> On Oct. 15, 2013, 10:41 a.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?
> 
> Chris Suich wrote:
>     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.

This priority sorting should be completely encapsulated.  Requiring clients to "know" leaks an important implementation detail that will lead to inconsistent behavior (i.e. bugs).  One approach would be to use a SortedSet with a Comparator implementation that encapsulates the logic. 


> On Oct. 15, 2013, 10:41 a.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
> 
> Chris Suich wrote:
>     Do we have an ACS eclipse profile to handle these coding conventions?

Check with Alex Huang (topcloud on IRC) -- he was talking about putting something together sometime ago, but I don't recall the result.  Personally, I use IntelliJ ...


> On Oct. 15, 2013, 10:41 a.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);
> 
> Chris Suich wrote:
>     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?

It's a question of fail fast or fail early.  If the snapshot data is corrupt then the operation should not continue.  Failing quickly will provide the operator with a clear explanation of the failure rather than appearing to be successful, but getting a corrupt result.  From a development perspective, we get earlier, more accurate feedback about database corruption. 


- John


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


On Oct. 14, 2013, 6: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, 6: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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


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

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.


> 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".
> 
> Chris Suich wrote:
>     Since this is a method, shouldn't it be isRevertable() as well?
> 
> John Burwell wrote:
>     I apologize for the confusion.  In JavaBeans property parlance, the name of the property is revertable which could have an accessor method named isRevertable and a mutator named setRevertable.  I simply referred to the property name instead of being specific about the accessor name.

Just to make sure we're clear, though, this is an interface. There is no property 'revertable'. There is only an isRevertable() method which allows the snapshot to dynamically determine whether it can be reverted to.


> 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?
> 
> Chris Suich wrote:
>     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).
> 
> John Burwell wrote:
>     While a valid approach for UI, it does not seem necessary in the bowels of the driver code.  Given the number of drivers, it would be quicker to simply spin the list once than sort and iterate.  As a knock-on, it will also simplify the method.

The issue with this is that the response may not be the same for every snapshot for every driver. For example, if I have the same driver applied to multiple primary storages and there are snapshots for the same volume on each primary storage, then the only snapshots which are revertable are those on the primary storage where the volume currently resides. Some storage drivers may support reverting snapshots from other primary storages, some may not.


> 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.
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This logic seems overly complicated.  There can be a number of reasons why a snapshot operation will not succeed for a device.  Among those reasons is that the device is being asked to perform an operation it does not support.  As I understand the operation, a snapshot is a operation directed to a particular volume, Volume A.  When the storage layer is asked to snapshot Volume A, the work will eventually be performed by the associated device driver, Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot operation fails.  There are not other options in this scenario.  Provided my understanding is correct, I don't see the need for an additional canHandle method.  Simply call the method, and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current interface seems to only concern itself with whether or not the driver can do something to the exclusion of whether or not the device is in a state to properly p
 erform the operation.

Ok, I think I see what you are saying. Once the strategies are sorted (by calling canHandle()), then we no longer need to check whether the strategy can handle the operation and should simply execute the operation on the top sorted strategy.

Is that what you are saying?


> 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?
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This priority sorting should be completely encapsulated.  Requiring clients to "know" leaks an important implementation detail that will lead to inconsistent behavior (i.e. bugs).  One approach would be to use a SortedSet with a Comparator implementation that encapsulates the logic.

Encapsulated where? The list of strategies needs to be sorted based on the object and operation.


> 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);
> 
> Chris Suich wrote:
>     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?
> 
> John Burwell wrote:
>     It's a question of fail fast or fail early.  If the snapshot data is corrupt then the operation should not continue.  Failing quickly will provide the operator with a clear explanation of the failure rather than appearing to be successful, but getting a corrupt result.  From a development perspective, we get earlier, more accurate feedback about database corruption.

Agreed - that is appropriate for an operator/admin. But what about the end user? If I have a VM and all of a sudden my list of snapshots simply won't load, what can I do about it? Instead, the list could load successfully, but not return that result, mark it as not revertable, etc.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> 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.
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     Based on your comments, I think we need to rethink the approach a bit.  The storage layer should *not* be concerned with the optimal snapshot strategy for a hypervisor/virtual machine instance/volume etc.  It should simply expose a snapshot capability and let the hypervisor layer determine the best approach.  Therefore, I think the whole notion of sorting strategies in the storage layer needs to pushed up to the hypervisor layer where this determination should be made.
>     
>     In terms of capability advertisement, the snapshot capability of a storage device will net down to the capabilities of the underlying driver.  Therefore, there must be some consider of the driver model when discussing higher-level storage operations.

There is a problem with that approach. The snapshot workflow of some storage vendors, myself included, does not match that of CloudStack. Because of this, we need our own SnapshotStrategy to handle our workflow. We should not be pushing snapshot strategies to the hypervisor layer because it is not just hypervisors that have snapshot strategies.

I will submit a patch tomorrow that removes the changes that were made to the storage drivers. In hindsight, they were unnecessary. However, I believe these changes pertaining to the snapshot strategy are necessary, either in this form or another. How else can we enable plugins to become the authorities on how to manage snapshots and ensure their implementations are invoked instead of the hypervisor/default?

Were you able to get understand the objective and approach of this through the dev list discussion? I'm frustrated that these issues weren't brought up until after the code has been written and submitted for review when there has been discussion that I thought explained all of this well before this review was posted.


> 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?
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This priority sorting should be completely encapsulated.  Requiring clients to "know" leaks an important implementation detail that will lead to inconsistent behavior (i.e. bugs).  One approach would be to use a SortedSet with a Comparator implementation that encapsulates the logic.
> 
> Chris Suich wrote:
>     Encapsulated where? The list of strategies needs to be sorted based on the object and operation.
> 
> John Burwell wrote:
>     My concern was that properly using a strategy list required every consumer of the list to "know" that it needed to be sorted.  I want to see this type of logic encapsulated such that consumers of the list can safely assume it is ordered properly.
>     
>     Based on my evolving understanding of the sort operation's purpose, I think we need to revisit this approach.

The order of the list is extremely dynamic. It depends on both the object being operated on and the operation to perform. How would you suggest we always ensure the list is ordered properly when there is no authority managing that list?


> 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.
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This logic seems overly complicated.  There can be a number of reasons why a snapshot operation will not succeed for a device.  Among those reasons is that the device is being asked to perform an operation it does not support.  As I understand the operation, a snapshot is a operation directed to a particular volume, Volume A.  When the storage layer is asked to snapshot Volume A, the work will eventually be performed by the associated device driver, Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot operation fails.  There are not other options in this scenario.  Provided my understanding is correct, I don't see the need for an additional canHandle method.  Simply call the method, and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current interface seems to only concern itself with whether or not the driver can do something to the exclusion of whether or not the device is in a state to properly p
 erform the operation.
> 
> Chris Suich wrote:
>     Ok, I think I see what you are saying. Once the strategies are sorted (by calling canHandle()), then we no longer need to check whether the strategy can handle the operation and should simply execute the operation on the top sorted strategy.
>     
>     Is that what you are saying?
> 
> John Burwell wrote:
>     Essentially, yes.  I also concerned with the atomicity of the operation once we properly consider the state of the device.  By calling canHandle, and then performing the operation, we creates a race condition.  Combining the check with the operation allows the system to atomicity check and perform the operation -- eliminating races with other threads performing the same operation.

How can we determine which strategy to use if we do not check whether the strategy can handle an operation? The canHandle method should not be doing some dynamic check of the state of a the storage to determine whether it can handle an operation. It should simply indicate whether a strategy supports the static operation. If there is a better way to approach this, then that is fine, but the strategies should be able to indicate that. Again, this has nothing to do with the underlying storage. Storage drivers that want to be baked into the default snapshotting workflow can implement the driver level takeSnapshot() and revertSnapshot() method.


> 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?
> 
> Chris Suich wrote:
>     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).
> 
> John Burwell wrote:
>     While a valid approach for UI, it does not seem necessary in the bowels of the driver code.  Given the number of drivers, it would be quicker to simply spin the list once than sort and iterate.  As a knock-on, it will also simplify the method.
> 
> Chris Suich wrote:
>     The issue with this is that the response may not be the same for every snapshot for every driver. For example, if I have the same driver applied to multiple primary storages and there are snapshots for the same volume on each primary storage, then the only snapshots which are revertable are those on the primary storage where the volume currently resides. Some storage drivers may support reverting snapshots from other primary storages, some may not.
> 
> John Burwell wrote:
>     See my comments above.  Now that I understand the purpose of this comparator, it feels like something that is better placed in the hypervisor layaer rather the storage layer.
>     
>     
>

I'm not sure where this hypervisor layer you're talking about is. When requests come in for an operation like snapshotting, it goes straight to the manager, which dispatches the request to a strategy. Snapshot strategies provide alternatives to the hypervisor/default workflow.


> 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);
> 
> Chris Suich wrote:
>     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?
> 
> John Burwell wrote:
>     It's a question of fail fast or fail early.  If the snapshot data is corrupt then the operation should not continue.  Failing quickly will provide the operator with a clear explanation of the failure rather than appearing to be successful, but getting a corrupt result.  From a development perspective, we get earlier, more accurate feedback about database corruption.
> 
> Chris Suich wrote:
>     Agreed - that is appropriate for an operator/admin. But what about the end user? If I have a VM and all of a sudden my list of snapshots simply won't load, what can I do about it? Instead, the list could load successfully, but not return that result, mark it as not revertable, etc.
> 
> John Burwell wrote:
>     Failing slowly hurts the end user the most because their backup data is corrupt.  Therefore, we should surface an error message to the end user that causes them their operator/admin.  This type of error should not occur when the system is operating.  In the rare event it does happen, we need to inform the end user as quickly as possible to prevent further corrupt of the snapshot chain.

If this is the approach the community agrees with, I have no problem with that. I just hadn't seen a scenario quite like this, so I went with what appeared to be the least destructive case - I have no strong feelings either way.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.


> 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.
> 
> Chris Suich wrote:
>     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.
> 
> John Burwell wrote:
>     This logic seems overly complicated.  There can be a number of reasons why a snapshot operation will not succeed for a device.  Among those reasons is that the device is being asked to perform an operation it does not support.  As I understand the operation, a snapshot is a operation directed to a particular volume, Volume A.  When the storage layer is asked to snapshot Volume A, the work will eventually be performed by the associated device driver, Device Driver 1.  If Device Driver 1 does not support the requested operation, then the snapshot operation fails.  There are not other options in this scenario.  Provided my understanding is correct, I don't see the need for an additional canHandle method.  Simply call the method, and if it doesn't throw an exception, the operation succeeded.  I am concerned that the current interface seems to only concern itself with whether or not the driver can do something to the exclusion of whether or not the device is in a state to properly p
 erform the operation.
> 
> Chris Suich wrote:
>     Ok, I think I see what you are saying. Once the strategies are sorted (by calling canHandle()), then we no longer need to check whether the strategy can handle the operation and should simply execute the operation on the top sorted strategy.
>     
>     Is that what you are saying?
> 
> John Burwell wrote:
>     Essentially, yes.  I also concerned with the atomicity of the operation once we properly consider the state of the device.  By calling canHandle, and then performing the operation, we creates a race condition.  Combining the check with the operation allows the system to atomicity check and perform the operation -- eliminating races with other threads performing the same operation.
> 
> Chris Suich wrote:
>     How can we determine which strategy to use if we do not check whether the strategy can handle an operation? The canHandle method should not be doing some dynamic check of the state of a the storage to determine whether it can handle an operation. It should simply indicate whether a strategy supports the static operation. If there is a better way to approach this, then that is fine, but the strategies should be able to indicate that. Again, this has nothing to do with the underlying storage. Storage drivers that want to be baked into the default snapshotting workflow can implement the driver level takeSnapshot() and revertSnapshot() method.

I forgot to mention - the reason I don't like having a completely static response to whether the strategy supports an operation (like the getCapabilities() posted above) is that there is some runtime logic that can determine whether a plugin strategy can handle an operation. My use case, for example, is that our strategy only works on VMs on our storage. A non-storage use case may be something like having a plugin snapshot strategy that is only used for VMs using a certain disk offering. In cases like this, the snapshot strategy needs to run some check against the VM/volume the request is for, not just a compile time declaration of what it can do.


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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by John Burwell <jb...@basho.com>.

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



api/src/org/apache/cloudstack/api/response/SnapshotResponse.java
<https://reviews.apache.org/r/14522/#comment52591>

    CanRevert is an awkward/non-standard property name.  Prefer an adverb such as revertable (e.g. isRevertable/setRevertable with an attribute named revertableFlag).



engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java
<https://reviews.apache.org/r/14522/#comment52592>

    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.



engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java
<https://reviews.apache.org/r/14522/#comment52593>

    Prefer an adverb property name such as "revertable".



engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
<https://reviews.apache.org/r/14522/#comment52595>

    How does the result of the canHandle method drive the comparison of two Snapshots?  Why not just compare the ops to each other other?



engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java
<https://reviews.apache.org/r/14522/#comment52594>

    Very poor performance as it circumvents the primitive pool.  Change to Integer.valueOf(i1).compareTo(Integer.valueOf(i2)).



engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java
<https://reviews.apache.org/r/14522/#comment52596>

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



engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java
<https://reviews.apache.org/r/14522/#comment52597>

    What is the purpose of sorting the strategies?



engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java
<https://reviews.apache.org/r/14522/#comment52598>

    Coding convention: Add curly braces



server/src/com/cloud/api/ApiResponseHelper.java
<https://reviews.apache.org/r/14522/#comment52599>

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


- John Burwell


On Oct. 14, 2013, 6: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, 6: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
> 
>

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by Chris Suich <ch...@netapp.com>.

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


Changes
-------

Revision 2 of this review updates SnapshotResponses to include whether the snapshot can be reverted by asking snapshot strategies whether they can handle the request. The patch also adds the UI logic necessary to show/hide the revert action depending on this field of the SnapshotResponse.


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 (updated)
-----

  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

Re: Review Request 14522: [CLOUDSTACK-4771] Support Revert VM Disk from Snapshot

Posted by edison su <ed...@citrix.com>.

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

Ship it!


Ship It!

- edison su


On Oct. 7, 2013, 8:26 p.m., Chris Suich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14522/
> -----------------------------------------------------------
> 
> (Updated Oct. 7, 2013, 8:26 p.m.)
> 
> 
> Review request for cloudstack, Brian Federle and edison su.
> 
> 
> 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/command/user/snapshot/RevertSnapshotCmd.java 946eebd 
>   client/WEB-INF/classes/resources/messages.properties f92b85a 
>   client/tomcatconf/commands.properties.in 58c770d 
>   engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotServiceImpl.java c09adca 
>   server/src/com/cloud/server/ManagementServerImpl.java 0a0fcdc 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 0b53cfd 
>   ui/dictionary.jsp f93f9dc 
>   ui/scripts/storage.js 88fb9f2 
> 
> 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
> 
>