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 CPUCFSQuotaPeriod values, set the minimum to 1ms #112123
clarify CPUCFSQuotaPeriod values, set the minimum to 1ms #112123
Conversation
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. |
Waiting for |
/ok-to-test Feel free to ping me when ready for me to look at. Took a brief look now, and things look good. The change of the validation is a breaking API change, but Imo. its very fine since users inserting lower values than |
/approve |
/assign @bobbypage |
Thanks, looks good now! /lgtm |
Thanks for the help clarifying our misinterpretation regarding the units. Couple suggestions to make it even more clear |
cpu.cfs_period_us is measured in microseconds in the kernel but provided in time.Duration by the user, that change clarifies the code to make this evident to the reader. Also, the minimum value for that feature is 1ms and not 1μs, and this change alters the validation to reject values smaller than 1ms.
/lgtm |
/approve |
[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:
Approvers can indicate their approval by writing |
cpu.cfs_period_us is measured in microseconds in the kernel but provided in time.Duration by the user, that change clarifies the code to make this evident to the reader.
Also, the minimum value for that feature is 1ms and not 1μs, and this change alters the validation to reject values smaller than 1ms (kernel code reference).
This PR is a revert of #111554 plus updates based on clarification of CFS work in kernel provided by @odinuge in #112108 (comment)
/kind documentation
/kind bug
Does this PR introduce a user-facing change?
Yes and no. Previously providing any value less than 1ms, like 1μs, would result in errors like the following:
After that change, any value below 1ms would fail the validation and die earlier and with a more straightforward error message.