Kubelet watches necessary secrets/configmaps instead of periodic polling by wojtek-t · Pull Request #64752 · kubernetes/kubernetes

wojtek-t · 2018-06-05T09:09:55Z

No description provided.

k8s-ci-robot · 2018-06-05T09:09:57Z

@wojtek-t: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

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.

wojtek-t · 2018-06-05T12:37:38Z

/test pull-kubernetes-integration

yujuhong · 2018-06-16T00:00:20Z

pkg/kubelet/apis/kubeletconfig/v1beta1/types.go

The name ManagerMode seems too generic. Can we make it more specific for ConfigMap & Secrets?

Also, is there any value to configure secrets and configmaps differently? I'm guessing not...

Changed to ConfigMapAndSecretManagerMode - if you have any better suggestion, I can change one more time.

Regarding configuring differently - I consciously merge those two - I think we don't want to diverge them.
Let me know if you disagree with that.

yujuhong · 2018-06-16T00:01:44Z

pkg/kubelet/apis/kubeletconfig/v1beta1/types.go

/cc @mtaufen FYI

Thanks :). Comments below:

Is it necessary to expose this as a config? How risky do we think the change is? We already watch Pods, dynamic Kubelet config watches ConfigMaps, so watch is not unprecedented in the Kubelet.

If so, I think we can potentially improve the naming and structure here. Naming is hard :). See below.

With respect to "manager", I really like the advice in https://blog.codinghorror.com/i-shall-call-it-somethingmanager/

Mode is more general than it has to be, specifically this is a strategy for observing resource updates.

ResourceNotifyStrategy was my first guess of what to call the strategy type, but this is still a bit vague, IMO, because it is not clear who is being notified (is it the resource, or the Kubelet?), or why there is a notification.

ResourceChangedEventStrategy might be a bit better, since ResourceChangedEvent makes it clear what's going on. But Event is overloaded for another Kubernetes concept, so I don't like this either.

ResourceChangeDetectionStrategy manages to avoid overloading Event. This is my best guess so far, IMO. Still feels verbose, maybe there's a way to simplify it.

For a field name, maybe ConfigMapAndSecretChangeDetectionStrategy. It's a bit verbose, so if there's a standard term that clearly captures ConfigMaps and Secrets, and whatever other objects this field will cover in the future, consider using that. But be wary of making it too general.

Other things:
Should we allow the strategies for ConfigMaps and Secrets to be configured independently?

I think we can simply use the infinitive of the verb. Instead of Caching and Watching, we can just say Cache and Watch. Simple is a bit vague, would it be more accurate to call this strategy Poll?

Also, structure-wise, is there a broader category for resource-consumption-lifecycles in the Kubelet, and should we invent a config substructure for that?

I like: ResourceChangeDetectionStrategy - changed.

Regarding specific configmap and secrets:

I don't have any idea for a common term for those two - so I left ConfigMapAndSecretChangeDetectionStrategy

I don't want to split them, because we want to avoid diverging those two in my opinion

About specific infinitive verbs - yeah, that sounds fine. Also, I decided for "Fetch" instead of "Poll" - it's more accurate I think.

About the structure - I don't think we will have more about that, so introducing substructure doesn't make sense now in my opinion.

yujuhong · 2018-06-16T00:56:14Z

pkg/kubelet/secret/secret_manager.go

Merge this with the previous imports?

yujuhong · 2018-06-16T00:58:03Z

pkg/kubelet/configmap/configmap_manager.go

Merge this with the previous import.

yujuhong · 2018-06-16T01:00:00Z

cmd/kubeadm/app/util/config/testdata/defaulting/master/defaulted.yaml

Since "watching" is the default, why do you need to set it explicitly everywhere?

Those are some kubeadm test data - it seems we are explicitly listing the whole config in mutliple places.
I didn't dig deeper into those kubeadm files, but the problem seems orthogonal to this PR.

k8s-ci-robot · 2018-06-16T01:00:06Z

