incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nitin Mehta" <nitin.me...@citrix.com>
Subject Re: Review Request: CS-15430 Create snapshot should fail if creating snapshot results in exceeding snapshot resource limit for domain-admin or user accounts.
Date Mon, 09 Jul 2012 18:24:41 GMT


> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 375
> > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line375>
> >
> >     Please use a finally() to do the decrement
> 
> Devdeep Singh wrote:
>     The intention here is to decrement the resource count if an exception is being thrown
on error. Are you suggesting that the entire if check needs to be put in in a try finally
and resource count decremented on error? Can you clarify more on your comment.

so why I want finally is to not to repeat the same code. Have a boolean flag and assume failure.
In case of success mark it as success. finally block will see the flag and accordingly decrement.
I think you can use the backedup flag in the code we are already using. So all you need to
do is move the first three checks under the try block. we already have finally block and we
are checking the backedup flag to decrement the count.


> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 1332
> > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line1332>
> >
> >     If you go inside the function checkResourceLimit and incrementResourceCount
you will see that they are synchronized by locking the rows so you don't require this.
> 
> Devdeep Singh wrote:
>     The intention is to synchronize across checkResourceLimit and incrementResourceCount
function and not just within these two functions. For example, if resource limit is set to
5 for a domain and already the limit has reached 4. If two requests comes in from different
accounts in the domain for creating a snapshot; if both of them are able to check the limit
before either of them increments the count, we can hit a scenario where the limits can be
exceeded.
>     
>     Am I missing something?

That's a valid point you have raised. I am curious how we solve it in CS in general or is
this corner case not covered. I am also thinking loud that if we are checking limit and incrementing
at the same place why do we need separate functions ? Why is checking not a part of increment
? Please raise this issue in the list if you think this hasnt been addressed in CS.


- Nitin


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


On July 6, 2012, 12:43 p.m., deepti dohare wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
> 
> (Updated July 6, 2012, 12:43 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Description
> -------
> 
> Change:
> 1. Before creating the snapshot, we synchronized checkResourcelimit to allow the users
to create the snapshot and increment the resource count.
> 2. Depending on the failure of snapshot creation/ backup, we are decrementing the resource
count.
> 
> 
> This addresses bug CS-15430.
> 
> 
> Diffs
> -----
> 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java b97a7d7 
> 
> Diff: https://reviews.apache.org/r/5806/diff/
> 
> 
> Testing
> -------
> 
> Steps to verify:
> 1.Login as admin, set snapshot limit '3' for a user account
> 2.login as user, create a VM1 with data volume
> 3.trigger 3 create snapshot command from the above data volume, succeeded
> 4.create one more snapshot, failed, "maximum limit exceeded for account user"
> 
> 
> Thanks,
> 
> deepti dohare
> 
>


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