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
apiserver identity: use persistent names for lease objects #113307
apiserver identity: use persistent names for lease objects #113307
Conversation
|
||
// NewControllerWithLeaseName is a copy of NewController but accepts a leaseName parameter. | ||
// Use this constructor in cases when the lease name and holder identity should be different. | ||
func NewControllerWithLeaseName(clock clock.Clock, client clientset.Interface, holderIdentity string, leaseDurationSeconds int32, onRepeatedHeartbeatFailure func(), renewInterval time.Duration, leaseName, leaseNamespace string, newLeasePostProcessFunc ProcessLeaseFunc) Controller { |
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.
Alternative approach: add a WithLeaseName()
method to the controller.
Don't feel too strongly and fine with either approach, both would prevent breaking compatibility for this package
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 like breaking it
/triage accepted |
@@ -80,6 +81,28 @@ func NewController(clock clock.Clock, client clientset.Interface, holderIdentity | |||
client: client, | |||
leaseClient: leaseClient, | |||
holderIdentity: holderIdentity, | |||
leaseName: holderIdentity, |
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.
Can this NewControllerWithLeaseName
to keep the paths unified?
59e29f2
to
ba1bcae
Compare
/lgtm the separation of leaseName and holderIdentity is an important distinction. Without it we were pushed towards some sub-par options in the kube-apiserver usage pattern. |
/assign @wojtek-t |
/assign @lavalamp |
Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
…e new naming format and hostname label Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
…upport cases when the lease name and holder identity differ Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
…entity Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
…eter, remove NewControllerWithLeaseName Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
ba1bcae
to
72f2e1c
Compare
/lgtm Thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, deads2k, 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 |
/retest |
1 similar comment
/retest |
klog.Fatalf("error getting hostname for apiserver identity: %v", err) | ||
} | ||
|
||
h := fnv.New32a() |
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 would prefer a real hash function, this is only called at startup, there's no performance problem. Can you switch to SHA256? It is OK to truncate the result a bit, but 4 bytes is too small.
If we assume there are 1M k8s clusters, each with 3 apiservers, there is a 1000000*(1-e^(-(3^2)/(2^32))) = .2%
one of them will have a collision. That's far too high, I would like a chance less than 1e-9. (google "birthday paradox estimator" if you want to check my math)
6 bytes is enough to pass that test, so I would go to 8 bytes selected from SHA256, where there is no question that it is very random.
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.
(and sorry I wasn't able to review this until now)
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.
(oh and if you want you can use the base58 encoder to save some characters)
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 bother truncating at all?
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.
Opened #113649 -- tests seem fine without needing to truncate, but still might be worthwhile just to avoid lease names that are really long. I don't feel too strongly with the lease name being that long but myabe some people do?
Aside: this should have been squashed before merge. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR updates the naming format for kube-apiserver Leases to use a format that persists even after a restart. The new format uses a hash of the hostname, so only apiservers using different hostnames will have unique identities. The holder identity of each Lease remains unique per start-up. This reduces system churn when lease objects are garbage collected, but still allows operators to identify when lease ownership is churning.
To support this change, the
NewController
constructor was updated for the lease controller to allow for overriding the lease name when it differs from the holder identity.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: