incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Prachi Damle <Prachi.Da...@citrix.com>
Subject RE: Review Request: listSnapshotPolicies command, volumeId parameter made optional.
Date Sat, 07 Jul 2012 00:34:03 GMT
>>>> 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
>> 
>>
>
>



Mime
View raw message