Repository navigation
fix #51135 make CFS quota period configurable #63437
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,6 +220,8 @@ type KubeletConfiguration struct { | |
| // cpuCFSQuota enables CPU CFS quota enforcement for containers that | ||
| // specify CPU limits | ||
| CPUCFSQuota bool | ||
| // CPUCFSQuotaPeriod sets the CPU CFS quota period value, cpu.cfs_period_us, defaults to 100ms | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm super confused about this one and can't figure this one out by myself:
In the change I'm commenting author clearly acted under assumptions that milliseconds are used in k8s and now it's all over the test cases and comments. But I wonder where that 1000x disperancy between k8s value and Linux value comes from? And does it exist, or k8s default is actually a microsecond, same as linux? Unless we multiple that default value of cc @vishh @sjenning and especially @hjacobs: I hope you all have some context about it and can tell me why I'm wrong and k8s code comments are right.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes cfs_period_us is in µs, that's why it has
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment in that MR says "ms". In contrast, I think it better be "microseconds". I can send MR to change it as well as explicit millisecond mentions in the current code, whereas it should be microseconds. I'll ping you in it. In this MR, you wrote milliseconds duration in multiple places in defaults and tests Also, the discussion which led to this MR has multiple people talking about changing the value from 100 milliseconds to 5/25 milliseconds for testing. Still, it doesn't make sense unless everyone were altering the source code in microseconds, thinking it's milliseconds. @hjacobs even did a talk about it talking about the default value as if it was in milliseconds. Your code introducing milliseconds and not microseconds (so 1000x) contributed to my confusion, and I wonder if it's intentional or if you are mistaken, and it should be changed microseconds to change the real default. The only way current code would make sense to me is if the value you feed to k8s (100 milliseconds) is divided by 1000 at some point in time and then matches the real Linux default of 100 microseconds, but I haven't found such a place yet, and I would doubt that's the case.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szuecs, the simplest form of the question I can form is the following. Is the 100ms value of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wrong, and the original code was correct. 100ms is indeed a default value in Linux, and I got confused by conversions in two places in code that had no comments about them. PR #112123 adds comments to the code added here. |
||
| CPUCFSQuotaPeriod metav1.Duration | ||
| // maxOpenFiles is Number of files that can be opened by Kubelet process. | ||
| MaxOpenFiles int64 | ||
| // contentType is contentType of requests sent to apiserver. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import ( | |
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| kruntime "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/kubernetes/pkg/features" | ||
| "k8s.io/kubernetes/pkg/kubelet/qos" | ||
| kubetypes "k8s.io/kubernetes/pkg/kubelet/types" | ||
| "k8s.io/kubernetes/pkg/master/ports" | ||
|
|
@@ -154,6 +155,9 @@ func SetDefaults_KubeletConfiguration(obj *KubeletConfiguration) { | |
| if obj.CPUCFSQuota == nil { | ||
| obj.CPUCFSQuota = utilpointer.BoolPtr(true) | ||
| } | ||
| if obj.CPUCFSQuotaPeriod == nil && obj.FeatureGates[string(features.CPUCFSQuotaPeriod)] { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if this actually works... We generally do not feature gate the defaulting of flags. We generally only place the feature gate around code we don't want to run, rather than around configuration.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does look like a problem. The flag should always default, it just shouldn’t have been used. Apologies for missing in review |
||
| obj.CPUCFSQuotaPeriod = &metav1.Duration{Duration: 100 * time.Millisecond} | ||
| } | ||
| if obj.MaxOpenFiles == 0 { | ||
| obj.MaxOpenFiles = 1000000 | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
defaults to Linux Kernel default (100ms)for clear?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.
See the former comment from ligitt. Cobra will render the default automatically.
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.
We should stop adding new flags to kubelet. Add directly to
KubeletConfigurationinstead?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.
i would prefer to not have the flag and just use via
KubeletConfiguration; we should note that dynamically changing the field value will not impact currently running pods (only new pods)