-
Notifications
You must be signed in to change notification settings - Fork 38.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
storage/etcd: skip SendInitialEvents if the request is backward compatible #117862
storage/etcd: skip SendInitialEvents if the request is backward compatible #117862
Conversation
/assign @wojtek-t |
/test pull-kubernetes-e2e-gce-cos-alpha-features |
@@ -865,8 +865,12 @@ func growSlice(v reflect.Value, maxCapacity int, sizes ...int) { | |||
} | |||
|
|||
// Watch implements storage.Interface.Watch. | |||
// TODO(#115478): for beta the WatchList feature also must/should support streaming. |
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 TODO is somewhat misleading to me. Watch by definition supports streaming :)
We probably want to say sth like:
TODO(...): For beta of WatchList feature, etcd3 implemenation should always support it.
[(or sth like that)]
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.
done, reworded the comment.
…tible otherwise an error will be returned. backward compatibility is defined as RV = "" || RV = "O" and AllowWatchBookmark is set to false. in that case we rely on https://github.com/kubernetes/kubernetes/blob/267eb25e60955fe8e438c6311412e7cf7d028acb/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go#L260
7a38647
to
f2de1a0
Compare
/test pull-kubernetes-e2e-gce-cos-alpha-features |
This LGTM. I'm holiding this until we have alpha suite passing, feel free to cancel once it passes. /lgtm |
LGTM label has been added. Git tree hash: 54f4f81bcb919ebe51c2e8b2ef10b576ecded597
|
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, 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 |
/hold cancel |
/triage accepted |
What type of PR is this?
/kind bug
What this PR does / why we need it:
It turns out that the watch-cache is disabled by default for
events
resource.When the WatchList feature is enabled and a watch request for events with empty ListOptions is send. Then default options are applied and the request is passed to the etcd implementation. In the end the request fails because etcd doesn't implement streaming API for alpha.
This PR provides a fix that allows for skipping SendInitialEvents when the request is considered to be backward compatible (thanks @wojtek-t for suggesting it!).
Backward compatibility is defined as RV = "" || RV = "O" and AllowWatchBookmark is set to false.
In that case we rely on
kubernetes/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go
Line 260 in 267eb25
Which issue(s) this PR fixes:
Fixes ##116357
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: