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

Use a single cache for all dynamic controllers (i.e. XRs and claims) #5651

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

negz
Copy link
Member

@negz negz commented May 7, 2024

Description of your changes

Fixes #5338
Fixes #2645
Closes #5463
Closes #5468
Closes #5422

It's possible this will fix #5400, #5151, #5533, and #5228 given that it makes large changes to how realtime compositions work. I'd rather not assume though - we can see if folks can reproduce if/when this PR is merged.

Crossplane uses a "controller engine" to dynamically start claim and XR controllers when a new XRD is installed, and stop them when the XRD is uninstalled.

Before this PR, each controller gets at least one cache. This is because when I built the controller engine, you couldn't stop a single informer within a cache (a cache is basically a map of informers by GVK). Instead, we stop entire caches.

When realtime composition is enabled, there are even more caches. One per composed resource GVK. A GVK routed cache routes cache lookups to these various delegate caches.

Meanwhile, controller-runtime recently made it possible to stop an informer within a cache. It's also been possible to remove an event handler from an informer for some time (since Kubernetes 1.26):

This PR uses a single client, backed by a single cache, across all dynamic controllers (specifically the definition, offered, claim, and XR controllers). Each controller tracks what watches ("sources" in controller-runtime) it has active and stops them when it doesn't need them anymore. The controller engine also garbage collects custom resource informers when their CRDs are deleted.

Compared to the current implementation, this:

  • Takes fewer global locks when realtime compositions are enabled. Locking is now mostly at the controller scope.
  • Works with the breaking changes to source.Source introduced in controller-runtime v0.18. :)

I also think this makes the realtime composition code a little easier to follow by consolodating it into the ControllerEngine (and into one repo), but that's pretty subjective.

Here's a questionable Mermaid diagram of how it works:

graph TD
	subgraph ITC[engine.InformerTrackingCache]
		I1[XFoo Informer]
		I2[XBar Informer]
		I3[ComposedResource Informer]
	end
	
	subgraph CE[engine.ControllerEngine]
		subgraph C1[XFoo Controller]
			R1[XFoo Reconciler] -- Get --> I1
			R1 -- Get --> I3
			S1[XFoo Source] -- Watch --> I1
			S1 -- Enqueue --> R1
			SC1[ComposedResource Source] -- Watch --> I3
			SC1 -- Enqueue --> R1
		end

		subgraph C2[XBar Controller]
			R2[XBar Reconciler] -- Get --> I2
			R2 -- Get --> I2
			S2[XFoo Source] -- Watch --> I2
			S2 -- Enqueue --> R2
			SC2[ComposedResource Source] -- Watch --> I3
			SC2 -- Enqueue --> R2
		end
	end

	subgraph C[cache.Cache]
	  I4[XRD Informer]
	end

	subgraph M[manager.Manager]
		subgraph C3[XRD Controller]
			R3[XRD Reconciler] -- Start --> C1
			R3 -- Start --> C2
			R3 -- Get --> I4
			S3[XRD Source] -- Watch --> I4
			S3 -- Enqueue --> R3
		end
	end

It's conceptually pretty similar to a typical set of controller-runtime controllers run by a controller.Manager, but with the ability to stop watches and informers.

Reviewer Note

This PR depends on crossplane/crossplane-runtime#689.

Unfortunately, this required bumping a bunch of dependencies, mostly to get the latest controller-runtime. Those dependency bumps in turn required a bunch of other little fixes (CRD generation issues, new linter warnings, etc).

I also opted to move the ControllerEngine implementation from c/cr to c/c. I think that will make changes like this easier in future.

While this all makes this PR pretty huge, it's all broken out into in separate commits. The last commit in the PR contains the bulk of the changes - reviewers can focus on that.

I'm opening this as draft since it's a big change. I still need to add more tests for the new code (e.g. the new engine implementation).

I have:

Need help with this checklist? See the cheat sheet.

@negz
Copy link
Member Author

negz commented May 7, 2024

CC @sttts - this makes big changes to the realtime compositions implementation.

of resource.CompositeClaimKind
with resource.CompositeKind
opts []ReconcilerOption
client client.Client
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes to this file are all mechanical - just changing how we pass a client.

Previously we passed a whole manager just to get its client. By passing a client specifically we can pass in a special client that is backed by our cache.

internal/controller/apiextensions/composite/reconciler.go Outdated Show resolved Hide resolved
internal/controller/apiextensions/composite/watch/watch.go Outdated Show resolved Hide resolved
Comment on lines +50 to +51
// TODO(negz): Check whether the revision's compositeTypeRef matches
// the supplied CompositeKind. If it doesn't, we can return early.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should do this in a future PR since this one is already very large.

