Make leader election lease duration configurable through manager options #412

vikaschoudhary16 · 2019-04-29T10:02:16Z

No description provided.

sttts · 2019-04-30T07:57:33Z

pkg/leaderelection/leader_election.go

+
+	// LeaseDurationSeconds is the duration that non-leader candidates will
+	// wait to force acquire leadership.
+	LeaseDurationSeconds int


any reason this is not time.Duration?

sttts · 2019-04-30T10:18:11Z

pkg/manager/internal.go

+
+	// leaseDurationSeconds is the duration that non-leader candidates will
+	// wait to force acquire leadership.
+	leaseDurationSeconds *time.Duration


would prefer to do the defaulting during contruction and have non-pointers here.

sttts · 2019-04-30T10:18:34Z

pkg/manager/manager.go

+	// LeaseDuration is the duration that non-leader candidates will
+	// wait to force acquire leadership. This is measured against time of
+	// last observed ack.
+	LeaseDurationSeconds *time.Duration


why are these pointers?

Trying to keep consistent with already existing syncPeriod, https://github.com/kubernetes-sigs/controller-runtime/pull/412/files#diff-7d903b129bc883285ffc5068c39c00b0R102

Please document the defaults.

sttts · 2019-04-30T11:08:05Z

pkg/manager/manager.go

