Replace concept of "local cluster" with "just another provider"

[crenshaw-dev]

Instead of configuring a "local cluster," I'd like to just be able to use different providers in For and Watches so I have unlimited possible configurations.

mcbuilder.ControllerManagedBy(mcMgr).
	For(&v1alpha1.ResourceA{},
		mcbuilder.WithEngageWithProviderClusters(provider1)).
	Watches(&v1alpha1.ResourceB{},
		mcbuilder.WithEngageWithProviderClusters(provider2)).
	Complete(r)

Local cluster could be its own provider that could be composed with other providers via a multi-provider provider.

Advantages:

• Allows more interesting topologies ("setup one watch for these 5 clusters and a different watch for these other 5")
• Simplifies the UX, since everything is just a provider
• Allows engaging/disengaging local clusters at runtime (we'd like to be able to hot-load these config changes in GitOps Promoter)
• Avoids subtle config bugs
• Avoids subtle differences in watch behavior

I don't know enough of the internals to understand the implications of having multiple providers at the Manager level. But I think this design would make the UX easier and the behavior more consistent.

[embik]

I can see the appeal of this, definitely not opposed to it, but I'm a little afraid of the implications for the overall architecture. Currently, we are channeling all requests for a cluster through the manager, and the manager knows only a single provider. @ntnn built the multi provider for a way to plug in more than one provider.

Changing this would potential require a complete rewrite with breaking changes of the provider and manager interaction. I wonder if we can build something with a similar UX on top of multi (i.e. e.g. filter by provider name prefix).

That being said, I'm not sure I see the connection to the "local cluster" part.

[crenshaw-dev]

Yep, that makes sense. I'm working on an PoC in GitOps Promoter based on a single (multi-)provider and per-watch prefix filtering. At a glance, it looks a little yuckier but seems workable.

The connection to local is just that, if you're able to specify different providers per watch, "local cluster" becomes an unnecessary concept. Removing it could make for a more consistent backend implementation and a more straightforward UX.

[ntnn]

I have thought about this because I hit the same issues. At the moment I define the prefixes used by the multi provider and match that against the cluster name the reconciler gets to see if a resource should be reconciled (and in which direction).
But that is not really a nice solution.

The limitations can't be worked around from a provider because the core problem is that the mcmanager tries to watch for all reconciled resources on all provided clusters. Which watches happen can only be filtered in the manager.

The multi provider is handy for a number of things but I do think the mcmanager should have the ability to know multiple providers and to offer filtering for the reconcilers.

That will need some work internally but for the interfaces that shouldn't be much of a problem - although I would be in favor of breaking changes this early on.
E.g. to keep the current interfaces mcmanager.New can still accept a provider and mcmanager.AddProvider can be added. mcmanager.GetProvider could return an internal version of the multi provider, giving access to all clusters.

[crenshaw-dev]

Sketchy example of what the prefix-based solution might look like: argoproj-labs/gitops-promoter#683

[ntnn]

On the local cluster: I also agree that the concept of local cluster doesn't need to exist in mcr. imho in a cross-cluster-native controller the cluster a manager runs in, the control plane it uses to coordinate with other managers and the control plane it manages are most likely different things:

So calling it local cluster is more of a misnomer. On top there is a case for multiple coordination control planes, say one for persistent configuration and one for ephemeral.

[embik]

I don't think I agree that local cluster is a big misnomer. Sure, in your picture the coordination control plane is different from the compute cluster control plane, but the default is definitely that you use the compute cluster control plane to do things like leader election (that's the whole appeal of MCR: You run a set of controllers on one cluster to reconcile multiple remote clusters).

I'm open to renaming it to something more snappy, but in general, I think the "local cluster" concept is required for e.g. leader election in the multicluster manager (if you switch that on), which I don't think we can funnel through a provider.

[ntnn]

I think "local cluster" is a misnomer because it suggests that it should be the cluster the manager is running in; instead it should be named appropriately that it is used for orchestrating multiple managers for leader election, leases etc.pp. That control plane can't come from a provider (and shouldn't, otherwise mcr would have to implement cross-cluster leader election, ...). In the picture above the local cluster would be the control plane with leases.

At the moment I call that "coordination control plane" and pass that to mcr as the "local cluster".

I also don't agree that we can expect all mcr operators to work out of the same cluster, hence I think "local cluster" is a misnomer on a cross-cluster infrastructure, where I expect mcr operators to use a remote control plane to orchestrate.

[crenshaw-dev]

I see how "local cluster" is a helpful concept (although it's possible there's a more fitting word?) when dealing with features that solely relate to the controller's host cluster, like leader election.

But I find that the local cluster concept is confusing when applied to features that relate to multiple clusters (which may include the local one). I start wondering "Is the local cluster treated differently somehow?" If there is something special about the local cluster in that context, maybe it does make sense to have that term there. But if it's just another cluster, I'd rather drop it in a multi-provider and treat it as such.

[ntnn]

Made a first draft on an in-tree engage option to skip engaging a cluster altogether for a controller:
https://github.com/ntnn/multicluster-runtime/tree/cluster-filtering

It's very hacky. Ideally .ForCluster would return (Source, bool, error), but Source/TypedSource being nil can be fine as well. But would need nicer handling in the callers.

The names aren't final (or nice :D), I just kinda went off of what's there already. WithEngageFilterClusters would probably be better as WithEngageForClusters(filterFn) or something to that degree. It's also not honouring the other options, as noted in the TODOs.

The type alias is not super nice but better than requiring users to import another package that they don't technically need.

Here's an example: https://github.com/ntnn/mcr-scheme-headaches
cmd/latest points to current mcr latest, cmd/forked points to the branch cluster-filtering of my fork.

	if err := mcbuilder.ControllerManagedBy(mgr).
		Named("target-controller").
		For(&headachev1alpha1.Target{}, mcbuilder.WithEngageFilterClusters(
			func(clusterName string, cluster cluster.Cluster) bool {
				return clusterName == "target"
			},
		)).
		Complete(tr); err != nil {
		setupLog.Error(err, "unable to create controller", "controller", "Target")
		os.Exit(1)
	}

Basically the WithEngageFilterClutsers takes a function and when a cluster is engaged this filter function decides whether the cluster should be engaged.
I could see a helper WithEngageForProvider that sets the same function (and hence is mutually exclusive with WithEngageFilterClusters) with something like this:

func WithEnageForProvider(provider multicluster.Provider) EngageOptions {
	return EngageOptions{
		engageFilterClusters: func(clusterName string, cluster cluster.Cluster) bool {
			cl, err := provider.Get(context.Background(), clusterName)
			if err != nil {
				return false
			}
			return cl == cluster
		},
	}
}

[ntnn]

Made a WIP PR for cluster filtering: #114