-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Optimistically update leader lock #122069
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Nov 27 09:58:46 UTC 2023. |
Partly fix #121806 |
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go
Outdated
Show resolved
Hide resolved
Name: "leader_election_slowpath_total", | ||
StabilityLevel: k8smetrics.ALPHA, | ||
Help: "Total number of slow path exercised in renewing leader leases. 'name' is the string used to identify the lease. Please make sure to group by name.", | ||
}, []string{"name"}) |
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.
This has in theory unbounded cardinality - we shouldn't make a name to be a label.
Why do we need a name? If we really need to check the name, we should use logs - for metrics we should focus oncount.
OTOH - I see other metrics above to be labeled with name - isn't that an issue?
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.
What name is this? If it's a per-cluster (or per control-plane node) name it should be practically bounded and we've traditionally been okay with this.
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.
It's "per component taking part in leader election".
So in general yes - it should be bounded [modulo bugs etc.]. So I'm assuming this is fine for you...
staging/src/k8s.io/component-base/metrics/prometheus/clientgo/leaderelection/metrics.go
Show resolved
Hide resolved
// and down. | ||
type SwitchMetric interface { | ||
// LeaderMetric instruments metrics used in leader election. | ||
type LeaderMetric interface { |
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.
Building a bit on @aojea comment from another PR - changing public interfaces in our official client library is always problematic.
Can we leave the SwitchMetric interface as it was and simply add a new LeaderMetric
interface as:
type LeaderMetric interface {
SwitchMetric
SlowPathExercised(name string)
}
I'm also thinking about better naming for the method - but don't have a better suggestion.
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.
OTOH - we're changing below MetricsProvider
interface so maybe that's an overkill.
I wouldn't expect people to generally overwrite provider factory and we're fixing all implementations.
So maybe that's actually fine as is.
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.
FTR, I keep it as it is.
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.
Coming back to this: it is problematic and causes another tight coupling on controller-runtime and client-go which provides its own implementation.
If the original name was kept, even adding the new method would have been more compatible, since it would allow a single consumer to support 1.29 and 1.30. By changing the name, it strictly forces 1.30 usage.
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.
This change breaks the upgrade to 1.30 for every controller built on top of controller-runtime e.g. Flux fluxcd/pkg#763. Hopefully the controller-runtime maintainers will do a release soon that deals with this breaking change.
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.
Working on #124372 to address this. Will cherry pick to 1.30 after merged.
/triage accepted |
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go
Outdated
Show resolved
Hide resolved
/cc @pohly |
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.
The core code LGTM - I added few more comments about the test.
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Show resolved
Hide resolved
Addressed all suggestions and updated |
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/client-go/tools/leaderelection/leaderelection_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Eric Lin <exlin@google.com>
Signed-off-by: Eric Lin <exlin@google.com>
/lgtm |
LGTM label has been added. Git tree hash: c9d315df71af36c8b687a357047a80f42ade1ead
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: linxiulei, wojtek-t 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?