Generalize node lease controller #95428

roycaihw · 2020-10-09T01:35:36Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Split off from #95222 (#95222 (review)). Move nodelease controller to k8s.io/component-helpers and generalize it, so it can be reused for the apiserver lease.

Does this PR introduce a user-facing change?:

NONE

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

/assign @caesarxuchao @deads2k
/sig api-machinery
/sig node

staging/src/k8s.io/controller-manager/pkg/controllers/lease/controller.go

roycaihw · 2020-10-09T19:44:04Z

/retest

roycaihw · 2020-10-12T23:17:03Z

/cc @cheftako
Is k8s.io/controller-manager a good place to host controllers used by multiple components (kubelet, kube-apiserver)?

cheftako · 2020-10-13T01:19:33Z

/cc Walter Fender
Is k8s.io/controller-manager a good place to host controllers used by multiple components (kubelet, kube-apiserver)?

Maybe. I think the critical question is whether we want the relevant controllers to be "public". By "public" I mean do we want the controllers to be run by processes outside of the primary Kubernetes components (Eg aws-controller-manager or gcp-controller-manager) If so then we can talk about it being in either k8s.io/controller-manager or k8s.io/cloud-provider. If no then I think you want to steer clear of putting it anywhere under staging. If its under staging its public and we need to be very aware of things like our backward compatibility and deprecation policies.

roycaihw · 2020-10-13T21:26:52Z

@cheftako That's a good point. I can imagine aggregated apiservers run this controller in future (either import the controller directly, or we make it part of generic apiserver). But currently it's not the goal, we only need it for kube-apiserver and kubelet. For this case, I guess we can just put it under k8s.io/kubernetes/pkg/controller, and move it to staging in future if needed.

roycaihw · 2020-10-13T22:22:46Z

/retest

cheftako · 2020-10-14T21:56:17Z

pkg/controller/lease/controller.go

@@ -14,15 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package nodelease
+package lease


Putting this under pkg/controller seems confusing. The rest of the controllers under pkg/controller are control plane logic/controller being run by a controller-manager. This seems more like an implementation of heartbeat for a process (Kubelet) than a control plane controller.

That's true. The lease controller is a heartbeat implementation. It is different from control plane controllers in terms of 1. it doesn't watch any resource 2. it doesn't run leader election-- each replica of a component (kubelet) runs the lease controller simultaneously.

The other options I can think of:

k8s.io/client-go/tools - the behavior is similar to leaderelection (no watch; all replicas run simultaneously)

pkg/util - we do have some utility that spawns goroutine, but it's less related to k8s resources.

or we may create a new in-tree directory (e.g. pkg/tools, pkg/client/tools) to avoid making the existing ones confusing.

@deads2k @caesarxuchao Any suggestions?

I don't think we are ready to support this library publicly, so I think we can keep it in k8s.io/kubernetes/pkg/client/leasecontroller.

When we want to make it public, k8s.io/client-go/tools seems to be the most suitable place so far.

Let's see what @deads2k think.

We trust it for kubelets, so I think we have a good degree of confidence. Because it is related to apiserver handling that would apply to any apiserver, I think it needs to be somewhere public. I think your suggestion of client-go/tools is a good one.

Thanks for the comments. I update the PR to put it under client-go/tools. Please take a look

I disagree that client-go is the right repo. component-base was invented to have reusable functionality shared by our binaries. Hence, lease controller belongs there.

Component-base hosts code and best practices used by all the core components. This lease controller is not as critical IMO.

On the other hand, the recently created component-helpers hosts helpers required by two or more components, and the definition for a "helper" seems more arbitrary than a "base". I moved the controller to component-helpers. PTAL

caesarxuchao · 2020-10-21T05:51:07Z

LGTM.

staging/src/k8s.io/client-go/tools/lease/controller.go

deads2k · 2020-10-21T18:35:13Z

/approve

sttts · 2020-10-22T17:32:44Z

client-go is very strange for a controller.

roycaihw · 2020-10-23T16:28:40Z

/retest

sttts · 2020-10-23T18:43:14Z

/approve

deads2k · 2020-10-23T18:56:21Z

/lgtm

derekwaynecarr · 2020-10-23T19:32:10Z

kubelet changes look good.

/lgtm
/approve

k8s-ci-robot · 2020-10-23T19:32:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, roycaihw, sttts

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~~ [derekwaynecarr]
~~staging/publishing/OWNERS~~ [sttts]
~~staging/src/k8s.io/component-helpers/OWNERS~~ [deads2k]
~~vendor/OWNERS~~ [sttts]

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

roycaihw · 2020-10-23T21:29:10Z

Thanks for reviewing!

ingvagabund · 2020-11-03T12:28:09Z

k8s.io/component-helpers/lease is not the right name for the root directory. The convention (although not described in the readme) is to use a team owning the code. E.g. scheduling, auth, etc. In this case, the right name is node and the expected path is k8s.io/component-helpers/node/lease. @deads2k since you were the one that suggested this structure.

ingvagabund · 2020-11-03T12:52:23Z

Also k8s.io/component-helpers staging repo is for high-level helpers. Node lease controller is not a helper. This PR was not meant to be merged.

ingvagabund · 2020-11-03T12:57:50Z

@roycaihw can you more elaborate on why you moved the code under `k8s.io/component-helpers and why it was done after the PR got mostly approved?

roycaihw · 2020-11-03T22:32:43Z

@ingvagabund Hi, @deads2k @sttts and I discussed offline about #95428 (comment) and we agreed on using component-helpers to host the lease controller. Options we discussed:

client-go: strange for hosting a controller (although leader elector is there, which is sort of questionable)
component-base: best practices used by all the core components. The lease controller is not as critical, and it's only used by two components (kube-apiserver and kubelet)
component-helper: the definition of a helper is unclear. The original proposed name for this repo was library-go, which seemed broader

also from previous discussion in this PR:

controller-manager: the lease controller is different from control plane controllers in terms of 1. it doesn't watch any resource 2. it doesn't run leader election-- each replica of a component (kubelet) runs the lease controller simultaneously. It is more like an implementation of heartbeat for a process

k8s-ci-robot assigned caesarxuchao Oct 9, 2020

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 9, 2020

k8s-ci-robot assigned deads2k Oct 9, 2020

k8s-ci-robot requested review from feiskyer and liggitt October 9, 2020 01:36

k8s-ci-robot added area/kubelet sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Oct 9, 2020

roycaihw commented Oct 9, 2020

View reviewed changes

staging/src/k8s.io/controller-manager/pkg/controllers/lease/controller.go Outdated Show resolved Hide resolved

roycaihw force-pushed the cleanup/generalize-lease-controller branch from 791109f to 1d92e3f Compare October 9, 2020 01:43

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Oct 9, 2020

roycaihw force-pushed the cleanup/generalize-lease-controller branch from 1d92e3f to d9ac686 Compare October 9, 2020 18:23

roycaihw force-pushed the cleanup/generalize-lease-controller branch from d9ac686 to be85edc Compare October 12, 2020 22:50

k8s-ci-robot requested a review from cheftako October 12, 2020 23:17

roycaihw force-pushed the cleanup/generalize-lease-controller branch from be85edc to 5a017ef Compare October 13, 2020 21:41

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Oct 13, 2020

This was referenced Oct 13, 2020

kube-apiserver identity #95222

Closed

Add kube-apiserver lease controller #95533

Merged

cheftako reviewed Oct 14, 2020

View reviewed changes

k8s-ci-robot assigned sttts Oct 20, 2020

wojtek-t reviewed Oct 21, 2020

View reviewed changes

staging/src/k8s.io/client-go/tools/lease/controller.go Outdated Show resolved Hide resolved

roycaihw force-pushed the cleanup/generalize-lease-controller branch from 7e8c4bf to 140d1ba Compare October 22, 2020 18:40

roycaihw added 3 commits October 22, 2020 11:58

move node lease controller to component-helpers

c70e364

generalize lease controller

c9bbd85

generated

4af9198

roycaihw force-pushed the cleanup/generalize-lease-controller branch from 140d1ba to 4af9198 Compare October 22, 2020 19:01

allow component-helpers to import util and klog

9861b9e

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 23, 2020

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2020

k8s-ci-robot assigned derekwaynecarr Oct 23, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2020

k8s-ci-robot merged commit f20a36f into kubernetes:master Oct 23, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 23, 2020

lavalamp mentioned this pull request Apr 2, 2021

Introduce concurrency limits to PriorityLevelConfigurationStatus #91389

Closed

cartermckinnon mentioned this pull request Jul 29, 2023

Handle errors when preparing lease for update #119661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize node lease controller #95428

Generalize node lease controller #95428

roycaihw commented Oct 9, 2020 •

edited

roycaihw commented Oct 9, 2020

roycaihw commented Oct 12, 2020

cheftako commented Oct 13, 2020 •

edited

roycaihw commented Oct 13, 2020

roycaihw commented Oct 13, 2020

cheftako Oct 14, 2020

roycaihw Oct 15, 2020

caesarxuchao Oct 16, 2020

deads2k Oct 20, 2020

roycaihw Oct 20, 2020

sttts Oct 22, 2020

roycaihw Oct 22, 2020

caesarxuchao commented Oct 21, 2020

deads2k commented Oct 21, 2020

sttts commented Oct 22, 2020

roycaihw commented Oct 23, 2020

sttts commented Oct 23, 2020

deads2k commented Oct 23, 2020

derekwaynecarr commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

roycaihw commented Oct 23, 2020

ingvagabund commented Nov 3, 2020

ingvagabund commented Nov 3, 2020

ingvagabund commented Nov 3, 2020

roycaihw commented Nov 3, 2020

Generalize node lease controller #95428

Generalize node lease controller #95428

Conversation

roycaihw commented Oct 9, 2020 • edited

roycaihw commented Oct 9, 2020

roycaihw commented Oct 12, 2020

cheftako commented Oct 13, 2020 • edited

roycaihw commented Oct 13, 2020

roycaihw commented Oct 13, 2020

cheftako Oct 14, 2020

Choose a reason for hiding this comment

roycaihw Oct 15, 2020

Choose a reason for hiding this comment

caesarxuchao Oct 16, 2020

Choose a reason for hiding this comment

deads2k Oct 20, 2020

Choose a reason for hiding this comment

roycaihw Oct 20, 2020

Choose a reason for hiding this comment

sttts Oct 22, 2020

Choose a reason for hiding this comment

roycaihw Oct 22, 2020

Choose a reason for hiding this comment

caesarxuchao commented Oct 21, 2020

deads2k commented Oct 21, 2020

sttts commented Oct 22, 2020

roycaihw commented Oct 23, 2020

sttts commented Oct 23, 2020

deads2k commented Oct 23, 2020

derekwaynecarr commented Oct 23, 2020

k8s-ci-robot commented Oct 23, 2020

roycaihw commented Oct 23, 2020

ingvagabund commented Nov 3, 2020

ingvagabund commented Nov 3, 2020

ingvagabund commented Nov 3, 2020

roycaihw commented Nov 3, 2020

roycaihw commented Oct 9, 2020 •

edited

cheftako commented Oct 13, 2020 •

edited