-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: use client-go leader election #1128
Conversation
6ffe025
to
0942f93
Compare
leader/election.go
Outdated
AcquireTime: &now, | ||
RenewTime: &now, | ||
OnStoppedLeading: func() { | ||
updateLeaderLabel(ctx, false) |
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 rely on this to add a label - if the pod is killed or partitions it will never run - The leader should first add its own label, and then remove the label from other others
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 think we should also stop and start all schedules here
a6d8014
to
a3d9785
Compare
leader/election.go
Outdated
app string, | ||
namespace string, | ||
service string, |
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.
Why do we need both app and service ?
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.
app would always be "config-db" which is hardcoded in flanksource/config-db
whereas service was meant to be the actual kubernetes service name.
Although, the we could just use the app name as the lease name.
31e08e1
to
5ff4d64
Compare
leader/election.go
Outdated
AcquireTime: &now, | ||
RenewTime: &now, | ||
OnStoppedLeading: func() { | ||
for _, k := range echo.Crons.Items() { |
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.
don't we also need to start the crons again when we become leader ?
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.
starting the jobs had a few pre-processing before the cron was started.
So the operator in config-db would start those jobs in the onLead
callback.
Although upon closer inspection, I see that's a one time thing.
24f0782
to
ded2120
Compare
b980ffe
to
7d27424
Compare
7d27424
to
9c45330
Compare
No description provided.