Make resourceVersion parameter semantics consistent across all storage.Interface implementations by jpbetz · Pull Request #72170 · kubernetes/kubernetes

jpbetz · 2018-12-18T22:22:12Z

[This is part of effort to improve Apiserver Resource Version Semantics (as previously circulated with sig-apimachinery) and fix the "stale read" issue]

Make the minimum resourceVersion semantics consistent across all storage.Interface implementations by:

enforcing "minimum RV" constraints in the etcd3 storage implementation per the storage.Interface documentation
modifying the etcd3 List() implementation to use "minimum RV" semantics instead of "exact RV" semantics when RV>0

Important: the watch cache is enabled by default on the kube-apiserver for all but a select few types (e.g. Events, CRDs) and so while API clients do not get to decide when they make requests what semantics they get, they typically will get the watch cache semantics.

Before:

	Watch Cache (RV=0 & RV>0)	etcd3 RV=0	etcd RV>0
Get	"min RV"	quorum read, RV ignored	quorum read, RV ignored
List	"min RV"	quorum read, RV ignored	"exact RV"
GetToList	"min RV"	quorum read, RV ignored	quorum read, RV ignored
Watch	"min RV"	"min RV"	"min RV"

Note that "quorum read, RV ignored" behaves like "min RV" for all RVs that were retrieved from some previous operation on the store because quorum reads are guaranteed to be processed with the latest RV. So it appears from these semantics that "min RV" semantics are pervasive across both implementations and what we should converge to.

After:

	Watch Cache (RV=0 & RV>0)	etcd3 (RV=0 & RV>0)
Get	"min RV"	"min RV"
List	"min RV"	"min RV"
GetToList	"min RV"	"min RV"
Watch	"min RV"	"min RV"

The change from "exact RV" to "min RV" for the List operation is the most notable change here. But given that the watch cache is enabled by default for all but a select few types, and that the semantics change here would only be visible if providing a non-zero RV, we don't expect this change to be particularly risky/visible. And we believe it to be lower risk than leaving the semantic inconsistent across implementations, esp. given that clients don't necessarily have any way of knowing if the watch cache is enabled and what semantics to expect.

As part of this change we're introducing a ResourceVersionTooLargeError type to the storage interface so that both the watch cache and etcd3 return the same error type if unable to serve a requested resource version because it's too high. Previously the watch cache would return a timeout error in the case because it attempts to wait for 3 seconds to see if the "too high" resource version might appear if it waits a bit. I've done a quick review and it looks like this change is low risk, but if reviewers know of reasons why this might be breaking, please let me know.

The resource version option, when passed to a list call, is now consistently interpreted as the minimum allowed resource version.  Previously when listing resources that had the watch cache disabled clients could retrieve a snapshot at that exact resource version.  If the client requests a resource version newer than the current state, a TimeoutError is returned suggesting the client retry in a few seconds.  This behavior is now consistent for both single item retrieval and list calls, and for when the watch cache is enabled or disabled.

cc @cheftako @lavalamp @wojtek-t @liggitt @smarterclayton

jpbetz · 2018-12-19T19:09:08Z

/test pull-kubernetes-integration

jpbetz · 2018-12-19T19:40:11Z

/test pull-kubernetes-integration

jpbetz · 2018-12-19T21:06:50Z

/test pull-kubernetes-kubemark-e2e-gce-big

wojtek-t

This looks reasonable to me - I just left one comment.

But I would like at least someone else to take a look into that too.
@liggitt @smarterclayton

jpbetz · 2019-09-20T20:42:02Z

This is ready for another review pass.

jpbetz · 2019-09-23T20:37:39Z

friendly nudge

wojtek-t

Just two minor nits - other than that LGTM.
Though I would also wait for @liggitt to make a final look too.

jpbetz · 2019-09-24T17:40:32Z

/test pull-kubernetes-dependencies

jpbetz · 2019-09-27T20:00:13Z

Friendly nudge

smarterclayton · 2019-10-01T20:29:48Z

But given that the watch cache is enabled by default for all but a select few types, and that the semantics change here would only be visible if providing a non-zero RV, we don't expect this change to be particularly risky/visible

I'm really hoping this is true. Is watch cache enabled for CRDs by default?

Edit: looks like not - so are we really sure no one is relying on this?

liggitt · 2019-10-01T20:36:36Z

I'm really hoping this is true. Is watch cache enabled for CRDs by default?

Edit: looks like not - so are we really sure no one is relying on this?

Yes, it is. I know because we found several watch-cache-related flakes testing custom resources.

#46967, #64796, #78713

jpbetz · 2019-10-01T21:23:54Z

I'm really hoping this is true. Is watch cache enabled for CRDs by default?
Edit: looks like not - so are we really sure no one is relying on this?

There is no in-tree code relying on it. We can't entirely eliminate the possibility there is out-of-tree code relying on it. The conditions they'd have to meet are quite specific: (1) they've disabled the watch cache, or are using events, and (2) they're performing a List(... ResourceVersion: SomeSpecificRevision) request and (3) they cannot tolerate receiving a more recent revision than the one they requested.

…s enabled and disabled

jpbetz · 2019-10-01T23:38:55Z

/retest

smarterclayton · 2019-10-02T20:03:09Z

So if we had to bring back exact semantics? Say we were wrong, we break a large chunk of people, they need a fix. How do we make them whole?

jpbetz · 2019-10-02T20:10:17Z

So if we had to bring back exact semantics? Say we were wrong, we break a large chunk of people, they need a fix. How do we make them whole?

