Controller-runtime: Controller reconciler GET is reading stale data

[Abhishek-Srivastava]

I have a query on the reconcile logic with 1 worker for a custom controller I have, generated using kubebuilder.

In the reconciler logic for a CR, I am doing the following in sequence:

• Update the status subresource (Mark status as Busy).
• Make a synchronous GRPC (blocking) call to a GRPC server running as a different pod.
• Update the status subresource (Mark status as Ready)

The updates status queues reconciliation. The immediate reconciliation after status update is picking up a stale version of the resource which doesn't reflect the updated version of the status (the status shows Busy). It was expected to pick the next generation of the resource which was updated by the previous reconciliation(the status should have been Ready).
Retrying a Get again though, gives the updated resource . Is this a known bug/expected behavior ? Is there a workaround/recommendation ?

```kubectl version
Client Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.0", GitCommit:"e19964183377d0ec2052d1f1fa930c4d7575bd50", GitTreeState:"clean", BuildDate:"2020-08-26T14:30:33Z", GoVersion:"go1.15", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-06-03T04:00:21Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}

Go deps:
k8s.io/api v0.17.2
k8s.io/apimachinery v0.17.2
k8s.io/client-go v0.17.2
sigs.k8s.io/controller-runtime v0.5.0```

[alvaroaleman]

The default client is backed by a cache which lags behind a bit. There is a couple of ways to cope with this, the easiest is probably to block in your reconcile after updating until you can read the updated data again, something like https://github.com/kubernetes/test-infra/blob/8f0f19a905a20ed6f76386e5e11343d4bc2446a7/prow/plank/reconciler.go#L516-L520

[kstiehl]

@alvaroaleman
Do you have more informations on that? I am currently running into the same problem. And a client cache that lags behind is just not acceptable imo.
But I dont find the place where the cache is actually evicted. If anyone could point this out to me I'd look into it

[alvaroaleman]

@kstiehl see https://github.com/kubernetes-sigs/controller-runtime/blob/master/FAQ.md#q-my-cache-might-be-stale-if-i-read-from-a-cache-how-should-i-deal-with-that

Caches lag behind by definition, if that is not acceptable for your use-case, you can use a live client and pay the performance-overhead for that.

Im closing this issue as working as intended.

[kstiehl]

I agree with you that a cache lags behind if someone else is doing the changes, so another process or another client. Since it bypasses the cache.
However if the controller itself is doing an update on the resource the cache should be invalidated, since we know that the data inside is outdated.

Caches lag behind by definition, if that is not acceptable for your use-case, you can use a live client and pay the performance-overhead for that.

Thats just not true.
Imagine your filesystem cache would lag behind a bit. You change a file and then you read it and receive old data.

[alvaroaleman]

Once your "cache" is strongly consistent with your main data store you don't have a cache anymore, you have a read-only replica.

The machinery underlying all of this works by establishing a watch and getting changes pushed from the apiserver. This has a bit of delay. It does not support using something else to update the cache and I doubt it ever will. You should generally always build your code under the assumption that other actors might be modifying the objects your dealing with.

[kstiehl]

Once your "cache" is strongly consistent with your main data store you don't have a cache anymore, you have a read-only replica.

Yes we are definitely on the same page here. We won't get strong consistency with a cache. Fully agree. Someone else could always make a change which is not immediately reflected in my cache. I am perfectly fine with that.

But when you write operators/controller you normally do incremental changes during your reconciliation loop to bring your resources closer to the desired state. That's a pattern most operators have in common. The second reconcile request will almost always get outdated data.

The machinery underlying all of this works by establishing a watch and getting changes pushed from the apiserver. This has a bit of delay. It does not support using something else to update the cache and I doubt it ever will.

Yes I also understand that part. The problem is that we have to watches. One watch which is used to update the cache. And another watch which triggers new reconileRequests. And the reconcileRequest watch tends to be faster than the watch which is used to keep the cache in sync.

[alvaroaleman]

Yes I also understand that part. The problem is that we have to watches

No we don't, the same watch both triggers and backs the client.

Controller-Runtime is mostly a bunch of wrappers around low-level client-go aspects plus a set of packages that are useful in the context of a controller. Extending the client-go informer cache to allow getting updates from a different source than the watches might or might not be possible with the interface client-go exposes today, but at the very least it will:

• Be very complex, thus hard to maintain and easy to have bugs
• Encourage ppl to write code under the assumption that the cache is current, which it won't be if something else updates objects

So I would be strongly opposed to add anything like that in controller-runtime. If anything, this would need to reside in upstream client-go. I encourage you to talk to the sig-apimachinery folks if you think this is a good idea.

[coderanger]

More to the point, we take advantage of the fact that the same informers drive both the cache and the reconcile event processing to ensure we never miss events. You might get more reconciles than you want but you'll never get too few. Changing the informers to a write-through mode (if it was even possible) would break that which is way more important for system safety :)

Also because it wasn't mentioned so far, you can very easily use a non-cached API client for weird cases where reading stale data is a problem (for example in integration tests). They are just much much slower than the cached clients for read operations.

[Abhishek-Srivastava]

@alvaroaleman @coderanger @kstiehl
Is there to know what would be the right amount of time to wait for the cache to be updated?
@alvaroaleman Like you pointed here: https://github.com/kubernetes/test-infra/blob/8f0f19a905a20ed6f76386e5e11343d4bc2446a7/prow/plank/reconciler.go#L516-L520 It probably tries 20 times in 2 seconds to match the state. Say, if want to make it configurable, how can we arrive at the right polling intervals and trials ? Is there something which can help us on that?

[kstiehl]

