Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically watch tracked resources #54

Open
scothis opened this issue Dec 8, 2020 · 3 comments
Open

Automatically watch tracked resources #54

scothis opened this issue Dec 8, 2020 · 3 comments
Assignees
Labels

Comments

@scothis
Copy link
Contributor

scothis commented Dec 8, 2020

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
},
@glyn
Copy link
Contributor

glyn commented Dec 8, 2020

  • 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
Copy link
Contributor

glyn commented Dec 8, 2020

  • 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.

@scothis
Copy link
Contributor Author

scothis commented Dec 8, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants