-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(proxy) give manager leader control over start #2053
Conversation
f33e6ff
to
de930e8
Compare
3da045e
to
60f8a12
Compare
Licenses differ between commit b53ddc4 and base:
|
60f8a12
to
8c2b7d4
Compare
ceba834
to
0bdd09f
Compare
0bdd09f
to
2304e30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! I have a few suggestions and comments I would like to see resolved before we merge but mostly LGTM.
7a27005
to
fa13ceb
Compare
fa13ceb
to
a71c15d
Compare
Rather than starting the proxy update loop immediately after setting it up, register the proxy with the manager and let the manager choose whether or not to start the update loop based on whether it is the leader, or leader election is disabled.
Add a Postgres-backed test with multiple replicas. The test initially starts one replica and scales to two. It checks that the second replica is not acting as a leader. It then destroys the original leader and confirms that the second replica becomes the leader. Add a utility function to port-forward to a Pod.
Refactor the status updater to provide a Kubebuilder LeaderElectionRunnable. Run the status updater via manager.Add() so that it only runs if the instance is a leader.
Add an --election-namespace flag. This sets the leader election namespace when an instance is running outside of the cluster. This is necessary to run integration tests.
Deprecate the --leader-elect flag. It is no longer used. Leader election is enabled or disabled based on whether the instance is running in DB-less mode or not.
Rebased to roll PR changes into appropriate commits, since we don't want to squash this/will need to cherry-pick pieces into a 2.0.x backport. |
What this PR does / why we need it:
manager.Add()
. These subsystems now only run when an instance is a leader. This fixes the part of Leader election defaults and implementation do not result in expected behavior #2052 where enabling leader election would actually attempt to push a null configuration because the proxy update loop was running, but the controllers were not. While the status updater caused no issues (it will simply do nothing when given an empty configuration), running multiple instances of it was pointless.--elect-leader
flag in favor of determining whether to elect a leader automatically based on the database mode (DB-less does not use leaders, database-backed does). This is a minor breaking change (the controller will reject the removed flag if present). We could optionally leave it as a deprecated no-op flag if we don't want to make the breaking change immediately.--election-namespace
flag. This is necessary to run integration tests when leader election is enabled, as the controller has no way of determining the election namespace when run outside a cluster. It is unlikely to have any production use.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #2052Special notes for your reviewer:
--election-namespace
flag. We could remove that and the automatic mode detection, but in that case we simply don't test leader election mode in the backport. This is maybe okay since the flag is only needed for test instances (unless you run leader-election-enabled instances outside the cluster in prod for some reason, but you can't do that with current 2.0.x anyway).PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR