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 Tue, 11 Sep 2012 05:32:04 GMT


> On Aug. 22, 2012, 9:11 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java, line 310
> > <https://reviews.apache.org/r/5806/diff/3/?file=141357#file141357line310>
> >
> >     By changing the functionality of checkResourceLimit you are risking it for all
the resource creation in the CS.
> >     Have you tested for them ? 
> >     
> >     Instead of touching the existing functions I advise you to rewrite it all together
and slowly all the owners will move their code to using your functions.
> 
> deepti dohare wrote:
>     Here we are not changing the functionality of checkresourceLimit. To avoid repetition
of the code, we created a new method checkResourceLimitforAccount, which is same as checkresourcelimit
without locks, such that it can be called in checkAndIncrementResourceCount.
>     
>     Tested for resources: instances, volume, snapshots and templates.

Wht i meant is changing fn name is not the right way
Suffixing 2 means this is the same function but the next version and people should start using
it just like we have product versions
This relates to more maintainable code as well
Also I don't want the existing functions to be touched simply bcz all the resource creation
depends on them
and this requires a lot of testing with all the edge cases ironed out. What you should do
is call the snapshot creating through this and do a thorough testing
Write a comment for this function so that the owners themselves would start migrating to the
new function you will write.


- Nitin


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


On Sept. 8, 2012, 9:50 a.m., deepti dohare wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2012, 9:50 a.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
> -----
> 
>   api/src/com/cloud/user/ResourceLimitService.java 98dfc11 
>   server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java 7f446d7 
>   server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 8ae8ddc 
> 
> 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