apibinding controller with root shard informers #1598
Conversation
| apiExportsIndexer cache.Indexer | ||
| getAPIExport func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIExport, error) | ||
| apiExportsIndexer cache.Indexer | ||
| rootShardApiExportsIndexer cache.Indexer |
| @@ -67,6 +67,8 @@ func NewController( | |||
| apiBindingInformer apisinformers.APIBindingInformer, | |||
| apiExportInformer apisinformers.APIExportInformer, | |||
| apiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, | |||
| temporaryRootShardApiExportInformer apisinformers.APIExportInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ | |||
| temporaryRootShardApiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ | |||
There was a problem hiding this comment.
in all this wiring. Shouldn't we go with temporaryOtherShard....Informer? The same code will run on the root shard, and it must watch the second shard.
There was a problem hiding this comment.
we could rename to temporaryLocalShard...Informer, wdyt?
p0lyn0mial
force-pushed
the
apibinding-controller-with-rootshard-informers
branch
from
8ac944b to
e943674
Compare
|
How is this different from #1572 ? #1572 (comment) applies here as well? |
I'm splitting 1572 into smaller chunks. Yes, the comment applies here as well. |
|
|
||
| getAPIResourceSchema: func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error) { | ||
| return apiResourceSchemaInformer.Lister().Get(clusters.ToClusterAwareKey(clusterName, name)) | ||
| apiResourceSchema, err := temporaryLocalApiResourceSchemaInformer.Lister().Get(clusters.ToClusterAwareKey(clusterName, name)) | ||
| if errors.IsNotFound(err) { |
There was a problem hiding this comment.
stevekuznetsov 43 minutes ago
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.
first, we check against a local shard and then the remote one (in that case the root shard). This is temporary until we have multi-shard informers.
There was a problem hiding this comment.
Doing it this way is easier and:
- give us a starting point
- allows us to find all places (controllers) that need to be aware of multiple shards.
- allows us to make some progress into multi-shard env
There was a problem hiding this comment.
@stevekuznetsov ^, does it make sense?
There was a problem hiding this comment.
this is definitely not the last words, it just gets us going.
There was a problem hiding this comment.
Why aren't we starting with the informer plumbing instead of a "temporary" hack that the user has to know how to use the two halves of individually?
There was a problem hiding this comment.
We will come to that. We won't augment more than 2 or 3 controllers with these temporary informers. Just to get it working in CI. Then we can iterate.
| apiExportInformer apisinformers.APIExportInformer, | ||
| apiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, | ||
| temporaryLocalApiExportInformer apisinformers.APIExportInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ | ||
| temporaryLocalApiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ |
p0lyn0mial
force-pushed
the
apibinding-controller-with-rootshard-informers
branch
from
e943674 to
ff2edc8
Compare
| apiExportInformer apisinformers.APIExportInformer, | ||
| apiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, | ||
| apiExportInformer apisinformers.APIExportInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ | ||
| apiResourceSchemaInformer apisinformers.APIResourceSchemaInformer, /*TODO(p0lyn0mial): replace with multi-shard informers*/ |
There was a problem hiding this comment.
nit: no TODOs either here for these two.
|
/lgtm |
|
[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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
p0lyn0mial
force-pushed
the
apibinding-controller-with-rootshard-informers
branch
from
ff2edc8 to
d769889
Compare
|
I had to gofmt the file |
|
/lgtm |
Summary
it enables non-root shards to read APIExports and APISchemas from the root shard so that they can create the root APIs for:
"shards.tenancy.kcp.dev", "tenancy.kcp.dev", "scheduling.kcp.dev", "workload.kcp.dev", "apiresource.kcp.dev"Related issue(s)
Fixes #