✨ Read your own write client design

[alvaroaleman]

Related to #3320
Minimal POC in #3472

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

This hinges on the assumption that we are guaranteed to get a delete event that contains the object with at least namespace, name and UID even if we end up getting a tombstome from the cache - I think that is the case, but would be good to get confirmation

[michaelasp]

I believe so as well, the RV is the main thing that cannot be guaranteed due to possibility of dropped events. We'd have to see what happens in the case that we drop watch events and we have to relist however, I wonder if anything strange occurs in the scenario that we delete -> recreate -> delete while our events get dropped. I don't think this would affect the internal impl though.

[alvaroaleman]

The whole having to observe the delete event business is quite finicky, it would be a lot easier if deletes would always provide an rv, but according to @liggitt that is problematic: kubernetes/kubernetes#134937 (comment)

[michaelasp]

Yeah, I wish there was a better way to deal with delete events. Might be good to discuss if there's anything we can do upstream to make this behavior work more nicely.

[serathius]

/cc @michaelasp

[michaelasp]

I believe so as well, the RV is the main thing that cannot be guaranteed due to possibility of dropped events. We'd have to see what happens in the case that we drop watch events and we have to relist however, I wonder if anything strange occurs in the scenario that we delete -> recreate -> delete while our events get dropped. I don't think this would affect the internal impl though.

[michaelasp]

Yeah, I wish there was a better way to deal with delete events. Might be good to discuss if there's anything we can do upstream to make this behavior work more nicely.

[michaelasp]

What happens in the case where we trigger the delete but then the object gets recreated and we drop events forcing a relist? UID should be enough for this case right?

[alvaroaleman]

Yeah, that is my understanding

[michaelasp]

This will cause certain high throughput cases to become slower, we should make sure that these cases are documented. The main case that comes to mind is some sort of controller that:

1. Receives an event
2. Does a list that does not need read your own write consistency
3. Does some operation on the GVK of that list

Now we would have to block for every write on that list until the cache catches up. Agreed however that most controllers would benefit from having the cache up to date with its writes.

[alvaroaleman]

yeah, fundamentally the tradeoff is performance vs consistency. I believe that the consistency is more important in most cases. For the cases where it is not, this whole feature can be disabled.

[sbueringer]

