kubelet: cgroupv2 disable memcg notifications #106332

bobbypage · 2021-11-10T23:45:07Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The kubelet memory notifier relies on reading
cgroup.event_control which is unsupported on cgroupv2. For now, let's
disable the memory notifier on cgroupv2.

Which issue(s) this PR fixes:

Fixes #106331

Special notes for your reviewer:

As a more short term solution, I propose to disable kernel memcg notifications on kubelet for cgroupv2 since it's currently not supported. Longer term, we should consider how we adapt kubelet eviction as a whole to take advtange of new cgroupv2 features.

Does this PR introduce a user-facing change?

NONE

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

The current memory notifier on cgroupv2 relies on reading `cgroup.event_control` which is unsupported on cgroupv2. For now, let's disable the feature on cgroupv2.

bobbypage · 2021-11-11T00:11:20Z

/hold

To get some feedback and perform some testing

giuseppe

LGTM

bobbypage · 2021-11-11T00:31:23Z

/assign @Dragoncell @mrunalp @giuseppe

bobbypage · 2021-11-11T01:10:21Z

Tested as follows, started a cluster on GCE with ubuntu cgroupv2 image with and without this change:

# ubuntu-2110 "Impish Indri" has cgroupv2 enabled
UBUNTU_IMAGE=$(gcloud compute images list --project="ubuntu-os-cloud" --no-standard-images --format=json | jq -r '.[] | select(.family=="ubuntu-2110") | .name')

KUBE_UBUNTU_INSTALL_CONTAINERD_VERSION=v1.5.7 \
  KUBE_UBUNTU_INSTALL_RUNC_VERSION=v1.0.2 \
  KUBE_MASTER_OS_DISTRIBUTION=ubuntu \
  KUBE_GCE_MASTER_IMAGE=${UBUNTU_IMAGE} \
  KUBE_GCE_MASTER_PROJECT=ubuntu-os-cloud \
  KUBE_NODE_OS_DISTRIBUTION=ubuntu \
  KUBE_GCE_NODE_IMAGE="${UBUNTU_IMAGE}" \
  KUBE_GCE_NODE_PROJECT=ubuntu-os-cloud \
  KUBE_KUBELET_EXTRA_ARGS="--kernel-memcg-notification=true" kubetest \
    --build \
    --up \
    --provider=gce \
    --gcp-node-image=ubuntu \
    --gcp-master-image=ubuntu \
    --gcp-zone=us-central1-c \
    --gcp-project=${GCP_PROJECT} \
    --gcp-nodes=1

Without this change (failed to update notifier due to reading non-existent cgroup.event_control file)

$ journalctl -u kubelet | grep "eviction"
Nov 11 00:36:37 e2e-test-porterdavid-minion-group-wsqk kubelet[6645]: I1111 00:36:37.426226    6645 memory_threshold_notifier.go:101] "Eviction manager: setting notifier to capacity" notifier="hard memory eviction threshold" capacity="9139312Ki"
Nov 11 00:36:37 e2e-test-porterdavid-minion-group-wsqk kubelet[6645]: I1111 00:36:37.426278    6645 eviction_manager.go:262] "Eviction manager: failed to update notifier" notifier="hard memory eviction threshold" err="no such file or directory"
Nov 11 00:36:37 e2e-test-porterdavid-minion-group-wsqk kubelet[6645]: I1111 00:36:37.426298    6645 memory_threshold_notifier.go:101] "Eviction manager: setting notifier to capacity" notifier="hard allocatable memory eviction threshold" capacity="7373584Ki"
Nov 11 00:36:37 e2e-test-porterdavid-minion-group-wsqk kubelet[6645]: I1111 00:36:37.426325    6645 eviction_manager.go:262] "Eviction manager: failed to update notifier" notifier="hard allocatable memory eviction threshold" err="no such file or directory"
Nov 11 00:36:37 e2e-test-porterdavid-minion-group-wsqk kubelet[6645]: I1111 00:36:37.426572    6645 eviction_manager.go:328] "Eviction manager: no resources are starved"

and with this change (no such errors):

Nov 11 01:07:20 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:20.364273    4160 memory_threshold_notifier.go:101] "Eviction manager: setting notifier to capacity" notifier="hard allocatable memory eviction threshold" capacity="7371440Ki"
Nov 11 01:07:20 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:20.365310    4160 eviction_manager.go:328] "Eviction manager: no resources are starved"
Nov 11 01:07:30 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:30.366155    4160 eviction_manager.go:237] "Eviction manager: synchronize housekeeping"
Nov 11 01:07:30 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:30.386698    4160 memory_threshold_notifier.go:101] "Eviction manager: setting notifier to capacity" notifier="hard memory eviction threshold" capacity="9134780Ki"
Nov 11 01:07:30 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:30.386719    4160 memory_threshold_notifier.go:101] "Eviction manager: setting notifier to capacity" notifier="hard allocatable memory eviction threshold" capacity="7371440Ki"
Nov 11 01:07:30 e2e-test-porterdavid-minion-group-05km kubelet[4160]: I1111 01:07:30.386949    4160 eviction_manager.go:328] "Eviction manager: no resources are starved"