internal/controller/engine/source.go Outdated Show resolved Hide resolved
go.mod Outdated

toolchain go1.22.2

// DO NOT MERGE. See https://github.com/crossplane/crossplane-runtime/pull/689
replace github.com/crossplane/crossplane-runtime => github.com/negz/crossplane-runtime v0.0.0-20240505223500-f00d4be5f64a
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't merge until this is gone. I like that there's a linter for this now.

cmd/crossplane/core/core.go Outdated Show resolved Hide resolved
internal/controller/engine/engine.go Outdated Show resolved Hide resolved
internal/controller/engine/engine.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@negz negz force-pushed the engine-swap branch 2 times, most recently from c590da9 to 0ba7db9 Compare May 8, 2024 05:35
@negz
Copy link
Member Author

negz commented May 8, 2024

Looks like the E2E tests are passing pretty reliably now. The only thing blocking moving this out of draft is writing some more unit tests.

@turkenh
Copy link
Member

turkenh commented May 8, 2024

I also opted to move the ControllerEngine implementation from c/cr to c/c. I think that will make changes like this easier in future.

I am working on implementing watches in provider-kubernetes, and the PR is based on the previous real-time composition implementation, which is being refactored here. While the provider-kubernetes use case is more straightforward (no controller start/stop at runtime), I believe the new structure is more usable with the unified/shared cache then the previous implementation. When I check the functionality introduced here, I have a feeling like I could leverage things like TrackingInformers/InformerTrackingCache and StoppableSource, and maybe even Controller Engine with a single controller (TBD) 🤔 So, I would need them to be importable, at least not under internal if not back to crossplane-runtime 🙏

go.mod Outdated Show resolved Hide resolved
@negz
Copy link
Member Author

negz commented May 8, 2024

So, I would need them to be importable

@turkenh How would you feel about duplicating them to start with? We could then consolidate later.

I'm hopeful we can get the tracking cache and stoppable source functionality added upstream to controller-runtime. If that happens I could imagine a world where:

  • ControllerEngine is only used by c/c, lives in c/c (internal)
  • Provider-kubernetes also uses tracking cache and stoppable sources, which live in controller-runtime

negz added 29 commits May 17, 2024 12:25
Signed-off-by: Nic Cope <nicc@rk0n.org>
This bumps crossplane-runtime, controller-runtime, and k8s.io
dependencies to latest.

Depends on crossplane/crossplane-runtime#689

Signed-off-by: Nic Cope <nicc@rk0n.org>
Updating our controller-runtime and Kubernetes dependencies bumped our
minimum Go version to v1.22. That in turn enables some new linters,
since we no longer need to copy range vars in Go v1.22.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
It started as a function, but now we pass several arguments that are all
fields of the Reconciler. It's only called once, by the Reconciler.
Making it a method shortens the function signature, and makes it clear
which things change on each reconcile and which are fixed.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Crossplane uses a controller engine to dynamically start claim and XR
controllers when a new XRD is installed.

Before this commit, each controller gets at least one cache. This is
because when I built this functionality, you couldn't stop a single
informer within a cache (a cache is basically a map of informers by
GVK).

When realtime composition is enabled, there are even more caches. One
per composed resource GVK. A GVK routed cache routes cache lookups to
these various delegate caches.

Meanwhile, controller-runtime recently made it possible to stop an
informer within a cache. It's also been possible to remove an event
handler from an informer for some time (since Kubernetes 1.26).

kubernetes-sigs/controller-runtime#2285
kubernetes-sigs/controller-runtime#2046

This commit uses a single client, backed by a single cache, across all
dynamic controllers (specifically the definition, offered, claim, and
XR controllers).

Compared to the current implementation, this commit:

* Takes fewer global locks when realtime compositions are enabled.
  Locking is now mostly at the controller scope.
* Works with the breaking changes to source.Source introduced in
  controller-runtime v0.18. :)

I think this makes the realtime composition code a little easier to
follow by consolodating it into the ControllerEngine, but that's
pretty subjective.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Just a little optimization. GCing informers requires taking a read lock
on the entire ControllerEngine. There's no reason to do that if we
didn't actually remove any of a controller's watches.

Signed-off-by: Nic Cope <nicc@rk0n.org>
The XRD controllers (definition, offered) start dynamic controllers for
XRs and claims. The XRD controllers aren't dynamic though. Their
lifecycle is coupled to the controller manager that starts when the
Crossplane pod starts, and stops when the Crossplane pod stops.

