-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Prevent scaling of cluster if count / resources exceed account resource limits #12167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.22
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #12167 +/- ##
============================================
- Coverage 17.56% 17.56% -0.01%
- Complexity 15545 15546 +1
============================================
Files 5910 5910
Lines 529123 529164 +41
Branches 64627 64636 +9
============================================
+ Hits 92937 92940 +3
- Misses 425733 425771 +38
Partials 10453 10453
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
@Pearl1594 IMHO, resource limitation could be one of the reasons what cause CKS scaling to fail |
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
| @Inject | ||
| ResourceLimitService resourceLimitService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sonar warning here is new to me. If we need to deal with this that is going to be a huge effort as this pattern is all over the project.
I don't quite understand what you mean @weizhouapache - what issues and what change in process are we talking about. Could you please give some clarity on that. Thanks. |
I was thinking other possible edge cases, for example, the 2nd vm is not deployed due to the capacity of the system/storage. |
weizhouapache
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
not tested yet


Description
This PR fixes: #12123
This pr prevents the scaling of the cluster by checking if the overall resources / vm count with the accounts resource limits to prevent cluster in an incorrect state.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?
Before FIx:
Set account's resource limit for VM count to 3

Deployed a CKS cluster with 1 worker node and then attempted to scale it to 3 and it fails
it scales by 1 node but increases the overall size of the cluster to 4 and puts the cluster in an incorrect state
After fix:
Performed the same test as above, here it prevents scaling altogether as it pre-emptively calculates if the overall resources exceeds what's set for the account.