@yujuhong: GitHub didn't allow me to request PR reviews from the following users: FYI.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @mtaufen FYI

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.

wojtek-t

@yujuhong - thanks a lot for review.

PTAL

wojtek-t · 2018-06-18T07:31:16Z

cmd/kubeadm/app/util/config/testdata/defaulting/master/defaulted.yaml

Those are some kubeadm test data - it seems we are explicitly listing the whole config in mutliple places.
I didn't dig deeper into those kubeadm files, but the problem seems orthogonal to this PR.

wojtek-t · 2018-06-18T07:34:14Z

pkg/kubelet/apis/kubeletconfig/v1beta1/types.go

Changed to ConfigMapAndSecretManagerMode - if you have any better suggestion, I can change one more time.

Regarding configuring differently - I consciously merge those two - I think we don't want to diverge them.
Let me know if you disagree with that.

wojtek-t · 2018-06-18T07:39:14Z

pkg/kubelet/configmap/configmap_manager.go

wojtek-t · 2018-06-18T07:39:58Z

pkg/kubelet/secret/secret_manager.go

wojtek-t

PTAL

wojtek-t · 2018-06-19T07:18:48Z

pkg/kubelet/apis/kubeletconfig/v1beta1/types.go

I like: ResourceChangeDetectionStrategy - changed.

Regarding specific configmap and secrets:

I don't have any idea for a common term for those two - so I left ConfigMapAndSecretChangeDetectionStrategy

I don't want to split them, because we want to avoid diverging those two in my opinion

About specific infinitive verbs - yeah, that sounds fine. Also, I decided for "Fetch" instead of "Poll" - it's more accurate I think.

About the structure - I don't think we will have more about that, so introducing substructure doesn't make sense now in my opinion.

mtaufen · 2018-06-19T16:53:44Z

cmd/kubeadm/app/util/config/testdata/conversion/master/v1alpha2.yaml

s/Watche/Watch
(and other places)

mtaufen · 2018-06-19T16:58:21Z

pkg/kubelet/apis/kubeletconfig/types.go

Had another thought on this - would it be more standard to call it Get, given that this strategy is just sending a GET request to the API server?

Makes sense - done.

yujuhong

/lgtm

yujuhong · 2018-06-19T21:47:03Z

cmd/kubeadm/app/util/config/testdata/conversion/master/internal.yaml

    clusterDNS:
    - 10.96.0.10
    clusterDomain: cluster.local
+    configMapAndSecretChangeDetectionStrategy: Watch


Thanks for the heads up!

mtaufen · 2018-06-19T22:46:03Z

/lgtm

k8s-ci-robot · 2018-06-19T22:46:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/kubeadm/OWNERS~~ [wojtek-t]
~~pkg/kubelet/OWNERS~~ [wojtek-t,yujuhong]

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

fejta-bot · 2018-06-20T02:42:31Z

/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 comment for consistent failures.

liggitt · 2018-06-21T03:25:59Z

@wojtek-t how did this affect things on the large scale tests?

k8s-github-robot · 2018-06-21T03:26:20Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@derekwaynecarr @mtaufen @wojtek-t @yujuhong

Action required: This pull request requires label changes. If the required changes are not made within 2 days, the pull request will be moved out of the v1.12 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

wojtek-t · 2018-06-21T07:33:50Z

@wojtek-t how did this affect things on the large scale tests?

@liggitt When I was running 5k-node tests this visibly reduced cpu-load on apiserver (though drop on apicall latencies wasn't significant). My goal of merging it early in the cycle is to have many more runs over the whole cycle so that in case of problems we will fix them (or worst case disable it). But my two experiments didn't show any.

k8s-github-robot · 2018-06-22T02:48:12Z

Automatic merge from submit-queue (batch tested with PRs 65187, 65206, 65223, 64752, 65238). If you want to cherry-pick this change to another branch, please follow the instructions here.

narjesshmida · 2023-03-21T09:34:08Z

Hi, i am in situation : some pods need to detect a changed configmap, and every deploy i am obliged to redeploy some pods to get the new values of configmap... i know that i can do the rolling and restart ... but i want to put this
feature : configMapAndSecretChangeDetectionStrategy resourceChangeDetectionStrategy
because even when i change some secrets i want that all the pods detect the changes.
Any help please

wojtek-t added this to the v1.12 milestone Jun 5, 2018

wojtek-t assigned derekwaynecarr and yujuhong Jun 5, 2018

wojtek-t mentioned this pull request Jun 5, 2018

Use watch-based secret/configmap managers in Kubelet #63461

Closed

k8s-ci-robot requested review from Random-Liu and derekwaynecarr June 5, 2018 09:10

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 5, 2018

wojtek-t force-pushed the default_to_watching_managers branch from 255ed93 to 75826ea Compare June 5, 2018 11:08

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jun 5, 2018

yujuhong reviewed Jun 16, 2018

View reviewed changes

k8s-ci-robot requested a review from mtaufen June 16, 2018 01:00

wojtek-t force-pushed the default_to_watching_managers branch from 75826ea to e55dcbb Compare June 18, 2018 07:46

wojtek-t commented Jun 18, 2018

View reviewed changes

wojtek-t force-pushed the default_to_watching_managers branch 2 times, most recently from ec37421 to a1fc044 Compare June 19, 2018 07:35

wojtek-t commented Jun 19, 2018

View reviewed changes

mtaufen reviewed Jun 19, 2018

View reviewed changes

wojtek-t added 2 commits June 19, 2018 22:12

Kubelet manager configuration

ffb3247

Enable watching secret and configmap manager

72a0f4d

wojtek-t force-pushed the default_to_watching_managers branch from a1fc044 to 72a0f4d Compare June 19, 2018 20:14

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2018

yujuhong reviewed Jun 19, 2018

View reviewed changes

k8s-ci-robot requested a review from luxas June 19, 2018 21:47

k8s-ci-robot assigned mtaufen Jun 19, 2018

k8s-github-robot added the milestone/incomplete-labels label Jun 20, 2018

k8s-github-robot merged commit 96c7f3a into kubernetes:master Jun 22, 2018

wojtek-t mentioned this pull request Jun 28, 2018

gce-scale-performance suite is failing #65441

Closed

wojtek-t deleted the default_to_watching_managers branch July 2, 2018 08:29

wojtek-t mentioned this pull request Aug 23, 2018

Fix secrets docs in 1.12 branch kubernetes/website#10056

Merged

yujuhong mentioned this pull request Sep 5, 2018

kubelet refresh times for configmaps is long and random #30189

Closed

neosab mentioned this pull request Sep 19, 2018

Create a Sidecar for fanning out HTTP requests knative/eventing#435

Merged

jveski mentioned this pull request Feb 8, 2019

Pods sit in ContainerCreating for a long time in presence of a large number of ServiceAccount imagePullSecrets #73541

Closed

qmfrederik mentioned this pull request Feb 22, 2019

Many ConfigMaps and Pods slow down cluster, until it becomes unavailable (since 1.12) #74412

Closed

logicalhan mentioned this pull request Feb 27, 2019

Remove reflector metrics since they are causing a memory leak #74636

Merged

Conversation

wojtek-t commented Jun 5, 2018

Uh oh!

k8s-ci-robot commented Jun 5, 2018

Uh oh!

wojtek-t commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 16, 2018

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yujuhong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtaufen commented Jun 19, 2018

Uh oh!

k8s-ci-robot commented Jun 19, 2018

Uh oh!

fejta-bot commented Jun 20, 2018

Uh oh!

liggitt commented Jun 21, 2018

Uh oh!

k8s-github-robot commented Jun 21, 2018

Uh oh!

wojtek-t commented Jun 21, 2018

Uh oh!

k8s-github-robot commented Jun 22, 2018

Uh oh!

narjesshmida commented Mar 21, 2023 •

edited

Loading