You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by deepti dohare <de...@citrix.com> on 2012/07/06 14:43:11 UTC
Review Request: CS-15430 Create snapshot should fail if creating snapshot
results in exceeding snapshot resource limit for domain-admin or user
accounts.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/
-----------------------------------------------------------
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
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Devdeep Singh <de...@citrix.com>.
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review8930
-----------------------------------------------------------
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18977>
Please use a finally() to do the decrement
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18978>
Please use a finally() to do the decrement
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18979>
Please use the finally above to do the decrement
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18980>
Again this is boilerplate code.
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18981>
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.
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment18982>
Please use a finally() to do the decrement
- Nitin Mehta
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
>
>
RE: Review Request: CS-15430 Create snapshot should fail if
creating snapshot results in exceeding snapshot resource limit for
domain-admin or user accounts.
Posted by Devdeep Singh <de...@citrix.com>.
We missed the scenario for management servers in clustered mode. Thanks for pointing it out.
We'll work on having a generic implementation in ResourceLimitManager to fix the problem.
Regards,
Devdeep
> -----Original Message-----
> From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com]
> Sent: Thursday, July 12, 2012 11:57 PM
> To: CloudStack DeveloperList; Nitin Mehta; Deepti Dohare
> 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.
>
> Synchronized isn't going to solve all your cases since management servers
> can be clustered.
>
> You need to use GenericDaoBase.acquireInLockTable for these kinds of
> synchronization.
>
> This kind of generic service (atomic increment up to a limit) should be in the
> ResourceLimitManager.
> Perhaps updateResourceCountForAccount can be modified to take into
> account the resource limit.
>
> --
> Chiradeep
>
> On 7/11/12 11:02 PM, "Nitin Mehta" <ni...@citrix.com> wrote:
>
> >
> >-----------------------------------------------------------
> >This is an automatically generated e-mail. To reply, visit:
> >https://reviews.apache.org/r/5806/#review9100
> >-----------------------------------------------------------
> >
> >Ship it!
> >
> >
> >Ship It!
> >
> >- Nitin Mehta
> >
> >
> >On July 10, 2012, 1:32 p.m., deepti dohare wrote:
> >>
> >> -----------------------------------------------------------
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/5806/
> >> -----------------------------------------------------------
> >>
> >> (Updated July 10, 2012, 1:32 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
> >>50dcf38
> >>
> >> 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
> >>
> >>
> >
RE: Review Request: CS-15430 Create snapshot should fail if
creating snapshot results in exceeding snapshot resource limit for
domain-admin or user accounts.
Posted by Rajesh Battala <ra...@citrix.com>.
Nice idea to have atomic increment in ResourceLimitManager class. Having it in ResourceLimitManager will solve the resource limit problem for other Resource Types (instances, templates ) and any Resource Types in future.
> -----Original Message-----
> From: Chiradeep Vittal [mailto:Chiradeep.Vittal@citrix.com]
> Sent: Thursday, July 12, 2012 11:57 PM
> To: CloudStack DeveloperList; Nitin Mehta; Deepti Dohare
> 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.
>
> Synchronized isn't going to solve all your cases since management servers
> can be clustered.
>
> You need to use GenericDaoBase.acquireInLockTable for these kinds of
> synchronization.
>
> This kind of generic service (atomic increment up to a limit) should be in the
> ResourceLimitManager.
> Perhaps updateResourceCountForAccount can be modified to take into
> account the resource limit.
>
> --
> Chiradeep
>
> On 7/11/12 11:02 PM, "Nitin Mehta" <ni...@citrix.com> wrote:
>
> >
> >-----------------------------------------------------------
> >This is an automatically generated e-mail. To reply, visit:
> >https://reviews.apache.org/r/5806/#review9100
> >-----------------------------------------------------------
> >
> >Ship it!
> >
> >
> >Ship It!
> >
> >- Nitin Mehta
> >
> >
> >On July 10, 2012, 1:32 p.m., deepti dohare wrote:
> >>
> >> -----------------------------------------------------------
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/5806/
> >> -----------------------------------------------------------
> >>
> >> (Updated July 10, 2012, 1:32 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
> >>50dcf38
> >>
> >> 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
> >>
> >>
> >
Re: Review Request: CS-15430 Create snapshot should fail if
creating snapshot results in exceeding snapshot resource limit for
domain-admin or user accounts.
Posted by Chiradeep Vittal <Ch...@citrix.com>.
Synchronized isn't going to solve all your cases since management servers
can be clustered.
You need to use GenericDaoBase.acquireInLockTable for these kinds of
synchronization.
This kind of generic service (atomic increment up to a limit) should be in
the ResourceLimitManager.
Perhaps updateResourceCountForAccount can be modified to take into account
the resource limit.
--
Chiradeep
On 7/11/12 11:02 PM, "Nitin Mehta" <ni...@citrix.com> wrote:
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/5806/#review9100
>-----------------------------------------------------------
>
>Ship it!
>
>
>Ship It!
>
>- Nitin Mehta
>
>
>On July 10, 2012, 1:32 p.m., deepti dohare wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/5806/
>> -----------------------------------------------------------
>>
>> (Updated July 10, 2012, 1:32 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
>>50dcf38
>>
>> 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
>>
>>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review9100
-----------------------------------------------------------
Ship it!
Ship It!
- Nitin Mehta
On July 10, 2012, 1:32 p.m., deepti dohare wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
>
> (Updated July 10, 2012, 1:32 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 50dcf38
>
> 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
>
>
RE: Review Request: CS-15430 Create snapshot should fail if
creating snapshot results in exceeding snapshot resource limit for
domain-admin or user accounts.
Posted by Nitin Mehta <Ni...@citrix.com>.
Thanks Devdeep and Deepti for your work. Please file a bug for this issue.
-----Original Message-----
From: Devdeep Singh [mailto:noreply@reviews.apache.org] On Behalf Of Devdeep Singh
Sent: Tuesday, July 10, 2012 12:03 PM
To: cloudstack; Deepti Dohare; Devdeep Singh; Nitin Mehta
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.
> On July 10, 2012, 2:23 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line
> > 1355
> > <https://reviews.apache.org/r/5806/diff/2/?file=121206#file121206lin
> > e1355>
> >
> > Did you inquire how we solve this problem in other places in CS ? I cant approve this change since we dont want to reinvent the wheel if this has already been solved. If it hasn't been solved then I would want to raise this in the mailing list and hear out a good way of doing this.
We looked into how check and increment resource limits were being handled for other resources, like instances. Looking at the code it seems a similar problem may be present there too. However, we have to simulate the scenario (by probably suspending threads in a debugger) and confirm it. If the issue is present I'll bring up the issue on the list and file bugs for the same.
- Devdeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review9018
-----------------------------------------------------------
On July 10, 2012, 1:32 p.m., deepti dohare wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
>
> (Updated July 10, 2012, 1:32 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
> 50dcf38
>
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Devdeep Singh <de...@citrix.com>.
> On July 10, 2012, 2:23 p.m., Nitin Mehta wrote:
> > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java, line 1355
> > <https://reviews.apache.org/r/5806/diff/2/?file=121206#file121206line1355>
> >
> > Did you inquire how we solve this problem in other places in CS ? I cant approve this change since we dont want to reinvent the wheel if this has already been solved. If it hasn't been solved then I would want to raise this in the mailing list and hear out a good way of doing this.
We looked into how check and increment resource limits were being handled for other resources, like instances. Looking at the code it seems a similar problem may be present there too. However, we have to simulate the scenario (by probably suspending threads in a debugger) and confirm it. If the issue is present I'll bring up the issue on the list and file bugs for the same.
- Devdeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review9018
-----------------------------------------------------------
On July 10, 2012, 1:32 p.m., deepti dohare wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
>
> (Updated July 10, 2012, 1:32 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 50dcf38
>
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review9018
-----------------------------------------------------------
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment19172>
Why this new if (!backedUp) block when you already have this block above ? Please put the decrementing logic in above block
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment19173>
Did you inquire how we solve this problem in other places in CS ? I cant approve this change since we dont want to reinvent the wheel if this has already been solved. If it hasn't been solved then I would want to raise this in the mailing list and hear out a good way of doing this.
- Nitin Mehta
On July 10, 2012, 1:32 p.m., deepti dohare wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
>
> (Updated July 10, 2012, 1:32 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 50dcf38
>
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by deepti dohare <de...@citrix.com>.
-----------------------------------------------------------
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.
Changes
-------
Changes made based on review comments. Verified locally for clustered management servers
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 (updated)
-----
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
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by deepti dohare <de...@citrix.com>.
> 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.
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.
- deepti
-----------------------------------------------------------
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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by Nitin Mehta <ni...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/#review10636
-----------------------------------------------------------
api/src/com/cloud/user/ResourceLimitService.java
<https://reviews.apache.org/r/5806/#comment22750>
Check the documentation for the function put in here
api/src/com/cloud/user/ResourceLimitService.java
<https://reviews.apache.org/r/5806/#comment22751>
Check the documentation for the function put in here
api/src/com/cloud/user/ResourceLimitService.java
<https://reviews.apache.org/r/5806/#comment22752>
Check the documentation for the function put in here
server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
<https://reviews.apache.org/r/5806/#comment22760>
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.
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment22753>
You are again repeating the code here. You can simply have a failure flag which u use in the finally block. I think I passed the feedback in earlier reviews for not repeating the code. Please refer to that
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment22755>
Dont need this loop at all for the same reasons as above
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment22756>
It would be helpful to say which snapshot failed here so include its uuid, name etc
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java
<https://reviews.apache.org/r/5806/#comment22757>
It would be helpful to say which snapshot failed here so include its uuid etc
- Nitin Mehta
On Aug. 16, 2012, 10:46 a.m., deepti dohare wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5806/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2012, 10:46 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 b285d2c
> server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 6e3f9c1
>
> 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
>
>
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by deepti dohare <de...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/
-----------------------------------------------------------
(Updated Aug. 16, 2012, 10:46 a.m.)
Review request for cloudstack.
Changes
-------
Based on review comments, changes made to check and increment resource count whenever a snapshot is created. Verified locally for clustered management servers
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 (updated)
-----
api/src/com/cloud/user/ResourceLimitService.java 98dfc11
server/src/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b285d2c
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 6e3f9c1
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
Re: Review Request: CS-15430 Create snapshot should fail if creating
snapshot results in exceeding snapshot resource limit for domain-admin or
user accounts.
Posted by deepti dohare <de...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5806/
-----------------------------------------------------------
(Updated July 10, 2012, 1:32 p.m.)
Review request for cloudstack.
Changes
-------
Updated the createSnapshot method to decrement the resource count at one place (in the finally code block) for all errors.
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 (updated)
-----
server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java 50dcf38
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