-
Notifications
You must be signed in to change notification settings - Fork 64
✨ release leader election lease on manager cancellation #1689
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
✨ release leader election lease on manager cancellation #1689
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I added the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1689 +/- ##
==========================================
- Coverage 67.74% 67.74% -0.01%
==========================================
Files 57 57
Lines 4620 4622 +2
==========================================
+ Hits 3130 3131 +1
- Misses 1265 1266 +1
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Ha. The latest release doesn't release the lease. So we'll have to make the changes in main.go's, then release again, then revert the e2e changes. |
Signed-off-by: Joe Lanford <[email protected]>
a008922
to
c83e3ed
Compare
Just for my understanding. The root-cause was something like this:
The proposed fix is to set: |
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.
/lgtm
Correct, and if it is not shut down properly, it likely won't give up its lease because giving up its lease is the last thing it does. The main risk is that we go out of our way to run a separate goroutine that does leader-y things whose lifetime goes beyond that of the manager. If we do that, then the manager will release the lease, some other process could assume leadership, and the our other goroutine and the new leader will be duking it out. I don't think that's a big risk though. |
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.
👍
/lgtm |
Metrics: metricsServerOptions, | ||
HealthProbeBindAddress: probeAddr, | ||
LeaderElection: enableLeaderElection, | ||
LeaderElectionID: "9c4404e7.operatorframework.io", |
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.
Nit: Could we not use operator-controller-lock here as well like we have in catalogd?
Maybe for another PR
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.
I don't think we can do that. Because then old code and new code could both get elected leader under different leader election IDs.
1a52e2e
Description
Fixes #1687
Reviewer Checklist