Skip to content

Commit

Permalink
Fix advice on using manager's client in tests
Browse files Browse the repository at this point in the history
Previous documentation erroneously misconfigured the cronjob's tutorial
to use the manager's client everywhere, which is poor practice because
using a cache client in assertions leads to flaky & slow tests.

A new PR overcorrected, and switched both assertions *and* the
reconciler to use a live client, which is also wrong, because the
reconciler needs to use a manager's client to function properly.

This corrects the documentation to indicate that reconcilers should use
managers' clients, and test assertions should use a live client, and
adds a full explanation as to why.
  • Loading branch information
DirectXMan12 committed Apr 26, 2021
1 parent d7c180d commit fc4f99a
Showing 1 changed file with 10 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,16 @@ var _ = BeforeSuite(func() {
The only difference is that the manager is started in a separate goroutine so it does not block the cleanup of envtest
when you’re done running your tests.
It is not recommended to use the manager client in tests because it is not strongly consistent. Indeed, the manager
client is designed to do the "right thing" for controllers by default which is to read from caches. The best solution
is to instantiate a new client using client.New for tests (as k8sClient above). It will provide a client that reads
directly from the API meaning that you can write tests expecting read-after-write consistency.
However, keep in mind that you should not do this in the controller's conciliation loop (read an object after you have
written it). Kubernetes favors an approach where you first do some reads, process and then do some writes and return.
This way, you let the queue take care of the next cycle of readings if they are necessary.
Note that we set up both a "live" k8s client, separate from the manager. This is because when making assertions in
tests, you generally want to assert against the live state of the API server. If you used the client from the
manager (`k8sManager.GetClient`), you'd end up asserting against the contents of the cache instead, which is slower
and can introduce flakiness into your tests. We could use the manager's `APIReader` to accomplish the same thing,
but that would leave us with two clients in our test assertions and setup (one for reading, one for writing), and
it'd be easy to make mistakes.
Note that we keep the reconciler running against the manager's cache client, though -- we want our controller to
behave as it would in production, and we use features of the cache (like indicies) in our controller which aren't
available when talking directly to the API server.
*/

k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
Expand Down

0 comments on commit fc4f99a

Please sign in to comment.