grpcproxy: add support for WatchRequest_ProgressRequest

[alexandrevilain]

Fixes #19710

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alexandrevilain
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @alexandrevilain. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[henrybear327]

/ok-to-test

[codecov]

Codecov Report

Attention: Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.76%. Comparing base (d6416b8) to head (63e4f0b).
Report is 70 commits behind head on main.

Files with missing lines	Patch %	Lines
server/proxy/grpcproxy/watch.go	61.53%	3 Missing and 2 partials ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/proxy/grpcproxy/watcher.go	92.15% <100.00%> (+0.32%)	⬆️
server/proxy/grpcproxy/watch.go	91.39% <61.53%> (-1.09%)	⬇️

... and 47 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19711      +/-   ##
==========================================
+ Coverage   65.98%   68.76%   +2.78%     
==========================================
  Files         421      421              
  Lines       35857    35878      +21     
==========================================
+ Hits        23659    24673    +1014     
+ Misses      10791     9766    -1025     
- Partials     1407     1439      +32     

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6416b8...63e4f0b. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alexandrevilain]

/retest

[alexandrevilain]

/retest

[serathius]

This is not the same progress notification source. In etcd there are two ways to get progress notification:

• Server configures a periodic progress notification period and client requests getting periodic notification. For example if it's configured for 5s, and there was no normal event in watch for 5s, server should send a notification to watch.
• Client requested notification, client can at any moment request a notification and server should imminently give a response about a watch progress.

This PR mixes those two, where it configures a grpc proxy to response to requested notification by enabling periodic notification.

[alexandrevilain]

Thanks for the explanation @serathius!

Just to clarify, are you saying I shouldn't permanently enable progress notifications on the watcher?
Instead, should I implement something like a one-time toggle on the watcher side to allow a notification for the next request only?

So, when the proxy receives a new WatchRequest_ProgressRequest, it would set this toggle to true and call RequestProgress on the server.
Then, once the proxy receives the progress notification, it sends it to the client and resets the toggle back to false.
Is that the intended behavior?

[serathius]

Just to clarify, are you saying I shouldn't permanently enable progress notifications on the watcher?
Instead, should I implement something like a one-time toggle on the watcher side to allow a notification for the next request only?

Yes

So, when the proxy receives a new WatchRequest_ProgressRequest, it would set this toggle to true and call RequestProgress on the server.
Then, once the proxy receives the progress notification, it sends it to the client and resets the toggle back to false.
Is that the intended behavior?

I haven't dive into what exactly the toggle does, but my initial thinking is that grpcproxy should just proxy WatchRequest_ProgressRequest request.

[alexandrevilain]

Thanks.
I've reworked the code to support both types of progress notifications.
Let me know if this looks better.