-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Document semantic of resourceVersion=0. #16944
Conversation
Welcome @oxddr! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -85,6 +85,8 @@ For example: | |||
} | |||
... | |||
|
|||
When no specific starting point is required, `resourceVersion` can be set to **0**. Client will receive current state cached in the apiserver and watch will start. |
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.
This isn't guaranteed to be served from the API server cache (watch cache is optional). I also don't think we should encourage use of this (see problems discussed in kubernetes/kubernetes#74022).
If we document this, it should come with big warnings and discourage use.
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.
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.
I agree with first two things:
- the cache is not guaranteed
- this requires a huge warning and we need to mention consequences and describe cases where it may work
But I actually don't agree with the "discouraging" part - there are some cases where it is really needed for scalability. But yeah - we need to be much more careful when adding note about it.
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.
I actually don't agree with the "discouraging" part - there are some cases where it is really needed for scalability
this is in the watch section. we don't want people watching from 0.
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.
Ooops sorry - I thought it's about LIST. I agree we don't want people watching from 0.
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.
So to clarify: I'm not trying to say that now everyone should be using it - absolutely not. But there are cases, where you may need it if you need to scale (though I fully agree that it requires very good understanding of potential consequences).
BTW - I believe that we will fix at least majority of time-travel problems in 1.17 - @jpbetz
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.
I strongly fall in the "first be correct, then be performant" camp.
With reflectors, we got ourselves in a situation where we realized too late the pitfalls of what we were doing, and couldn't easily back out changes without regressing performance to the point of breaking existing clusters.
I would not recommend anyone who is not using this already start using it until the issues are resolved. If that is in 1.17, great, we can make the warning conditional at that point.
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.
I strongly fall in the "first be correct, then be performant" camp.
I agree - but I don't think that exclude what I'm saying.
What I'm saying is that, we know the circumstances under which time-travel can happen - so we can describe those here. It's up to the user to understand what are the consequences of time-travel in their case and if those can be mitigated on their side - we will never make it a default - it will always be user`s decision.
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.
In #16910 I've attempted to put in ample warnings about the RV="0" semantic. E.g. "...any starting resource version is allowed. In high availabiliy configurations, it is possible for the watch to start at a much older resource version that the client has previously observed, due to partitions or stale caches. Clients that cannot tolerate this should not start a watch with this semantic"
(https://github.com/kubernetes/website/pull/16910/files#diff-909a5da840997c07f02b50d20399d9a1R725).
The language is not quite as strong as "using RV=0 voids your warranty", but tries to express the "be sure you know what you're doing". WDYT?
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.
The language is not quite as strong as "using RV=0 voids your warranty", but tries to express the "be sure you know what you're doing". WDYT?
That is exactly the message that I would like to pass here.
Deploy preview for kubernetes-io-master-staging ready! Built with commit 35108ec https://deploy-preview-16944--kubernetes-io-master-staging.netlify.com |
I've opened a PR yesterday to document the resourceVersion semantic as well: #16910 Okay if I incorporate thoughts in the comments here into that PR? |
Sure, let's move the discussion to your PR! /close |
@oxddr: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @mm4tt @wojtek-t