incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Devdeep Singh" <devdeep.si...@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 Sat, 07 Jul 2012 03:03:28 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

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.


> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 380
> > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line380>
> >
> >     Please use a finally() to do the decrement

Same query as above. Can you clarify more on this comment. What additional purpose will adding
a try finally around this code block do, if we are only throwing an exception in the if code
block.


> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 391
> > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line391>
> >
> >     Please use the finally above to do the decrement
> >

decrement is already being done in catch exception. Any particular reason why a finally needs
to be added to do a decrement. We only want to decrement on error and not otherwise.


> On July 6, 2012, 9:52 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 497
> > <https://reviews.apache.org/r/5806/diff/1/?file=119898#file119898line497>
> >
> >     Again this is boilerplate code.

We are already in a finally code block and we want to decrement only if snapshot backup fails
and it is in error state.


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

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?


- Devdeep


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