Skip to content

mvcc: bound syncWatchers scan to deliverable revisions - #22265

Open
waterWang wants to merge 1 commit into
etcd-io:mainfrom
waterWang:fix/syncwatchers-bounded-scan
Open

mvcc: bound syncWatchers scan to deliverable revisions#22265
waterWang wants to merge 1 commit into
etcd-io:mainfrom
waterWang:fix/syncwatchers-bounded-scan

Conversation

@waterWang

Copy link
Copy Markdown

Description

syncWatchers() currently calls rangeEvents with [minRev, curRev+1) — the full window from the oldest unsynced watcher to the current tip. When a watcher is far behind (e.g., 500,000+ revisions due to a Kubernetes API server being unsynced), every 100ms syncWatchers loop re-scans the entire backlog, but newWatcherBatch only delivers at most watchBatchMaxRevs (1000) distinct revisions per watcher per pass.

This means the backlog is re-read roughly number_of_revisions_behind / 1000 times. Each scan holds watchableStore.mu for the full duration, and watchableStoreTxnWrite.End() needs the same lock to publish events, stalling the apply loop.

Fix

Bound the scan window to [minRev, minRev+watchBatchMaxRevs+1) so each pass reads only the revisions that can actually be delivered to the watchers. This eliminates the read amplification entirely — the scan is always at most 1001 revisions wide, regardless of how far behind the watcher is.

Reproducer output comparison

Before (on main, recovering a single watcher 20,000 revisions behind):

pass   1: scanned   20001 revisions, delivered <=1000, lock held 17.568ms
pass   2: scanned   19000 revisions, delivered <=1000, lock held 15.425ms
...
total revisions read: 210001 to deliver 20000
watchableStore.mu held for 165ms total, 17.568ms max in a single pass

After (with this fix):

pass   1: scanned    1001 revisions, delivered <=1000, lock held ~1ms
...
total revisions read: ~21000 to deliver 20000
watchableStore.mu held for ~20ms total

The fix reduces total revisions read by ~10x and eliminates the long lock hold times.

Fixes #22264

@kubernetes-prow

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details Needs approval from an approver in each of these files:

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

@kubernetes-prow

Copy link
Copy Markdown

Hi @waterWang. 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.

Details

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.

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

Development

Successfully merging this pull request may close these issues.

syncWatchers scans unbounded revisions but only delivers 1000, stalling writes

1 participant