Apiserver lease garbage collector #95895

roycaihw · 2020-10-27T05:10:49Z

Implements KEP. Split off from #95222.

Does this PR introduce a user-facing change?:

kube-apiserver now deletes expired kube-apiserver Lease objects:
- The feature is under feature gate `APIServerIdentity`.
- A flag is added to kube-apiserver: `identity-lease-garbage-collection-check-period-seconds`

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

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1965-kube-apiserver-identity/README.md

/sig api-machinery
/assign @caesarxuchao

fejta-bot · 2020-10-27T05:22:20Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

fedebongio · 2020-10-27T20:10:58Z

/triage accepted

lavalamp · 2020-11-06T19:20:19Z

Actually, even in that case. I don't think the identity system should have external-to-apiserver dependencies.

roycaihw · 2020-11-09T19:11:53Z

It only manages apiserver leases. The reason for running the GC in KCM was that nothing in KAS today runs leader election-- but that's totally achievable.

Leases of apiservers other than KAS:

aggregated server: aggregated servers always require KAS to exist
extensions server: extensions server running standalone is a non-goal, and it won't work with KCM today. If we ever want to do that, it would be a beta/GA requirement.

I like the idea that identity system should have external-to-apiserver dependencies. I will update the PR to do that.

roycaihw · 2020-11-09T21:02:34Z

nothing in KAS today runs leader election

Actually I won't add leader election to KAS. This GC has no retry logic, since all it does is a few GET and DELETE every hour. Adding leader election to KAS will cost more leader election traffic.

lavalamp · 2020-11-09T21:34:02Z

Yeah we shouldn't need leader election for this, it is (or should be) safe to be run multiple times concurrently.

caesarxuchao · 2020-11-10T18:09:11Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+	leaseInformer := informers.NewFilteredLeaseInformer(
+		clientset,
+		metav1.NamespaceSystem,
+		12*time.Hour,


Do we need to resync?

no. Changed to 0

caesarxuchao · 2020-11-10T18:11:45Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+	}
+	selector = selector.Add(*r)
+
+	leases, err := c.leaseLister.Leases(metav1.NamespaceSystem).List(selector)


The informer has already filtered with the labelselector, do you need the selector here?

removed the selector and added test cases to make sure we don't GC leases that we don't intend to manage

