If the two managers share the same cache, the parent manager needs to have some way of making sure child managers don't un-watch resources that the parent manager is interested in (e.g. Constraint Templates here).

I believe that is handled here?

The link is broken for me. Where is this handled? I kept an eye out, but couldn't find anything obvious.

Let me make sure I understand the question.
Within a watch manger, with multiple registrars watching the same resource, the watch manager performs reference counting and only removes the informer when the last registrar unregisters. That happens in this code in doRemoveWatch:

// Skip if there are additional watchers that would prevent us from removing it
m := wm.managedKinds.Get() // Not a deadlock but beware if assumptions change...
if _, ok := m[gvk]; ok {
	return nil
}

If you're asking about having multiple watch managers in-process, they would be unaware of each other. I'd say that is an unsupported use-case - should we document that to make it more obvious? Do you have another idea?

Also one more clarification - we no longer need multiple controller-runtime managers in a single process. The parent/child manger relationship previously used has been removed.

The controller runtime manager and the watch manager share the same cache right?

What happens if I add a sync for constraint templates and then remove it. Is the watch that the controller manager established preserved somehow?

In practice they wouldn't affect each other as the watch manger is only dealing in unstructured informers and the constrainttemplate controller would be using a structured informer. These are completely separate in the InformerCache.
I could think of safeguards to put in but perhaps we should handle those as followup tasks?

Ah, I see the unstructured/structured non-interaction thing.

I think safeguards would be good as this seems like a very subtle behavioral difference to rely on and I wonder if the fact that they are different caches is officially part of the contract.

That being said, I'm fine with deferring more explicit safeguards to a follow-on change. Hopefully sooner rather than later.

Asking the watch manager seems superfluous, as the channel wouldn't have received the event w/o registration.

A scenario I was considering was when events are in-flight in the various queues and are received after we have deregistered the watch. I think concretely for this controller it will be fine - de-registration happens after the parent template is removed from OPA, by which point either constraints should have been removed by the garbage collector or if their cache lags, a lookup in OPA should fail and we should drop the request.

Is there an upper limit to how many sources we can have?

There is not. Each controller has their own channel to receive event fan-out from the watch manager. The channel is buffered only to provide some cushion in case the controller is slow to consume from that channel. Consumption is comprised of encoding the event and putting it into a work queue, so it is not blocked on reconciliation time and I do not expect this to become an issue.

The current event distribution is a bit naive and can be improved:

	// Distribute the event
	for _, w := range watchers {
		select {
		case w <- e:
		// TODO(OREN) add timeout
		case <-stop:
		}
	}

^ theoretically a slow consumer that ends up blocking would cause event distribution to block for other consumers as well. This can be improved with a goroutine per consumer in distributeEvent, but at the cost of additional complexity.

nit: consolidate import groups

I've been keeping separation between standard library and external imports. Is that acceptable or do you prefer a flat list mixing both?

That separation is fine, there are some places where there are more than 2 import blocks though.

Sorry about that, these are artifacts of GoLand / goimports introducing breaks and me not paying much attention to the collapsed blocks. I've fixed where you called them out.

ACk, thanks. I hear you WRT the goimports thing. I get the same thing and try to stay on it, but it's definitely not 100% success rate on my end either.

The link is broken for me. Where is this handled? I kept an eye out, but couldn't find anything obvious.

Should inProgress be triggered by whether the replay is in byRegistrar?

What happens if Registrar A adds a watch, Registrar B adds a watch (and gets replay), and then Registrar C adds a watch? Will a replay trigger?

You're right, this looks like a bug. I'll fix and add a corresponding test scenario. Good catch!

Extended test (which initially failed 👍) and fixed.

nit: change -> changed

Fixed.

Separate reader and writer because manager's default client bypasses the cache for unstructured resources.

Is there any documentations around this?

could there be any issues with reader.Get or reader.List when reading from the cache? e.g. data freshness

I don't know that it's documented, but the code is here: https://github.com/kubernetes-sigs/controller-runtime/blob/v0.5.0/pkg/manager/manager.go#L328-L330

Regarding data freshness - we should be good. Although the watch.Manager does not block for cache sync when adding a new watch, if you do a reader.Get or reader.List, controller-runtime will block on WaitForCacheSync here:

whats the reason for removing this for sync?

The ControllerSwitch was only needed in controllers that would fight conflict with the teardown logic in main.go:L194. The sync controller isn't writing status or removing finalizers so there shouldn't be any contention to worry about.