Skip to content

✨ Add index support to providers#16

Merged
sttts merged 7 commits into
kubernetes-sigs:mainfrom
sttts:sttts-cluster-indexes
Mar 9, 2025
Merged

✨ Add index support to providers#16
sttts merged 7 commits into
kubernetes-sigs:mainfrom
sttts:sttts-cluster-indexes

Conversation

@sttts
Copy link
Copy Markdown
Contributor

@sttts sttts commented Mar 6, 2025

A first attempt on Index support. Is this what we want?

See in particular:

  1. GetFieldIndex() in the manager interface.
  2. IndexField() in the provider interface.

Motivation:

Indexes must be created before cache start. They are usually uniform for multicluster controllers. Only the provider really knows how to set them up (some providers have dedicated clusters, others have one central cluster object and scoped down cluster shims on-top).

TODOs:

  • add unit tests to the providers, in particular the namespace provider. This is not obvious.

@embik
Copy link
Copy Markdown
Member

embik commented Mar 6, 2025

I think this makes sense, it's a relatively trivial function to implement on a provider that juggles clusters (as seen in the capi example provider), if we figure out we are missing something we can re-adjust.

@sttts sttts force-pushed the sttts-cluster-indexes branch 3 times, most recently from 8accffa to b6e1fb2 Compare March 6, 2025 19:28
Copy link
Copy Markdown
Collaborator

@mjudeikis mjudeikis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, if somebody want to add dynamic indexes to only certain clusters?
In example:

  1. I reconcile 100 clusters.
  2. Only 20 clusters has Karpetner/Crossplane installed. Now my controller needs dynamically add indexes for these clusters for certain objects.

This would not be supported, right?

@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Mar 7, 2025

This would not be supported, right?

It depends on the provider. A cluster-api provider with real cluster.Cluster instances, there you can add indexes manually. But then this is not a uniform setup anymore. You would likely start multiple managers to separate.

But you have a point. Wondering whether we need something more flexible. I am also now calling the host cluster IndexField. And the host cluster most likely looks different to the other clusters.

@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Mar 7, 2025

I am also now calling the host cluster IndexField

I have changed that.

@sttts
Copy link
Copy Markdown
Contributor Author

sttts commented Mar 7, 2025

@mjudeikis I feel like such a custom logic is probably better suited to be put into the provider? It heavily depends on how you discover clusters, how you classify them through some means. I could imagine there to be some pre-engage and also engage-filter mechanism. Wdyt?

@sttts sttts force-pushed the sttts-cluster-indexes branch from d54a102 to 3abea72 Compare March 7, 2025 07:44
@mjudeikis
Copy link
Copy Markdown
Collaborator

I was thinking about uniform-cases here, where each cluster is separate.

But you have a point. Wondering whether we need something more flexible. I am also now calling the host cluster IndexField. And the host cluster most likely looks different to the other clusters.

For me, this was the biggest issue. How do we know these indexers will work on other clusters? Should these be per cluster?

need to read implementation more to understand how things are wired in

sttts added 5 commits March 9, 2025 10:50
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
@sttts sttts force-pushed the sttts-cluster-indexes branch 2 times, most recently from 9b79885 to 622e3d1 Compare March 9, 2025 09:52
sttts added 2 commits March 9, 2025 11:09
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
Signed-off-by: Dr. Stefan Schimanski <[email protected]>
@sttts sttts force-pushed the sttts-cluster-indexes branch from 622e3d1 to 127907d Compare March 9, 2025 10:09
@sttts sttts changed the title ✨ RFC: Add index support to providers ✨ Add index support to providers Mar 9, 2025
@sttts sttts merged commit 83466f4 into kubernetes-sigs:main Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants