Consistent reads mitigation

[wojtek-t]

Ref #2340

/cc @serathius @deads2k

[wojtek-t]

I also propagated table for watch requests.

[wojtek-t]

@p0lyn0mial @serathius

[serathius]

I largely agree with @wojtek-t. After further consideration, I don't think a dedicated "SkipCache" API option is necessary in this scenario. We shouldn't view watchcache as a traditional database cache like memcache or varnish. Skipping cache for debugging purposes is suitable for periodically refreshed and invalidated caches to identify corrupted data. It operates under the expectation of consistent results. However, in distributed systems, it's normal to receive different results from concurrent consistent read requests.

In distributed systems like etcd, two concurrent read requests can target different instances, leading to different pathing and arbitrary network delays, allowing for interleaved transactions and reordering. The only way to determine their "real" execution order is by checking the revision returned by etcd. Responses from two different reads should only match when they have the same revision. However, even in medium-sized clusters, there's enough traffic that two concurrent consistent read requests will often return different revisions. To better understand that I recommend reading https://jepsen.io/consistency/models/linearizable

The only way to debug watch cache corruption is to verify if the cached data matches the data in etcd. If you want to use the K8s API for this, the only way is to make two requests that share the same ResourceVersion (RV). This means the correct flow to compare watchcache state to etcd state is:

1. Make a request without an RV (which will be served from the watch cache).
2. Follow it with a request that includes the exact RV from the first request and uses ResourceVersionMatch=Exact. (which is served from etcd)

[p0lyn0mial]

My view on this case is that instead of adding a new list option that allows bypassing the cache, we should better test the feature before enabling it globally.

Mainly, my concern is that today, when the feature ConsistentListFromCache is enabled (or any other), it becomes available to all clients simultaneously. In my opinion, we should have a graduation mechanism and initially enable the feature for a specific client, a client with some attribute, or for a percentage of traffic coming to the server. Subsequently, we should gradually increase the number of clients or the percentage of traffic directed to the server until finally enabling the feature for all clients.

Additionally, as @serathius mentioned, we should (especially in CI) have a mechanism for detecting inconsistencies in data by comparing data returned from the cache and directly from etcd. For that we could use the data_consistency_detector by issuing two request to the server.

[deads2k]

Thank you @wojtek-t and @serathius for taking the time to put together the list, describe the potential debugging flows, and answer questions live. I think I've reached where you are: we should not create a new option for bypassing the watch cache.

While it seems congruent to add a parameter for "bypass watch cache" and it could be useful if the watch cache is misbehaving, such an option is not useful for debugging answers from the watch cache. This is because the wache cache cannot support Exact and etcd cannot support NotOlderThan and that prevents any congruent etcd and watch cache access.

This means that the only debugging flow available is make a watch cache request and then use the existing option RV Exact unset unset. I like the idea of making this an opt-in self-check available in our client as referenced above.

Additionally, as @serathius mentioned, we should (especially in CI) have a mechanism for detecting inconsistencies in data by comparing data returned from the cache and directly from etcd. For that we could use the data_consistency_detector by issuing two request to the server.

[wojtek-t]

@deads2k - I updated the PR to reflect our discussion today, PTAL

[deads2k]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, wojtek-t

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

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/sig-api-machinery/OWNERS [deads2k]

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

[serathius]

Awesome!