feat: add eviction plugin config#203
Conversation
📝 WalkthroughWalkthroughAdds NIC bandwidth eviction and PID overuse eviction configuration to the AdminQoSConfiguration API (Go types, CRD schema, and deepcopy generation), and introduces new exported pod sale-mode annotation constants with an accompanying test. ChangesNIC Bandwidth and PID Overuse Eviction Configuration
Pod Sale Mode Constants
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)Not applicable — changes are configuration schema/type additions and constant definitions without multi-component control flow. Suggested labels: api-change, config, needs-generated-code-review Suggested reviewers: kubewharf/katalyst-api maintainers familiar with eviction configuration and CRD generation 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
pkg/consts/pod.go (1)
45-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment only covers one of five exported constants.
The comment above the block describes only
PodAnnotationSaleModeKey, but the block also exports fourPodSaleMode*value constants without their own documentation.✏️ Suggested doc comment tweak
-// PodAnnotationSaleModeKey is a const variable for pod annotation about pod sale mode. +// PodAnnotationSaleModeKey is the pod annotation key for pod sale mode, and +// PodSaleMode* are the supported sale mode values. const ( PodAnnotationSaleModeKey = "katalyst.kubewharf.io/sale_mode"🤖 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/consts/pod.go` around lines 45 - 53, The const block in PodAnnotationSaleModeKey also exports PodSaleModeSpot, PodSaleModeScheduled, PodSaleModeReserved, and PodSaleModeDefault, but only the first constant is documented. Update the documentation near the const group in pod.go so each exported symbol is covered, either by adding a block comment that describes the sale mode values or by splitting the constants into separately documented declarations.
🤖 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 849-857: The AdminQoS validation for NIC bandwidth ring settings
only enforces independent minimums, so `NICBandwidthRingMetThreshold` can exceed
`NICBandwidthRingSize` and still pass. Update the validation for the `AdminQoS`
API fields in `adminqos.go` to compare these two knobs and reject any spec where
`nicBandwidthRingMetThreshold` is greater than `nicBandwidthRingSize`, using the
existing `NICBandwidthRingSize` and `NICBandwidthRingMetThreshold` symbols as
the anchor for the check.
- Around line 883-886: Update the field comment on GracePeriod in adminqos.go so
the generated CRD description matches PID overuse eviction instead of nic health
eviction. Keep the kubebuilder validation/tag and the GracePeriod symbol
unchanged, and only revise the doc comment text above GracePeriod *int64 to
describe the correct eviction behavior so kubectl explain reflects the right
knob.
---
Nitpick comments:
In `@pkg/consts/pod.go`:
- Around line 45-53: The const block in PodAnnotationSaleModeKey also exports
PodSaleModeSpot, PodSaleModeScheduled, PodSaleModeReserved, and
PodSaleModeDefault, but only the first constant is documented. Update the
documentation near the const group in pod.go so each exported symbol is covered,
either by adding a block comment that describes the sale mode values or by
splitting the constants into separately documented declarations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4520941d-bfdb-4be4-8290-033bf1b78aa3
📒 Files selected for processing (5)
config/crd/bases/config.katalyst.kubewharf.io_adminqosconfigurations.yamlpkg/apis/config/v1alpha1/adminqos.gopkg/apis/config/v1alpha1/zz_generated.deepcopy.gopkg/consts/pod.gopkg/consts/pod_test.go
| // NICBandwidthRingSize is the size of the bandwidth pressure ring buffer. | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +optional | ||
| NICBandwidthRingSize *int64 `json:"nicBandwidthRingSize,omitempty"` | ||
|
|
||
| // NICBandwidthRingMetThreshold is the number of ring hits required for eviction. | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +optional | ||
| NICBandwidthRingMetThreshold *int64 `json:"nicBandwidthRingMetThreshold,omitempty"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject nicBandwidthRingMetThreshold values larger than nicBandwidthRingSize.
These knobs are only validated independently right now, so a config like ringSize=3 and ringMetThreshold=5 passes schema even though the ring-hit condition can never be satisfied. Please enforce nicBandwidthRingMetThreshold <= nicBandwidthRingSize in API/admission validation.
🤖 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 849 - 857, The AdminQoS
validation for NIC bandwidth ring settings only enforces independent minimums,
so `NICBandwidthRingMetThreshold` can exceed `NICBandwidthRingSize` and still
pass. Update the validation for the `AdminQoS` API fields in `adminqos.go` to
compare these two knobs and reject any spec where `nicBandwidthRingMetThreshold`
is greater than `nicBandwidthRingSize`, using the existing
`NICBandwidthRingSize` and `NICBandwidthRingMetThreshold` symbols as the anchor
for the check.
| // GracePeriod is the grace period of nic health eviction | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +optional | ||
| GracePeriod *int64 `json:"gracePeriod,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the GracePeriod description for PID overuse eviction.
The comment still says "nic health eviction", and that typo is already flowing into the generated CRD text, so kubectl explain will describe this new knob incorrectly.
✏️ Proposed fix
- // GracePeriod is the grace period of nic health eviction
+ // GracePeriod is the grace period of pid overuse eviction📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GracePeriod is the grace period of nic health eviction | |
| // +kubebuilder:validation:Minimum=0 | |
| // +optional | |
| GracePeriod *int64 `json:"gracePeriod,omitempty"` | |
| // GracePeriod is the grace period of pid overuse eviction | |
| // +kubebuilder:validation:Minimum=0 | |
| // +optional | |
| GracePeriod *int64 `json:"gracePeriod,omitempty"` |
🤖 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 883 - 886, Update the
field comment on GracePeriod in adminqos.go so the generated CRD description
matches PID overuse eviction instead of nic health eviction. Keep the
kubebuilder validation/tag and the GracePeriod symbol unchanged, and only revise
the doc comment text above GracePeriod *int64 to describe the correct eviction
behavior so kubectl explain reflects the right knob.
1 participant
What type of PR is this?
Features
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Summary by CodeRabbit
New Features
Tests