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
Clarify cpu.cfs_period_us default value #111554
Clarify cpu.cfs_period_us default value #111554
Conversation
cpu.cfs_period_us is 100μs by default despite having an "ms" unit for some unfortunate reason. Documentation: https://www.kernel.org/doc/html/latest/scheduler/sched-bwc.html#management The desired effect of that change is more clarity on the default value so users would be aware that the 10ms custom value would be not 0.1x of the default, but 100x of it.
Hi @paskal. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
cc @bobbypage @liggitt @szuecs for review. |
/ok-to-test |
cpu.cfs_period_us is 100μs by default despite having an "ms" unit for some unfortunate reason. Documentation: https://www.kernel.org/doc/html/latest/scheduler/sched-bwc.html#management The desired effect of that change is to match k8s default `CPUCFSQuotaPeriod` value (100ms before that change) with one used in k8s without the `CustomCPUCFSQuotaPeriod` flag enabled and Linux CFS (100us, 1000x smaller than 100ms).
Feel free to review now, tests are passing. |
/lgtm |
Gentle ping for approval: @Random-Liu @dchen1107 @derekwaynecarr @yujuhong @sjenning @mrunalp @klueska. (I hope it's the right way of doing that, I took the list from the approvers bot message as I don't know who in particular is the right person to ping. Apologies if it's not.) |
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.
/lgtm
/triage accepted
/priority important-soon
/lgtm |
/assign @mrunalp |
Pinging @lavalamp @smarterclayton @thockin @liggitt for review: default value under Alpha flag already changed in #111520, and this PR is only clarifying documentation and should be harmless to merge. It's quite important to have these two merged around the same time, I originally thought that this code comments change would be merged faster than default changing counterpart. |
/approve |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: endocrimes, klueska, liggitt, paskal The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I was wrong, and the default value is 100ms and not 100μs: PR #112123 reverts this PR and clarifies the code to prevent mistakes about units of measurement around this feature in the future. I'll try clarifying linked kernel.org documentation as well. Apologies for added confusion; I got the clarification from the kernel developer about the real state of things (#112108 (comment)) only after this was merged. |
No worries, thanka for clarifying it |
cpu.cfs_period_us is 100μs by default despite having an "ms" unit for some unfortunate reason. Documentation: https://www.kernel.org/doc/html/latest/scheduler/sched-bwc.html#management
The desired effect of that change is more clarity on the default value so users would be aware that the 10ms custom value would be not 0.1x of the default but 100x of it.
Followup of the #63437, which introduced the ability to customise the value but did not do the job of clarifying the default well enough. Documentation-only counterpart of #111520.
/kind documentation
Does this PR introduce a user-facing change? Nope.