Repository navigation
✨ Read your own write client design - #3473
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| called, all `Get` requests to the GVK+key and all `List` requests to the GVK will be blocked until the cache observed the | ||
| passed rv or the request times out. The rv is copied before waiting to avoid subsequent calls to `SetMinimumRVForGVKAndKey` | ||
| blocking the request | ||
| * Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| * Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key | ||
| of object and all `List` requests to the GVK of object will be blocked until a delete event for GVK+UID of object was | ||
| observed OR the requests context times our OR `RemoveRequiredDeleteForObject` is called for the same object. The object(s) | ||
| whose deletion are awaited are copied before waiting to avoid subsequent `AddRequiredDeleteForObject` calls to block existing |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
|
/cc @michaelasp |
| called, all `Get` requests to the GVK+key and all `List` requests to the GVK will be blocked until the cache observed the | ||
| passed rv or the request times out. The rv is copied before waiting to avoid subsequent calls to `SetMinimumRVForGVKAndKey` | ||
| blocking the request | ||
| * Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key |
There was a problem hiding this comment.
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.
| * Add an internal-only `AddRequiredDeleteForObject(obj client.Object) error` method. Once set, all `Get` requests to the GVK+key | ||
| of object and all `List` requests to the GVK of object will be blocked until a delete event for GVK+UID of object was | ||
| observed OR the requests context times our OR `RemoveRequiredDeleteForObject` is called for the same object. The object(s) | ||
| whose deletion are awaited are copied before waiting to avoid subsequent `AddRequiredDeleteForObject` calls to block existing |
There was a problem hiding this comment.
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.
| The basic idea of the implementation is to make writes block concurrent reads to the same GVK+Key for Get and GVK for List | ||
| before the request is sent. If the request succeeds, the client provide the cache with either the returned resourceVersion | ||
| or the gvk+objectkey+uid of an object if it was deleted from storage, then unblock the reads. The cache will then copy this | ||
| RV/deleted object within the get/list and block the request until it observed it or the requests context times out. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah, that is my understanding
|
|
||
| ## Goals | ||
|
|
||
| * Any read from the client (`Get` or `List`) contains all writes performed through the same client that were |
There was a problem hiding this comment.
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:
- Receives an event
- Does a list that does not need read your own write consistency
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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).
DisableReadYourWritesConsistency option was added to all read & write requests
There was a problem hiding this comment.
DisableReadYourWritesConsistency option was added to all read & write requests
Not sure what you mean to say with this comment, the design mentions this
There was a problem hiding this comment.
Not sure what you mean to say with this comment, the design mentions this
I just wanted to update this thread with an info what we did :) (I think when we discussed this in March we didn't have the option yet)
| back to the client for any reason like the connection breaking. This is primarily for practical | ||
| purposes, if the write doesn't get a response indicating success or failure back, it is impossible to tell if | ||
| the write did or did not succeed | ||
| * Automatically dealing with configurations where the cache doesn't contain all objects that are written, for |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
|
||
| ## Goals | ||
|
|
||
| * Any read from the client (`Get` or `List`) contains all writes performed through the same client that were |
There was a problem hiding this comment.
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")
| or the gvk+objectkey+uid of an object if it was deleted from storage, then unblock the reads. The cache will then copy this | ||
| RV/deleted object within the get/list and block the request until it observed it or the requests context times out. | ||
| It is important that we block before executing the write request and not after, because we can not know when exactly the server | ||
| commits it, only when it tells us having committed it. |
There was a problem hiding this comment.
(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).
There was a problem hiding this comment.
cc @michaelasp regarding the We can rely on events from a watch stream being ordered by RV, right? question - I hope so
There was a problem hiding this comment.
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.
| Controller-Runtimes default client writes to the api and reads from an informercache. As a result, performing a read | ||
| right after a write tends to not return that write since it hasn't made it back to the informer cache yet. This | ||
| leads to bugs and workarounds like replacing the read portion of the client to read from the API instead. The goal of | ||
| this proposal is to provide the same consistency in the cache-reading client a live client would with regards to |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
+1 k/k's method of reading its own writes is that it will only pause reconciliation if:
- An object the reconciler wrote has not been read back yet
- 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.
There was a problem hiding this comment.
More context on the changes in k/k: kubernetes/website#54792 (Thx @michaelasp for writing the blog!)
There was a problem hiding this comment.
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")
There was a problem hiding this comment.
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
....There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sounds good, maybe mention that topic as a future goal to somewhat keep track of it?
There was a problem hiding this comment.
Sounds good, maybe mention that topic as a future goal to somewhat keep track of it?
Done
There was a problem hiding this comment.
Lgtm (leaving this conversation open in case others want to respond as well)
d442929 to
24b8a9e
Compare
|
/retest (will try to take another look soon) |
|
Latest changes lgtm from my side |
24b8a9e to
3eacdf4
Compare
kubernetes-prow
Bot
added
size/L
3eacdf4 to
32ab2bb
Compare
32ab2bb to
cdbaec4
Compare
|
Thx! /lgtm |
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. DetailsGit tree hash: 24e013d42ed2a57a87b9da0a72b9f9b8a7d2bad0 |
|
/hold cancel |
kubernetes-prow
Bot
removed
the
do-not-merge/hold
Related to #3320
Minimal POC in #3472
/hold