feat: add eviction plugin config - #203
Conversation
3005843 to
61a746d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds NIC bandwidth eviction and PID overuse eviction configuration to the AdminQoSConfiguration API, and introduces exported pod sale-mode annotation constants with a test. ChangesNIC Bandwidth and PID Overuse Eviction Configuration
Pod Sale Mode Constants
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
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