You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/11/15 16:05:39 UTC
[GitHub] [cloudstack] ravening opened a new pull request #5687: db: Avoid db locks if account resource limit is -1
ravening opened a new pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687
### Description
If account resource limit is -1 then there is no need to lock the
account along with its domain entries to check for resource count
as it wont matter anyway.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754107334
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening , I think @weizhouapache is right; If you have unlimited resources that would be still limited to the hierarchy above you, mainly your domain/project or what ever you are in. There is also a posibility that it would be limited by any domain above it, and in the end by the capacity of the cloud of course. A domain limit would otherwise not be useful at all.
say Dom1 has a limit of 10 and has two subdomains Dom2 and Dom3 which each have a limit of 6. Now if Dom2 uses all 6, Dom3 has an effective limit of 4 until Dom2 releases some. I think the same should be true for unlimited.
I thought `findCorrectResourceLimitForAccount()` already took care of that, but it seems it doesn't. I am also sure we can find more bugs in this respect somewhere ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754089656
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
I think I do not need to explain you again. You know what I mean.
Assume the domain resource limit is 20, account resource limit is 80. what is the max resource the account can use ? it is 20, not 80. -1 just means the account can use all resources of the domain.
there is no special account in a domain, who can use real-unlimited resource (ignore the domain resource limit) and domain resource count does not take the account resource count into calculation. It is very strange. This change is unacceptable in my opinion.
If you want to have such account, add it to `ROOT` domain or update the domain resource limit to -1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754120015
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
no it doesn't @ravening
say Dom1 has a limit of 10
acc1 has a limit of 4
acc2 has a limit of -1
now acc1 can get 4 resources unless acc2 takes 8, but still together they can't take more than 10.
in this case the -1 is a convenience of not having to copy the domain limit, but also when changing the domain limit, you don't have to adjust acc2 for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753165644
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache
lets say a domain has limit of 100 cpu count and lets the account under this domain has cpu limit as -1.
Technically we can create as many vm's in this account without any cpu limit restrictions as the limit for this account is -1. So I can create 100 vm's each with 10 cpu cores thus making a total of 1000 cpu count . but if i increase the domain resource limit then i will get error as soon as 100 is reached.
So in that case whats the use of having -1 as resource limit for account?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753149008
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
> @weizhouapache if the account resource limit is -1 then the resource count are not incremented for this account and so the resource count wont be incremented for domain as well. so why do we need to do extra check?
>
@ravening
Even if the account can use unlimited resource, the domain resource limit is still effective.
why not ignore the domain resource check and not updated the resource count (of account and domain) ?
> if domain has two accounts: A with limit as -1 and B with limit as 100, then account A can use unlimited resources. so the used account resource value will not be added to domain resource count
why not ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-976414589
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753336315
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache yes there are two isolation levels but can you answer the below question?
Domain has three account A, B and C
Domain resource limit is 100
A : resource limit is -1 and current resource count is 200
B: resource limit is 50 and current resource count is 30
C : resource limit is 50 and current resource count is 40
So you are saying that the total domain count should be 270 (200 + 40 + 30) and not 70 (30 + 40) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-972640589
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753344656
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache
Also technically speaking, I dont think there will be a case where account resource limit is -1 and domain resource limit is not -1.
if thats the case then the below check will always return false
https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java#L439
So even though the account resource limit is -1 then it cant exceed domain resource limit, thus making "-1" value meaningless
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753287353
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
you know there are two level resource limitation: domain and account
if account limit is -1, then account level limit is skipped
domain resource count is the total resource of all accounts in the domain, including the accounts which have unlimit resource (to be clear, resource limit is -1).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754118765
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
you'd better describe the problem you are facing. we can work it out together.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754116626
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@DaanHoogland if thats the case then whats the use of using `-1` as value for resource limit? Also whats the use of having the check like `accountResourceLimit == -1 then skip something` ?
Having -1 on account resource limit and limiting it on the domain level, defeats the whole purpose of `-1` right?
If that is the case then -1 shouldnt be allowed at all on account level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753400695
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
you may have noticed that there are two checks.
https://github.com/apache/cloudstack/blob/4.16.0.0/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java#L529-L532
```
// Check account limits
checkAccountResourceLimit(account, projectFinal, type, numResources);
// check all domains in the account's domain hierarchy
checkDomainResourceLimit(account, projectFinal, type, numResources);
```
these ensure that the resouce will not exceed the account and domain limitation..
the 1st check will always pass if account resource limit is -1, but 2nd check will not be skipped.
It makes sense to me.
You may need to discuss your colleagues if they expect an account with resource limit =-1 in a domain can exceed the domain resource limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753120231
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache if the account resource limit is -1 then the resource count are not incremented for this account and so the resource count wont be incremented for domain as well. so why do we need to do extra check?
if domain has two accounts: A with limit as -1 and B with limit as 100, then account A can use unlimited resources. so the used account resource value will not be added to domain resource count
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753149008
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
> @weizhouapache if the account resource limit is -1 then the resource count are not incremented for this account and so the resource count wont be incremented for domain as well. so why do we need to do extra check?
>
@ravening
Even if the account can use unlimited resource, the domain resource limit is still effective.
why ignore the domain resource check and not update the resource count (of account and domain) ?
> if domain has two accounts: A with limit as -1 and B with limit as 100, then account A can use unlimited resources. so the used account resource value will not be added to domain resource count
why not ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r756681894
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@DaanHoogland @weizhouapache im closing this pr as this wont give much improvement in vm deployment time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753149008
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
> @weizhouapache if the account resource limit is -1 then the resource count are not incremented for this account and so the resource count wont be incremented for domain as well. so why do we need to do extra check?
>
@ravening
Even if the account can use unlimited resource, the domain resource limit is still effective.
why ignore the domain resource check and not updated the resource count (of account and domain) ?
> if domain has two accounts: A with limit as -1 and B with limit as 100, then account A can use unlimited resources. so the used account resource value will not be added to domain resource count
why not ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r752003502
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
Review comment:
This comment says no more than the code below, we might as well drop it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r752012111
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753174914
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
-1 means the accounts can use resource as much as they can, but the domain resource can not exceed the domain's limitataion.
when check account resource limit, the domain resource limit is also checked.
when update account resource count, the domain resource count is updated as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-972836789
Packaging result: :heavy_check_mark: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_check_mark: suse15. SL-JID 1727
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754045354
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache if you logically think, if account resource limit is -1 then there is no point in having domain resource limit as not -1.
Our team is fine with this. this should be same for everyone i guess.
My point is, if the domain resource limit is not -1 then there is no point in having account resource limit as -1 since the second check you mentioned above will always fail as soon as the limit exceeds
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753379488
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
> @weizhouapache yes there are two isolation levels but can you answer the below question?
>
> Domain has three account A, B and C Domain resource limit is 100
>
> A : resource limit is -1 and current resource count is 200 B: resource limit is 50 and current resource count is 30 C : resource limit is 50 and current resource count is 40
>
> So you are saying that the total domain count should be 270 (200 + 40 + 30) and not 70 (30 + 40) ?
@ravening yes, the total resource count of domain is 270.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r752059171
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@ravening
if account resource limit is -1, it does not mean the account can use resources without limitation. the domain resource limit should be checked as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r754154022
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@DaanHoogland @weizhouapache now i got your point. i will make the code change to check for -1 on both account and domain level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-976500152
Packaging result: :heavy_multiplication_x: el7 :heavy_multiplication_x: el8 :heavy_multiplication_x: debian :heavy_multiplication_x: suse15. SL-JID 1769
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening closed pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening closed pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] ravening commented on a change in pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#discussion_r753218799
##########
File path: server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
##########
@@ -521,13 +520,21 @@ public void checkResourceLimit(final Account account, final ResourceType type, l
}
final Project projectFinal = project;
+
+ // Check account limits. If it's unlimited then don't lock the db rows
+ long accountResourceLimit = findCorrectResourceLimitForAccount(account, type);
+ if (Resource.RESOURCE_UNLIMITED == accountResourceLimit) {
Review comment:
@weizhouapache but whats the point in updating the account resource when account limit is -1? as you said above, -1 means unlimited resources. then updating the domain resource limit doesnt make any sense.
if we update the domain resource count then -1 no longer has its meaning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-972640418
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5687: db: Avoid db locks if account resource limit is -1
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5687:
URL: https://github.com/apache/cloudstack/pull/5687#issuecomment-976414963
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org