You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Likitha Shetty <li...@citrix.com> on 2012/06/27 08:47:04 UTC
Review Request: listSnapshotPolicies command,
volumeId parameter made optional.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5604/
-----------------------------------------------------------
Review request for cloudstack.
Description
-------
Changes made to allow the admin to see all the the policies in the system when 'listSnapshotPolicies' is executed. The optional parameter 'volumeId' can be used to narrow down the search.
This addresses bug CS-4109.
Diffs
-----
api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7
Diff: https://reviews.apache.org/r/5604/diff/
Testing
-------
Thanks,
Likitha Shetty
Re: Review Request: listSnapshotPolicies command,
volumeId parameter made optional.
Posted by Prachi Damle <pr...@citrix.com>.
> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
> > Ideally listSnapshotPolicy should be part of BaseListProjectAndAccountResourcesCmd.
listSnapshotPolicy cannot extend BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity does not carry the account/domain attributes. Instead access checks are made to the volume to which the policy refers.
- Prachi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5604/#review8706
-----------------------------------------------------------
On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5604/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:47 a.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> Changes made to allow the admin to see all the the policies in the system when 'listSnapshotPolicies' is executed. The optional parameter 'volumeId' can be used to narrow down the search.
>
>
> This addresses bug CS-4109.
>
>
> Diffs
> -----
>
> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7
>
> Diff: https://reviews.apache.org/r/5604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Likitha Shetty
>
>
RE: Review Request: listSnapshotPolicies command, volumeId
parameter made optional.
Posted by Likitha Shetty <li...@citrix.com>.
Thank you all for the review. I will submit a new patch based on the approach mentioned by Alena.
Regards,
Likitha
-----Original Message-----
From: Prachi Damle
Sent: Saturday, July 07, 2012 6:04 AM
To: cloudstack-dev@incubator.apache.org; Nitin Mehta; Likitha Shetty
Subject: RE: Review Request: listSnapshotPolicies command, volumeId parameter made optional.
>>>> Also with this implementation what happens is that when the admin
>>>>calls the api without volumeid he sees all the policies
>That¹s exactly what the bug CS-4109 asked for.
>
>But yes as you point out -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+cha
>nge
>s says otherwise.
>
>
>Alena,
>Is CS 4109 moot now, considering the API policy since 3.0.x?
>We should still fix the bug; none of the parameters should be required
>in
>list* calls. But we should fix it differently, following the way Nitin suggested. It will require DB changes + DB upgrade:
>* add account/domainId info to snapshot_policy table
>* for the customers upgrading to the new release, auto populate these field with account/domainId info from the corresponding volume record.
>* listSnapshotsCmd should extend BaseListProjectAndAccountResourcesCmd
>We don't have to do it in Burbank as it's a minor bug to begin with and requires DB changes. Lets punt it to the next release.
>-Alena.
Thanks, updated the bug with these details.
Likitha,
Will be ignoring this patch as per the review discussion.
-Prachi
>-----Original Message-----
>From: Nitin Mehta [mailto:noreply@reviews.apache.org] On Behalf Of
>Nitin Mehta
>Sent: Friday, July 06, 2012 4:52 PM
>To: cloudstack; Prachi Damle; Likitha Shetty; Nitin Mehta
>Subject: Re: Review Request: listSnapshotPolicies command, volumeId
>parameter made optional.
>
>
>
>> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
>> > Ideally listSnapshotPolicy should be part of
>>BaseListProjectAndAccountResourcesCmd.
>>
>> Prachi Damle wrote:
>> listSnapshotPolicy cannot extend
>>BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity
>>does not carry the account/domain attributes. Instead access checks
>>are made to the volume to which the policy refers.
>
>That's a fair point Prachi. But, then I think snapshotPolicy should
>carry the account/domain entity. Figuring it out from the volume is not
>the right way to do so. This is not how we do it in CS correct ?
>Also with this implementation what happens is that when the admin calls
>the api without volumeid he sees all the policies whereas he should see
>only HIS policies. wiki -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+cha
>nge
>s says that.
>So I say that we need to introduce account and domain info in the
>snapshot_policy table and also have listSnapshotPolicy extend
>BaseListProjectAndAccountResourcesCmd.
>
>
>- Nitin
>
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/5604/#review8706
>-----------------------------------------------------------
>
>
>On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/5604/
>> -----------------------------------------------------------
>>
>> (Updated June 27, 2012, 6:47 a.m.)
>>
>>
>> Review request for cloudstack.
>>
>>
>> Description
>> -------
>>
>> Changes made to allow the admin to see all the the policies in the
>>system when 'listSnapshotPolicies' is executed. The optional parameter
>>'volumeId' can be used to narrow down the search.
>>
>>
>> This addresses bug CS-4109.
>>
>>
>> Diffs
>> -----
>>
>> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
>> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
>> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
>>b97a7d7
>>
>> Diff: https://reviews.apache.org/r/5604/diff/
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Likitha Shetty
>>
>>
>
>
RE: Review Request: listSnapshotPolicies command, volumeId
parameter made optional.
Posted by Prachi Damle <Pr...@citrix.com>.
>>>> Also with this implementation what happens is that when the admin
>>>>calls the api without volumeid he sees all the policies
>That¹s exactly what the bug CS-4109 asked for.
>
>But yes as you point out -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+cha
>nge
>s says otherwise.
>
>
>Alena,
>Is CS 4109 moot now, considering the API policy since 3.0.x?
>We should still fix the bug; none of the parameters should be required in
>list* calls. But we should fix it differently, following the way Nitin suggested. It will require DB changes + DB upgrade:
>* add account/domainId info to snapshot_policy table
>* for the customers upgrading to the new release, auto populate these field with account/domainId info from the corresponding volume record.
>* listSnapshotsCmd should extend BaseListProjectAndAccountResourcesCmd
>We don't have to do it in Burbank as it's a minor bug to begin with and requires DB changes. Lets punt it to the next release.
>-Alena.
Thanks, updated the bug with these details.
Likitha,
Will be ignoring this patch as per the review discussion.
-Prachi
>-----Original Message-----
>From: Nitin Mehta [mailto:noreply@reviews.apache.org] On Behalf Of
>Nitin Mehta
>Sent: Friday, July 06, 2012 4:52 PM
>To: cloudstack; Prachi Damle; Likitha Shetty; Nitin Mehta
>Subject: Re: Review Request: listSnapshotPolicies command, volumeId
>parameter made optional.
>
>
>
>> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
>> > Ideally listSnapshotPolicy should be part of
>>BaseListProjectAndAccountResourcesCmd.
>>
>> Prachi Damle wrote:
>> listSnapshotPolicy cannot extend
>>BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity
>>does not carry the account/domain attributes. Instead access checks
>>are made to the volume to which the policy refers.
>
>That's a fair point Prachi. But, then I think snapshotPolicy should
>carry the account/domain entity. Figuring it out from the volume is not
>the right way to do so. This is not how we do it in CS correct ?
>Also with this implementation what happens is that when the admin calls
>the api without volumeid he sees all the policies whereas he should see
>only HIS policies. wiki -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+cha
>nge
>s says that.
>So I say that we need to introduce account and domain info in the
>snapshot_policy table and also have listSnapshotPolicy extend
>BaseListProjectAndAccountResourcesCmd.
>
>
>- Nitin
>
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/5604/#review8706
>-----------------------------------------------------------
>
>
>On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/5604/
>> -----------------------------------------------------------
>>
>> (Updated June 27, 2012, 6:47 a.m.)
>>
>>
>> Review request for cloudstack.
>>
>>
>> Description
>> -------
>>
>> Changes made to allow the admin to see all the the policies in the
>>system when 'listSnapshotPolicies' is executed. The optional parameter
>>'volumeId' can be used to narrow down the search.
>>
>>
>> This addresses bug CS-4109.
>>
>>
>> Diffs
>> -----
>>
>> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
>> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
>> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
>>b97a7d7
>>
>> Diff: https://reviews.apache.org/r/5604/diff/
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Likitha Shetty
>>
>>
>
>
Re: Review Request: listSnapshotPolicies command, volumeId
parameter made optional.
Posted by Alena Prokharchyk <Al...@citrix.com>.
On 7/6/12 5:10 PM, "Prachi Damle" <Pr...@citrix.com> wrote:
>>>> Also with this implementation what happens is that when the admin
>>>>calls the api without volumeid he sees all the policies
>That¹s exactly what the bug CS-4109 asked for.
>
>But yes as you point out -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+change
>s says otherwise.
>
>
>Alena,
>Is CS 4109 moot now, considering the API policy since 3.0.x?
We should still fix the bug; none of the parameters should be required in
list* calls. But we should fix it differently, following the way Nitin
suggested. It will require DB changes + DB upgrade:
* add account/domainId info to snapshot_policy table
* for the customers upgrading to the new release, auto populate these
field with account/domainId info from the corresponding volume record.
* listSnapshotsCmd should extend BaseListProjectAndAccountResourcesCmd
We don't have to do it in Burbank as it's a minor bug to begin with and
requires DB changes. Lets punt it to the next release.
-Alena.
>
>
>-Prachi
>
>-----Original Message-----
>From: Nitin Mehta [mailto:noreply@reviews.apache.org] On Behalf Of Nitin
>Mehta
>Sent: Friday, July 06, 2012 4:52 PM
>To: cloudstack; Prachi Damle; Likitha Shetty; Nitin Mehta
>Subject: Re: Review Request: listSnapshotPolicies command, volumeId
>parameter made optional.
>
>
>
>> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
>> > Ideally listSnapshotPolicy should be part of
>>BaseListProjectAndAccountResourcesCmd.
>>
>> Prachi Damle wrote:
>> listSnapshotPolicy cannot extend
>>BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity
>>does not carry the account/domain attributes. Instead access checks are
>>made to the volume to which the policy refers.
>
>That's a fair point Prachi. But, then I think snapshotPolicy should
>carry the account/domain entity. Figuring it out from the volume is not
>the right way to do so. This is not how we do it in CS correct ?
>Also with this implementation what happens is that when the admin calls
>the api without volumeid he sees all the policies whereas he should see
>only HIS policies. wiki -
>http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+change
>s says that.
>So I say that we need to introduce account and domain info in the
>snapshot_policy table and also have listSnapshotPolicy extend
>BaseListProjectAndAccountResourcesCmd.
>
>
>- Nitin
>
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/5604/#review8706
>-----------------------------------------------------------
>
>
>On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/5604/
>> -----------------------------------------------------------
>>
>> (Updated June 27, 2012, 6:47 a.m.)
>>
>>
>> Review request for cloudstack.
>>
>>
>> Description
>> -------
>>
>> Changes made to allow the admin to see all the the policies in the
>>system when 'listSnapshotPolicies' is executed. The optional parameter
>>'volumeId' can be used to narrow down the search.
>>
>>
>> This addresses bug CS-4109.
>>
>>
>> Diffs
>> -----
>>
>> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
>> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
>> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
>>b97a7d7
>>
>> Diff: https://reviews.apache.org/r/5604/diff/
>>
>>
>> Testing
>> -------
>>
>>
>> Thanks,
>>
>> Likitha Shetty
>>
>>
>
>
RE: Review Request: listSnapshotPolicies command, volumeId
parameter made optional.
Posted by Prachi Damle <Pr...@citrix.com>.
>>> Also with this implementation what happens is that when the admin calls the api without volumeid he sees all the policies
That’s exactly what the bug CS-4109 asked for.
But yes as you point out - http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+changes says otherwise.
Alena,
Is CS 4109 moot now, considering the API policy since 3.0.x?
-Prachi
-----Original Message-----
From: Nitin Mehta [mailto:noreply@reviews.apache.org] On Behalf Of Nitin Mehta
Sent: Friday, July 06, 2012 4:52 PM
To: cloudstack; Prachi Damle; Likitha Shetty; Nitin Mehta
Subject: Re: Review Request: listSnapshotPolicies command, volumeId parameter made optional.
> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
> > Ideally listSnapshotPolicy should be part of BaseListProjectAndAccountResourcesCmd.
>
> Prachi Damle wrote:
> listSnapshotPolicy cannot extend BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity does not carry the account/domain attributes. Instead access checks are made to the volume to which the policy refers.
That's a fair point Prachi. But, then I think snapshotPolicy should carry the account/domain entity. Figuring it out from the volume is not the right way to do so. This is not how we do it in CS correct ?
Also with this implementation what happens is that when the admin calls the api without volumeid he sees all the policies whereas he should see only HIS policies. wiki - http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+changes says that.
So I say that we need to introduce account and domain info in the snapshot_policy table and also have listSnapshotPolicy extend BaseListProjectAndAccountResourcesCmd.
- Nitin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5604/#review8706
-----------------------------------------------------------
On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5604/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:47 a.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> Changes made to allow the admin to see all the the policies in the system when 'listSnapshotPolicies' is executed. The optional parameter 'volumeId' can be used to narrow down the search.
>
>
> This addresses bug CS-4109.
>
>
> Diffs
> -----
>
> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7
>
> Diff: https://reviews.apache.org/r/5604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Likitha Shetty
>
>
Re: Review Request: listSnapshotPolicies command,
volumeId parameter made optional.
Posted by Nitin Mehta <ni...@citrix.com>.
> On June 28, 2012, 12:21 p.m., Nitin Mehta wrote:
> > Ideally listSnapshotPolicy should be part of BaseListProjectAndAccountResourcesCmd.
>
> Prachi Damle wrote:
> listSnapshotPolicy cannot extend BaseListProjectAndAccountResourcesCmd, since the snapshotPolicy entity does not carry the account/domain attributes. Instead access checks are made to the volume to which the policy refers.
That's a fair point Prachi. But, then I think snapshotPolicy should carry the account/domain entity. Figuring it out from the volume is not the right way to do so. This is not how we do it in CS correct ?
Also with this implementation what happens is that when the admin calls the api without volumeid he sees all the policies whereas he should see only HIS policies. wiki - http://confluence.cloudstack.org/display/gen/3.0+API+list*+commands+changes says that.
So I say that we need to introduce account and domain info in the snapshot_policy table and also have listSnapshotPolicy extend BaseListProjectAndAccountResourcesCmd.
- Nitin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5604/#review8706
-----------------------------------------------------------
On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5604/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:47 a.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> Changes made to allow the admin to see all the the policies in the system when 'listSnapshotPolicies' is executed. The optional parameter 'volumeId' can be used to narrow down the search.
>
>
> This addresses bug CS-4109.
>
>
> Diffs
> -----
>
> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7
>
> Diff: https://reviews.apache.org/r/5604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Likitha Shetty
>
>
Re: Review Request: listSnapshotPolicies command,
volumeId parameter made optional.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5604/#review8706
-----------------------------------------------------------
Ideally listSnapshotPolicy should be part of BaseListProjectAndAccountResourcesCmd.
- Nitin Mehta
On June 27, 2012, 6:47 a.m., Likitha Shetty wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5604/
> -----------------------------------------------------------
>
> (Updated June 27, 2012, 6:47 a.m.)
>
>
> Review request for cloudstack.
>
>
> Description
> -------
>
> Changes made to allow the admin to see all the the policies in the system when 'listSnapshotPolicies' is executed. The optional parameter 'volumeId' can be used to narrow down the search.
>
>
> This addresses bug CS-4109.
>
>
> Diffs
> -----
>
> api/src/com/cloud/api/commands/ListSnapshotPoliciesCmd.java c19924b
> api/src/com/cloud/storage/snapshot/SnapshotService.java 0500061
> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7
>
> Diff: https://reviews.apache.org/r/5604/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Likitha Shetty
>
>