reduce configmap and secret watch of kubelet #99393

chenyw1990 · 2021-02-24T08:55:51Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

In a large cluster, if a large number of pods reference configmap/secret as environment variables, kubelet will generate a lot of watches for configmap and secret, and kube-apiserver will use too much memory due to too many watches.
Although we have added Immutable features, in real usage scenarios, many configmaps are not marked as Immutable.

Which issue(s) this PR fixes:

Fixes #98660

Special notes for your reviewer:

@wojtek-t

Does this PR introduce a user-facing change?

When the kubelet is watching a ConfigMap or Secret purely in the context of setting environment variables
for containers, only hold that watch for a defined duration before cancelling it. This change reduces the CPU
and memory usage of the kube-apiserver in large clusters.

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

wojtek-t · 2021-02-24T09:04:11Z

pkg/kubelet/util/manager/watch_based_manager.go

+		return
+	}
+
+	go wait.Forever(func() {


I don't think this PR works.
The fact the secrets whenever changed re automatically updated to all pods mounting them:
https://kubernetes.io/docs/concepts/configuration/secret/#mounted-secrets-are-updated-automatically
Means that they are periodically checked if nothing changed.

So this PR doesn't work.

The following uses configmap as an example. The secret is the same as the configmap,
The configmap is used in the following two places:

Used by a container as volume.

Information in the configmap is injected to the container as environment variables.

For the configmap volume, we expect that the watch link can be maintained. Therefore, we set maxIdleTime to three times the value of resyncInterval. (Each resyncInterval period, VolumeManager queries the configmap information, lastAcquiredTime will be updated.)

For the configmap used by environment variables, it is expected that after the container is created, the kubelet can disconnect the watch link for the configmap.

OK - I see what you're saying now - that makes sense.
(we can argue if three times resyncInterval isn't too small - I would feel more comfortable with something higher, but that's a minor thing)

But this kind of change for sure deserves a test.

If a ConfigMap or Secret is only used to set container environment variables, why watch it at all?

chenyw1990 · 2021-02-24T12:25:09Z

/retest

ehashman · 2021-02-24T18:50:39Z

/priority important-longterm
/triage accepted

waiting on author for @wojtek-t's comments

pkg/kubelet/util/manager/watch_based_manager.go

pkg/kubelet/configmap/configmap_manager.go

chenyw1990 · 2021-02-27T06:12:14Z

/retest

chenyw1990 · 2021-02-27T09:21:21Z

@wojtek-t I have perfected the code. And I tested on the 4000-node cluster. This solution can significantly reduce the number of watches. Could you help me to review the code again？

pkg/kubelet/util/manager/watch_based_manager.go

chenyw1990 · 2021-03-03T15:04:59Z

@wojtek-t Thanks for your suggestions, I have modified the code based on the review comments.

chenyw1990 · 2021-03-04T01:17:39Z

/retest

pkg/kubelet/util/manager/watch_based_manager.go

pkg/kubelet/util/manager/watch_based_manager_test.go

wojtek-t · 2021-03-05T14:55:42Z

test/integration/kubelet/watch_manager_test.go

-	store := manager.NewObjectCache(listObj, watchObj, newObj, isImmutable, schema.GroupResource{Group: "v1", Resource: "secrets"})
+
+	fakeClock := clock.NewFakeClock(time.Now())
+


nit: remove this empty line

not done yet

staging/src/k8s.io/apimachinery/pkg/util/wait/wait.go

pkg/kubelet/util/manager/watch_based_manager.go

k8s-ci-robot · 2021-03-08T07:45:36Z

@chenyw1990: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel-test	820af60db0a76b0494b148b68580ee7c11066753	link	`/test pull-kubernetes-bazel-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

pkg/kubelet/util/manager/watch_based_manager.go

wojtek-t · 2021-03-08T07:49:55Z

pkg/kubelet/util/manager/watch_based_manager_test.go

+
+		item.lock.Lock()
+		defer item.lock.Unlock()
+		select {


nit: you can just return item.stopped here (instead of checking channels)

staging/src/k8s.io/apimachinery/pkg/util/wait/wait.go

wojtek-t · 2021-03-08T07:55:26Z

test/integration/kubelet/watch_manager_test.go

-	store := manager.NewObjectCache(listObj, watchObj, newObj, isImmutable, schema.GroupResource{Group: "v1", Resource: "secrets"})
+
+	fakeClock := clock.NewFakeClock(time.Now())
+


not done yet

pkg/kubelet/util/manager/watch_based_manager.go

wojtek-t

Just some very minor comments.

pkg/kubelet/util/manager/watch_based_manager.go

wojtek-t · 2021-03-08T09:13:19Z

/lgtm
/approve

k8s-ci-robot · 2021-03-08T09:13:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chenyw1990, wojtek-t

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~~ [wojtek-t]
~~test/OWNERS~~ [wojtek-t]

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

k8s-ci-robot requested review from aojea and krmayankk February 24, 2021 08:56

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

wojtek-t reviewed Feb 24, 2021

View reviewed changes

ehashman added this to Triage in SIG Node PR Triage Feb 24, 2021

ehashman moved this from Triage to Waiting on Author in SIG Node PR Triage Feb 24, 2021

lala123912 reviewed Feb 27, 2021

View reviewed changes

chenyw1990 force-pushed the reduceKubeletResourceWatch branch 2 times, most recently from 0a243e6 to 2e1ddc6 Compare February 27, 2021 04:41

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 27, 2021

wojtek-t reviewed Mar 2, 2021

View reviewed changes

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Mar 2, 2021

chenyw1990 force-pushed the reduceKubeletResourceWatch branch from 8e86aeb to afd27c9 Compare March 3, 2021 15:00

wojtek-t reviewed Mar 5, 2021

View reviewed changes

chenyw1990 force-pushed the reduceKubeletResourceWatch branch from afd27c9 to dba1b3e Compare March 8, 2021 07:10

wojtek-t reviewed Mar 8, 2021

View reviewed changes

chenyw1990 force-pushed the reduceKubeletResourceWatch branch 2 times, most recently from c6d5d6a to 390be42 Compare March 8, 2021 08:37

wojtek-t reviewed Mar 8, 2021

View reviewed changes

pkg/kubelet/util/manager/watch_based_manager.go Show resolved Hide resolved

pkg/kubelet/util/manager/watch_based_manager.go Outdated Show resolved Hide resolved

pkg/kubelet/util/manager/watch_based_manager.go Outdated Show resolved Hide resolved

reduce configmap and secret watch of kubelet

57a3b0a

chenyw1990 force-pushed the reduceKubeletResourceWatch branch from 390be42 to 57a3b0a Compare March 8, 2021 08:56

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

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

k8s-ci-robot merged commit a517ecc into kubernetes:master Mar 8, 2021

SIG Node PR Triage automation moved this from Waiting on Author to Done Mar 8, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 8, 2021

openshift-ci-robot mentioned this pull request Apr 20, 2021

Bug 1951815: UPSTREAM: 99393: kubelet: reduce configmap and secret watch openshift/kubernetes#692

Merged

openshift-ci-robot mentioned this pull request May 12, 2021

[release-4.6] Bug 1960002: UPSTREAM: 99393: kubelet: reduce configmap and secret watch openshift/kubernetes#745

Merged

openshift-ci-robot mentioned this pull request Jul 8, 2021

[WIP] [release-4.6] Rebase onto v1.19.12 openshift/kubernetes#850

Closed

jingxu97 mentioned this pull request Aug 25, 2021

Revert "reduce configmap and secret watch of kubelet" #104579

Closed

openshift-ci-robot mentioned this pull request Sep 7, 2021

Bug 2003027: Rebase 1.20.10 openshift/kubernetes#935

Merged

yb01 mentioned this pull request Sep 10, 2021

reduce number of secret watchers for scalability CentaurusInfra/arktos#1170

Open

openshift-ci-robot mentioned this pull request Sep 16, 2021

[release-4.6] Bug 2008266: Rebase 1.19.14 openshift/kubernetes#962

Merged

wojtek-t mentioned this pull request Jan 25, 2022

Sporadic "MountVolume.SetUp failed for volume ... not registered" failures seen in 1.22.0-1.22.8, 1.23.0-1.23.5 #105204

Closed

pacoxu mentioned this pull request Nov 1, 2022

kubelet: fix nil pointer in startReflector for standalone mode #113501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reduce configmap and secret watch of kubelet #99393

reduce configmap and secret watch of kubelet #99393

chenyw1990 commented Feb 24, 2021 •

edited

wojtek-t Feb 24, 2021

chenyw1990 Feb 24, 2021

wojtek-t Feb 25, 2021

sftim Feb 27, 2021

chenyw1990 commented Feb 24, 2021

ehashman commented Feb 24, 2021

chenyw1990 commented Feb 27, 2021

chenyw1990 commented Feb 27, 2021

chenyw1990 commented Mar 3, 2021

chenyw1990 commented Mar 4, 2021

wojtek-t Mar 5, 2021

wojtek-t Mar 8, 2021

k8s-ci-robot commented Mar 8, 2021 •

edited

wojtek-t Mar 8, 2021

wojtek-t Mar 8, 2021

wojtek-t left a comment

wojtek-t commented Mar 8, 2021

k8s-ci-robot commented Mar 8, 2021

		store := manager.NewObjectCache(listObj, watchObj, newObj, isImmutable, schema.GroupResource{Group: "v1", Resource: "secrets"})

		fakeClock := clock.NewFakeClock(time.Now())

reduce configmap and secret watch of kubelet #99393

reduce configmap and secret watch of kubelet #99393

Conversation

chenyw1990 commented Feb 24, 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.:

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

chenyw1990 commented Feb 24, 2021

ehashman commented Feb 24, 2021

chenyw1990 commented Feb 27, 2021

chenyw1990 commented Feb 27, 2021

chenyw1990 commented Mar 3, 2021

chenyw1990 commented Mar 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

wojtek-t commented Mar 8, 2021

k8s-ci-robot commented Mar 8, 2021

chenyw1990 commented Feb 24, 2021 •

edited

k8s-ci-robot commented Mar 8, 2021 •

edited