OCPCLOUD-492: Run all machine api Controllers using leader election #122

Danil-Grigorev · 2020-04-15T14:27:14Z

What this PR does / why we need it:
Implemented leader election for azure provider

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

OCPCLOUD-492

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Couple of new cli arguments for configuring leader election

  -leader-elect
    	Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability. (default true)
  -leader-elect-lease-duration int
    	The duration that non-leader candidates will wait after observing a leadership renewal until attempting to acquire leadership of a led but unrenewed leader slot. This is effectively the maximum duration that a leader can be stopped before it is replaced by another candidate. This is only applicable if leader election is enabled. (default 15)
  -leader-elect-resource-namespace string
    	The namespace of resource object that is used for locking during leader election. If unspecified, the controller watches for machine-api objects across all namespaces.

JoelSpeed

Added some comments

JoelSpeed · 2020-04-16T09:39:18Z

cmd/manager/main.go

+	flag.StringVar(
+		&watchNamespace,
+		"namespace",
+		"",
+		"Namespace that the controller watches to reconcile machine-api objects. If unspecified, the controller watches for machine-api objects across all namespaces.",
+	)
+
+	flag.StringVar(
+		&leaderElectResourceNamespace,
+		"leader-elect-resource-namespace",
+		"",
+		"The namespace of resource object that is used for locking during leader election. If unspecified, the controller watches for machine-api objects across all namespaces.",
+	)
+
+	flag.BoolVar(
+		&leaderElect,
+		"leader-elect",
+		true,
+		"Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability.",
+	)
+
+	flag.Int64Var(
+		&leaderElectLeaseDuration,
+		"leader-elect-lease-duration",
+		15,
+		"The duration that non-leader candidates will wait after observing a leadership renewal until attempting to acquire leadership of a led but unrenewed leader slot. This is effectively the maximum duration that a leader can be stopped before it is replaced by another candidate. This is only applicable if leader election is enabled.",
+	)


Any reason not to declare these in the way the watchNamespace was?

JoelSpeed · 2020-04-16T09:39:41Z

cmd/manager/main.go

+	flag.BoolVar(
+		&leaderElect,
+		"leader-elect",
+		true,


Given there's no default namespace, and this would be a breaking change (technically), I think this should default to false

Yes, I saw this happening locally. Will adjust the value.

JoelSpeed · 2020-04-16T09:40:27Z

cmd/manager/main.go

+		&leaderElectResourceNamespace,
+		"leader-elect-resource-namespace",
+		"",
+		"The namespace of resource object that is used for locking during leader election. If unspecified, the controller watches for machine-api objects across all namespaces.",


The second half of this sentence doesn't make any sense in this context, can you verify the behaviour if it is unset? Does controller-runtime allow this to be empty?

JoelSpeed · 2020-04-16T09:41:27Z

cmd/manager/main.go

+		"Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability.",
+	)
+
+	flag.Int64Var(


Could use an actual Duration flag?

JoelSpeed · 2020-04-16T10:45:00Z

cmd/manager/main.go

+
+	leaderElectLeaseDuration := flag.Duration(
+		"leader-elect-lease-duration",
+		15,


Probably still want a default of 15 seconds 😉

Suggested change

15,

15 * time.Second,

JoelSpeed · 2020-04-16T10:47:51Z

cmd/manager/main.go

+	leaderElectResourceNamespace := flag.String(
+		"leader-elect-resource-namespace",
+		"",
+		"The namespace of resource object that is used for locking during leader election. If unspecified, the controller watches for the namespace currently in-use in the cluster",


Suggested change

"The namespace of resource object that is used for locking during leader election. If unspecified, the controller watches for the namespace currently in-use in the cluster",

"The namespace of resource object that is used for locking during leader election. If unspecified and running in cluster, defaults to the service account namespace for the controller. Required for leader-election outside of a cluster.",

JoelSpeed · 2020-04-16T10:49:07Z

cmd/manager/main.go

 	cfg := config.GetConfigOrDie()

-	opts := manager.Options{}
+	leaseDuration := time.Second * *leaderElectLeaseDuration


The duration will already be in seconds if we set the default to seconds, that or the user will specify something on the command line like 10s, 1m, 1h depending on their preference

JoelSpeed · 2020-04-23T10:31:27Z

/approve

openshift-ci-robot · 2020-04-23T10:31:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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:

~~OWNERS~~ [JoelSpeed]

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

Danil-Grigorev · 2020-05-04T11:08:25Z

/retest

alexander-demicev

/lgtm

enxebre · 2020-06-10T10:24:56Z

can we please use imperative mood in the commit instead of past?
We follow https://chris.beams.io/posts/git-commit/#imperative

enxebre · 2020-06-10T10:25:54Z

Is there counter part PRs for other providers and in MAO to use the flags that we can reference in one single place?

openshift-bot · 2020-06-10T11:28:29Z

/retest