caesarxuchao · 2020-11-10T18:37:53Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+				continue
+			}
+			if errors.IsNotFound(err) || lease == nil {
+				continue


Is there any component other than this GC expected to delete a lease? If not, let's log a warning.

multiple GC controllers can delete the lease simultaneously. Added comments

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

caesarxuchao · 2020-11-10T18:45:34Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+	// Copied from controlplane.IdentityLeaseComponentLabelKey and controlplane.KubeAPIServer
+	// to avoid import cycle.
+	identityLeaseComponentLabelKey = "k8s.io/component"
+	kubeAPIServer                  = "kube-apiserver"


Can we move them to a common package?

made both the label selector and the namespace parameters to the gc controller. I think this way better reflects what the controller is capable of-- it operates on Leases in one namespace with a specific label set. Not saying we will make the code public/reusable, but we could if we want in future.

test/integration/master/apiserver_identity_test.go

spiffxp · 2020-11-10T21:06:48Z

possible github issue, please link if you run into this on other PR's kubernetes/test-infra#19910

roycaihw · 2020-11-10T21:14:56Z

@spiffxp Got it. Thanks

lavalamp · 2020-11-10T22:27:12Z

cmd/kube-apiserver/app/options/options.go

@@ -117,6 +118,7 @@ func NewServerRunOptions() *ServerRunOptions {
 		EndpointReconcilerType:            string(reconcilers.LeaseEndpointReconcilerType),
 		IdentityLeaseDurationSeconds:      3600,
 		IdentityLeaseRenewIntervalSeconds: 10,
+		IdentityLeaseGCCheckPeriodSeconds: 3600,


Does this need to be separate from IdentityLeaseDurationSeconds?

good point. If we know all leases have the same duration (which is the case here), there is no point to configure the check period differently. Reverted

the knob made sense when/if:

the controller lived in KCM-- not true anymore

we want the controller to manage leases from aggregated servers (with different duration)-- the more I think about it, aggregated servers shouldn't rely on this GC controller:
a. they don't need heartbeat-based identity at all-- downward API plus owner reference is more efficient
b. if they really want, they can start a similar GC controller that operates their leases in a user namespace

(just adding some thoughts why we don't need a separate flag) another benefit of having this flag is to make the GC more responsive-- the controller deletes a lease if the apiserver failed to refresh its lease in:

(leaseDuration, leaseDuration+checkPeriod] seconds

instead of (without the flag)

(leaseDuration, 2*leaseDuration] seconds

but that is not our goal. We prefer that expired Leases remain for a longer duration as opposed to collecting them quickly. That's why we choice a super long leaseDuration by default (1h, compared with 10s renewInterval). If people want to GC leases sooner after an apiserver is not active, they should start with shortening the leaseDuration. Having a separate flag for this level of control seems like an overkill for an alpha feature.

lavalamp · 2020-11-10T22:53:47Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+		if err := c.kubeclientset.CoordinationV1().Leases(c.leaseNamespace).Delete(context.TODO(), lease.Name, metav1.DeleteOptions{}); err != nil && !errors.IsNotFound(err) {
+			// If we get a 404, the lease was deleted by the same GC controller
+			// in another apiserver
+			if !errors.IsNotFound(err) {


It's probably not needed to call IsNotFound twice :)

now I see why @caesarxuchao asks me to break the long lines.. this is what happens :)

lavalamp · 2020-11-10T23:09:28Z

/lgtm
/approve

k8s-ci-robot · 2020-11-10T23:09:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, roycaihw

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~~ [lavalamp]
~~test/OWNERS~~ [lavalamp]

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

caesarxuchao

One nit. Otherwise lgtm.

/lgtm

caesarxuchao · 2020-11-10T23:12:53Z

pkg/controlplane/controller/apiserverleasegc/gc_controller.go

+			continue
+		}
+		if errors.IsNotFound(err) || lease == nil {
+			// the lease was deleted by the same GC controller in another apiserver


nit: can you state this could be the case if it's an HA-master? Also state that we don't expect other components to delete the lease?

Can you add a klog.V(4)?

done. Also squashed the commits

…reacts

caesarxuchao · 2020-11-10T23:37:31Z

/lgtm

k8s-ci-robot assigned caesarxuchao Oct 27, 2020

k8s-ci-robot requested review from a-robinson and cheftako October 27, 2020 05:11

roycaihw force-pushed the apiserver-lease-gc branch from 943ab29 to 6aa7787 Compare October 27, 2020 06:37

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, 2020

roycaihw force-pushed the apiserver-lease-gc branch 5 times, most recently from 78533eb to 5d45512 Compare November 4, 2020 22:45

This was referenced Nov 6, 2020

kube-apiserver identity kubernetes/enhancements#1965

Open

Add kube-apiserver lease controller #95533

Merged

annajung mentioned this pull request Nov 6, 2020

document kube-apiserver identity kubernetes/website#24921

Merged

roycaihw force-pushed the apiserver-lease-gc branch from 5d45512 to 258bcd8 Compare November 9, 2020 22:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 9, 2020

caesarxuchao reviewed Nov 10, 2020

View reviewed changes

roycaihw force-pushed the apiserver-lease-gc branch from 27111d3 to ddaac64 Compare November 10, 2020 20:39

lavalamp reviewed Nov 10, 2020

View reviewed changes

k8s-ci-robot assigned lavalamp Nov 10, 2020

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

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

add apiserver lease garbage collector

1d43fd4

caesarxuchao reviewed Nov 10, 2020

View reviewed changes

roycaihw added 3 commits November 10, 2020 15:32

integration test

4794ba1

generated

bfebc7a

add V(4) log when apiserver lease was deleted before this controller …

e363709

…reacts

roycaihw force-pushed the apiserver-lease-gc branch from cadc6c2 to e363709 Compare November 10, 2020 23:32

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

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

k8s-ci-robot merged commit f102cc8 into kubernetes:master Nov 11, 2020

k8s-ci-robot added this to the v1.20 milestone Nov 11, 2020

github-actions bot mentioned this pull request Nov 18, 2020

Week Ending November 15, 2020 dev-obs/actus#273

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apiserver lease garbage collector #95895

Apiserver lease garbage collector #95895

roycaihw commented Oct 27, 2020 •

edited

Loading

fejta-bot commented Oct 27, 2020

fedebongio commented Oct 27, 2020

lavalamp commented Nov 6, 2020

roycaihw commented Nov 9, 2020

roycaihw commented Nov 9, 2020

lavalamp commented Nov 9, 2020

caesarxuchao Nov 10, 2020

roycaihw Nov 10, 2020

caesarxuchao Nov 10, 2020

roycaihw Nov 10, 2020

caesarxuchao Nov 10, 2020

roycaihw Nov 10, 2020

caesarxuchao Nov 10, 2020

roycaihw Nov 10, 2020 •

edited

Loading

spiffxp commented Nov 10, 2020

roycaihw commented Nov 10, 2020

lavalamp Nov 10, 2020

roycaihw Nov 10, 2020 •

edited

Loading

roycaihw Nov 11, 2020

lavalamp Nov 10, 2020

roycaihw Nov 10, 2020

lavalamp commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

caesarxuchao left a comment

caesarxuchao Nov 10, 2020

caesarxuchao Nov 10, 2020

roycaihw Nov 10, 2020 •

edited

Loading

caesarxuchao commented Nov 10, 2020

Apiserver lease garbage collector #95895

Apiserver lease garbage collector #95895

Conversation

roycaihw commented Oct 27, 2020 • edited Loading

fejta-bot commented Oct 27, 2020

fedebongio commented Oct 27, 2020

lavalamp commented Nov 6, 2020

roycaihw commented Nov 9, 2020

roycaihw commented Nov 9, 2020

lavalamp commented Nov 9, 2020

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

roycaihw Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

spiffxp commented Nov 10, 2020

roycaihw commented Nov 10, 2020

Choose a reason for hiding this comment

roycaihw Nov 10, 2020 • 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

lavalamp commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

caesarxuchao commented Nov 10, 2020

roycaihw commented Oct 27, 2020 •

edited

Loading

roycaihw Nov 10, 2020 •

edited

Loading

roycaihw Nov 10, 2020 •

edited

Loading

roycaihw Nov 10, 2020 •

edited

Loading