Skip to content
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

Add progress operation to watch API #9855

Closed
jpbetz opened this issue Jun 15, 2018 · 7 comments
Closed

Add progress operation to watch API #9855

jpbetz opened this issue Jun 15, 2018 · 7 comments

Comments

@jpbetz
Copy link
Contributor

jpbetz commented Jun 15, 2018

There is currently no general way to check how far a watch stream has progressed relative to a specific etcd revision. The revisions in watch events are only enough to establish a lower bound for the revision the watch stream has progressed past. It would be useful for the client to be able to query "what's the progress of this watch stream?" and get a "this watch stream has received all events it is subscribed for up to revision N", where N is the highest revision the server can guarantee the response is accurate for.

E.g. if the latest event revision received on a stream is r_event, and the latest applied revision observed by the etcd server is r_applied, then we currently can only know that the watch progress r_watch_progress is . r_event <= r_watch_progress <= r_applied, and that range can be arbitrarily large because events are only received for changes to the watch range.

kubernetes/kubernetes#59848 describes a use case where this would be valuable-- Determining if a client side cache that updated via a watch, is up-to-date to a particular revision. @xiang90's proposal in this case was to consider adding a progress operation to the watch API.

Since the Watch API s a bi-directional gRPC stream, the progress operation might look something like:

  • Add a WatchProgressRequest union member to WatchRequest
  • Add fields (or a union member?) to WatchResponse to identify progress responses and provide the max etcd storage revision that the server has no pending watch events to send to this watch stream for.

Specifically, the progress revision should be

  • >= the revision of the most recent WatchResponse event sent in the watch stream to the client
  • < the revision of WatchResponse events yet to be sent in the watch stream to the client
  • == to the latest etcd storage revision the etcd member has applied if there are no pending events to to be send in the watch stream to the client

cc @xiang90 @gyuho @wenjiaswe

@gyuho
Copy link
Contributor

gyuho commented Jun 15, 2018

Since the Watch API s a bi-directional gRPC stream, the progress operation might look something like:
Add a WatchProgressRequest union member to WatchRequest

Q. How WatchProgressRequest would be different than WatchCreateRequest (that returns all events since the specified revision)? Would it duplicate all fields from WatchCreateRequest?

@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 15, 2018

Q. How WatchProgressRequest would be different than WatchCreateRequest (that returns all events since the specified revision)? Would it duplicate all fields from WatchCreateRequest?

I don't think it would need any of the fields. A progress request would just indicate to the server that a progress status is desired in the response stream as soon as possible. And a progress status would just be a single revision number, which can be global to the watch stream. It just needs to provide the highest possible revision number that server can guarantee all watch events in the future will be higher than.

@xiang90
Copy link
Contributor

xiang90 commented Jun 18, 2018

@jpbetz

Right. This exactly matches my expectation.

@mbrannock
Copy link
Contributor

Would it be possible to use the progress_notify flag in the WatchCreateRequest to supply the requested progress information? If I understand correctly, etcd starts sending zero-event WatchResponses only when there are no more events to send and the watch is caught-up. Clients could read the header revision to determine the current etcd revision even if the watch has events only sporadically. As more empty WatchResponses come in, the client will know that the watch is current to that new revision.

@gyuho
Copy link
Contributor

gyuho commented Jun 28, 2018

@mbrannock Currently, progress_notify has a minimum interval and cannot be triggered manually. And we want to broadcast progress updates to all sub watch streams. Which also can be done by extending current progress notify API, but we may introduce some behavior changes.

It's easier to define a separate API as in #9869.

@jpbetz
Copy link
Contributor Author

jpbetz commented Jun 28, 2018

@mbrannock @gyuho This has me thinking..I wonder if there is some stopgap we might be able to use until #9869 is available for use by Kubernetes. The problem is that kubernetes is on etcd 3.2 (we'll go to etcd 3.3 for kubernetes 1.12) so we don't be able to leverage the RequestProgress improvement in the near term (it will be on 3.4 plus Kubernetes has a backward compatibility against older etcd versions that we need to support). @mbrannock's comment about WatchCreateRequest got me wondering if I there might be some trick we could do like create and close a watch stream to get the current watch progress of a client?

@gyuho
Copy link
Contributor

gyuho commented Jun 29, 2018

@jpbetz Maybe empty key with WatchCreateRequest? Currently, watch create with empty key is just no-op.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants