Feat(numa cpu pressure eviction config) add upper and lower bound - #209
Conversation
|
|
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe NUMA CPU pressure eviction configuration adds optional soft and hard threshold ratios, validates them between 0 and 1 in the API and CRD, and updates generated deepcopy logic to preserve both values. ChangesNUMA CPU pressure eviction ratios
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/apis/config/v1alpha1/adminqos.go`:
- Around line 536-552: Add cross-field validation enforcing each lower threshold
to be less than or equal to its corresponding upper threshold in the AdminQOS
configuration, including the ratio fields and all load/usage upper/lower pairs
in AdminQOSConfiguration. Update the API validation markers and regenerate
config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml so
admission rejects invalid threshold ordering; the CRD site requires only the
generated validation output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b379d464-67e3-4487-889a-10a30d4ee36e
📒 Files selected for processing (3)
config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlpkg/apis/config/v1alpha1/adminqos.gopkg/apis/config/v1alpha1/zz_generated.deepcopy.go
| // UpperBoundRatio is the hard threshold ratio (0~1) applied to the NUMA CPU | ||
| // usage ratio. When enough recent samples cross this bound, the plugin reports | ||
| // HARD_MET together with a NodeCondition + NoSchedule taint, which allows the | ||
| // eviction manager to actually evict candidate pods. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Maximum=1 | ||
| // +optional | ||
| UpperBoundRatio *float64 `json:"upperBoundRatio,omitempty"` | ||
|
|
||
| // LowerBoundRatio is the soft threshold ratio (0~1) applied to the NUMA CPU | ||
| // usage ratio. When enough recent samples cross this bound but the upper bound | ||
| // is not yet reached, the plugin reports SOFT_MET together with a NodeCondition and | ||
| // NoSchedule taint, which only prevents new scheduling without evicting pods. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Maximum=1 | ||
| // +optional | ||
| LowerBoundRatio *float64 `json:"lowerBoundRatio,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant symbols =="
rg -n "UpperBoundRatio|LowerBoundRatio|validation:.*(UpperBoundRatio|LowerBoundRatio)|CEL|XValidation|cross-field|upper.*lower|lower.*upper" \
pkg/apis/config/v1alpha1 config/crd/bases -S
echo
echo "== file size =="
wc -l pkg/apis/config/v1alpha1/adminqos.go config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml
echo
echo "== ast-grep outline for adminqos.go =="
ast-grep outline pkg/apis/config/v1alpha1/adminqos.go --view expanded | sed -n '1,220p'
echo
echo "== relevant CRD excerpt around lower/upper =="
sed -n '240,330p' config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlRepository: kubewharf/katalyst-api
Length of output: 27476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search for cross-field validation annotations =="
rg -n "XValidation|x-kubernetes-validations|kubebuilder:validation:.*(XValidation|oneOf|anyOf)|rule:" pkg/apis/config/v1alpha1 config/crd/bases -S
echo
echo "== inspect load and usage bound pairs in adminqos.go =="
sed -n '424,450p' pkg/apis/config/v1alpha1/adminqos.go
echo
sed -n '807,845p' pkg/apis/config/v1alpha1/adminqos.go
echo
echo "== inspect corresponding CRD excerpts =="
sed -n '188,230p' config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml
echo
sed -n '378,430p' config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlRepository: kubewharf/katalyst-api
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cross-field validation annotations (if any) =="
rg -n "XValidation|x-kubernetes-validations|kubebuilder:validation:.*(oneOf|anyOf|allOf)|rule:" \
pkg/apis/config/v1alpha1 config/crd/bases -S || true
echo
echo "== NUMA and system CPU pressure snippets =="
sed -n '498,555p' pkg/apis/config/v1alpha1/adminqos.go
echo
sed -n '807,845p' pkg/apis/config/v1alpha1/adminqos.go
echo
echo "== generated CRD snippets =="
sed -n '250,310p' config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml
echo
sed -n '404,435p' config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlRepository: kubewharf/katalyst-api
Length of output: 10468
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== usages of upper/lower bound ratios in repo =="
rg -n "UpperBoundRatio|LowerBoundRatio|loadUpperBoundRatio|loadLowerBoundRatio|usageUpperBoundRatio|usageLowerBoundRatio" -S .
echo
echo "== inspect likely implementation file(s) if present =="
fd -a -e go . | rg "evict|pressure|qos|adminqos|numa|load|usage" | head -n 50Repository: kubewharf/katalyst-api
Length of output: 8094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read the utility helpers =="
sed -n '1,260p' pkg/utils/adminqos.go
echo
echo "== check for ratio-related logic in eviction helpers =="
sed -n '1,260p' pkg/utils/eviction.go
echo
echo "== any cross-field checks in API package =="
rg -n ">=|<=|lower.*upper|upper.*lower|compare|validate|normalize" pkg/apis/config/v1alpha1 pkg/utils -SRepository: kubewharf/katalyst-api
Length of output: 3244
Enforce lower ≤ upper for these ratio fields.
LowerBoundRatio can still exceed UpperBoundRatio here, and the same gap exists for the load/usage upper/lower pairs in pkg/apis/config/v1alpha1/adminqos.go. Add cross-field validation and regenerate config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml so invalid threshold order is rejected at admission.
📍 Affects 2 files
pkg/apis/config/v1alpha1/adminqos.go#L536-L552(this comment)config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml#L262-L270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/apis/config/v1alpha1/adminqos.go` around lines 536 - 552, Add cross-field
validation enforcing each lower threshold to be less than or equal to its
corresponding upper threshold in the AdminQOS configuration, including the ratio
fields and all load/usage upper/lower pairs in AdminQOSConfiguration. Update the
API validation markers and regenerate
config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml so
admission rejects invalid threshold ordering; the CRD site requires only the
generated validation output.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/apis/config/v1alpha1/zz_generated.deepcopy.go (1)
2226-2235: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the duplicate ratio deepcopy blocks.
UpperBoundRatioandLowerBoundRatioare already deep-copied at Lines 2216-2225. These blocks allocate and copy both pointers again, then overwrite the first allocations. Regenerate this file and keep one deepcopy block per field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/config/v1alpha1/zz_generated.deepcopy.go` around lines 2226 - 2235, Remove the duplicate UpperBoundRatio and LowerBoundRatio deepcopy blocks from the generated deepcopy method, retaining exactly one allocation-and-copy block for each field. Regenerate the file so the method does not repeat or overwrite those pointer copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/apis/config/v1alpha1/zz_generated.deepcopy.go`:
- Around line 2226-2235: Remove the duplicate UpperBoundRatio and
LowerBoundRatio deepcopy blocks from the generated deepcopy method, retaining
exactly one allocation-and-copy block for each field. Regenerate the file so the
method does not repeat or overwrite those pointer copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a219f69-419a-4b40-9407-1ee3a45a242c
📒 Files selected for processing (3)
config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlpkg/apis/config/v1alpha1/adminqos.gopkg/apis/config/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (2)
- config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yaml
- pkg/apis/config/v1alpha1/adminqos.go
4993224 to
c9c3bb2
Compare
fix: generate crd and fix deepcopy function
97b1a5b to
74ef6f1
Compare
3 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
change for katalyst qrm plugin numa_cpu_pressure_eviction update
updated plugin will evict workload by bound instead of prior threshold
Special notes for your reviewer:
Summary by CodeRabbit
lowerBoundRatio(soft) andupperBoundRatio(hard).