Reuse events used for syncing watchers

[serathius]

Part of #16839
Improve etcd behavior during watch congestion by reducing the memory needed.
Congestion causes watch requests to backup and resync loop to allocate tons of memory. This patch has shown to reduce memory needed 5-10 times.

Reuse events	object size [KB]	etcd memory[MB]	latency 50%ile	latency 90%ile	latency 99%ile
FALSE	5	111.72	0.1005	0.1543	0.1856
TRUE	5	109.096	0.1012	0.1556	0.1874
FALSE	10	1103.924	2.7177	7.3661	11.9127
TRUE	10	291.184	2.6623	7.687	12.5456
FALSE	100	5138.712	14.0618	26.2721	29.1691
TRUE	100	641.844	13.9372	25.8267	28.519

Tested using command go run ./tools/benchmark/main.go watch-latency --watch-per-stream 1000 --streams 1 --put-total 200 --val-size 100000. Parameters adjusted for total object size be a 20MB.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[chaochn47]

Limit event prefix by slowest watch instead of compaction.

Do we expect sending events to compacted watchers (the slowest one) though? I think it should not. WDYT?

[serathius]

Do we expect sending events to compacted watchers (the slowest one) though? I think it should not. WDYT?

I expect that minRev and rev of victims is above compactedRev, if now we can just add a protection.

[codecov]

Codecov Report

Attention: Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.71%. Comparing base (78885f6) to head (348c0cb).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
server/storage/mvcc/watchable_store.go	97.14%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/storage/mvcc/watchable_store.go	93.91% <97.14%> (+0.41%)	⬆️

... and 22 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #17563      +/-   ##
==========================================
- Coverage   68.80%   68.71%   -0.09%     
==========================================
  Files         420      420              
  Lines       35599    35619      +20     
==========================================
- Hits        24494    24476      -18     
- Misses       9675     9712      +37     
- Partials     1430     1431       +1     

---

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 78885f6...348c0cb. Read the comment docs.

[serathius]

Ready to review
ping @ahrtr

[serathius]

/retest

[ahrtr]

These two cases are duplicated? see line 238-239. Better to sort the cases by minRev or maxRev?

[serathius]

Yes, it's intentional. We group multiple cases into a scenarios. Added comments to make them easier to see. This is to better test the reuse, as the function call has side effects with modifying evs.

[ahrtr]

LGTM

It would be wonderful if we have more performance data (i.e. in K8s scale test) to showcase the benefit of such improvement.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, serathius

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:

• OWNERS [ahrtr,serathius]

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

[serathius]

It would be wonderful if we have more performance data (i.e. in K8s scale test) to showcase the benefit of such improvement.

Don't think K8s scale are any close to push etcd watch into congestion, if they did they K8s would not work. See kubernetes/kubernetes#123448

That's why I'm using benchmarks. They are pretty good, however without a easily accessible metric they are not used. I'm thinking about doing some MVP to run a benchmark and upload results to perf-dash K8s, however the problem is that they have a very strict schema requirement, which doesn't match etcd.