The XRD controllers are added to the controller manager, so they use the
manager's informers to enqueue reconciles. This means they should also
use the manager's clients. If they don't, there's a risk their client is
cache-backed and lagging behind the manager's cache. This causes them to
return early from a reconcile due to the resource they're reconciling
appearing to not exist. When they return early, they don't requeue. The
resource never gets reconciled.

This commit updates the XRD controllers to use the controller manager's
client, not the controller engine's client. The controller engine's
client should only be used by dynamic controllers - i.e. XR and claim
controllers.

I don't love the controller-runtime style GetClient pattern this uses,
but the reality is the client is strongly coupled to the engine. It's
important that the client is backed by the same cache the engine uses to
setup watches for its controllers.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Crossplane uses various types that embed Kubernetes's
unstructured.Unstructured type and add convenience getters and setters.
Crossplane's unstructured.NewClient automatically wraps and unwraps our
unstructured types before passing them to a controller-runtime Client.

This doesn't appear to be necessary unless the Client is backed by a
Cache. Our types satisfy the runtime.Unstructured interface, so the
Client seems to handle them correctly.

When the Client is backed by a Cache, controller-runtime seems to mostly
setup an informer for our types correctly (it tries to support any type
that satisfies the runtime.Unstructured interface). This falls apart
because it populates the cache using a client-go DynamicClient, which is
hardwired to store *unstructured.Unstructured specifically. This results
in a type mismatch somewhere in the watch machinery.

Signed-off-by: Nic Cope <nicc@rk0n.org>
We need to get and remove informers inside the critical sections to
avoid a race where e.g.:

1. Goroutine A marks informer as active
2. Goroutine B marks informer as inactive
3. Goroutine B removes informer
4. Goroutine A gets (starts) informer

This would result in an untracked informer.

Now we'll either:

* Return an active informer while we have the read lock.
* Use the read lock to determine the informer is inactive, then take a
  write lock to mark the informer as active and return it.

Either way when we potentially create an informer we have a lock that
prevents another goroutine from marking it inactive and removing it.

I'm still using a RWLock to try to avoid taking a write lock any time
a controller reads from the cache. We don't need an upgradeable lock
though - it's fine if e.g. another Goroutine marks an informer as
inactive between when we release our read lock and take a write lock.
The write lock section will undo that - it will mark the informer active
and restart it.

Signed-off-by: Nic Cope <nicc@rk0n.org>
In a bunch of places I was:

1. Taking a read lock, reading state, releasing the read lock.
2. Computing something based on the read state.
3. Taking a write lock and using the computation to mutate the state.

The issue with this approach is that Goroutine B can take the write
lock and mutate state between when Goroutine A reads state and when it
writes new state based on what it read. Goroutine A is operating on
stale state.

I think there's some benefit in using an RWMutex to avoid blocking
everyone when there's no work to do, but a better approach is to:

1. Take the read lock, figure out what work needs to be done, release
   the read lock and return early if there's no work to be done.
2. If there's work to be done, take the write lock, figure out again
   what work there is to be done, and do it.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Before this commit the engine treats watches as unique by GVK, but
they're not. They're unique by (GVK, handler, predicates) tuple.

After this commit, the engine considers watches unique by (WatchType,
GVK) tuple. A WatchType essentially represents a unique combination of
handler and predicates.

I considered using reflection to use handler and predicate type as part
of the keys used to store the map of a controller's watches but it felt
too magic and fragile.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
This commit updates the controller engine to _only_ garbage collect
informers when:

* The informer is for a CR
* The CRD that defines the CR is deleted

This is necessary to avoid informers getting stuck in an error state
when the CRD that defines the CR they inform on is deleted.

Notably, this allows the engine to cache unstructured resources (i.e.
XRs, composed resources) even when realtime compositions is disabled.
This is because informer garbage collection no longer relies on tracking
watches to know when its safe to garbage collect an informer.

The downside of this approach is that informers are only garbage
collected when a CRD is deleted. This is (in theory) an issue for
composed resources. When an XR starts composing a new kind of resource,
the engine will start an informer for it. If eventually no XR composes
the kind of resource anymore, but the CRD defining it isn't deleted, the
informer will run needlessly. This will consume Crossplane memory and
keep a watch open on the API server.

I'm not sure how much of a problem this will be in practice. I'm
thinking we should not address it to begin with. If it does prove to be
an issue we could periodically garbage collect unused informers - or
just stop them all and let them be restarted if needed.

Signed-off-by: Nic Cope <nicc@rk0n.org>
I prefer to keep internal/controller limited to implementations of
controllers.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
It did in a previous implementation. Now they're only GCed when their
CRD is deleted. This updates the stale comments to avoid mentioning
GCing informers.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants