KEP-1965: update with Beta criteria/milestone and PRR questions answered #3589

andrewsykim · 2022-10-05T20:40:49Z

Signed-off-by: Andrew Sy Kim [email protected]
Co-authored-by: Monis Khan [email protected]

One-line PR description:

Update KEP-1965 to include all Beta criterias and answer remaining PRR questions.

Issue link:

#1965

Other comments:

wojtek-t

I'm fairly interested in this graduating to Beta too.

wojtek-t · 2022-10-06T08:32:32Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

@@ -95,7 +120,7 @@ will only delay the storage migration for the same period of time.

 ## Design Details

-The [kubelet heartbeat](https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/0009-node-heartbeat.md)
+The [kubelet heartbeat](https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/589-efficient-node-heartbeats)


L128: This was slightly misleading to me as it was suggesting the main GC controller in kcm.
In fact, there is a separate controller inside kcm that is doing exactly that:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/storageversiongc/gc_controller.go

[and then it makes much more sense to use hardcoded namespaces, etc.]

My current understanding is that GC controller in KCM removes stales Leases, but a post start hook in kube-apiserver starts the lease heart beat: https://github.com/kubernetes/kubernetes/blob/725993afce11082b973901d2e92553640771d55d/pkg/controlplane/instance.go#L466-L483

Oh I see what you're saying, let me reword this a little so it's clearer

In the code it's called storageversiongc:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/storageversiongc/gc_controller.go#L17

maybe let's make it clear too?

Updated, thanks

wojtek-t · 2022-10-06T08:34:07Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+
+- [X] Feature gate (also fill in values in `kep.yaml`)
+  - Feature gate name: APIServerIdentity
+  - Components depending on the feature gate: kube-apiserver


Should this also be kube-controller-manager?

I guess the lease-gc controller is also gated on this feature?

Thanks, updated

wojtek-t · 2022-10-06T08:34:49Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

-* **Does enabling the feature change any default behavior?**
-  A namespace "kube-apiserver-lease" will be used to store kube-apiserver
-  identity Leases.
+A namespace "kube-apiserver-lease" will be used to store kube-apiserver identity Leases.


[This namespace will also be created, and old leases will be GC-ed].

wojtek-t · 2022-10-06T08:36:12Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+
+###### Are there any tests for feature enablement/disablement?
+
+Yes, see [apiserver_identity_test.go](https://github.com/kubernetes/kubernetes/blob/24238425492227fdbb55c687fd4e94c8b58c1ee3/test/integration/controlplane/apiserver_identity_test.go).


I would say that these test the feature itself (they both are explicitly enabling the feature).

I think we don't have tests that change the FG in the middle of the test (which effectively is the enablement/disablement test).

Good point -- I added a note saying we should have this done before Beta

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

wojtek-t · 2022-10-06T08:38:58Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?
+
+A rough SLO here is that kube-apiserver updates leases at the same frequency as kubelet node heart beats,
+since the same mechanism is being used.


But the configured frequency period might be different.

I would rather go towards something like "healthy kube-apiservers has a lease which is not older than 2*frequency XX% of time".

wojtek-t · 2022-10-06T08:42:06Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+
+###### Are there any missing metrics that would be useful to have to improve observability of this feature?
+
+Yes, heart beat latency could be useful.


LastUpdateTime of my own lease object.
But that has a potential cardinality problem, given the lease is changed on every kube-apiserver restart...

We could consider something like "oldest lease", but given we leave old leases for a longer time (1h), that's not particularly useful...

So I'm not entirely sure how we could construct such a metric here...

Yeah, my understanding for this section was "metrics that would be nice but we wouldn't added due to cardinality issues, etc"

Updated so it's clear we're not actually going to add this metric

wojtek-t · 2022-10-06T08:42:48Z

keps/prod-readiness/sig-api-machinery/1965.yaml

@@ -0,0 +1,3 @@
+kep-number: 1965
+beta:
+  approver: "@deads2k"


I'm happy to take this too (I already added some comments below).

Does the format take multiple PRR approvers, maybe comma separted?

wojtek-t

Just one minor comment - other than that it LGTM.

wojtek-t · 2022-10-06T10:08:28Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

@@ -95,7 +120,7 @@ will only delay the storage migration for the same period of time.

 ## Design Details

-The [kubelet heartbeat](https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/0009-node-heartbeat.md)
+The [kubelet heartbeat](https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/589-efficient-node-heartbeats)


In the code it's called storageversiongc:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/storageversiongc/gc_controller.go#L17

maybe let's make it clear too?

wojtek-t · 2022-10-06T10:09:08Z

keps/prod-readiness/sig-api-machinery/1965.yaml

@@ -0,0 +1,3 @@
+kep-number: 1965
+beta:
+  approver: "@deads2k" # and @wojtek-t


I guess David will be approving from SIG level anyway, so you can also switch to me.
But I don't have any strong opinion (I could probably approve PRR before he wakes up then).

wojtek-t · 2022-10-06T12:00:34Z

/approve PRR :)

deads2k · 2022-10-06T16:19:23Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

-cache. On processing an item, the controller will delete the Lease if the last
-`renewTime` was more than `leaseDurationSeconds` ago (default to 1h). The
-default `leaseDurationSeconds` is chosen to be way longer than the default
+Each kube-apiserver will run a lease controller in a post-start-hook to refresh


Leases have a thing called graceful release in the standard client-go code. For beta, can we leverage this mechanism so that a server in a controlled shutdown can remove itself? Do we not want graceful release for cases where we expect to be restarted and desire our indicated state of the world to persist over the gap?

I think I've settled on wanting the apiserver identity to persist over the gap because we'd generally expect consistency of the identity on a host or member over time.

@lavalamp make sense to you too?

I think I've settled on wanting the apiserver identity to persist over the gap because we'd generally expect consistency of the identity on a host or member over time.

So to clarify, we're saying the holder identity should not change over a restart? i.e. remove the PID from the ID?

Turns out the actual ID being used is just kube-apiserver-<uuid> (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/config.go#L326). We'll have to revisit that during implementation.

If we want identity to persist then we'd need to make a bunch of changes like not put the PID in the identity -- we put that in there in case you run two apiservers on the same system.

I don't think I want people to get attached to specific apiservers (don't make them pets) but it is handy to have a more durable name so you can find the thing or notice if one is consistently misbehaving. I'd personally keep this lease ephemeral but consider adding a nickname to help with readability / persistence over time.

Maybe a kubernetes.io/hostname label on the Lease might be sufficient?

I see value in having the lease naming mirror what already exists for kube-controller-manager and kube-scheduler for consistency.

Will definitely give this more thought during implementation.

deads2k · 2022-10-06T16:19:44Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+
+Proposed e2e tests:
+- an e2e test that validates the existence of the Lease objects per kube-apiserver
+- an e2e test that restarts a kube-apiserver and validates that a new Lease is created


not sure how a standard e2e test could do this.

not sure how a standard e2e test could do this.

Oh, possibly an upgrade test could since at the end of the test you could compare against the starting state.

Thinking of re-using something similar to RestartControllerManager, but for kube-apiserver

I'm willing to do either or both.

deads2k · 2022-10-06T16:22:38Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+
+###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?
+
+A rough SLO here is that healthy kube-apiservers has a lease which is not older than 2 times the frequency of


how would a cluster-admin know whether this has been achieved? Perhaps its more practical to know that the number of leases in kube-apiserver-lease equals the number of kube-apiservers expected 95% of the time.

how would a cluster-admin know whether this has been achieved?

They could check the Lease's spec.renewTime, but I guess there would be no way of knowing if it was under some threshold 95% of the time. Let me add this as another potential SLO

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

Signed-off-by: Andrew Sy Kim <[email protected]> Co-authored-by: Monis Khan <[email protected]>

Signed-off-by: Andrew Sy Kim <[email protected]>

…unning in kube-controller-manager Signed-off-by: Andrew Sy Kim <[email protected]>

Signed-off-by: Andrew Sy Kim <[email protected]>

deads2k · 2022-10-06T17:11:11Z

/lgtm
/approve
/hold

holding for the afternoon for @lavalamp to have a look

k8s-ci-robot · 2022-10-06T17:11:20Z

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

~~keps/prod-readiness/OWNERS~~ [deads2k,wojtek-t]
~~keps/sig-api-machinery/OWNERS~~ [deads2k]

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

lavalamp · 2022-10-06T18:12:37Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

-garbage collect expired Leases.
+will be stored in a special namespace `kube-apiserver-lease`. The Lease creation
+and heart beat will be managed by a controller that is started in kube-apiserver's
+post startup hook. A separate controller in kube-controller-manager will be responsible


I'm not sure I agree with putting that in controller manager, apiserver gets updated first and the new version of apiserver needs to get to healthy without assistance from controller-manager.

The controller in kube-controller-manager is only for garbage collecting stale leases (https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/storageversiongc/gc_controller.go#L17), so I don't think it should impact health of newly started apiservers, but I have not fully tested this myself.

If it's not running at all, leases will build up over time and be confusing at best and affect the performance of consumers at worst, right? It feels like bad karma but I guess I can't think of anything disastrous...

lavalamp · 2022-10-06T18:20:26Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

 refresh period, to tolerate clock skew and/or accidental refresh failure. The
 default resync period is 1h. By default, assuming negligible clock skew, a Lease
 will be deleted if the kube-apiserver fails to refresh its Lease for one to two
-hours. The GC controller will run in kube-controller-manager, to leverage leader
+hours. The `storageversiongc` controller will run in kube-controller-manager, to leverage leader


I think this needs to go in apiserver at the very least for the first release in which we turn this on by default. I don't like having the cleanup routine in a different place than the writing routine in general, either.

I think the main reason for running it in kube-controller-manager is to leverage leader election there. If we move this to kube-apiserver, wouldn't we need to wire in leader election there? (maybe that's fine, but seems like a valid reason to avoid it in the first place)

Since this controller can work purely off of external observed state, it seems like a good candidate to run in KCM?

It's fine if every apiserver runs the cleanup loop, for things that take an hour to expire it shouldn't hurt to race to delete them.

... I guess I don't care THAT much. Last argument: people forking & reusing apiserver (which I strongly discourage) but not controller manager will get a surprise

It's fine if every apiserver runs the cleanup loop, for things that take an hour to expire it shouldn't hurt to race to delete them.

I suppose this is true. My natural inclination is still kube-controller-manager, but I probably won't fight too hard since active-active for this purpose is easy to reason about.

lavalamp · 2022-10-06T18:26:14Z

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

+### Non-Goals
+
+* improving the availability of kube-apiserver
+
 ## Proposal

 We will use “hostname+PID+random suffix (e.g. 6 base58 digits)” as the ID.


Are all e.g. cloud providers actually OK with letting the hostname of systems running apiserver be known?

My first idea: just use a UUID. Also add a "nickname" (label on the lease object?), something persisted to disk that is the same across restarts of the "same" apiserver, to help humans understand continuity.

Are all e.g. cloud providers actually OK with letting the hostname of systems running apiserver be known?

I think most cloud providers work around this by just overriding the hostname or using an opaque value that doesn't reveal anything about the system. We also use hostnames for kube-controller-manager/kube-scheduler leases
already right?

My first idea: just use a UUID. Also add a "nickname" (label on the lease object?), something persisted to disk that is the same across restarts of the "same" apiserver, to help humans understand continuity.

Fwiw, seems like we are already using UUID: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/config.go#L326, so hostname for apiserver identity specifically doesn't seem like an issue in the current form. I like the label idea but will give it more thought.

My first idea: just use a UUID. Also add a "nickname" (label on the lease object?), something persisted to disk that is the same across restarts of the "same" apiserver, to help humans understand continuity.

I'm ok with this as long as its also easy for someone to run as a pod in the cluster. I'm also supportive of hashing whatever name we choose

…e naming, persistence policy and components Signed-off-by: Andrew Sy Kim <[email protected]>

lavalamp · 2022-10-06T20:44:10Z

/lgtm

lavalamp · 2022-10-06T20:49:06Z

/hold cancel

k8s-ci-robot requested review from apelisse and jpbetz October 5, 2022 20:40

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2022

andrewsykim force-pushed the kep-1965 branch 2 times, most recently from 42968e1 to 1560bab Compare October 5, 2022 20:58

andrewsykim changed the title ~~[WIP] KEP-1965: update with Beta criteria/milestone and PRR questions answered~~ KEP-1965: update with Beta criteria/milestone and PRR questions answered Oct 5, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2022

andrewsykim force-pushed the kep-1965 branch 3 times, most recently from b3b8d0e to 4cbc197 Compare October 6, 2022 01:23

rhockenbury mentioned this pull request Oct 6, 2022

kube-apiserver identity #1965

Open

wojtek-t reviewed Oct 6, 2022

View reviewed changes

wojtek-t assigned lavalamp and deads2k Oct 6, 2022

andrewsykim force-pushed the kep-1965 branch 4 times, most recently from 1a94d52 to 41547e0 Compare October 6, 2022 09:54

wojtek-t reviewed Oct 6, 2022

View reviewed changes

andrewsykim force-pushed the kep-1965 branch from 41547e0 to 18e7783 Compare October 6, 2022 10:20

deads2k reviewed Oct 6, 2022

View reviewed changes

keps/sig-api-machinery/1965-kube-apiserver-identity/README.md Show resolved Hide resolved

KEP-1965: update with Beta criteria/milestone and PRR questions answered

3c01537

Signed-off-by: Andrew Sy Kim <[email protected]> Co-authored-by: Monis Khan <[email protected]>

andrewsykim added 3 commits October 6, 2022 12:31

KEP-1965: address PRR comments from wojtek-t

3fe6a38

Signed-off-by: Andrew Sy Kim <[email protected]>

KEP-1965: clarify design details around storageversiongc controller r…

03e1e9b

…unning in kube-controller-manager Signed-off-by: Andrew Sy Kim <[email protected]>

KEP-1965: add SLO and metrics for # of expected leases

3e6d42d

Signed-off-by: Andrew Sy Kim <[email protected]>

andrewsykim force-pushed the kep-1965 branch from 18e7783 to 3e6d42d Compare October 6, 2022 16:35

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 6, 2022

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

lavalamp reviewed Oct 6, 2022

View reviewed changes

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

KEP-1965: add future looking GA criteria, requiring consensus on leas…

8a9df00

…e naming, persistence policy and components Signed-off-by: Andrew Sy Kim <[email protected]>

andrewsykim force-pushed the kep-1965 branch from 2dc9350 to 8a9df00 Compare October 6, 2022 19:39

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2022

k8s-ci-robot merged commit e634c15 into kubernetes:master Oct 6, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 6, 2022

andrewsykim mentioned this pull request Nov 4, 2022

Promote APIServerIdentity to Beta kubernetes/kubernetes#113629

Merged

4 tasks


		###### Are there any tests for feature enablement/disablement?

		Yes, see [apiserver_identity_test.go](https://github.com/kubernetes/kubernetes/blob/24238425492227fdbb55c687fd4e94c8b58c1ee3/test/integration/controlplane/apiserver_identity_test.go).


		###### Are there any missing metrics that would be useful to have to improve observability of this feature?

		Yes, heart beat latency could be useful.


		###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?

		A rough SLO here is that healthy kube-apiservers has a lease which is not older than 2 times the frequency of

KEP-1965: update with Beta criteria/milestone and PRR questions answered #3589

KEP-1965: update with Beta criteria/milestone and PRR questions answered #3589

Conversation

andrewsykim commented Oct 5, 2022 • edited Loading

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Oct 6, 2022

lavalamp commented Oct 6, 2022

andrewsykim commented Oct 5, 2022 •

edited

Loading

andrewsykim Oct 6, 2022 •

edited

Loading

andrewsykim Oct 6, 2022 •

edited

Loading