Worst case it that it's widespread (i.e. we really misread the situation), in which case we could revert the part of this change that applies to lists. We'd then have to regroup and figure out how to deprecate the old behavior and migrate.

Slightly less improbable is that there are a limited set of use cases, but they're important and cannot easily migrate on to "minimum RV" semantics. In this case I suppose we could introduce a separate param to GetOptions to support exact RV semantics. I'd really like to avoid doing that, but it would be an option..

smarterclayton · 2019-10-02T20:50:14Z

I updated the release note, please double check it for me to see if you agree.

jpbetz · 2019-10-02T20:54:54Z

I updated the release note, please double check it for me to see if you agree.

Release note looks great. Thanks!

smarterclayton · 2019-10-02T21:07:15Z

/lgtm
/approve

I hope I don't regret this.

k8s-ci-robot · 2019-10-02T21:07:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, smarterclayton

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:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [smarterclayton]

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

fejta-bot · 2019-10-03T01:40:35Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Make resourceVersion parameter semantics consistent across all storage.Interface implementations

jpbetz added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Dec 18, 2018

jpbetz self-assigned this Dec 18, 2018

k8s-ci-robot requested review from hongchaodeng and xiang90 December 18, 2018 22:23

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 18, 2018

jpbetz force-pushed the stale-read-2 branch from f56f54b to 5f22687 Compare December 18, 2018 22:36

jpbetz added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Dec 18, 2018

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 18, 2018

jpbetz added this to the v1.14 milestone Dec 18, 2018

jpbetz force-pushed the stale-read-2 branch from 5f22687 to 484a255 Compare December 18, 2018 23:14

jpbetz changed the title ~~[WIP] Make resourceVersion parameter semantics consistent across all storage.Interface implementations~~ Make resourceVersion parameter semantics consistent across all storage.Interface implementations Dec 19, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2018

wojtek-t reviewed Dec 20, 2018

View reviewed changes

Comment thread staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go Outdated

wojtek-t assigned liggitt, smarterclayton and wojtek-t and unassigned jpbetz Dec 20, 2018

xiang90 reviewed Dec 20, 2018

View reviewed changes

Comment thread staging/src/k8s.io/apiserver/pkg/storage/tests/cacher_test.go Outdated

xiang90 reviewed Dec 20, 2018

View reviewed changes

Comment thread staging/src/k8s.io/apiserver/pkg/storage/errors.go Outdated

jpbetz force-pushed the stale-read-2 branch 2 times, most recently from c9a3909 to 1b2c3b7 Compare September 20, 2019 18:20

wojtek-t reviewed Sep 24, 2019

View reviewed changes

Comment thread staging/src/k8s.io/apiserver/pkg/storage/cacher/watch_cache.go Outdated

Comment thread staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go Outdated

jpbetz force-pushed the stale-read-2 branch from 1b2c3b7 to d9c7dac Compare September 24, 2019 16:56

smarterclayton reviewed Oct 1, 2019

View reviewed changes

Comment thread staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go Outdated

jpbetz force-pushed the stale-read-2 branch from d9c7dac to 3b50bdd Compare October 1, 2019 21:26

Use same 'minimum resource version' semantics both when watch cache i…

1406f7a

…s enabled and disabled

jpbetz force-pushed the stale-read-2 branch from 3b50bdd to 1406f7a Compare October 1, 2019 22:29

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2019

k8s-ci-robot merged commit 54d49bd into kubernetes:master Oct 3, 2019

liggitt mentioned this pull request Oct 9, 2019

WIP - Switch to minimum resource version semantics for paginated list #83658

Closed

Alger7w mentioned this pull request May 27, 2020

feat(controller): Use resource version latch to disable stale objects… argoproj/argo-workflows#3078

Closed

6 tasks

xuzhenglun mentioned this pull request Jan 16, 2023

Fix bug in reflector not detecting "Too large resource version" error before 1.17.0 #115093

Merged

mhan8796 pushed a commit to mhan8796/kubernetes that referenced this pull request Jun 26, 2026

Merge pull request kubernetes#72170 from jpbetz/stale-read-2

0a3815a

Make resourceVersion parameter semantics consistent across all storage.Interface implementations

Uh oh!

Conversation

jpbetz commented Dec 18, 2018 • edited by smarterclayton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before:

After:

Uh oh!

jpbetz commented Dec 19, 2018

Uh oh!

jpbetz commented Dec 19, 2018

Uh oh!

jpbetz commented Dec 19, 2018

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpbetz commented Sep 20, 2019

Uh oh!

jpbetz commented Sep 23, 2019

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jpbetz commented Sep 24, 2019

Uh oh!

jpbetz commented Sep 27, 2019

Uh oh!

smarterclayton commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

liggitt commented Oct 1, 2019

Uh oh!

jpbetz commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpbetz commented Oct 1, 2019

Uh oh!

smarterclayton commented Oct 2, 2019

Uh oh!

jpbetz commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Oct 2, 2019

Uh oh!

jpbetz commented Oct 2, 2019

Uh oh!

smarterclayton commented Oct 2, 2019

Uh oh!

k8s-ci-robot commented Oct 2, 2019

Uh oh!

fejta-bot commented Oct 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jpbetz commented Dec 18, 2018 •

edited by smarterclayton

Loading

smarterclayton commented Oct 1, 2019 •

edited

Loading

jpbetz commented Oct 1, 2019 •

edited

Loading

jpbetz commented Oct 2, 2019 •

edited

Loading