Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock #94603

wojtek-t · 2020-09-08T06:23:10Z

Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock

/kind cleanup
/priority important-longterm

wojtek-t · 2020-09-08T06:28:08Z

staging/src/k8s.io/component-base/config/v1alpha1/defaults.go

@@ -44,7 +44,7 @@ func RecommendedDefaultLeaderElectionConfiguration(obj *LeaderElectionConfigurat
 		obj.RetryPeriod = metav1.Duration{Duration: 2 * time.Second}
 	}
 	if obj.ResourceLock == "" {
-		// TODO: Migrate to LeaseLock.
+		// TODO(#80289): Migrate to LeaseLock when graduating to v1beta1.


@liggitt - who owns that part now?
I would be happy to promote it to v1beta1 in 1.20 to clean this up, but wanted to check with the owner if there aren't any known blockers for doing that (or other things we want to fix during this migration).

I'm not sure... cc @mtaufen?

I don't think it's a particularly active file, but it's a core dependency so it would be good to stabilize the version. I'm not sure what the blockers would be off the top of my head. I'm happy to help if you want to work on moving it forward.

OK - I will try to get back to in in the next couple weeks.

Ok, no rush as I'll be OOO for the next couple weeks anyway.

wojtek-t · 2020-09-08T07:15:40Z

/retest

fejta-bot · 2020-09-08T08:18:33Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

liggitt · 2020-09-08T15:48:13Z

staging/src/k8s.io/component-base/config/v1alpha1/defaults.go

@@ -44,7 +44,7 @@ func RecommendedDefaultLeaderElectionConfiguration(obj *LeaderElectionConfigurat
 		obj.RetryPeriod = metav1.Duration{Duration: 2 * time.Second}
 	}
 	if obj.ResourceLock == "" {
-		// TODO: Migrate to LeaseLock.
+		// TODO(#80289): Migrate to LeaseLock when graduating to v1beta1.


I'm not sure... cc @mtaufen?

liggitt · 2020-09-08T15:48:43Z

staging/src/k8s.io/component-base/config/v1alpha1/defaults.go

@@ -44,7 +44,7 @@ func RecommendedDefaultLeaderElectionConfiguration(obj *LeaderElectionConfigurat
 		obj.RetryPeriod = metav1.Duration{Duration: 2 * time.Second}
 	}
 	if obj.ResourceLock == "" {
-		// TODO: Migrate to LeaseLock.
+		// TODO(#80289): Migrate to LeaseLock when graduating to v1beta1.


Going straight from Endpoints -> Lease isn't safe, right? We have to go Endpoints -> EndpointsLeases -> Lease over three releases, right?

Separately, I'm not sure we can know when it's safe to migrate this function to LeaseLock. With components we control, we know their rollout cadence and skew support, so we can go endpoints -> endpointsleases -> leases over three versions. With this helper method, we have no idea what the consuming component's release schedule and skew support is.

Yes - going Endpoints->Leases isn't safe.

liggitt · 2020-09-09T14:22:59Z

cmd/cloud-controller-manager/app/options/options.go

@@ -115,6 +116,10 @@ func NewCloudControllerManagerOptions() (*CloudControllerManagerOptions, error)
 // NewDefaultComponentConfig returns cloud-controller manager configuration object.
 func NewDefaultComponentConfig(insecurePort int32) (*ccmconfig.CloudControllerManagerConfiguration, error) {
 	versioned := &ccmconfigv1alpha1.CloudControllerManagerConfiguration{}
+	// Use lease-based leader election to reduce cost.
+	// The default endpoints-leases one has already been used for couple releases.
+	versioned.LeaderElection.ResourceLock = resourcelock.LeasesResourceLock


be specific about which release we switched the default to endpointslease. also, why not change the default in SetDefaults_CloudControllerManagerConfiguration instead of here?

Added a comment about introducing in 1.17 (same below).

Modifying defaulting seems like backward-incompatible (though technically it's probably very unlikely someone will reuse it for something else...).

Should I really change to modifying the defaulting?

liggitt · 2020-09-09T14:23:51Z

cmd/kube-controller-manager/app/options/options.go

@@ -210,6 +211,10 @@ func NewKubeControllerManagerOptions() (*KubeControllerManagerOptions, error) {
 // NewDefaultComponentConfig returns kube-controller manager configuration object.
 func NewDefaultComponentConfig(insecurePort int32) (kubectrlmgrconfig.KubeControllerManagerConfiguration, error) {
 	versioned := kubectrlmgrconfigv1alpha1.KubeControllerManagerConfiguration{}
+	// Use lease-based leader election to reduce cost.
+	// The default endpoints-leases one has already been used for couple releases.
+	versioned.LeaderElection.ResourceLock = resourcelock.LeasesResourceLock


be specific about which release we switched the default to endpointslease. also, why not change the default in SetDefaults_KubeControllerManagerConfiguration instead of here?

liggitt · 2020-09-09T14:24:29Z

cmd/kube-scheduler/app/options/options.go

@@ -138,6 +138,9 @@ func splitHostIntPort(s string) (string, int, error) {
 func newDefaultComponentConfig() (*kubeschedulerconfig.KubeSchedulerConfiguration, error) {
 	versionedCfg := kubeschedulerconfigv1beta1.KubeSchedulerConfiguration{}
 	versionedCfg.DebuggingConfiguration = *configv1alpha1.NewRecommendedDebuggingConfiguration()
+	// Use lease-based leader election to reduce cost.
+	// The default endpoints-leases one has already been used for couple releases.
+	versionedCfg.LeaderElection.ResourceLock = resourcelock.LeasesResourceLock


be specific about which release we switched the default to endpointslease. also, why not change the default in SetDefaults_KubeSchedulerConfiguration instead of here?

wojtek-t · 2020-09-10T11:09:59Z

@liggitt - PTAL

liggitt · 2020-09-10T13:07:56Z

/lgtm

would like an ack from someone on those component's teams:
/cc @ahg-g @cheftako

liggitt · 2020-09-10T13:08:12Z

/approve
/hold for component team acks

k8s-ci-robot · 2020-09-10T13:08:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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:

~~cmd/cloud-controller-manager/OWNERS~~ [liggitt,wojtek-t]
~~cmd/kube-scheduler/OWNERS~~ [liggitt,wojtek-t]
~~pkg/controller/apis/config/OWNERS~~ [liggitt,wojtek-t]
~~pkg/scheduler/apis/config/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/component-base/config/OWNERS~~ [liggitt]

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

ahg-g · 2020-09-10T14:13:36Z

are we making an exception here in the sense that a CC default value is being changed without increasing the version (to v1beta2 in the scheduler case)? Note that we are planning to make some changes to scheduler's CC and introduce v1beta2, so we could make this change in v1beta2 and keep the old behavior for v1beta1.

liggitt · 2020-09-10T14:16:09Z

are we making an exception here in the sense that a CC default value is being changed without increasing the version (to v1beta2 in the scheduler case)? Note that we are planning to make some changes to scheduler's CC and introduce v1beta2, so we could make this change in v1beta2 and keep the old behavior for v1beta1.

that would be fine as well. the controller manager configs aren't exposed as config files yet, so the default change is only affecting the CLI flag defaults

ahg-g · 2020-09-10T14:19:54Z

/lgtm

cheftako · 2020-09-10T18:15:09Z

pkg/controller/apis/config/v1alpha1/defaults.go

@@ -130,7 +130,10 @@ func RecommendedDefaultGenericControllerManagerConfiguration(obj *kubectrlmgrcon
 	}

 	if len(obj.LeaderElection.ResourceLock) == 0 {
-		obj.LeaderElection.ResourceLock = "endpointsleases"
+		// Use lease-based leader election to reduce cost.
+		// We migrated for EndpointsLease lock in 1.17 and starting in 1.20 we


We migrated to EndpointLease lock in 1.17

cheftako · 2020-09-10T18:47:31Z

/lgtm

wojtek-t · 2020-09-11T06:03:50Z

Cancelling hold based on Abdullah and Walter lgtms above.

/hold cancel

Watch events for objects in the kube-system namespace were previously ignored. In certain situations, this would cause the destination service to return invalid (outdated) endpoints for services in kube-system - including unmeshed services. It was suggested [1] that kube-system events were ignored to avoid handling frequent Endpoint updates - specifically from controllers using Endpoints for leader elections [2]. As of Kubernetes 1.20, these controllers default to using Leases instead of Endpoints for their leader elections [3], obviating the need to exclude (or filter) updates from kube-system. The exclusions have been removed accordingly. [1]: linkerd#4133 (comment) [2]: kubernetes/kubernetes#86286 [3]: kubernetes/kubernetes#94603 Signed-off-by: Jacob Henner <code@ventricle.us>

Watch events for objects in the kube-system namespace were previously ignored. In certain situations, this would cause the destination service to return invalid (outdated) endpoints for services in kube-system - including unmeshed services. It [was suggested][1] that kube-system events were ignored to avoid handling frequent Endpoint updates - specifically from [controllers using Endpoints for leader elections][2]. As of Kubernetes 1.20, these controllers [default to using Leases instead of Endpoints for their leader elections][3], obviating the need to exclude (or filter) updates from kube-system. The exclusions have been removed accordingly. [1]: #4133 (comment) [2]: kubernetes/kubernetes#86286 [3]: kubernetes/kubernetes#94603 Signed-off-by: Jacob Henner <code@ventricle.us>

wojtek-t assigned liggitt Sep 8, 2020

k8s-ci-robot requested review from mwielgus and pwittrock September 8, 2020 06:23

wojtek-t commented Sep 8, 2020

View reviewed changes

liggitt reviewed Sep 8, 2020

View reviewed changes

wojtek-t force-pushed the migrate_leader_election_to_leases_todos branch from 5af5011 to f34cb47 Compare September 9, 2020 14:09

wojtek-t changed the title ~~TODOs about migrating to LeaseLock in leaderelection~~ Upgrade scheduler, controller-manager and cloud-controller-manager to use LeaseLock Sep 9, 2020

wojtek-t changed the title ~~Upgrade scheduler, controller-manager and cloud-controller-manager to use LeaseLock~~ Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock Sep 9, 2020

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 Sep 9, 2020

wojtek-t force-pushed the migrate_leader_election_to_leases_todos branch from f34cb47 to 3ddbb04 Compare September 9, 2020 14:22

liggitt reviewed Sep 9, 2020

View reviewed changes

wojtek-t force-pushed the migrate_leader_election_to_leases_todos branch 2 times, most recently from 8940a77 to 0d3216e Compare September 9, 2020 19:10

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 10, 2020

k8s-ci-robot requested review from ahg-g and cheftako September 10, 2020 13:07

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

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

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

k8s-ci-robot assigned ahg-g Sep 10, 2020

cheftako reviewed Sep 10, 2020

View reviewed changes

k8s-ci-robot assigned cheftako Sep 10, 2020

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

wojtek-t added this to the v1.20 milestone Sep 11, 2020

k8s-ci-robot merged commit d39214a into kubernetes:master Sep 11, 2020

github-actions bot mentioned this pull request Sep 15, 2020

Week Ending September 13, 2020 dev-obs/actus#223

Open

liggitt mentioned this pull request Feb 10, 2021

addon-manager uses obsolete leader election mechanism #98966

Closed

ialidzhikov mentioned this pull request Aug 9, 2021

Migrate all uses of leader-election to use Lease API #80289

Open

This was referenced Jun 21, 2022

Remove kube-system exclusions from watchers linkerd/linkerd2#8720

Merged

Exclusion of kube-system in EndpointsWatcher leads to broken DNS resolution linkerd/linkerd2#8721

Closed

csuzhangxc mentioned this pull request Jul 19, 2023

bump K8s to v1.24.15 pingcap/advanced-statefulset#94

Closed

csuzhangxc mentioned this pull request Dec 7, 2023

bump K8s to v1.24.17 pingcap/advanced-statefulset#104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock #94603

Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock #94603

wojtek-t commented Sep 8, 2020 •

edited

Loading

wojtek-t Sep 8, 2020

liggitt Sep 8, 2020

mtaufen Sep 10, 2020

wojtek-t Sep 11, 2020

mtaufen Sep 11, 2020

wojtek-t commented Sep 8, 2020

fejta-bot commented Sep 8, 2020

liggitt Sep 8, 2020

liggitt Sep 8, 2020

liggitt Sep 8, 2020 •

edited

Loading

wojtek-t Sep 8, 2020

wojtek-t Sep 9, 2020

liggitt Sep 9, 2020

wojtek-t Sep 9, 2020

liggitt Sep 9, 2020

liggitt Sep 9, 2020

wojtek-t commented Sep 10, 2020

liggitt commented Sep 10, 2020

liggitt commented Sep 10, 2020

k8s-ci-robot commented Sep 10, 2020

ahg-g commented Sep 10, 2020

liggitt commented Sep 10, 2020

ahg-g commented Sep 10, 2020

cheftako Sep 10, 2020

cheftako commented Sep 10, 2020

wojtek-t commented Sep 11, 2020

Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock #94603

Migrate scheduler, controller-manager and cloud-controller-manager to use LeaseLock #94603

Conversation

wojtek-t commented Sep 8, 2020 • edited Loading

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

Choose a reason for hiding this comment

wojtek-t commented Sep 8, 2020

fejta-bot commented Sep 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Sep 8, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Sep 10, 2020

liggitt commented Sep 10, 2020

liggitt commented Sep 10, 2020

k8s-ci-robot commented Sep 10, 2020

ahg-g commented Sep 10, 2020

liggitt commented Sep 10, 2020

ahg-g commented Sep 10, 2020

Choose a reason for hiding this comment

cheftako commented Sep 10, 2020

wojtek-t commented Sep 11, 2020

wojtek-t commented Sep 8, 2020 •

edited

Loading

liggitt Sep 8, 2020 •

edited

Loading