✨ Switch to cluster-aware k8s clients, listers & informers #2104

stevekuznetsov · 2022-09-29T17:51:38Z

No description provided.

openshift-ci · 2022-10-17T17:25:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ncdc for approval by writing /assign @ncdc in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

s-urbaniak · 2022-10-20T14:04:17Z

pkg/admission/webhook/generic_webhook.go

+	}
+
+	source = c.factory(cluster)
+	c.sources[cluster] = source


how is this map ever being cleaned up?

It's not - seems like a good follow-up. We need the rebase to 1.25 to get resource event handler function cleanup on the (shared, cross-cluster) list.

pkg/authorization/apibinding_authorizer.go

stevekuznetsov · 2022-10-20T14:25:34Z

go.mod now points at kcp-dev/kubernetes#105

pkg/server/handler.go

pkg/cache/server/crd_lister.go

pkg/server/handler.go

p0lyn0mial · 2022-10-20T14:27:20Z

pkg/cache/server/crd_lister.go

+
+func (c *crdClusterLister) Cluster(name logicalcluster.Name) kcp.ClusterAwareCRDLister {
+	if name != bootstrap.SystemCRDLogicalCluster {
+		klog.Background().Error(fmt.Errorf("cluster-unaware crd lister got asked for %v cluster", name), "programmer error")


why do we need this?

Hmmm ... good question - it must have come from somewhere previous. Do you know? :D

Looks like it's only ever valid to use this particular cluster - maybe we don't need a Cluster() call here at all, let me remove

Oh, I see - when it's the APIBinding-aware one, this does not apply. I am just carrying forward the previous impl IUIC

(and it was your first-pass impl that hard-coded this value, and has a TODO for future improvements ;) )

i'd like to see how the apiextention server will be using crdClusterLister, could you please provide me with a link to your fork?

okay, clarified with Steve in a gmeet.

p0lyn0mial · 2022-10-20T14:32:59Z

reconciler/cache: make tests cluster-aware lgtm

ncdc · 2022-10-20T19:24:31Z

Makefile

@@ -208,6 +208,9 @@ GO_TEST = $(GOTESTSUM) $(GOTESTSUM_ARGS) --
 endif

 COUNT ?= 1


What if instead we defaulted to unset, and if it's not empty, then set TEST_COUNT? Otherwise, you can't specify COUNT=1 and have it force running a test w/o hitting the cache.

ACK - can drop this commit & do it stand-alone

pkg/authorization/local_authorizer.go

pkg/syncer/spec/spec_controller.go

pkg/virtual/framework/forwardingregistry/store.go

ncdc · 2022-10-20T20:28:58Z

go.mod

@@ -189,30 +189,30 @@ require (

 replace (
 	github.com/kcp-dev/kcp/pkg/apis => ./pkg/apis
-	k8s.io/api => github.com/kcp-dev/kubernetes/staging/src/k8s.io/api v0.0.0-20221005071841-6cfb7d485cbf


I assume you'll drop/replace this commit?

Yes, that's the last commit & will be replaced with the fork once we merge the k8s PR

stevekuznetsov · 2022-10-20T21:42:31Z

Almost forgot ... TODO:

=== Skipped
=== SKIP: pkg/virtual/workspaces/registry TestDeleteWorkspace (0.00s)
    rest_test.go:1065: fake client does not support DeleteCollection, so this test never worked

Daniel Smith suggested not implementing delete collection in the fake, said it was not well supported in k8s as it were ... can we get away with not using that verb?

p0lyn0mial · 2022-10-21T10:47:26Z

pkg/reconciler/apis/permissionclaimlabel/permissionclaimlabel_reconcile.go

@@ -112,7 +112,7 @@ func (c *controller) reconcile(ctx context.Context, apiBinding *apisv1alpha1.API
 		}

 		claimLogger.V(4).Info("listing resources")
-		objs, err := informer.Informer().GetIndexer().ByIndex(indexers.ByLogicalCluster, clusterName.String())
+		objs, err := informer.Lister().ByCluster(clusterName).List(labels.Everything())


where can I check the new interface of a lister?
what is ByCluster? an index?
how does it differ from configMapInformer.Lister().Cluster(cluster).ConfigMaps(namespace).Get(name)?

This was following the pattern of dynamic/generic listers doing ByNamespace() - but perhaps we should not do that and make it .Cluster()? You can see the implementation used in this PR already published to kcp-dev/client-go - https://github.com/kcp-dev/client-go/blob/272de79b2a7ab1d29e7d0db2d458258d035124d4/clients/metadata/metadatalister/shim.go#L54-L56

yes, I'd prefer to have Cluster() method on the dynamic client

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

We turned off the controller upstream, so we can stop serving the types entirely. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

openshift-ci bot requested review from davidfestal and jmprusi September 29, 2022 17:51

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2022

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 29, 2022

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch 3 times, most recently from dc388b5 to 9a09395 Compare October 6, 2022 12:58

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch 2 times, most recently from 502785a to a54c7a1 Compare October 17, 2022 17:25

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch 3 times, most recently from dfbd0ff to 73d7d54 Compare October 20, 2022 13:57

s-urbaniak reviewed Oct 20, 2022

View reviewed changes

pkg/authorization/apibinding_authorizer.go Outdated Show resolved Hide resolved

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch from 73d7d54 to 504302c Compare October 20, 2022 14:14

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch from 504302c to 3908bfc Compare October 20, 2022 14:25

p0lyn0mial reviewed Oct 20, 2022

View reviewed changes

ncdc reviewed Oct 20, 2022

View reviewed changes

pkg/authorization/local_authorizer.go Outdated Show resolved Hide resolved

p0lyn0mial mentioned this pull request Oct 20, 2022

🌱 cache: add replicateAPIResourceSchema test scenario #2240

Merged

ncdc reviewed Oct 20, 2022

View reviewed changes

pkg/syncer/spec/spec_controller.go Outdated Show resolved Hide resolved

ncdc reviewed Oct 20, 2022

View reviewed changes

pkg/virtual/framework/forwardingregistry/store.go Outdated Show resolved Hide resolved

ncdc reviewed Oct 20, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022

p0lyn0mial reviewed Oct 21, 2022

View reviewed changes

stevekuznetsov added 20 commits October 21, 2022 07:53

admission/kubequota: implement using generated factories

2f16495

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

builtins: remove AP&F types

cd678ab

We turned off the controller upstream, so we can stop serving the types entirely. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

docs: update key section

c8282c1

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

admission/*webhook: reimplement with cluster-aware delegation

96a37b0

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

authorization: use scoped listers

577fa50

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

workspace_content_authorizer: fill indexers using client in tests

c10556e

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

workspace_content_authorizer: actually name objects correctly

b07cfb3

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

admission/namespacelifecycle: feed the cluster-aware namespace admission

edd323b

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

server/cache: implement a cluster-aware CRD lister

8422002

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

server: implement a cluster-aware apibinding-aware crd lister

21fcd85

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

reconciler/cache: make tests cluster-aware

fcd70eb

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

server: consume the informers + client from the first creation

5a50dc4

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

syncer: remove the scoping lister implementation

0ab0550

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

virtual/framework: remove a dynamic cluster client implementation

b12141e

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

dynamicsharedinformerfactory: simplify by using generated code

ba7de6f

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

virtual/framework: adapt client getter for new types

5f4277d

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

virtual/framework: adapt deleter implemenetation

cd96188

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

virtual/framework: adapt tests to be cluster-aware

849e26d

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

virtual/workspaces: adapt plumbing of auth cache

d001afd

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

test/e2e/reconciler/namespace: adapt expecter to do a cross-cluster list

be2357b

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch from 61cfd4c to fd306c5 Compare October 21, 2022 13:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2022

stevekuznetsov added 3 commits October 21, 2022 08:15

*: mostly-automated, boring type/callsite refactors

2b1d382

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

*: miscellany to pass linters and reiew comments

baaded5

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

go.mod: bump k8s to latest commit

c948a85

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov force-pushed the skuznets/clients-listers-informers branch from fd306c5 to c948a85 Compare October 21, 2022 14:15

ncdc changed the title ~~[PROOF] use cluster-aware k8s clients, listers & informers~~ ✨ Switch to cluster-aware k8s clients, listers & informers Oct 21, 2022

ncdc merged commit c8f1c4c into kcp-dev:main Oct 21, 2022

astefanutti mentioned this pull request Oct 24, 2022

✨ Add garbage collector controller #2112

Merged

waltforme mentioned this pull request Jan 17, 2023

Placement controller needs updates to work with kcp latest release kubestellar/kubestellar#126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Switch to cluster-aware k8s clients, listers & informers #2104

✨ Switch to cluster-aware k8s clients, listers & informers #2104

stevekuznetsov commented Sep 29, 2022

openshift-ci bot commented Oct 17, 2022

s-urbaniak Oct 20, 2022

stevekuznetsov Oct 20, 2022

stevekuznetsov commented Oct 20, 2022

p0lyn0mial Oct 20, 2022

stevekuznetsov Oct 20, 2022

stevekuznetsov Oct 20, 2022

stevekuznetsov Oct 20, 2022

stevekuznetsov Oct 20, 2022

p0lyn0mial Oct 21, 2022

p0lyn0mial Oct 21, 2022

p0lyn0mial commented Oct 20, 2022

ncdc Oct 20, 2022

stevekuznetsov Oct 20, 2022

ncdc Oct 20, 2022

stevekuznetsov Oct 20, 2022

stevekuznetsov commented Oct 20, 2022

p0lyn0mial Oct 21, 2022 •

edited

stevekuznetsov Oct 21, 2022

p0lyn0mial Oct 21, 2022

		@@ -208,6 +208,9 @@ GO_TEST = $(GOTESTSUM) $(GOTESTSUM_ARGS) --
		endif

		COUNT ?= 1

✨ Switch to cluster-aware k8s clients, listers & informers #2104

✨ Switch to cluster-aware k8s clients, listers & informers #2104

Conversation

stevekuznetsov commented Sep 29, 2022

openshift-ci bot commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Oct 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Oct 20, 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

stevekuznetsov commented Oct 20, 2022

p0lyn0mial Oct 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial Oct 21, 2022 •

edited