fix: clamp ResourcePool available at zero and stop corrupting Hard#2024
Open
AruneshDwivedi wants to merge 4 commits into
Open
fix: clamp ResourcePool available at zero and stop corrupting Hard#2024AruneshDwivedi wants to merge 4 commits into
AruneshDwivedi wants to merge 4 commits into
Conversation
CalculateAvailableResources subtracted the claimed amount from the live Hard map entry (qt.Sub mutates the range value in place), so an over-subscribed pool wrote a negative value into both Status.Allocation.Available and Status.Allocation.Hard. Clamp available at zero and compute into a copy so Hard stays intact. Fixes projectcapsule#1977. Signed-off-by: Arunesh Dwivedi <arunesh.dwivedi@example.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes ResourcePool available-capacity calculation so it can’t go negative and avoids mutating/corrupting the underlying Hard quantities when subtracting claimed resources (addressing oversubscription behavior reported in #1977).
Changes:
- Compute “available” from a copied quantity instead of mutating the
Hardquantity during subtraction. - Clamp computed availability at zero to prevent negative capacity reporting.
- Add a regression test covering oversubscription and ensuring
Hardstays intact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/v1beta2/resourcepool_func.go | Avoids mutating underlying quota quantities during available computation and clamps available at zero. |
| api/v1beta2/resourcepool_func_test.go | Adds regression coverage for oversubscription to prevent negative available and ensure Hard remains unchanged. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
166
to
+168
| amount, exists := r.Status.Allocation.Claimed[res] | ||
| if exists { | ||
| qt.Sub(amount) | ||
| remaining.Sub(amount) |
Comment on lines
+171
to
175
| // A pool can never offer a negative amount of resources, so clamp | ||
| // at zero. This also keeps Hard intact for downstream math. | ||
| if remaining.Sign() < 0 { | ||
| remaining = resource.MustParse("0") | ||
| } |
…rces The follow-up edit removed the 'remaining := qt.DeepCopy()' line, leaving 'remaining' undefined and breaking the build. Re-add the declaration so the available-resources computation compiles and Hard is not mutated in place. Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
Signed-off-by: AruneshDwivedi <arunesh.dwivedi@users.noreply.github.com>
Author
|
Added the same zero clamp to GetAvailableClaimableResources so an over-claimed pool no longer surfaces negative available quantities in PoolExhausted messages, with a regression test covering the over-subscription case. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
CalculateAvailableResourcessubtracted the claimed amount from the liveHardmap entry (qt.Sub(amount)mutates the range value in place). When claims exceeded the hard limit this wrote a negative value into bothStatus.Allocation.AvailableandStatus.Allocation.Hard, so the pool reported negative capacity andHarditself got corrupted on every reconcile.Fix
DeepCopy()of the hard quantity, leavingHardintact.Why this is the right place
With available clamped at zero,
CanClaimFromPoolcorrectly rejects a new oversized claim (available.Cmp(req) < 0), andHardstays correct for downstream math. The webhook re-validates a patchedResourcePoolClaimagainst the pool, so an over-subscribed patch now surfaces aPoolExhaustedcondition instead of silently leaving a negative pool.Verification
Added
TestCalculateResources_OverSubscriptionDoesNotGoNegativeinapi/v1beta2/resourcepool_func_test.go.go test ./api/v1beta2/passes, including the existingTestCalculateResources.Fixes #1977