Add "node-high" priority-level #101151

mborsz · 2021-04-15T14:31:44Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

It adds "node-high" priority-level that is used by kubelets to report their status.
It has two goal:

making sure that kubelets are able to report their status even if control plane is overloaded by high pod churn (e.g. pod creation events, fetching secrets, fetching pods).
increasing total shares assigned to traffic that before this PR used "system" (in large clusters this is ~1K QPS, up to 90% of traffic in the cluster).

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

New "node-high" priority-level has been added to Suggested API Priority and Fairness configuration.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/assign @wojtek-t @MikeSpreitzer @lavalamp

wojtek-t · 2021-04-15T14:54:54Z

I'm definitely supportive for this change, but I would like to hear others opinions too.

@deads2k @tkashem @MikeSpreitzer

fejta-bot · 2021-04-15T15:19:49Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

tkashem · 2021-04-15T15:37:45Z

staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go

+		flowcontrol.PriorityLevelConfigurationSpec{
+			Type: flowcontrol.PriorityLevelEnablementLimited,
+			Limited: &flowcontrol.LimitedPriorityLevelConfiguration{
+				AssuredConcurrencyShares: 100,


The node health traffic, is it O(number of nodes) in the cluster and each node sends health status to the apiserver on a regular interval? If so, 100 seems a bit high to me especially when we don't have borrowing.

With 100 added for node health, we are roughly allocating 1/3 of the global concurrency share to node-health traffic.

Some current stats:

currently we allocate concurrency share of 30 for all kubelet traffic.

workload-low has the highest concurrency share of 100, this is pretty much for all in-cluster service account traffic.

With 100 added for node health, we are roughly allocating 1/3 of the global concurrency share to node-health traffic.

And this 1/3 is roughly what we see in large clusters. Hence the value.
[I will let @mborsz to describe a bit more on the reasoning behind this number.]

If our numbers are visibly different than yours, I'm wondering how should we proceed.
I strongly believe that the rule itself is very important, because health reporting is more important than any other actions done by Kubelets (otherwise nodes may become unready etc.). But I'm wondering how should we solve the value here (I would really like to avoid exposing this as a knob of kube-apiserver).
Any ideas?

Sorry, it's a bug (I incorrectly overestimated global concurrency share). Indeed, it should be lower value, like ~30-50.

Reasoning behind this number: In the idle cluster (no pod change), nearly all requests are for node health checking (~1.1K QPS in 5k scale) and we see approx. ~6 cores usage for kube-apiserver, so we can assume that node-health check consumes approx. 10-15% of total master capacity (depending on how we exactly calculate this).

The current global concurrency share is 205, so to get 15% we need 36 shares (36 / (205 + 36). Rounding up, I suggest using 40.

lavalamp · 2021-04-15T16:52:00Z

staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go

+				resourceRule(
+					[]string{flowcontrol.VerbAll},
+					[]string{coordinationv1.GroupName},
+					[]string{"leases"},


It's confusing to include leases in something named "node-health".

Maybe use "kubelet-high" instead?

Naming is hard :)

I think nodes aren't just about Kubelet - IIRC, NodeProblemDetector is also running under "NodeGroup".
So I think kubelet is not the best.

I actually like node (but it's in a sense of "node" (as a machine/VM where we run pods) not in the context of "node" as an API object. But I see where the confusion is coming from.

I with I have a better proposal (node-heartbeats? [but that doesn't reflect the node status changes]).

Oh, if it's NPD also, then how about "node-high"? (but should NPD really be included?)

"node-high" sounds fine to me.

Re NPD - in my opinion it should, because if there is a problem with a node, I think reporting the status (to reflect the actual state and e.g. block scheduling more pods on that in case of issues) is more important than other operations (e.g. reporting new status of pod or fetching secrets to start it or more like that).

Thanks for naming suggestions :) I renamed to "node-high" :)

wojtek-t · 2021-04-16T12:17:40Z

staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go

@@ -48,6 +48,11 @@ var (
 		// cluster and the availability of those running pods in the cluster, including kubelet and
 		// kube-proxy.
 		SuggestedPriorityLevelConfigurationSystem,
+		// "node-high" priority-level is for the kubelet health reporting. It is separated from "system"


s/kubelet/node/

[NPD might be in this group too.]

wojtek-t · 2021-04-16T13:38:36Z

/sig api-machinery

/lgtm
[I can safely do that because it requires api-approval anyway]

@tkashem - does the new number (and Maciek`s explanation) look reasonable to you? Are you ok with the current state?

lavalamp · 2021-04-16T22:20:38Z

/approve

We can always change the number again if we come up with a better estimate.

k8s-ci-robot · 2021-04-16T22:21:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, mborsz

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:

~~staging/src/k8s.io/apiserver/pkg/apis/OWNERS~~ [lavalamp]

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

fejta-bot · 2021-04-17T01:34:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-04-17T06:14:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-04-17T10:15:08Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fedebongio · 2021-04-20T06:31:13Z

/triage accepted

create public patch for github.com/kubernetes/pull/101151 See merge request eks-dataplane/eks-kubernetes-patches!55

k8s-ci-robot assigned lavalamp and MikeSpreitzer Apr 15, 2021

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 15, 2021

k8s-ci-robot assigned wojtek-t Apr 15, 2021

k8s-ci-robot requested review from justinsb and yujuhong April 15, 2021 14:33

wojtek-t assigned deads2k Apr 15, 2021

tkashem reviewed Apr 15, 2021

View reviewed changes

lavalamp reviewed Apr 15, 2021

View reviewed changes

mborsz force-pushed the nodehealth branch from b89ef08 to 167e537 Compare April 16, 2021 09:55

mborsz changed the title ~~Add "node-health" priority-level~~ Add "node-high" priority-level Apr 16, 2021

mborsz force-pushed the nodehealth branch from 167e537 to 81bad9b Compare April 16, 2021 09:58

wojtek-t reviewed Apr 16, 2021

View reviewed changes

Add "node-high" priority-level

8d6e76f

mborsz force-pushed the nodehealth branch from 81bad9b to 8d6e76f Compare April 16, 2021 12:20

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 16, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2021

k8s-ci-robot merged commit 09bd596 into kubernetes:master Apr 17, 2021

k8s-ci-robot added this to the v1.22 milestone Apr 17, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 20, 2021

github-actions bot mentioned this pull request Apr 21, 2021

Week Ending April 14, 2021 dev-obs/actus#357

Open

Swizzmaster pushed a commit to Swizzmaster/kubernetes that referenced this pull request Feb 29, 2024

create public patch for github.com/kubernetes/pull/101151

02646e9

Swizzmaster pushed a commit to Swizzmaster/kubernetes that referenced this pull request Feb 29, 2024

Merge branch 'nodehigh' into 'mainline'

3f5d2f1

create public patch for github.com/kubernetes/pull/101151 See merge request eks-dataplane/eks-kubernetes-patches!55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "node-high" priority-level #101151

Add "node-high" priority-level #101151

mborsz commented Apr 15, 2021 •

edited

Loading

wojtek-t commented Apr 15, 2021

fejta-bot commented Apr 15, 2021

tkashem Apr 15, 2021

wojtek-t Apr 15, 2021

mborsz Apr 16, 2021

lavalamp Apr 15, 2021

wojtek-t Apr 15, 2021

lavalamp Apr 15, 2021

wojtek-t Apr 15, 2021

mborsz Apr 16, 2021

wojtek-t Apr 16, 2021

mborsz Apr 16, 2021

wojtek-t commented Apr 16, 2021

lavalamp commented Apr 16, 2021

k8s-ci-robot commented Apr 16, 2021

fejta-bot commented Apr 17, 2021

fejta-bot commented Apr 17, 2021

fejta-bot commented Apr 17, 2021

fedebongio commented Apr 20, 2021

Add "node-high" priority-level #101151

Add "node-high" priority-level #101151

Conversation

mborsz commented Apr 15, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

wojtek-t commented Apr 15, 2021

fejta-bot commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Apr 16, 2021

lavalamp commented Apr 16, 2021

k8s-ci-robot commented Apr 16, 2021

fejta-bot commented Apr 17, 2021

fejta-bot commented Apr 17, 2021

fejta-bot commented Apr 17, 2021

fedebongio commented Apr 20, 2021

mborsz commented Apr 15, 2021 •

edited

Loading