-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move KEP 2340 to Beta #4134
Move KEP 2340 to Beta #4134
Conversation
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
ping @wojtek-t |
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
user can reconfigure kube-apiserver to always pass request to etcd by | ||
disabling the feature flag. Metric `apiserver_watch_cache_read_wait` | ||
will measure wait time experienced by reads for watch cache to become fresh. | ||
Per request override will be done via a HTTP request header `k8s.io/apiserver/skip-watch-cache`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k - I would like to get a feedback about it from you
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
Results from testing manual progress notification on broken etcd versions v3.4.24: Full report
|
Conclusion, manual watch notification are totally broken on unpatched etcd versions. We cannot depend on them at all, meaning that for this feature cannot be enabled by default without confirming that administrator runs etcd that has been patched. Options for making ConsistentListFromCache default:
|
cc @ahrtr |
Option 5, @jpbetz is working on a component resource API, specifically so that we can support mixed version states of the apiserver, but it should be generic enough for component registration for any control-plane component. There could be a path where etcd registers itself to the apiserver along with the version. |
Option 6, we don't enable the feature by default for Beta in 1.29 and wait |
49686b6
to
eb7c1ac
Compare
Are we still wanting to bump this to beta in 1.29? I just reviewed the list of options, it looks like the viable ones are down to either the etcd version flag or modifying etcd to make the version available in the API? |
Don't have capacity to finish the design nor to implement for 1.29 |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Want to resurrect the KEP and move it to Beta before enhancement freeze on Friday 9th February. |
b5b97eb
to
d950ae5
Compare
d950ae5
to
53af7e3
Compare
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
endpoints have etcd version that includes the fix for [etcd-io/etcd#15220]. | ||
|
||
As we cannot break users we propose that during Beta, the failed condition will | ||
cause kube-apiserver to fallback to reading to etcd (as before) and write a warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expanding a couple possible cases here
- version return not recognized or not etcd - in this case we should warn, but continue with whatever the current value of the featuregate is
- etcd too old, featuregate not explicitly specified, featuregate not locked to true - disable the featuregate
- etcd too old, featuregate not explicitly specified, featuregate locked to true - apiserver should fail to start
- etcd too old, featuregate explicitly enabled - apiserver should fail to start because the user wanted this featuregate and the intent cannot be fulfilled.
- etcd new enough - use whatever the current value of the featuregate is
- etcd does not respond (recall we don't have an ordering requirement) - in this case we should warn, but continue with whatever the current value of the featuregate is
I think that handles all the known cases, prevents the most common failure modes, and doesn't restrict cases where we aren't sure that there is a correctness problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How the first and sixth case differ. In both cases we cannot confirm that feature works. Would it be safer to disable the feature gate?
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
@deads2k addressed the comments, please take another look. |
|
||
Per request override should allow user to compare request results without | ||
impacting other requests or requiring to redeploy whole cluster. The exact | ||
details of override API will be clarified during API review. In healthy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC @deads2k wants the ListOptions field, but I would leave it to him to approve during API review.
Thanks - that all looks reasonable to me. The PRR looks good too. /lgtm Still @deads2k approval from SIG level is needed. |
6c5246c
to
199b907
Compare
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
eb62839
to
9fb245d
Compare
keps/sig-api-machinery/2340-Consistent-reads-from-cache/README.md
Outdated
Show resolved
Hide resolved
Co-authored-by: David Eads <[email protected]>
da89524
to
4e9c207
Compare
Thanks for spending time digging into the details of this one with me. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, serathius, 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:
Approvers can indicate their approval by writing |
cc @wojtek-t @jpbetz @deads2k