Avoid lowering revision of watchers in the future after restore #20281

serathius · 2025-07-04T12:57:01Z

Fixes #20221

The bug is in resync setting minRev. It assumes that watcher can only be behind, so after synchronizing it sets revision to current. However restore moves all watcher to unsynced, even those that are in the future. Those watchers don't get any events from resync, but their minRev is overriden.

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius · 2025-07-04T13:17:10Z

server/storage/mvcc/watchable_store.go

 			// Next retry of syncWatchers would try to resend the compacted watch response to w.ch
 			continue
 		}
-		w.minRev = curRev + 1


It only happens when watching a future revision?

We might want to add such protection for all places which update w.minRev.

func (w *watcher) updateMinRev(rev) { if rev > w.minRev { w.minRev = rev } }

Applying this everywhere might hide other kinds of bugs. I would prefer to cover the code with tests and then rewrite it.

codecov · 2025-07-04T13:32:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.95%. Comparing base (866bc07) to head (cf0369e).
Report is 4 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/storage/mvcc/watchable_store.go	`93.73% <100.00%> (-0.34%)`	⬇️

... and 45 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20281      +/-   ##
==========================================
+ Coverage   68.96%   69.95%   +0.98%     
==========================================
  Files         415      399      -16     
  Lines       34595    34027     -568     
==========================================
- Hits        23860    23804      -56     
+ Misses       9340     8838     -502     
+ Partials     1395     1385      -10

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 866bc07...cf0369e. Read the comment docs.

🚀 New features to boost your workflow:

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

ahrtr

LGTM

Thanks for the great work!

k8s-ci-robot · 2025-07-04T15:27:22Z

[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

fuweid

LGTM

Good catch!

ivanvc · 2025-07-06T04:49:34Z

/cherry-pick release-3.6

k8s-infra-cherrypick-robot · 2025-07-06T04:50:10Z

@ivanvc: new pull request created: #20286

In response to this:

/cherry-pick release-3.6

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.

serathius · 2025-07-07T07:30:19Z

/cherry-pick release-3.5
/cherry-pick release-3.4

k8s-infra-cherrypick-robot · 2025-07-07T07:30:54Z

@serathius: #20281 failed to apply on top of branch "release-3.4":

Applying: Avoid lowering revision of watchers in the future after restore
Using index info to reconstruct a base tree...
A	server/storage/mvcc/watchable_store.go
A	server/storage/mvcc/watchable_store_test.go
Falling back to patching base and 3-way merge...
Auto-merging mvcc/watchable_store_test.go
CONFLICT (content): Merge conflict in mvcc/watchable_store_test.go
Auto-merging mvcc/watchable_store.go
CONFLICT (content): Merge conflict in mvcc/watchable_store.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Avoid lowering revision of watchers in the future after restore

In response to this:

/cherry-pick release-3.5
/cherry-pick release-3.4

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.

serathius · 2025-07-07T08:35:49Z

/cherry-pick release-3.5

k8s-infra-cherrypick-robot · 2025-07-07T08:36:24Z

@serathius: #20281 failed to apply on top of branch "release-3.5":

Applying: Avoid lowering revision of watchers in the future after restore
Using index info to reconstruct a base tree...
A	server/storage/mvcc/watchable_store.go
A	server/storage/mvcc/watchable_store_test.go
Falling back to patching base and 3-way merge...
Auto-merging server/mvcc/watchable_store_test.go
CONFLICT (content): Merge conflict in server/mvcc/watchable_store_test.go
Auto-merging server/mvcc/watchable_store.go
CONFLICT (content): Merge conflict in server/mvcc/watchable_store.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 Avoid lowering revision of watchers in the future after restore

In response to this:

/cherry-pick release-3.5

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.

serathius · 2025-07-07T08:42:19Z

Manual backport #20290

k8s-ci-robot requested review from ahrtr and fuweid July 4, 2025 12:57

k8s-ci-robot added approved size/L labels Jul 4, 2025

serathius force-pushed the watch-restore-future branch 2 times, most recently from 0536c4d to a3ac879 Compare July 4, 2025 13:02

Avoid lowering revision of watchers in the future after restore

cf0369e

Signed-off-by: Marek Siarkowicz <[email protected]>

serathius force-pushed the watch-restore-future branch from a3ac879 to cf0369e Compare July 4, 2025 13:09

serathius mentioned this pull request Jul 4, 2025

Plan to release v3.6.2 #20268

Closed

2 tasks

serathius commented Jul 4, 2025

View reviewed changes

ahrtr approved these changes Jul 4, 2025

View reviewed changes

ahrtr added backport/v3.4 backport/v3.5 backport/v3.6 labels Jul 4, 2025

serathius merged commit 9fb8c36 into etcd-io:main Jul 4, 2025
34 checks passed

serathius mentioned this pull request Jul 4, 2025

[Anithesis] Inject assertions in place etcd verify. #20283

Closed

fuweid reviewed Jul 5, 2025

View reviewed changes

k8s-infra-cherrypick-robot mentioned this pull request Jul 6, 2025

[release-3.6] Avoid lowering revision of watchers in the future after restore #20286

Merged

This was referenced Jul 7, 2025

[release-3.5] Avoid lowering revision of watchers in the future after restore #20290

Merged

[release-3.4] Avoid lowering revision of watchers in the future after restore #20291

Merged

kishen-v mentioned this pull request Jul 14, 2025

[release-3.6] Prevent flakes with TestWatchRestore #20330

Merged

serathius mentioned this pull request Jul 16, 2025

[Anithesis] Inject assertions in place gofail #20346

Closed

silentred mentioned this pull request Aug 15, 2025

Watch response revision can be lower than the required starting revision #19487

Closed

4 tasks

Avoid lowering revision of watchers in the future after restore #20281

Avoid lowering revision of watchers in the future after restore #20281

Uh oh!

Conversation

serathius commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serathius Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

ahrtr Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

ahrtr Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

serathius Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

ahrtr Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ahrtr left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 4, 2025

Uh oh!

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

ivanvc commented Jul 6, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Jul 6, 2025

Uh oh!

serathius commented Jul 7, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Jul 7, 2025

Uh oh!

serathius commented Jul 7, 2025

Uh oh!

k8s-infra-cherrypick-robot commented Jul 7, 2025

Uh oh!

serathius commented Jul 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

serathius commented Jul 4, 2025 •

edited

Loading

codecov bot commented Jul 4, 2025 •

edited

Loading