Skip to content

Commit fc4f99a

Browse files
committed
Fix advice on using manager's client in tests
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.
1 parent d7c180d commit fc4f99a

File tree

1 file changed

+10
-8
lines changed
  • docs/book/src/cronjob-tutorial/testdata/project/controllers

1 file changed

+10
-8
lines changed

docs/book/src/cronjob-tutorial/testdata/project/controllers/suite_test.go

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

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

0 commit comments

Comments
 (0)