-
Notifications
You must be signed in to change notification settings - Fork 331
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
【WIP】koordlet: perform operations only under the specified cpuburst strategy and improve config logging #2159
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
=======================================
Coverage 67.30% 67.31%
=======================================
Files 453 453
Lines 43707 43709 +2
=======================================
+ Hits 29419 29423 +4
+ Misses 11742 11741 -1
+ Partials 2546 2545 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b.applyCFSQuotaBurst(cpuBurstCfg, podMeta, nodeState) | ||
klog.V(5).Infof("get pod %v/%v cpu burst config: %#v", podMeta.Pod.Namespace, podMeta.Pod.Name, cpuBurstCfg) | ||
|
||
if cpuBurstEnabled(cpuBurstCfg.Policy) { |
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.
please note that the strategy should support reseting the related cgroups when the policy disables
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.
please note that the strategy should support reseting the related cgroups when the policy disables
When both CPUNormalization and CPUBurst are enabled, CFS will be repeatedly changed by the plugins.
How about adding a flag variable, when CPUBurstPolicy
changed,call b.applyCPUBurst(cpuBurstCfg, podMeta)
and b.applyCFSQuotaBurst(cpuBurstCfg, podMeta, nodeState)
as before. @saintube
func (b *cpuBurst) start(){
...
var flag bool
if nodeSLO.Spec.CPUBurstStrategy changed {
flag = true
}
...
if cpuBurstEnabled(cpuBurstCfg.Policy) || flag {
// set cpu.cfs_burst_us for pod and containers
b.applyCPUBurst(cpuBurstCfg, podMeta)
}
if cfsQuotaBurstEnabled(cpuBurstCfg.Policy) || flag {
// scale cpu.cfs_quota_us for pod and containers
b.applyCFSQuotaBurst(cpuBurstCfg, podMeta, nodeState)
}
}
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.
Maybe we can add a new policy "ignored"
to describe the behavior that stops the cgroups reconciliation, so the existing policies are still backward-compatible.
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.
Maybe we can add a new policy
"ignored"
to describe the behavior that stops the cgroups reconciliation, so the existing policies are still backward-compatible.
But someone may still want to use cpuBurstOnly
, applyCPUBurst()
is also needed. @saintube
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.
Yes. The user may switch the policy from auto
to cpuBurstOnly
. And he/she should reset the cfs_quota.
Adding a separate attribute is a good idea not to break the backward compatibility of the current policies. Possibly, we can define a new field in the CPUBurstStrategy to adjust the configurations dynamically.
…gy and improve config logging Signed-off-by: yangfeiyu20102011 <yangfeiyu20102011@163.com>
ec3d84d
to
2160c5f
Compare
Under the specified cpuBurstOnly strategy, the plugin cpuburst still periodically updates the CFS values, which is not expected. Determine the corresponding cpuburst strategy at the beginning and then perform the appropriate actions.
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews
V. Checklist
make test