Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yangfeiyu20102011
Copy link
Contributor

image
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.
image

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

…gy and improve config logging

Signed-off-by: yangfeiyu20102011 <[email protected]>
@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jasonliu747 after the PR has been reviewed.
You can assign the PR to them by writing /assign @jasonliu747 in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.90%. Comparing base (65bfe26) to head (1f76a85).
Report is 4 commits behind head on main.

Files Patch % Lines
.../koordlet/qosmanager/plugins/cpuburst/cpu_burst.go 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2159      +/-   ##
==========================================
+ Coverage   67.88%   67.90%   +0.01%     
==========================================
  Files         443      443              
  Lines       41921    41923       +2     
==========================================
+ Hits        28460    28466       +6     
+ Misses      11006    11004       -2     
+ Partials     2455     2453       -2     
Flag Coverage Δ
unittests 67.90% <71.42%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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) {
Copy link
Member

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

Copy link
Contributor Author

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)
}

}

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@saintube saintube Aug 8, 2024

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.

@yangfeiyu20102011 yangfeiyu20102011 changed the title koordlet: perform operations only under the specified cpuburst strategy and improve config logging 【WIP】koordlet: perform operations only under the specified cpuburst strategy and improve config logging Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants