-
Notifications
You must be signed in to change notification settings - Fork 1
allow optimizing guaranteed pods #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new configuration option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/task/taskApplyRecommendation.go`:
- Around line 318-321: The memory-clamping logic in applyMemoryRecommendation()
can lower a guaranteed container's request while its limit remains unchanged,
converting it to Burstable QoS on clusters that can't reduce limits; modify
applyMemoryRecommendation() to first detect guaranteed containers by checking if
OptimizeGuaranteedPods is true and currentMemoryLimit == currentMemoryRequest
(or request==limit) and skip any memory update (i.e., do not adjust
recommendedMemoryLimit/request) for those containers until limit-reduction
support exists, leaving the existing Math.Ceil clamp only for non-guaranteed
containers.
| // TODO: will be possible from 1.34 | ||
| if recommendedMemoryLimit < currentMemoryLimit { | ||
| recommendedMemoryLimit = math.Ceil(currentMemoryLimit) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
wc -l pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 105
🏁 Script executed:
sed -n '310,330p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 1001
🏁 Script executed:
sed -n '300,340p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 1764
🏁 Script executed:
sed -n '450,475p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 1129
🏁 Script executed:
sed -n '280,350p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 3206
🏁 Script executed:
grep -n "func.*Apply\|OptimizeGuaranteedPods\|IsGuaranteedPod" pkg/task/taskApplyRecommendation.go | head -20Repository: truefoundry/CruiseKube
Length of output: 1414
🏁 Script executed:
sed -n '1,50p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 1720
🏁 Script executed:
sed -n '153,210p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2607
🏁 Script executed:
grep -n "applyMemoryRecommendation\|applyCPURecommendation" pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 448
🏁 Script executed:
sed -n '195,250p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2209
🏁 Script executed:
sed -n '433,480p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2272
🏁 Script executed:
grep -n "IsGuaranteedPod" pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 161
🏁 Script executed:
rg "IsGuaranteedPod" --context 3Repository: truefoundry/CruiseKube
Length of output: 1075
🏁 Script executed:
rg "type PodInfo" pkg/task/utils/ -A 15Repository: truefoundry/CruiseKube
Length of output: 1331
🏁 Script executed:
sed -n '314,342p' pkg/task/utils/node_stats.goRepository: truefoundry/CruiseKube
Length of output: 48
🏁 Script executed:
grep -B 5 -A 10 "func.*IsGuaranteedPod" pkg/task/utils/node_stats.goRepository: truefoundry/CruiseKube
Length of output: 560
🏁 Script executed:
sed -n '286,342p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2557
🏁 Script executed:
sed -n '220,270p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2383
🏁 Script executed:
grep -n "OptimizeGuaranteedPods" pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 161
🏁 Script executed:
rg "OptimizeGuaranteedPods" --context 2Repository: truefoundry/CruiseKube
Length of output: 930
🏁 Script executed:
sed -n '354,420p' pkg/task/taskApplyRecommendation.goRepository: truefoundry/CruiseKube
Length of output: 2824
🏁 Script executed:
grep -B 5 -A 15 "func.*applyCPURecommendation" pkg/task/taskApplyRecommendation.go | head -40Repository: truefoundry/CruiseKube
Length of output: 783
Prevent QoS degradation for guaranteed containers when memory limits cannot be reduced.
When OptimizeGuaranteedPods is true, guaranteed containers (where request==limit) can reach applyMemoryRecommendation() and the memory clamping logic at lines 318-321 may allow the request to be lowered while the limit remains unchanged. This converts the container to Burstable QoS on clusters that don't yet support limit reduction. To ensure guaranteed containers stay protected, skip memory updates for containers where the current limit equals the current request until limit updates are supported.
Suggested guard
// TODO: will be possible from 1.34
if recommendedMemoryLimit < currentMemoryLimit {
+ if currentMemoryLimit == currentMemoryRequest {
+ logging.Infof(ctx, "Skipping memory recommendation for guaranteed pod %s/%s; limit cannot be decreased yet", rec.PodInfo.Namespace, rec.PodInfo.Name)
+ return false, true, nil
+ }
recommendedMemoryLimit = math.Ceil(currentMemoryLimit)
}🤖 Prompt for AI Agents
In `@pkg/task/taskApplyRecommendation.go` around lines 318 - 321, The
memory-clamping logic in applyMemoryRecommendation() can lower a guaranteed
container's request while its limit remains unchanged, converting it to
Burstable QoS on clusters that can't reduce limits; modify
applyMemoryRecommendation() to first detect guaranteed containers by checking if
OptimizeGuaranteedPods is true and currentMemoryLimit == currentMemoryRequest
(or request==limit) and skip any memory update (i.e., do not adjust
recommendedMemoryLimit/request) for those containers until limit-reduction
support exists, leaving the existing Math.Ceil clamp only for non-guaranteed
containers.
| currentMemoryLimitQuantity := currentContainerResources.Limits[corev1.ResourceMemory] | ||
| currentMemoryLimit := float64(currentMemoryLimitQuantity.Value()) / utils.BytesToMBDivisor | ||
|
|
||
| if currentMemoryRequest == currentMemoryLimit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to mark a pod that has either equal request and limit for memory or cpu for eviction like here -
| sort.Slice(podInfosClone, func(i, j int) bool { |
so that it can be evicted and recreated with corrected request and limit
Summary by CodeRabbit
New Features
optimizeGuaranteedPodsconfiguration option to control whether guaranteed pods are eligible for optimization.Improvements