Change CPUCFSQuotaPeriod default value from 100ms to 100us to match Linux default

[paskal]

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 when CPUCFSQuotaPeriod is not set, and Linux CFS (100μs, 1000x smaller than 100ms).

This PR is a followup of the #63437, which introduced the default of 100ms in k8s v1.12.

/kind api-change

Does this PR introduce a user-facing change? Yes, described above.

[kubelet] Change default `cpuCFSQuotaPeriod` value with enabled `cpuCFSQuotaPeriod` flag from 100ms to 100µs to match the Linux CFS and k8s defaults. `cpuCFSQuotaPeriod` of 100ms now requires `customCPUCFSQuotaPeriod` flag to be set to work.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: paskal / name: Dmitry Verkhoturov (c74977235aabe8ce87587c1eb20890190a7357ba)

[k8s-ci-robot]

Welcome @paskal!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

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.

[szuecs]

/retest

[paskal]

Unit tests failed due to yaml file having us instead of µs, I fixed it now. I don't know why e2e tests are failing and how to fix them. Reverting yaml files to 100ms produce same failure in e2e tests, so they are not relevant.

[bobbypage]

Unit tests failed due to yaml file having us instead of µs, I fixed it now. I don't know why e2e tests are failing and how to fix them. Reverting yaml files to 100ms produce same failure in e2e tests, so they are not relevant.

E2E tests need to be passing to get the PR merged.

Please see here - https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/111520/pull-kubernetes-e2e-gce-ubuntu-containerd/1554830092503355392/artifacts/e2e-e2a60cb2ac-a7d53-minion-group-05r1/kubelet.log

Kubelet is failing to start because:

Aug 03 14:35:19.780469 e2e-e2a60cb2ac-a7d53-minion-group-05r1 kubelet[4188]: E0803 14:35:19.772966    4188 run.go:74] "command failed" err="failed to validate kubelet configuration, error: invalid configuration: cpuCFSQuotaPeriod (--cpu-cfs-quota-period) {100µs} requires feature gate CustomCPUCFSQuotaPeriod, path: &TypeMeta{Kind:,APIVersion:,}"

[paskal]

Thanks a lot for pointer to the logs location! I'll fix it, but I'm struggling to understand of why my changes triggered that problem, as I only changed existing value and haven't introduced configuration overrides anywhere in the code.

[paskal]

I'm clueless about how to properly fix the error which is highlighted by CI now: for some reason thousands of lines supposed to be added to api/openapi-spec/swagger.json and others like api/openapi-spec/v3/apis__autoscaling__v2beta2_openapi.json. For me as a naive contributor it looks like I've triggered generation for whole new version of APIs. Can someone please tell me what is the right course of actions here?

[liggitt]

It was an unrelated failure now resolved in master

/retest

[paskal]

Thanks!

Reviewers, please review the code and the changelog entry part in the PR description: the PR changed the scope since the original introduction and requires a proper review before merging. #111554 (code-comments-only change) should be reviewed as well.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, paskal, szuecs

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:

• pkg/generated/openapi/OWNERS [liggitt]
• pkg/kubelet/OWNERS [liggitt]
• pkg/kubelet/apis/config/OWNERS [liggitt]
• staging/src/k8s.io/kubelet/config/OWNERS [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[paskal]

@bobbypage, could you please tell me if something is still missing here and in #111554, which prevents merging them into master? I'm not familiar with k8s merge flow enough to know what's missing at this moment here, and in another PR, approval is missing, but I already pinged all approvers once, and nothing happened.

[PixelOrange]

@paskal It looks to me like there are three people still pending approval on this one:

bobbypage
liggitt
mrunalp

It also says tide is pending and isn't mergeable, although I am not familiar with tide. I hope they see it soon, I've been anxiously awaiting this change since you first opened it.

[endocrimes]

/lgtm

it'll merge when we thaw the codebase after the release of 1.25.

[dims]

/hold cancel

[bobbypage]

/lgtm

[paskal]

For history, I was wrong, and the default value is 100ms and not 100μs: PR #112077 reverted these breaking changes, and PR #112123 clarified 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.

[PixelOrange]

@paskal I read through your discussion in #112108 and while I now also follow, I believe that the setting should not include a unit if the default value is a conflicting unit. If the value can never be less than 1ms then there's never a reason for microseconds to come into play. I think the "_us" should be removed from the setting and the current description stating the range of valid values be left unchanged.

Unfortunately, I don't know how to go about requesting that change. I'm coming at this from the same perspective as many other people who've stumbled on these conversations due to poorly performing applications under CFS. What do you suggest?

[paskal]

@PixelOrange, I would propose you check changes in #112123 and verify if something is missing there from your perspective. The valid range of values is 1ms-1s, and the k8s source code will be updated to reflect it.

_us postfix comes into play only in the Linux kernel and is part of the public API, meaning that logic will never change. The only thing which could be changed is documentation around the topic, and I'll try to come up with that change.

[PixelOrange]

Everything looks good as far as I can tell.

That's unfortunate about the public API. I found an article on how to submit kernel patches but I imagine that's a daunting task.

If you want a second set of eyes for the documentation updates, please feel free to message me.

[szuecs]

Thanks @paskal for going the long way.