KEP-4205: KEP to add PSI metrics in Summary API, and for PSI based node actions

[ndixita]

• One-line PR description: This KEP lays the initial foundations to use PSI metrics for setting Node Conditions.

• Issue link: Support PSI based on cgroupv2 #4205

[ndixita]

/assign @rphillips
/assign @haircommander
/assign @bobbypage

[tzneal]

+1, this may help provide more observability to users for nodes that are overloaded without requiring logging into the node to troubleshoot.

[rphillips]

I think we will want to propagate the PSI metrics from the main Kubelet slices separately: /, system.slice, and kubepods.slice. I do not see that reflected in this enhancement. Thoughts?

[bobbypage]

Thank you for proposing this KEP, I am super excited to see us build on top of cgroupv2 using features like PSI in the kubelet! Left a few comments.

[haircommander]

A couple more nits, and we still need some resolution on the multi-tiered node condition (separate for kubepods cgroup vs system cgroup), or at least a mention of it as we have a couple of releases between now and then

[haircommander]

indentation is weird here

[haircommander]

still is

[soltysh]

This comment still holds

[haircommander]

may be tricky to have e2e tests if we need CRI extensions to collect the metrics

[ndixita]

Yeah we can decide on the coverage later?

[haircommander]

TBH I'd drop this and replace it with unit testing

[soltysh]

What kind of CRI extensions are we talking about here? Are those optional, iow. the functionality will be only available for some container runtimes? (I'm not a CRI expert 😉 )

[haircommander]

a couple more nits. I propose we mostly focus reviews on phase 1 now--which to me mostly looks good--as the following phases will likely change with time

[soltysh]

#prr-shadow

[rphillips]

I took a pass on a review. @soltysh's comments should be addressed before we merge this; otherwise, lgtm.

[tzneal]

/lgtm

[ndixita]

/assign deads2k

[ndixita]

/assign @soltysh

[johnbelamaric]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, mrunalp, ndixita

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:

• keps/prod-readiness/OWNERS [johnbelamaric]
• keps/sig-node/OWNERS [johnbelamaric,mrunalp]

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

[tzneal]

/lgtm

[BojanZelic]

I've been playing around with a similar idea to this, using node-problem-detector to set node conditions based off of PSI pressure indicators and ran into an issue that might affect this KEP.

Basically while CPU PSI pressure information is a good indicator of how overloaded a node is, it also gets set whenever CFS cpu throttling occurs.

For example if a user schedules a pod and sets their cpu limits too low so that throttling happens frequently we see that the PSI metrics for that cgroup also get elevated

root@ip-10-4-172-240:/sys/fs/cgroup/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-podd75d892c_9bb2_4b9c_8ce2_7868eb91e665.slice/cri-containerd-fc6e9647769050baf7ea79855ca22b8496ed9b6dd2b7f06c2e583e18a25dc920.scope# cat cpu.pressure

some avg10=58.43 avg60=57.45 avg300=57.33 total=27907960587
full avg10=58.12 avg60=57.28 avg300=57.17 total=27494547488

which ends up being propagated to the system level metric:

cat /proc/pressure/cpu
some avg10=58.51 avg60=58.82 avg300=58.91 total=1062145857327
full avg10=0.00 avg60=0.00 avg300=0.00 total=0

cpu thottling:

root@ip-10-4-172-240:/sys/fs/cgroup/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-podd75d892c_9bb2_4b9c_8ce2_7868eb91e665.slice/cri-containerd-fc6e9647769050baf7ea79855ca22b8496ed9b6dd2b7f06c2e583e18a25dc920.scope# cat cpu.stat
usage_usec 139474622887
user_usec 133159602378
system_usec 6315020509
core_sched.force_idle_usec 0
nr_periods 478231
nr_throttled 459361
throttled_usec 437837995163
nr_bursts 0
burst_usec 0

so elevated cpu PSI pressure could also mean heavy throttling on a container which doesn't necessarily mean there's a node-level issue occurring