apiserver identity: use persistent names for lease objects #113307

andrewsykim · 2022-10-24T16:12:12Z

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?

Update the Lease identity naming format for the APIServerIdentity feature to use a persistent name

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

andrewsykim · 2022-10-24T19:30:12Z

staging/src/k8s.io/component-helpers/apimachinery/lease/controller.go

+
+// 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 {


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

I like breaking it

leilajal · 2022-10-27T16:48:27Z

/triage accepted

deads2k · 2022-10-31T20:44:46Z

staging/src/k8s.io/component-helpers/apimachinery/lease/controller.go

@@ -80,6 +81,28 @@ func NewController(clock clock.Clock, client clientset.Interface, holderIdentity
 		client:                     client,
 		leaseClient:                leaseClient,
 		holderIdentity:             holderIdentity,
+		leaseName:                  holderIdentity,


Can this NewControllerWithLeaseName to keep the paths unified?

deads2k · 2022-11-01T16:52:03Z

/lgtm
/approve

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.

andrewsykim · 2022-11-01T17:01:34Z

/assign @wojtek-t

andrewsykim · 2022-11-02T16:00:14Z

/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>

wojtek-t · 2022-11-04T08:21:35Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2022-11-04T08:21:58Z

[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:

~~pkg/controlplane/OWNERS~~ [deads2k,wojtek-t]
~~pkg/kubelet/OWNERS~~ [wojtek-t]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,wojtek-t]
~~staging/src/k8s.io/component-helpers/apimachinery/OWNERS~~ [deads2k,wojtek-t]
~~test/OWNERS~~ [andrewsykim,deads2k,wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewsykim · 2022-11-04T13:12:27Z

/retest

andrewsykim · 2022-11-04T13:17:30Z

/retest

lavalamp · 2022-11-04T21:33:12Z

staging/src/k8s.io/apiserver/pkg/server/config.go

+			klog.Fatalf("error getting hostname for apiserver identity: %v", err)
+		}
+
+		h := fnv.New32a()


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.

(and sorry I wasn't able to review this until now)

(oh and if you want you can use the base58 encoder to save some characters)

Why bother truncating at all?

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?

enj · 2022-11-05T00:34:23Z

Aside: this should have been squashed before merge.

k8s-ci-robot requested review from caesarxuchao and dchen1107 October 24, 2022 16:12

andrewsykim commented Oct 24, 2022

View reviewed changes

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 27, 2022

andrewsykim mentioned this pull request Oct 31, 2022

KEP-1965: update apiserver lease identity format kubernetes/enhancements#3635

Merged

deads2k reviewed Oct 31, 2022

View reviewed changes

andrewsykim force-pushed the apiserver-identity-hostname branch from 59e29f2 to ba1bcae Compare November 1, 2022 16:25

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Nov 1, 2022

k8s-ci-robot assigned deads2k Nov 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2022

k8s-ci-robot assigned wojtek-t Nov 1, 2022

k8s-ci-robot assigned lavalamp Nov 2, 2022

andrewsykim added 5 commits November 3, 2022 22:53

apiserver identity: use persistent identity format based on hostname

2150790

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

apiserver identity: update Lease creation integration test to validat…

f74f819

…e new naming format and hostname label Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

lease controller: add new constructor NewControllerWithLeaseName to s…

6925bee

…upport cases when the lease name and holder identity differ Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

apiserver identity: always use a unique value for the Lease holder id…

3c0b75f

…entity Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

lease controller: update NewController to accept leaseName as a param…

72f2e1c

…eter, remove NewControllerWithLeaseName Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the apiserver-identity-hostname branch from ba1bcae to 72f2e1c Compare November 4, 2022 04:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2022

k8s-ci-robot merged commit c8a3657 into kubernetes:master Nov 4, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022

andrewsykim mentioned this pull request Nov 4, 2022

Promote APIServerIdentity to Beta #113629

Merged

4 tasks

lavalamp reviewed Nov 4, 2022

View reviewed changes

andrewsykim mentioned this pull request Nov 5, 2022

apiserver identity : use SHA256 hash in lease names #113649

Merged

RainbowMango mentioned this pull request Jan 29, 2023

Update Kubenetes dependencies to v1.26.1 karmada-io/karmada#3080

Merged

roycaihw mentioned this pull request Jul 6, 2023

Add random suffix to apiserver id #118958

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apiserver identity: use persistent names for lease objects #113307

apiserver identity: use persistent names for lease objects #113307

andrewsykim commented Oct 24, 2022 •

edited

andrewsykim Oct 24, 2022 •

edited

deads2k Oct 31, 2022

leilajal commented Oct 27, 2022

deads2k Oct 31, 2022

deads2k commented Nov 1, 2022

andrewsykim commented Nov 1, 2022

andrewsykim commented Nov 2, 2022

wojtek-t commented Nov 4, 2022

k8s-ci-robot commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

lavalamp Nov 4, 2022

lavalamp Nov 4, 2022

lavalamp Nov 4, 2022

enj Nov 5, 2022

andrewsykim Nov 5, 2022

enj commented Nov 5, 2022

apiserver identity: use persistent names for lease objects #113307

apiserver identity: use persistent names for lease objects #113307

Conversation

andrewsykim commented Oct 24, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

andrewsykim Oct 24, 2022 • edited

Choose a reason for hiding this comment

deads2k Oct 31, 2022

Choose a reason for hiding this comment

leilajal commented Oct 27, 2022

deads2k Oct 31, 2022

Choose a reason for hiding this comment

deads2k commented Nov 1, 2022

andrewsykim commented Nov 1, 2022

andrewsykim commented Nov 2, 2022

wojtek-t commented Nov 4, 2022

k8s-ci-robot commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

andrewsykim commented Nov 4, 2022

lavalamp Nov 4, 2022

Choose a reason for hiding this comment

lavalamp Nov 4, 2022

Choose a reason for hiding this comment

lavalamp Nov 4, 2022

Choose a reason for hiding this comment

enj Nov 5, 2022

Choose a reason for hiding this comment

andrewsykim Nov 5, 2022

Choose a reason for hiding this comment

enj commented Nov 5, 2022

andrewsykim commented Oct 24, 2022 •

edited

andrewsykim Oct 24, 2022 •

edited