Change leader election flags for MAO support #81
Conversation
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
If these all need to be the same across providers and the set of flags is defined by the MAO, is there a package in the MAO code that we could import to get them?
The change looks fine as it is. Maybe we could centralize the definition as a follow-up.
|
/test e2e-metal-ipi |
openshift-ci-robot
added
the
approved
cmd/manager/main.go
Outdated
| leaderElect := flag.Bool( | ||
| "leader-elect", | ||
| false, | ||
| "Start a leader election client and gain leadership before executing the main loop. Enable this when running replicated components for high availability.", |
There was a problem hiding this comment.
Leader election is recommended also for:
- upgrade safety, to ensure only one of the old and new controller is running at a time
- recovery safety, to ensure that when a singleton is on a Node that's disappeared, there's a reasonable guarantee that the old process has stopped trying to be the leader before a new process starts being the leader.
For a controller, if you want to give advice on when it should use leader election, I would just say that leader election is always recommended.
|
@dhellmann I don't think we have it like that, each provider has its own set of flags, they just happen to be similar now with recent additions and restructuring. Only some of them should be similar, but the |
Danil-Grigorev
force-pushed
the
fix-leader-election
branch
from
4d2a35d to
723caff
Compare
So different providers have different flags related to leader elections? |
Using leader election by default will add stronger guarantees than we have today that only one controller is running at a time to protect against edge cases where the deployment replica could be increased or upgrades with permissive maxSurge. Relevant provider PRs: - openshift/cluster-api-provider-gcp#85 - openshift/cluster-api-provider-aws#315 - openshift/cluster-api-provider-azure#122 - openshift/cluster-api-provider-openstack#108 - openshift/cluster-api-provider-baremetal#81 - openshift/cluster-api-provider-ovirt#55 - openshift#571
|
/test e2e-metal-ipi |
|
/lgtm |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Danil-Grigorev, dhellmann, mhrivnak, zaneb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-metal-ipi |
|
I guess things can't really get much worse, let's just land it so it makes it ASAP into new builds. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
Using leader election by default will add stronger guarantees than we have today that only one controller is running at a time to protect against edge cases where the deployment replica could be increased or upgrades with permissive maxSurge. Relevant provider PRs: - openshift/cluster-api-provider-gcp#85 - openshift/cluster-api-provider-aws#315 - openshift/cluster-api-provider-azure#122 - openshift/cluster-api-provider-openstack#108 - openshift/cluster-api-provider-baremetal#81 - openshift/cluster-api-provider-ovirt#55 - openshift#571
✨ Add ChecksumType field in image
7 participants
Fix
leader-electioncli option for machine controller, allowing to eliminate edge cases when multiple controllers could be running. Allows integration with MAO PR: openshift/machine-api-operator#571