SergeyKanzhelev · 2021-11-11T05:47:55Z

/triage accepted
/priority important-soon

mrunalp · 2021-11-11T17:34:08Z

/approve

k8s-ci-robot · 2021-11-11T17:34:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobbypage, giuseppe, mrunalp

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/kubelet/OWNERS~~ [mrunalp]

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

bobbypage · 2021-11-11T20:29:14Z

/unhold

249043822 · 2021-11-12T08:44:08Z

pkg/kubelet/eviction/threshold_notifier_linux.go

+	// Instead long term, on cgroupv2 kubelet should rely on combining usage of memory.low on root pods cgroup with inotify notifications on memory.events and or PSI pressure.
+	// For now, let's return a fake "disabled" cgroup notifier on cgroupv2.
+	// https://github.com/kubernetes/kubernetes/issues/106331
+	if libcontainercgroups.IsCgroup2UnifiedMode() {


Should we not start the notifier at all in eviction_manager while enable kernelMemcgNotification
but on cgroupv2 ?

kubernetes/pkg/kubelet/eviction/eviction_manager.go

Line 184 in c98b388

notifier, err := NewMemoryThresholdNotifier(threshold, m.config.PodCgroupRoot, &CgroupNotifierFactory{}, thresholdHandler)

that would work too, but I choose to disable it in the NewCgroupNotifier, so calling Start() will be a no-op. I guess there's not big difference on where exactly to disable it.

The other reason it seems like eviction manager logic there is a bit "higher level" in general, and doesn't really have any cgroup related logic, so I wanted to keep this check contained in the lower level package closer to the other cgroup related logic.

mrunalp · 2021-11-13T00:49:34Z

/lgtm

kubelet: cgroupv2 disable memcg notifications

f5140d3

The current memory notifier on cgroupv2 relies on reading `cgroup.event_control` which is unsupported on cgroupv2. For now, let's disable the feature on cgroupv2.

k8s-ci-robot requested review from matthyx and yujuhong November 10, 2021 23:48

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 10, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2021

giuseppe approved these changes Nov 11, 2021

View reviewed changes

k8s-ci-robot assigned Dragoncell, giuseppe and mrunalp Nov 11, 2021

SergeyKanzhelev added this to Triage in SIG Node PR Triage Nov 11, 2021

SergeyKanzhelev moved this from Triage to Needs Reviewer in SIG Node PR Triage Nov 11, 2021

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2021

249043822 reviewed Nov 12, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 13, 2021

k8s-ci-robot merged commit e4c7951 into kubernetes:master Nov 13, 2021

SIG Node PR Triage automation moved this from Needs Reviewer to Done Nov 13, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: cgroupv2 disable memcg notifications #106332

kubelet: cgroupv2 disable memcg notifications #106332

bobbypage commented Nov 10, 2021 •

edited

bobbypage commented Nov 11, 2021

giuseppe left a comment

bobbypage commented Nov 11, 2021

bobbypage commented Nov 11, 2021 •

edited

SergeyKanzhelev commented Nov 11, 2021

mrunalp commented Nov 11, 2021

k8s-ci-robot commented Nov 11, 2021

bobbypage commented Nov 11, 2021

249043822 Nov 12, 2021

bobbypage Nov 12, 2021 •

edited

mrunalp commented Nov 13, 2021

kubelet: cgroupv2 disable memcg notifications #106332

kubelet: cgroupv2 disable memcg notifications #106332

Conversation

bobbypage commented Nov 10, 2021 • edited

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.:

bobbypage commented Nov 11, 2021

giuseppe left a comment

Choose a reason for hiding this comment

bobbypage commented Nov 11, 2021

bobbypage commented Nov 11, 2021 • edited

SergeyKanzhelev commented Nov 11, 2021

mrunalp commented Nov 11, 2021

k8s-ci-robot commented Nov 11, 2021

bobbypage commented Nov 11, 2021

249043822 Nov 12, 2021

Choose a reason for hiding this comment

bobbypage Nov 12, 2021 • edited

Choose a reason for hiding this comment

mrunalp commented Nov 13, 2021

bobbypage commented Nov 10, 2021 •

edited

bobbypage commented Nov 11, 2021 •

edited

bobbypage Nov 12, 2021 •

edited