feat(aqc): add max reclaimed resource for memory and cpu#205
feat(aqc): add max reclaimed resource for memory and cpu#205JustinChengLZ wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdmin QoS configuration adds optional CPU and memory reclaim ratios plus a consumer-to-resource percentage map to the API types and CRD schema. Generated deepcopy logic now copies the new pointer and map fields. ChangesAdmin QoS reclaim configuration fields
Estimated code review effort: 2 (Simple) | ~10 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 |
1039b1c to
10f1404
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/apis/config/v1alpha1/adminqos.go (2)
254-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment ordering differs from sibling fields.
Validation markers precede the description here, whereas other fields (e.g.
FreeBasedRatioat Line 284) place the description first. Functionally harmless since controller-gen still produced the correct CRD description, but worth aligning for consistency.🤖 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 254 - 259, The comment order for ReclaimedCPUMaxRatio is inconsistent with sibling fields like FreeBasedRatio in AdminQOSSpec. Reorder the kubebuilder validation markers and field description so the human-readable description comes first, followed by the validation tags and +optional, matching the style used by the other fields in this struct.
254-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueJSON tag drops the "Ratio" suffix present in the Go field name.
ReclaimedCPUMaxRatio/ReclaimedMemoryMaxRatiomap toreclaimedCPUMax/reclaimedMemoryMax, while sibling fields keep full name correspondence (e.g.FreeBasedRatio→freeBasedRatio,CacheBasedRatio→cacheBasedRatio). Consider aligning the JSON key naming convention for consistency, though this is a cosmetic API surface choice rather than a defect.Also applies to: 313-318
🤖 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 254 - 259, Align the JSON field names for the AdminQoS ratio fields so they consistently preserve the “Ratio” suffix like the other ratio properties. Update the ReclaimedCPUMaxRatio and ReclaimedMemoryMaxRatio struct tags in adminqos.go to use matching JSON keys, and verify the same naming convention is applied wherever these fields are defined or referenced, including the duplicate declaration noted by the review.
🤖 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/adminqos.go`:
- Around line 254-259: The comment order for ReclaimedCPUMaxRatio is
inconsistent with sibling fields like FreeBasedRatio in AdminQOSSpec. Reorder
the kubebuilder validation markers and field description so the human-readable
description comes first, followed by the validation tags and +optional, matching
the style used by the other fields in this struct.
- Around line 254-259: Align the JSON field names for the AdminQoS ratio fields
so they consistently preserve the “Ratio” suffix like the other ratio
properties. Update the ReclaimedCPUMaxRatio and ReclaimedMemoryMaxRatio struct
tags in adminqos.go to use matching JSON keys, and verify the same naming
convention is applied wherever these fields are defined or referenced, including
the duplicate declaration noted by the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe4f41c9-97f9-4611-a1cd-a35fad60141c
📒 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 due to trivial changes (1)
- pkg/apis/config/v1alpha1/zz_generated.deepcopy.go
10f1404 to
78a7ab0
Compare
78a7ab0 to
170135c
Compare
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 158-162: Add per-entry 0–100 validation to
ReclaimedConsumerToReclaimedResourcePercentage using the project’s established
map-value validation marker, then regenerate the CRD manifests so admission
rejects negative values and values above 100.
🪄 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: 25090732-f0aa-4eb9-a238-bc69b0cc4b03
📒 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/zz_generated.deepcopy.go
| // ReclaimedConsumerToReclaimedResourcePercentage is a configuration for reclaimed consumer to a percentage of the total | ||
| // reclaimed resource, used for both headroom and provision. | ||
| // For example, {"generic": 40, "custom-consumer-1": 60} | ||
| // +optional | ||
| ReclaimedConsumerToReclaimedResourcePercentage map[string]int `json:"reclaimedConsumerToReclaimedResourcePercentage,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Constrain consumer percentages to the valid range.
This field is documented as a percentage, but its values have no validation marker; the CRD currently accepts arbitrary integers, including negative values and values above 100. Add per-entry 0..100 validation and regenerate the CRD so invalid allocation percentages are rejected at admission.
🤖 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 158 - 162, Add per-entry
0–100 validation to ReclaimedConsumerToReclaimedResourcePercentage using the
project’s established map-value validation marker, then regenerate the CRD
manifests so admission rejects negative values and values above 100.
819e40d to
fa0e81f
Compare
1 participant
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Summary by CodeRabbit