feat: make CLE timers configurable #132433
Conversation
|
@michaelasp: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @michaelasp. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/assign @Jefftree |
|
@michaelasp: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
cb55a96 to
4fbdf2f
Compare
|
/ok-to-test |
pkg/controlplane/apiserver/config.go
Outdated
| @@ -102,6 +102,11 @@ type Extra struct { | |||
| SystemNamespaces []string | |||
|
|
|||
| VersionedInformers clientgoinformers.SharedInformerFactory | |||
|
|
|||
| // Coordinated Leader Election timers | |||
| LeaseDuration time.Duration | |||
There was a problem hiding this comment.
nit: We have many other leases in the apiserver that these variable names are probably a bit confusing. Perhaps prepend a CLE or CoordinatedLeaderElection prefix?
There was a problem hiding this comment.
Yeah I wasn't sure what to name them exactly but agreed that this is too generic. I'll add CLE as a prefix to not make the variable name too long.
| @@ -29,12 +29,11 @@ import ( | |||
| "k8s.io/klog/v2" | |||
| ) | |||
|
|
|||
| var ( | |||
| // TODO: Eventually these should be configurable | |||
| LeaseDuration = 15 * time.Second | |||
There was a problem hiding this comment.
These are also defined in a way that can be overridden in integration tests:
https://github.com/kubernetes/kubernetes/blob/master/test/integration/apiserver/coordinatedleaderelection/leaderelection_test.go#L50-L54
I'd imagine these tests will fail at the moment, can you update the references to set these up properly?
There was a problem hiding this comment.
Yep, let me update those.
b0fb019 to
4d3b1e1
Compare
4d3b1e1 to
0550785
Compare
0550785 to
066002c
Compare
| "--cle-lease-duration=10s", | ||
| "--cle-renew-deadline=5s", | ||
| "--cle-retry-period=1s", |
There was a problem hiding this comment.
nit: We typically avoid using acronyms in flag names. Can we find some prefix that's not horribly verbose as an alternative to "cle" here?
There was a problem hiding this comment.
Makes sense, how about coordinated-leadership as the prefix?
fceb8ac to
2ca17a8
Compare
| @@ -202,6 +209,13 @@ func (s *Options) AddFlags(fss *cliflag.NamedFlagSets) { | |||
|
|
|||
| fs.StringVar(&s.ServiceAccountSigningEndpoint, "service-account-signing-endpoint", s.ServiceAccountSigningEndpoint, ""+ | |||
| "Path to socket where a external JWT signer is listening. This flag is mutually exclusive with --service-account-signing-key-file and --service-account-key-file. Requires enabling feature gate (ExternalServiceAccountTokenSigner)") | |||
|
|
|||
| fs.DurationVar(&s.CoordinatedLeadershipLeaseDuration, "coordinated-leadership-lease-duration", s.CoordinatedLeadershipLeaseDuration, | |||
There was a problem hiding this comment.
You could also add validation for these flags under https://github.com/kubernetes/kubernetes/blob/master/pkg/controlplane/apiserver/options/validation.go and have corresponding tests in pkg/controlplane/apiserver/options/validation_test.go if it makes sense
There was a problem hiding this comment.
This is a good point, I think we have certain invariants like that we would need to renew the lease with a lower interval than the lease duration. Let me add those validations.
I don't think we can check whether the feature gate is enabled here though since we have default values and it's difficult to check whether the timers are set or not without the flags. IMO this should be fine since it's all feature gated regardless.
2ca17a8 to
54f8d10
Compare
| defaultLeaseDuration := leaderelection.LeaseDuration | ||
| defaultRenewDeadline := leaderelection.RenewDeadline | ||
| defaultRetryPeriod := leaderelection.RetryPeriod | ||
| defer func() { |
There was a problem hiding this comment.
Love that we can get rid of the defer since this isn't shared anymore :)
|
LGTM label has been added. Git tree hash: 2a74087f548d4994f4f7d3a8f7cd3222de21da24
|
|
/lgtm need to squash commits? |
54f8d10 to
1a59c25
Compare
|
/triage accepted |
|
/assign @jpbetz for final LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jefftree, jpbetz, michaelasp 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 |
|
/retest Unrelated, let me check if a flake is filed for this. #132506 seems to be the same issue, filed last week. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds configurable timers to coordinated leader election, cleaning up TODOs.
Which issue(s) this PR is related to:
Special notes for your reviewer:
Adds flags to apiserver, wasn't entirely sure where to place this, looking for advice here.
Does this PR introduce a user-facing change?