@@ -231,22 +242,42 @@ func New(config *rest.Config, options Options) (Manager, error) {

 	stop := make(chan struct{})

+	var leaseDuration, renewDeadline, retryPeriod time.Duration


`leastDuration, renewDeadline, retryPeriod := defaultLeastDuration, ...``
Then if clauses without else. Makes it much easier to read.

sttts · 2019-04-30T11:29:13Z

pkg/manager/internal.go

+
+	// leaseDurationSeconds is the duration that non-leader candidates will
+	// wait to force acquire leadership.
+	leaseDurationSeconds time.Duration


drop the seconds suffix, and the * time.Second below. Durations have units already.

sttts · 2019-04-30T11:30:37Z

pkg/manager/internal.go

@@ -40,6 +40,13 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )

+const (
+	// Values taken from: https://github.com/kubernetes/apiserver/blob/master/pkg/apis/config/v1alpha1/defaults.go
+	defaultLeaseDuration = 15


make these durations (* time.Second)

sttts · 2019-04-30T12:04:35Z

/assign @droot @DirectXMan12

For approval.

/lgtm

shawn-hurley · 2019-04-30T13:03:28Z

I think we need to turn off cyclomatic complexity for the New function for this to pass CI.

Personally, I think that makes sense as the new function is still very readable and I don't think it makes sense to make another function just for this.

shawn-hurley · 2019-05-06T12:11:04Z

example_test.go

@@ -37,7 +37,11 @@ import (
 func Example() {
 	var log = controllers.Log.WithName("builder-examples")

-	manager, err := controllers.NewManager(controllers.GetConfigOrDie(), controllers.Options{})
+	manager, err := controllers.NewManager(controllers.GetConfigOrDie(), controllers.Options{


Can you instead create a new example to show how to change leader election lease duration?

vikaschoudhary16 · 2019-05-06T17:11:18Z

/retest

shawn-hurley · 2019-05-06T21:06:36Z

/lgtm

DirectXMan12

minor comments inline, otherwise seems reasonable

I'm slightly worried about the increase of options in the main controller-manager struct. We might want to consider folding out leader election into its own struct, but I'm not sure we're there yet, and that the ergonomics are worth it.

what do other people thing @droot @shawn-hurley @coderanger

DirectXMan12 · 2019-05-06T21:08:29Z

pkg/manager/manager.go

@@ -173,6 +184,7 @@ func (r RunnableFunc) Start(s <-chan struct{}) error {
 }

 // New returns a new Manager for creating Controllers.
+// nolint: gocyclo


please don't just turn off the lints because they're annoying. This seems to suggest that perhaps leader election setup should be in a separate helper function.

DirectXMan12 · 2019-05-06T21:53:36Z

pkg/manager/manager.go

+	}
+	if options.RetryPeriod == nil {
+		retryPeriod = defaultRetryPeriod
+	}


this should be in setOptionsDefaults, as per everything else that's a default

vikaschoudhary16 · 2019-05-14T13:05:39Z

/retest

vikaschoudhary16 · 2019-05-14T13:19:15Z

/retest

vikaschoudhary16 · 2019-05-14T13:24:44Z

/test all

vikaschoudhary16 · 2019-05-14T14:13:13Z

ping @DirectXMan12 @sttts @shawn-hurley .

droot

Looks good to me. minor nit.

droot · 2019-05-16T20:34:39Z

pkg/manager/manager.go

@@ -302,6 +316,18 @@ func setOptionsDefaults(options Options) Options {
 	if options.newMetricsListener == nil {
 		options.newMetricsListener = metrics.NewListener
 	}
+	leaseDuration, renewDeadline, retryPeriod := defaultLeaseDuration, defaultRenewDeadline, defaultRetryPeriod


We should pull the options.newResourceLock = leaderelection.NewResourceLock code fragment above closer to this block so that all leader election bits are in same place.

droot · 2019-05-16T20:35:54Z

I'm slightly worried about the increase of options in the main controller-manager struct. We might want to consider folding out leader election into its own struct, but I'm not sure we're there yet, and that the ergonomics are worth it.

@DirectXMan12 I agree we are not there yet.

DirectXMan12 · 2019-05-16T20:37:45Z

/lgtm
/approve

k8s-ci-robot · 2019-05-16T20:37:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, vikaschoudhary16

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:

~~OWNERS~~ [DirectXMan12]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 29, 2019

k8s-ci-robot requested review from droot and pwittrock April 29, 2019 10:02

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 29, 2019

vikaschoudhary16 mentioned this pull request Apr 29, 2019

Extend ResourceLock interface to include leader election config kubernetes/kubernetes#77204

Closed

sttts reviewed Apr 30, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from a9e062c to e7a24d7 Compare April 30, 2019 09:55

sttts reviewed Apr 30, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from e7a24d7 to abbfe3e Compare April 30, 2019 10:41

sttts reviewed Apr 30, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch 2 times, most recently from 8abca02 to bac717e Compare April 30, 2019 11:22

sttts reviewed Apr 30, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch 2 times, most recently from 40076ec to 982c34a Compare April 30, 2019 12:01

k8s-ci-robot assigned DirectXMan12, sttts and droot Apr 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from 982c34a to 64cbce7 Compare May 6, 2019 11:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2019

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from 64cbce7 to 1c6d891 Compare May 6, 2019 12:07

shawn-hurley reviewed May 6, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch 3 times, most recently from a50eae8 to 2560635 Compare May 6, 2019 14:25

k8s-ci-robot assigned shawn-hurley May 6, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2019

DirectXMan12 suggested changes May 6, 2019

View reviewed changes

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from 2560635 to 7205f87 Compare May 14, 2019 12:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2019

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from 7205f87 to 839f168 Compare May 14, 2019 12:44

Make leader election lease duration configurable through manager options

b8b7071

vikaschoudhary16 force-pushed the le-leaseduration-configurable branch from 839f168 to b8b7071 Compare May 14, 2019 12:51

droot reviewed May 16, 2019

View reviewed changes

DirectXMan12 approved these changes May 16, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2019

k8s-ci-robot merged commit f39791e into kubernetes-sigs:master May 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make leader election lease duration configurable through manager options #412

Make leader election lease duration configurable through manager options #412

vikaschoudhary16 commented Apr 29, 2019 •

edited

Loading

sttts Apr 30, 2019

sttts Apr 30, 2019

sttts Apr 30, 2019

vikaschoudhary16 Apr 30, 2019

sttts Apr 30, 2019

sttts Apr 30, 2019

sttts Apr 30, 2019 •

edited

Loading

sttts Apr 30, 2019

sttts commented Apr 30, 2019

shawn-hurley commented Apr 30, 2019

shawn-hurley May 6, 2019

vikaschoudhary16 commented May 6, 2019

shawn-hurley commented May 6, 2019

DirectXMan12 left a comment

DirectXMan12 May 6, 2019

DirectXMan12 May 6, 2019

DirectXMan12 May 6, 2019

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

droot left a comment

droot May 16, 2019

droot commented May 16, 2019

DirectXMan12 commented May 16, 2019

k8s-ci-robot commented May 16, 2019

		@@ -231,22 +242,42 @@ func New(config *rest.Config, options Options) (Manager, error) {

		stop := make(chan struct{})

		var leaseDuration, renewDeadline, retryPeriod time.Duration

Make leader election lease duration configurable through manager options #412

Make leader election lease duration configurable through manager options #412

Conversation

vikaschoudhary16 commented Apr 29, 2019 • 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

sttts Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Apr 30, 2019

shawn-hurley commented Apr 30, 2019

Choose a reason for hiding this comment

vikaschoudhary16 commented May 6, 2019

shawn-hurley commented May 6, 2019

DirectXMan12 left a 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

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

vikaschoudhary16 commented May 14, 2019

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented May 16, 2019

DirectXMan12 commented May 16, 2019

k8s-ci-robot commented May 16, 2019

vikaschoudhary16 commented Apr 29, 2019 •

edited

Loading

sttts Apr 30, 2019 •

edited

Loading