server: add real-time memory validation for limit updates #9385
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sohankunkerkar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2b64053 to
e2db63a
Compare
e2db63a to
60c8932
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9385 +/- ##
==========================================
- Coverage 67.31% 67.28% -0.03%
==========================================
Files 201 202 +1
Lines 27720 27760 +40
==========================================
+ Hits 18660 18679 +19
- Misses 7515 7529 +14
- Partials 1545 1552 +7 🚀 New features to boost your workflow:
|
91632fb to
87fed4e
Compare
0a9a319 to
7f2d633
Compare
f16a01d to
5a37807
Compare
5a37807 to
ef34e5c
Compare
This change validates memory limits against current usage from cgroup files instead of stale cAdvisor cache. Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
ef34e5c to
a6e9311
Compare
|
/retest |
| currentUsage := int64(usageBytes.GetValue()) | ||
|
|
||
| // Check if new limit is below current usage. | ||
| if newMemoryLimit < currentUsage { | ||
| return fmt.Errorf("cannot decrease memory limit to %d bytes: current usage is %d bytes", | ||
| newMemoryLimit, currentUsage) | ||
| } |
There was a problem hiding this comment.
How should we handle if currentUsage is 0 (= unlimited?)?
There was a problem hiding this comment.
IIRC, currentUsage = 0 doesn't mean unlimited, it means the container hasn't allocated any memory yet (or is using negligible memory). This is different from newMemoryLimit = 0 which means unlimited. When currentUsage = 0, it either means the container just started and hasn't allocated significant memory or the container is idle. I think we should allow setting any reasonable memory limit when the container isn't using memory yet.
There was a problem hiding this comment.
ah sorry I got the actual usage and the current limit mixed up. thanks for clarification.
|
/lgtm |
|
/cherry-pick release-1.33 |
|
@sohankunkerkar: new pull request created: #9404 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This change validates memory limits against current usage from cgroup files instead of stale cAdvisor cache.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?