So the use case I have is that I do an update in one reconcile loop. Which triggers another reconcile loop which also triggers an update to the resource.
In the second reconcile loop I use r.Get which gives me an object with a too old resourceVersion and since the resourceVersion doesn't match with the one in k8s, the update fails.

So what would be the correct way to retrieve the resource from the cluster. Yes you guys keep mentioning that I could just disable the cache, but is this use-case I describe really too special? I mean that's something that probably every operator does.
So either I am missing something very obvious or the cache is totally useless and just causes problem.

And you guys must agree that I can't go through my code and do this in every operator we have in our company?
https://github.com/kubernetes/test-infra/blob/8f0f19a905a20ed6f76386e5e11343d4bc2446a7/prow/plank/reconciler.go#L516-L520

EDIT:
Sorry if I get annoying about this topic. I am just trying to understand the intentions behind this whole cache thing. Since I really don't see a use case where it wouldn't at least cause random bugs.

[Abhishek-Srivastava]

@kstiehl This is the exact use-case I have.

[alvaroaleman]

In the second reconcile loop I use r.Get which gives me an object with a too old resourceVersion and since the resourceVersion doesn't match with the one in k8s, the update fails.

You are not supposed to do anything about this. The Update will fail, your reconciler will return an error, that will result in a delayed retry, by which time the cache caught up, so on that attempt it will succeed. If not, same loop again until it does.

[coderanger]

Once again Alvaro gets there first and correctly but to give the broader answer: I think you need to rework your mental model of controllers :) A common thing for new operator devs is they think in terms of procedural code, steps happening in order. Controllers aren't like that, they are convergent systems. This is an intentional design choice in Kubernetes to force you into building things in a more robust way. As the previous comment said, if you ever can't make forward progress towards convergence, you try again later, possibly forever. In databases we call this "eventual consistency" but it's the same idea, your controller will just keep trying to make the current state match the requested state over and over.

[kstiehl]

@alvaroaleman Thanks for the clarification. This is actually the way it is implemented currently, so in the end the resource can be provisioned, I just have a log full of reconciliation exceptions, but if you say thats intended this I am fine with that.

@coderanger
Actually my operator is not written procedural at all, that's why I am doing incremental changes which get me closer to the desired state of the resource.

This is an intentional design choice in Kubernetes to force you into building things in a more robust

Yes you are right it is robust in a way that in the end everything works and the resource is provisioned successfully. So I get the advantages of that programming model. However I come from a time where a log full of exceptions isn't considered robust software.

Anyways thanks to both of you for clarifying this to me, this helped me a lot to understand the behind the scenes stuff and also better understand the intentions behind the client cache.

I know sometimes these discussions on GitHub get a little heated, but I guess thats always the case when different opinions clash (especially in software development) .
So I just want to clarify I am thankful that you both took the time to answer my questions and of course for your work on the project, although we obviously have different opinions in some stuff 😄

Cheers 🍻

[coderanger]

If your only concern is the error log, you can instead return from the reconciler with no error and set Requeue: true and the same retry behavior will activate :) It being an actual error or not is up to whatever serves your needs.

[kstiehl]

I am more worried about changes I potentially miss.
For example when a reconcile is triggered because something was changed in the resource. I try to check what actually changed but I get the old resource from the cache. So my reconcile loop would do nothing and not return an error. And simply miss this change. And I guess the only way to deal with this is to disable the cache?

Or is there a mechanism that helps me to catch up with missed changes?

[coderanger]

That is not possible. As I said the notifications for the reconcile and the cache are sync'd to the same informer data stream so whatever event triggered things, it will 100% be visible in its cache. There might also be later events still pending but when those get to you, you'll get another reconcile.

[kstiehl]

I can confirm that the sync that you describe works perfectly fine when the resource was updated using r.Update().
But it's not when using r.Status().Update(). A change to the Status Subresource also changes the resourceVersion on the root resource.
So this example code here works exactly in the first reconcile loop. Every subsequent reconcile loops will fail since the reconcile are not in sync as you say.

func (r *ExampleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
    resource := &v1.Example{}
    _ = r.Get(ctx, req.NamespacedName, resource) // gets always cached data
    resource.Status.SomeValue = strconv.Itoa(rand.Int())
    err := r.Status().Update(ctx, resource)  // will fail in every reconcile loop except the first one
    return ctrl.Result{}, err
}

[coderanger]

Yes, it will. Which is why you shouldn't write code like that. Use Patch requests instead :)

https://github.com/coderanger/controller-utils/blob/e26c5734ecc941a03e9bf8798886a419e5202126/core/reconciler.go#L325-L330

That's my standard one. Otherwise as mentioned several times already you can check if the error is a version conflict and return with Requeue set. Either works, obviously I like my solution better.

[kstiehl]

https://github.com/coderanger/controller-utils/blob/e26c5734ecc941a03e9bf8798886a419e5202126/core/reconciler.go#L325-L330

Thank you so much that's finally the missing puzzle piece 🥳
@Abhishek-Srivastava I guess this is also the code you were looking for?

[Abhishek-Srivastava]

@kstiehl I am yet to read the full thread, but this what I have been doing:

    // create a patch object from the existing/original object
    origResource := client.MergeFrom(instance.DeepCopy())

    // use the patch method to update object by appling merge-patch on origResource
    if err := r.Status().Patch(context.Background(), server, origResource); err != nil {
        if errors.IsConflict(err) {
            // retry the status update ONCE. If errored again, return.
            if err := r.Status().Patch(context.Background(), server, origResource); err != nil {
                logger.Log.Error(err, "Failed again to update the status of the server")
                return err
            }
        }
        logger.Log.Error(err, "Failed to update the status of the server")
        return err
    }