We could add an option to Get/List calls that allows to disable consistent read (not sure if we should do that initially though, but I think it's something that we could add later as well).

EDIT: Saw that the option is mentioned below

[sbueringer]

Somewhat related. I think sooner or later we should document some patterns on how to use this feature (including trade-offs involved).

E.g. I'm not sure how much sense it makes to share one consistency client across many (different) reconcilers in the same controller. If the client is shared all reconcilers are getting blocked, but because these reconcilers are running concurrently anyway there might be not much need/benefit for read-your-own-write across reconcilers.

Concrete example from CAPI:

• It would be helpful to use a consistency client for the MachineSet reconciler so subsequent Reconciles of the same MachineSet can observe the Machines that have been created/deleted in previous Reconcile calls
• But it doesn't necessarily make sense to use the same consistency client for the Cluster / MachineDeployment / Machine / ... reconcilers, because they have to deal with the fact that the MachineSet reconciler is running concurrently anyway (i.e. no need to introduce a "global mutex")

[michaelasp]

Yeah this is tricky, if we don't have all the objects then it is not clear how we track the RV in the store. I'd like to look more at how the cache in controller runtime currently works with field/label selectors.

[alvaroaleman]

This here is how it is configured: https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.23.3/pkg/cache#Config

Fundemantelly it boils down to being able to configure field and/or labelSelector per GVK and or namespace and namespace(s) per gvk. The latter is implemented clientside, since the KAS does not allow watchichg multiple namespaces

[sbueringer]

Somewhat related. I think sooner or later we should document some patterns on how to use this feature (including trade-offs involved).

E.g. I'm not sure how much sense it makes to share one consistency client across many (different) reconcilers in the same controller. If the client is shared all reconcilers are getting blocked, but because these reconcilers are running concurrently anyway there might be not much need/benefit for read-your-own-write across reconcilers.

Concrete example from CAPI:

• It would be helpful to use a consistency client for the MachineSet reconciler so subsequent Reconciles of the same MachineSet can observe the Machines that have been created/deleted in previous Reconcile calls
• But it doesn't necessarily make sense to use the same consistency client for the Cluster / MachineDeployment / Machine / ... reconcilers, because they have to deal with the fact that the MachineSet reconciler is running concurrently anyway (i.e. no need to introduce a "global mutex")

[sbueringer]

It would be good to also describe what happens in the following case:

• Write succeeds
• RV doesn't show up in the cache (either not at all or it takes a long time (minutes))

E.g. we could return an error in the Get/List calls after some timeout. We could also fallback to a live read to the apiserver (I assume that's not what we want to do, IIRC some of the caches in the apiserver works that way)

(EDIT: I saw this case is mentioned in the implementation, but would be probably good to highlight it here as well)

[sbueringer]

I think it could be a worthwhile optimization to only block for the same GVK+namespace for List for namespaced List calls

[alvaroaleman]

I don't disagree, but maybe we can leave this out of scope initially?

[sbueringer]

I think that's okay, WDYT about adding this as a future goal to keep track of it?

[sbueringer]

nit: "then unblocks the reads" seems wrong at this point? (the reads are unblocked "when it observed it" (below) right?)

[alvaroaleman]

nit: "then unblocks the reads" seems wrong at this point? (the reads are unblocked "when it observed it" (below) right?)

Yes, fixed

[sbueringer]

Let's introduce some metrics to make it possible to gather data on how this feature behaves (e.g. in something similar in CAPI I introduced a metric for the "wait duration". Number of wait timeouts would be probably also good.

Probably worth checking if k/k introduced metrics for related work

[sbueringer]

I wonder if we should add an additional request-level timeout per default in the consistency client. I would not be surprised if a huge majority of controller-runtime users are not setting an overall Reconciliation or request timeout.

IIRC we didn't set a ReconciliationTimeout per default when we introduced it to avoid breaking anyone.
A cached get/list call today is not something that typically times out. So I think a lot of folks didn't realize yet that they should set timeouts because they might never have hit issues without a timeout

[alvaroaleman]

We can do that, we should probably have an options struct for the whole thing, so maybe use that to allow configuring a timeout for this specially and default that timeout if unset?

[sbueringer]

Sounds good. We can figure out exact details of how this can be configured on the implementation PR

[sbueringer]

nit: Would it make sense to "reduce" the parameter we take in this func from a full client.Object to: gvk, key, uid? (similar for RemoveRequiredDeleteForObject)

[alvaroaleman]

Lets do whichever is easiest to implement and then update the doc based on that

[sbueringer]

I assume this is referring to client.Options.

Would be great if it would be possible to enable/disable this per GVK

Wondering if we should simplify the field / option below to something like "ConsistentRead" (we shouldn't do that if it would be misleading though)

[sbueringer]

Do we also need an option to avoid blocking Reads on the Write methods?

(Specifically wondering what happens out of the box for a Write that never shows up in the cache because the object doesn't match the selectors)

[alvaroaleman]

So an opt out for this on the mutating methods of the client?

[sbueringer]

Would it make sense to short-circuit some of the code if the write request didn't change the RV?

I think there are many cases where that can happen:

• SSA no-op
• Empty patches
• Patches that apply changes that are changed back by conversion or mutating webhooks (so effectively they don't change anything)

None of these are ideal, but stuff like this happens

[alvaroaleman]

Sure, but how do we determine that? IIRC the kube-apiserver only sometimes supresses no-op updates?

[sbueringer]

IIRC the kube-apiserver only sometimes supresses no-op updates?

As far as I'm aware any cases where the kube-apiserver increments the RV without a change is a bug (and a bunch of them have been fixed in the past). (Example: repeated SSA calls with the same object should not increase the RV)

But I think it's not something that we should try to figure out on client side before we do the write call.

It should be easy to figure out after the call though by comparing before/after RV of the object.

But I don't know if there is anything that we can optimize for that case after the call

[alvaroaleman]

Yeah we can try to do that, but both in the SSA case and the patch case it will most of the time not work I think, because these requests typically do not have an RV. The one thing I do not want to do is try to figure out based on the request body if it changed something, because that seems pretty unsafe, we don't know what mutation webhooks there are

[sbueringer]

Let's assume that for some reason the cache doesn't catch up at all. (I'm aware that today we don't have a way to handle that)

Would it be possible now to handle cases like that, if yes how? (not saying we have to do this automatically, just wondering if we expose something that makes it possible to handle cases like that)

I'm wondering if in cases like that it would be e.g. desirable that the entire controller binary restarts, similar to what happens on cache timeout during startup.

IIRC some controllers in k/k (in kcm?) basically stop reconciling now until the cache catches up. I think that's also what we would get with what is currently described in the proposal. I don't know if there is any additional handling if the cache never catches up.

[alvaroaleman]

At the moment that would also happen, you would eventually time out, propagate the error back, retry, timeout again etc. We can return a sentinel error for this if we want to allow special handling?

[sbueringer]

At the moment that would also happen, you would eventually time out, propagate the error back, retry, timeout again etc.

Makes sense

We can return a sentinel error for this if we want to allow special handling?

Sounds like a good idea in general, so folks can differentiate between cache stale vs all other errors

[alvaroaleman]

Added that to the doc

[sbueringer]

(comment belongs slightly above, but collides with other comments)

Probably a stupid question, just want to double check. We can rely on events from a watch stream being ordered by RV, right? (otherwise we would potentially unblock to early)

Similarly I got this from an LLM and I'm honestly not sure if this is an issue or not, it sounds to me like it might be. I unfortunately don't have the time before PTO to figure it out (feel free to ignore if it doesn't make sense to you)

## Behavior during informer relist is unspecified

When an informer's watch connection breaks and it relists, the reflector replays
the full state. If a minimum RV was set but the relist's "initial list" RV is
lower than the target (because the list happened before the write was committed),
the `ConsistencyHandler` might not advance `observedRV` during the relist. The
subsequent synthetic Add events would then advance it, but there's a window
during relist where the cache state is being replaced and objects might
temporarily disappear.

The design should discuss whether relist can violate the consistency guarantee,
and whether the `ConsistencyHandler` needs to handle relist events specially
(e.g., the `isInInitialList` parameter in `OnAdd` that the POC ignores).

[alvaroaleman]

cc @michaelasp regarding the We can rely on events from a watch stream being ordered by RV, right? question - I hope so

[sbueringer]

Based on what I learned in the meantime about some improvements in client-go I think we can, but would be good to get confirmation if our understanding is correct.

[sbueringer]

Is there anything special about the following cases that should be called out?

• subresource writes (e.g. status, scale)
• create with GenerateName (there is no Name available when Create is called)

[alvaroaleman]

I don't think so, in both of these the request is somewhat special, but the response is not and the response is what we use to determine currentness

[sbueringer]

Not sure if this is even worth calling out, but Transform funcs that strip metadata (specifically kind/apiVersion/name/namespace/UID/RV (?)) are not supported

But maybe it's worth mentioning what we rely on

[alvaroaleman]

But maybe it's worth mentioning what we rely on

Mentioned being able to deal with that as a non-goal

[sbueringer]

I was chatting a bit with @serathius at KubeCon and he mentioned that in core Kubernetes they intentionally didn't implement that kind of "strong consistency" (if I got it right they are not blocking all reads after writes, they do it more fine-granularly).

I don't have time right now to lookup the k/k implementations, but maybe we should:

• ensure that we have metrics & data before we make a decision to turn this feature on per default
• also enable cases with lower consistency guarantees, e.g. a controller might only want to wait until writes for a specific reconcile.Request have been observed and not also for other writes of other controller workers or other controllers (requires more research on what exactly was done in k/k, just wanted to surface it already in case that's useful)

[serathius]

Right, while controller-runtime can provide more options to user, I think the defaults should be aligned with k/k to avoid surprises by both operator users and authors. Eventual consistency of controllers is one of the reasons Kubernetes can scale so well. Operators should also take advantage of it.

[michaelasp]

+1 k/k's method of reading its own writes is that it will only pause reconciliation if:

1. An object the reconciler wrote has not been read back yet
2. Only objects the requested object has written previously will stall reconciliation(this one is tricky to generalize, especially in cases that we still want to pause reconciliation on other resources)

In an example statefulset Foo may create pods -> foo1, foo2, foo3 and statefulset Bar creates bar1, bar2 and bar3

If foo2 is stuck and we have not seen the write, that does not prevent bar from processing its objects. I wonder if we want to only wait for writes inside of that request by default but expose helpers that can pause for the whole reconciler if necessary.

[sbueringer]

More context on the changes in k/k: kubernetes/website#54792 (Thx @michaelasp for writing the blog!)

[sbueringer]

We're implementing stale controller mitigation in Cluster API on top of controller-runtime.
It's a bit Cluster API specific because we have an additional wrapper for our reconcilers but just fyi in case it's helpful anyway: kubernetes-sigs/cluster-api#13720 (commit: "Implement stale controller mitigation in MD controller")

[alvaroaleman]

Right, while controller-runtime can provide more options to user, I think the defaults should be aligned with k/k to avoid surprises by both operator users and authors

wrt defaults I don't think I necessarily agree, because the average controller author in k/k can be expected to have a very good understanding of controllers and that is absolutely not true for the average controller-runtime user. I do agree however that it should be possible for everyone who knows what they are doing to opt out of safe-but-slow defaults.

If foo2 is stuck and we have not seen the write, that does not prevent bar from processing its objects. I wonder if we want to only wait for writes inside of that request by default but expose helpers that can pause for the whole reconciler if necessary.

Thats a good point, also relates to this comment. https://github.com/kubernetes-sigs/controller-runtime/pull/3473/changes#r2934843480. How about maybe doing something like a WithConsistentReadsForKey(key string) client.Client that will then only enforce consistent reads of objects mutated through a consistent read client with that key? This would allow to do this arbitrarily, per controller, per object key or share it within some controllers but not all (say replicaset may want to be consistent with deployment to not waste time on making an outdated .spec.replicas happen)

(Better naming suggestions for the method welcome)

[sbueringer]

Sounds like a good idea.

What do you think about the following on top (can be a future goal).

With the way this feature currently works it will return errors on Reads if the relevant Writes have not been seen. In general I see an advantage in being able to check very early in a Reconcile if the cache is up-to-date.
This is also what achieved with the implementations in k/k because they check the consistency store at the beginning of a reconcile if all relevant writes have made it into the cache.

I think we could also enable this use case by having an additional method on the "consistency client" that allows to check if all writes made it into the cache (either all or just for the specific key).

Example:

// Early in reconcile
c := client.WithConsistentReadsForKey("namespace/name")
if !c.IsUpTodate() {
   // Requeue
}

c.Write
....

[alvaroaleman]

What do you think about the following on top (can be a future goal).

Its a good idea, but it also makes me think that maybe for the first implementation we should have neither what I proposed nor what you proposed and just implement the somewhat inefficient "wait for everything", because that is good enough in many cases already and the topic of "What ways should we have to control what to block on" seems like it may warrant its own discussion?

[sbueringer]

Sounds good, maybe mention that topic as a future goal to somewhat keep track of it?

[alvaroaleman]

Sounds good, maybe mention that topic as a future goal to somewhat keep track of it?

Done

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/retest

(will try to take another look soon)