add leader election options #133

hanqiuzh · 2021-09-30T21:42:15Z

Updates:

added leader election options, same as CCCMO

Tests:

enable leader election, non leader pods will not start controllers (includes status controller)
enable leader election and delete leader pod, new leader will be elected, and controllers will start
disable leader election, all pods works

hanqiuzh · 2021-10-04T14:00:18Z

/retest-required

enxebre · 2021-10-05T08:44:34Z

/retest

hanqiuzh · 2021-10-05T15:16:14Z

/retest

enxebre · 2021-10-11T11:00:49Z

main.go

+		MetricsBindAddress:      metricsPort,
+		LeaderElectionNamespace: leaderElectionConfig.ResourceNamespace,
+		LeaderElection:          leaderElectionConfig.LeaderElect,
+		LeaseDuration:           &leaderElectionConfig.LeaseDuration.Duration,


Please let's set LeaderElectionResourceLock: resourcelock.LeasesResourceLock
otherwise controller runtime defaults to configmapsleases

enxebre · 2021-10-11T11:01:46Z

manifests/01-rbac.yaml

+  - ""
+  - "coordination.k8s.io"
+  resources:
+  - configmaps


There's no need for additional rbac for configmaps, only leases https://github.com/openshift/cluster-machine-approver/pull/133/files#r726013249

enxebre · 2021-10-11T11:02:24Z

main.go

