reconciler-runtime icon indicating copy to clipboard operation
reconciler-runtime copied to clipboard

Automatically watch tracked resources

Open scothis opened this issue 4 years ago • 3 comments

Both the ParentReconciler and ChildReconcilers properly watch resources for when the resource changes, the resource is enqueued for reconciliation. This doesn't happen automatically with the SyncReconciler when manually looking up and tracking a resource. Currently the watches are configured when the reconciler is setup, which requires knowledge of which resources to watch.

Duck types enable consuming a resource without requiring advanced knowledge of the concrete resource in advance, often we won't know the concrete resource until see an ObjectReference that has the address coordinates for the concrete type. We should enhance (or wrap) the tracker so that when a reference is tracked, there is a corresponding informer for the gvk started so we can enqueue the object.

A few thoughts that may be useful, or a distraction:

  • the version of tracked resources is irrelevant as versions are a projection of an underlying resource, yet informers must be configured for a specific version
  • metadata only informers contain everything we need to track a resource
  • informers should be started lazily, we don't need to wait for the cache to sync
  • gvks that are no longer tracked should have their informers stopped and cache released
  • reusing existing informers may be useful to reduce load, but may make cleanup harder
  • RBAC issues accessing a new gvk should be handled gracefully. The RBAC policies may change at runtime granting or revoking access
  • new apiGroups, versions and kinds can be added and removed at runtime
  • Knative is able to dynamically spin up informers for references resources, so there's precedent that this is possible (although controller-runtime may throw a wrench at us)

An example of SyncReconciler that has foreknowledge of the resource that are being tracked:

https://github.com/projectriff/system/blob/1fcdb7a090565d6750f9284a176eb00a3fe14663/pkg/controllers/core/deployer_reconciler.go#L66-L148

The Setup method should be able to go away if this issue is resolved:

Setup: func(mgr reconcilers.Manager, bldr *reconcilers.Builder) error {
	bldr.Watches(&source.Kind{Type: &buildv1alpha1.Application{}}, reconcilers.EnqueueTracked(&buildv1alpha1.Application{}, c.Tracker, c.Scheme))
	bldr.Watches(&source.Kind{Type: &buildv1alpha1.Container{}}, reconcilers.EnqueueTracked(&buildv1alpha1.Container{}, c.Tracker, c.Scheme))
	bldr.Watches(&source.Kind{Type: &buildv1alpha1.Function{}}, reconcilers.EnqueueTracked(&buildv1alpha1.Function{}, c.Tracker, c.Scheme))
	return nil
},

scothis avatar Dec 08 '20 01:12 scothis

  • gvks that are no longer tracked should have their informers stopped and cache released

It looks like controller-runtime's Controller interface doesn't allow watches to be removed. How hard a requirement is this?

glyn avatar Dec 08 '20 11:12 glyn

  • gvks that are no longer tracked should have their informers stopped and cache released

Also, a different issue, I'm wondering how often in practice the tracker Lookup method is called. If it's infrequently or never called, then we may need to add time-based cleanup of expired trackers so they don't "pin" the corresponding informers.

It seems that Lookup is called by the informer callback, so I guess that's sufficient because if a watch is causing overhead, then it's informer callback will be driven, any expired trackers will be deleted, and the corresponding informers will no longer be pinned.

glyn avatar Dec 08 '20 14:12 glyn

We don't need to get hung up on removing informers on the first pass. Let's get it working and then figure out what we need to optimize. If we have to push support upstream or create a different informer stack we can cross that bridge when we get there.

scothis avatar Dec 08 '20 14:12 scothis