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

Jump to bottom

Clarify cpu.cfs_period_us default value #111554

Merged
merged 2 commits into from Aug 25, 2022
Merged

Clarify cpu.cfs_period_us default value #111554

merged 2 commits into from Aug 25, 2022

Conversation

paskal
Copy link
Contributor

@paskal paskal commented Jul 29, 2022 •
edited

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.

Clarified the default CFS quota period as being 100µs and not 100ms.

@paskal
clarify cpu.cfs_period_us default value
5126192 
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.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/documentation Categorizes issue or PR as related to documentation. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 29, 2022
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 29, 2022
@paskal paskal mentioned this pull request Jul 29, 2022
Merged
@paskal
Copy link
Contributor Author

paskal commented Aug 1, 2022

cc @bobbypage @liggitt @szuecs for review.

@szuecs
Copy link
Member

szuecs commented Aug 1, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 1, 2022
@paskal
change CPUCFSQuotaPeriod default value to 100us to match Linux default
32df800 
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).
@paskal
Copy link
Contributor Author

paskal commented Aug 2, 2022

Feel free to review now, tests are passing.

@pacoxu pacoxu added this to Triage in SIG Node PR Triage Aug 3, 2022
@szuecs
Copy link
Member

szuecs commented Aug 3, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2022
@paskal
Copy link
Contributor Author

paskal commented Aug 3, 2022

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

endocrimes
endocrimes approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@endocrimes endocrimes left a 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

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 23, 2022
@k8s-ci-robot k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 23, 2022
@endocrimes endocrimes moved this from Triage to Needs Approver in SIG Node PR Triage Aug 23, 2022
@bobbypage
Copy link
Member

/lgtm

@bobbypage
Copy link
Member

/assign @mrunalp

@paskal
Copy link
Contributor Author

paskal commented Aug 25, 2022

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.

@klueska
Copy link
Contributor

klueska commented Aug 25, 2022

/approve
/lgtm

@liggitt
Copy link
Member

liggitt commented Aug 25, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit 2b5475b into kubernetes:master Aug 25, 2022
SIG Node PR Triage automation moved this from Needs Approver to Done Aug 25, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Aug 25, 2022
@paskal paskal deleted the paskal/clarify_default_cfs_period branch August 25, 2022 14:31
liggitt added a commit to liggitt/kubernetes that referenced this pull request Aug 26, 2022
@liggitt
Revert "Merge pull request kubernetes#111554 from paskal/paskal/clari…
a0e03c7 
…fy_default_cfs_period"

This reverts commit 2b5475b, reversing
changes made to b87a436.
@paskal paskal mentioned this pull request Aug 30, 2022
Merged
@paskal
Copy link
Contributor Author

paskal commented Aug 31, 2022

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.

@aojea
Copy link
Member

aojea commented Aug 31, 2022

No worries, thanka for clarifying it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@endocrimes endocrimes endocrimes approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

Assignees

@szuecs szuecs

@mrunalp mrunalp

@klueska klueska

@bobbypage bobbypage

@endocrimes endocrimes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@paskal @k8s-ci-robot @szuecs @bobbypage @klueska @liggitt @aojea @endocrimes @mrunalp