@@ -56,7 +73,11 @@ func main() {
 	flagSet.StringVar(&machineNamespace, "machine-namespace", "", "restrict machine operations to a specific namespace, if not set, all machines will be observed in approval decisions")
 	flagSet.StringVar(&workloadKubeConfigPath, "workload-cluster-kubeconfig", "", "workload kubeconfig path")

-	flagSet.Parse(os.Args[1:])
+	// Once all the flags are regitered, switch to pflag


not sure I follow this comment, mind elaborating?

sure BindLeaderElectionFlags is expecting a pflag as input variable, so I converted the original flags into pflag type. (same as cccmo)

Isn't this exposing more than we need as a flags, e.g ResourceLock?
If I would just expose the ones we need manually, no need for the library.

enxebre · 2021-10-11T11:03:04Z

what is CCCMO?

enxebre · 2021-10-11T11:03:21Z

main.go

+	// Default leader electrion configuration.
+	leaderElectionConfig = config.LeaderElectionConfiguration{
+		LeaderElect:       true,
+		LeaseDuration:     metav1.Duration{Duration: 137 * time.Second},


why did you choose this values?

copied from this commit of cccmo: openshift/cluster-cloud-controller-manager-operator@445bfe3

elmiko · 2021-10-11T14:52:45Z

what is CCCMO?

https://github.com/openshift/cluster-cloud-controller-manager-operator/ the new operator for managing CCMs on openshift

hanqiuzh · 2021-10-12T12:31:46Z

+1 that CCCMO is cluster-cloud-controller-manager-operator, and please see pr comment, I added a link of the reference commit I used 👍

enxebre · 2021-10-13T11:41:39Z

Thanks! Just one comment #133 (comment)

enxebre · 2021-10-13T15:59:46Z

/lgtm

hanqiuzh · 2021-10-13T17:28:22Z

/test e2e-aws

hanqiuzh · 2021-10-13T18:09:52Z

/test e2e-upgrade

hanqiuzh · 2021-10-13T20:06:10Z

/retest

enxebre · 2021-10-15T09:09:17Z

main.go

@@ -58,6 +66,12 @@ func main() {
 	flagSet.StringVar(&workloadKubeConfigPath, "workload-cluster-kubeconfig", "", "workload kubeconfig path")
 	flagSet.BoolVar(&disableStatusController, "disable-status-controller", false, "disable status controller that will update the machine-approver clusteroperator status")

+	flagSet.BoolVar(&leaderElect, "leader-elect", true, "use leader election when starting the manager.")


do we want it true by default?

JoelSpeed

One quick change, otherwise LGTM

JoelSpeed · 2021-10-15T09:25:25Z

main.go

@@ -90,7 +104,14 @@ func main() {
 	// Create a new Cmd to provide shared dependencies and start components
 	klog.Info("setting up manager")
 	mgr, err := manager.New(workloadConfig, manager.Options{
-		MetricsBindAddress: metricsPort,
+		MetricsBindAddress:         metricsPort,


Please add ReleaseOnCancel: true else this will cause approx 3 minute delays during upgrades

updated with LeaderElectionReleaseOnCancel: true

JoelSpeed · 2021-10-15T09:26:27Z

main.go

+		go func() {
+			<-mgr.Elected()
+			statusController.Run(1, stop)
+		}()


Weird that we run this separately, I'll make a note to look into this

Signed-off-by: Hanqiu Zhang <hanzhang@redhat.com>

JoelSpeed · 2021-10-15T14:43:57Z

/approve

openshift-ci · 2021-10-15T14:44:30Z

[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

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

hanqiuzh · 2021-10-15T15:24:12Z

/retest

hanqiuzh · 2021-10-15T17:47:36Z

/retest

openshift-ci · 2021-10-15T19:57:22Z

@hanqiuzh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-disruptive	`a135d50`	link	false	`/test e2e-aws-disruptive`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

elmiko

thanks @hanqiuzh !
/lgtm

openshift-ci bot requested review from elmiko and michaelgugino September 30, 2021 21:42

hanqiuzh force-pushed the add-leader-election branch 3 times, most recently from 4ad3723 to 4039544 Compare October 5, 2021 01:20

enxebre reviewed Oct 11, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021

hanqiuzh force-pushed the add-leader-election branch from 4039544 to f9b0b6e Compare October 12, 2021 14:55

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2021

hanqiuzh force-pushed the add-leader-election branch from f9b0b6e to b979639 Compare October 13, 2021 15:49

hanqiuzh force-pushed the add-leader-election branch from b979639 to 33128e4 Compare October 13, 2021 15:59

openshift-ci bot assigned enxebre Oct 13, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2021

enxebre reviewed Oct 15, 2021

View reviewed changes

JoelSpeed reviewed Oct 15, 2021

View reviewed changes

add leader election options

a135d50

Signed-off-by: Hanqiu Zhang <hanzhang@redhat.com>

hanqiuzh force-pushed the add-leader-election branch from 33128e4 to a135d50 Compare October 15, 2021 13:15

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2021

elmiko reviewed Oct 18, 2021

View reviewed changes

openshift-ci bot assigned elmiko Oct 18, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2021

openshift-merge-robot merged commit 7b7289a into openshift:master Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add leader election options #133

add leader election options #133

hanqiuzh commented Sep 30, 2021 •

edited

Loading

hanqiuzh commented Oct 4, 2021

enxebre commented Oct 5, 2021

hanqiuzh commented Oct 5, 2021

enxebre Oct 11, 2021

enxebre Oct 11, 2021

enxebre Oct 11, 2021

hanqiuzh Oct 12, 2021 •

edited

Loading

enxebre Oct 13, 2021

enxebre commented Oct 11, 2021

enxebre Oct 11, 2021

hanqiuzh Oct 12, 2021

elmiko commented Oct 11, 2021

hanqiuzh commented Oct 12, 2021 •

edited

Loading

enxebre commented Oct 13, 2021

enxebre commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

enxebre Oct 15, 2021

JoelSpeed left a comment

JoelSpeed Oct 15, 2021

hanqiuzh Oct 15, 2021

JoelSpeed Oct 15, 2021

JoelSpeed commented Oct 15, 2021

openshift-ci bot commented Oct 15, 2021

hanqiuzh commented Oct 15, 2021

hanqiuzh commented Oct 15, 2021

openshift-ci bot commented Oct 15, 2021 •

edited

Loading

elmiko left a comment

add leader election options #133

add leader election options #133

Conversation

hanqiuzh commented Sep 30, 2021 • edited Loading

hanqiuzh commented Oct 4, 2021

enxebre commented Oct 5, 2021

hanqiuzh commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanqiuzh Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmiko commented Oct 11, 2021

hanqiuzh commented Oct 12, 2021 • edited Loading

enxebre commented Oct 13, 2021

enxebre commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

hanqiuzh commented Oct 13, 2021

Choose a reason for hiding this comment

JoelSpeed 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

JoelSpeed commented Oct 15, 2021

openshift-ci bot commented Oct 15, 2021

hanqiuzh commented Oct 15, 2021

hanqiuzh commented Oct 15, 2021

openshift-ci bot commented Oct 15, 2021 • edited Loading

elmiko left a comment

Choose a reason for hiding this comment

hanqiuzh commented Sep 30, 2021 •

edited

Loading

hanqiuzh Oct 12, 2021 •

edited

Loading

hanqiuzh commented Oct 12, 2021 •

edited

Loading

openshift-ci bot commented Oct 15, 2021 •

edited

Loading