wire TemporaryRootShardKcpSharedInformerFactory #1572

p0lyn0mial · 2022-07-20T10:48:13Z

Summary

wires TemporaryRootShardKcpSharedInformerFactory, essentially when c.Options.Extra.RootShardKubeconfigFile is set then:

we start a new SharedInformerFactory for the root shard
we create a KcpClusterClient to the root shard
we registerer a new shard in the root namespace

Related issue(s)

Fixes #

p0lyn0mial · 2022-07-21T16:29:38Z

/retest

stevekuznetsov · 2022-07-25T13:10:36Z

pkg/reconciler/apis/apibinding/apibinding_controller.go


 		getAPIResourceSchema: func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error) {
-			return apiResourceSchemaInformer.Lister().Get(clusters.ToClusterAwareKey(clusterName, name))
+			apiResourceSchema, err := apiResourceSchemaInformer.Lister().Get(clusters.ToClusterAwareKey(clusterName, name))


What's the expected sequencing here? I'm not sure we want to have this structure around in the code for a long time, it seems easy to do incorrectly from the dev side.

sttts · 2022-07-26T12:15:38Z

pkg/server/server.go

@@ -28,6 +28,7 @@ import (
 	apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"


needs better commit name

sttts · 2022-07-26T12:15:53Z

pkg/server/server.go

@@ -121,6 +122,9 @@ type ExtraConfig struct {
 	// resource identities for the root shard into the rest.Config used by the client.
 	// Only after it succeeds, the clients can wildcard-list/watch most kcp resources.
 	ResolveRootKcpShardIdentities func(ctx context.Context) error
+
+	// RootShardKcpClusterClient holds a cluster-wide client to the root shard
+	RootShardKcpClusterClient *kcpclient.Cluster


move up to the clients

sttts · 2022-07-26T12:16:57Z

pkg/server/server.go

+	// ResolveRootKcpShardIdentities is to be called on server start until it succeeds. It injects the kcp
+	// resource identities for the root shard into the rest.Config used by the client.
+	// Only after it succeeds, the clients can wildcard-list/watch most kcp resources.
+	ResolveRootKcpShardIdentities func(ctx context.Context) error


we have that at the top already

this is for the root shard only, resolved in a different place. I think we need both.

pkg/server/server.go

sttts · 2022-07-26T12:47:13Z

pkg/server/server.go

+	// Only after it succeeds, the clients can wildcard-list/watch most kcp resources.
+	ResolveRootKcpShardIdentities func(ctx context.Context) error
+
+	identityConfig *rest.Config


am still confused about the new fields. The existing should do exactly that.

ah, I think I know what you mean :)

I think this is what we need #1620

can you wire it here?

sttts · 2022-07-28T12:19:53Z

pkg/server/server.go

+			return nil, err
+		}
+		var rootKcpShardIdentityConfig *rest.Config
+		rootKcpShardIdentityConfig, c.ResolveRootKcpShardIdentities = boostrap.NewConfigWithWildcardIdentities(nonIdentityRootKcpShardSystemAdminConfig, boostrap.KcpRootGroupExportNames, boostrap.KcpRootGroupResourceExportNames, nonIdentityRootKcpShardClient.Cluster(tenancyv1alpha1.RootCluster))


I am lost in this wiring. Don't we have these two cases:

we are the root shard. Then we need the old, staged flow where we start the local informer only for apis.kcp.dev first, then pick the identities from the APIExports, inject them into the roundtripper and then start the rest.

we are NOT the root shard. Then we can run resolveRootKcpShardIdentities here in-place very early before anything of the code further down runs, i.e. before any of the informers have been started.

p0lyn0mial · 2022-07-28T12:35:53Z

pkg/server/server.go

+	}
+
+	if rootKcpShardIdentityRoundTripper != nil {
+		c.identityConfig = rest.CopyConfig(c.GenericConfig.LoopbackClientConfig)


we could move this inside the if clause in L190

p0lyn0mial · 2022-07-28T12:36:18Z

pkg/server/server.go

+		c.identityConfig = rest.CopyConfig(c.GenericConfig.LoopbackClientConfig)
+		c.identityConfig.Wrap(rootKcpShardIdentityRoundTripper)
+	} else {
+		// The informers here are not  used before the informers are actually started (i.e. no race).


and this into the else clause in L214

…or the root shard

NewWildcardIdentitiesWrappingRoundTripper creates a HTTP RoundTripper that injected resource identities for individual group or group resources. Each group or resource is coming from one APIExport whose names are passed in as a map. The RoundTripper is exposed as a function that allows wrapping the RoundTripper. The method also returns the resolve function that gets the APIExports and extract the identities. The resolve func might return an error if the APIExport is not found or for other reason. Only after it succeeds a client using the returned RoundTripper can use the group and group resources with identities.

p0lyn0mial · 2022-08-01T07:05:18Z

pkg/server/config.go

+			return nil, err
+		}
+		var rootKcpShardIdentityRoundTripper func(rt http.RoundTripper) http.RoundTripper
+		rootKcpShardIdentityRoundTripper, c.resolveIdentities = boostrap.NewWildcardIdentitiesWrappingRoundTripper(boostrap.KcpRootGroupExportNames, boostrap.KcpRootGroupResourceExportNames, nonIdentityRootKcpShardClient.Cluster(tenancyv1alpha1.RootCluster))


I don't want to resolve it here, config is for setting up configuration

that's an argument indeed

p0lyn0mial · 2022-08-01T07:07:01Z

pkg/server/server.go

@@ -172,24 +173,76 @@ func (s *Server) Run(ctx context.Context) error {
 				return nil // don't klog.Fatal. This only happens when context is cancelled.
 			}
 			klog.Infof("Bootstrapped root workspace phase 0")
-		}



now the flow is:

common

root

non-root

common

root (moving 4 and 5 breaks the server, not sure it can be done in any other way)

sttts · 2022-08-01T08:14:33Z

pkg/server/config.go

@@ -86,6 +87,8 @@ type ExtraConfig struct {
 	KubeClusterClient          kubernetes.ClusterInterface
 	ApiExtensionsClusterClient apiextensionsclient.ClusterInterface
 	KcpClusterClient           kcpclient.ClusterInterface
+	// RootShardKcpClusterClient holds a cluster-wide client to the root shard
+	RootShardKcpClusterClient *kcpclient.Cluster


use ClusterInterface

sttts · 2022-08-01T08:14:47Z

pkg/server/config.go

@@ -86,6 +87,8 @@ type ExtraConfig struct {
 	KubeClusterClient          kubernetes.ClusterInterface
 	ApiExtensionsClusterClient apiextensionsclient.ClusterInterface
 	KcpClusterClient           kcpclient.ClusterInterface
+	// RootShardKcpClusterClient holds a cluster-wide client to the root shard


no need for the comment. Doesn't add anything :)

sttts · 2022-08-01T08:18:50Z

pkg/server/config.go

+		rootKcpShardIdentityRoundTripper, c.resolveIdentities = boostrap.NewWildcardIdentitiesWrappingRoundTripper(boostrap.KcpRootGroupExportNames, boostrap.KcpRootGroupResourceExportNames, nonIdentityRootKcpShardClient.Cluster(tenancyv1alpha1.RootCluster))
+		rootKcpShardIdentityConfig := rest.CopyConfig(nonIdentityRootKcpShardSystemAdminConfig)
+		rootKcpShardIdentityConfig.Wrap(rootKcpShardIdentityRoundTripper)
+		c.RootShardKcpClusterClient, err = kcpclient.NewClusterForConfig(rootKcpShardIdentityConfig) // this is now generic to be used for all kcp API groups


comment is not correct. The client can be used after resolving via c.resolveIdentities. Would just drop the comment.

pkg/server/config.go

sttts · 2022-08-01T09:26:22Z

/lgtm
/approve

openshift-ci · 2022-08-01T09:26:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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:

~~OWNERS~~ [sttts]

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

p0lyn0mial · 2022-08-01T10:11:18Z

/retest

sttts · 2022-08-01T10:18:09Z

/retest

sttts · 2022-08-01T10:34:16Z

/retest

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2022

openshift-ci bot requested review from davidfestal and sttts July 20, 2022 10:48

p0lyn0mial force-pushed the multiple-shard-support branch from c3fc225 to eb2c902 Compare July 21, 2022 15:22

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

stevekuznetsov reviewed Jul 25, 2022

View reviewed changes

stevekuznetsov mentioned this pull request Jul 25, 2022

apibinding controller with root shard informers #1598

Merged

p0lyn0mial force-pushed the multiple-shard-support branch from eb2c902 to 3112756 Compare July 26, 2022 11:12

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

p0lyn0mial changed the title ~~[WIP] multiple shard support~~ multiple shard support Jul 26, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2022

sttts reviewed Jul 26, 2022

View reviewed changes

p0lyn0mial changed the title ~~multiple shard support~~ wire TemporaryRootShardKcpSharedInformerFactory Jul 26, 2022

sttts reviewed Jul 26, 2022

View reviewed changes

pkg/server/server.go Outdated Show resolved Hide resolved

p0lyn0mial force-pushed the multiple-shard-support branch from f0a34db to e6a75d7 Compare July 26, 2022 12:30

sttts reviewed Jul 26, 2022

View reviewed changes

sttts reviewed Jul 28, 2022

View reviewed changes

p0lyn0mial force-pushed the multiple-shard-support branch from e6a75d7 to 0df6f34 Compare July 28, 2022 12:27

p0lyn0mial commented Jul 28, 2022

View reviewed changes

p0lyn0mial force-pushed the multiple-shard-support branch from 0df6f34 to 99baec0 Compare July 28, 2022 12:55

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

sharded-test-server: pass a shard name and a path to the kubeconfig f…

0bce4bf

…or the root shard

p0lyn0mial force-pushed the multiple-shard-support branch from 99baec0 to eb21f33 Compare August 1, 2022 07:03

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

p0lyn0mial force-pushed the multiple-shard-support branch from eb21f33 to 3639d33 Compare August 1, 2022 07:03

p0lyn0mial commented Aug 1, 2022

View reviewed changes

p0lyn0mial force-pushed the multiple-shard-support branch from 3639d33 to ad0010a Compare August 1, 2022 07:44

sttts reviewed Aug 1, 2022

View reviewed changes

pkg/server/config.go Outdated Show resolved Hide resolved

sttts reviewed Aug 1, 2022

View reviewed changes

pkg/server/config.go Show resolved Hide resolved

sttts reviewed Aug 1, 2022

View reviewed changes

pkg/server/config.go Outdated Show resolved Hide resolved

p0lyn0mial force-pushed the multiple-shard-support branch from ad0010a to 64aa157 Compare August 1, 2022 09:09

wire TemporaryRootShardKcpSharedInformerFactory

c881323

p0lyn0mial force-pushed the multiple-shard-support branch from 64aa157 to c881323 Compare August 1, 2022 09:24

openshift-ci bot assigned sttts Aug 1, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022

p0lyn0mial mentioned this pull request Aug 1, 2022

introduce NewWildcardIdentitiesRoundTripper #1620

Closed

openshift-merge-robot merged commit 614a724 into kcp-dev:main Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wire TemporaryRootShardKcpSharedInformerFactory #1572

wire TemporaryRootShardKcpSharedInformerFactory #1572

p0lyn0mial commented Jul 20, 2022 •

edited

Loading

p0lyn0mial commented Jul 21, 2022

stevekuznetsov Jul 25, 2022

sttts Jul 26, 2022

sttts Jul 26, 2022

sttts Jul 26, 2022

p0lyn0mial Jul 26, 2022

sttts Jul 26, 2022

p0lyn0mial Jul 26, 2022

p0lyn0mial Jul 26, 2022

sttts Jul 27, 2022

p0lyn0mial Jul 28, 2022

sttts Jul 28, 2022

p0lyn0mial Jul 28, 2022

p0lyn0mial Jul 28, 2022

p0lyn0mial Aug 1, 2022

sttts Aug 1, 2022

p0lyn0mial Aug 1, 2022

sttts Aug 1, 2022

sttts Aug 1, 2022

sttts Aug 1, 2022

sttts commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022

p0lyn0mial commented Aug 1, 2022

sttts commented Aug 1, 2022

sttts commented Aug 1, 2022

		@@ -28,6 +28,7 @@ import (
		apiextensionsapiserver "k8s.io/apiextensions-apiserver/pkg/apiserver"

wire TemporaryRootShardKcpSharedInformerFactory #1572

wire TemporaryRootShardKcpSharedInformerFactory #1572

Conversation

p0lyn0mial commented Jul 20, 2022 • edited Loading

Summary

Related issue(s)

p0lyn0mial commented Jul 21, 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

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

sttts commented Aug 1, 2022

openshift-ci bot commented Aug 1, 2022

p0lyn0mial commented Aug 1, 2022

sttts commented Aug 1, 2022

sttts commented Aug 1, 2022

p0lyn0mial commented Jul 20, 2022 •

edited

Loading