mvcc: restore unsynced watchers #9281

yudai · 2018-02-06T01:27:31Z

In case syncWatchersLoop() starts before Restore() is called,
watchers already added by that moment are moved to s.synced by the loop.
However, there is a broken logic that moves watchers from s.synced
to s.uncyned without setting keyWatchers of the watcherGroup.
Eventually syncWatchers() fails to pickup those watchers from s.unsynced
and no events are sent to the watchers, because newWatcherBatch() called
in the function uses wg.watcherSetByKey() internally that requires
a proper keyWatchers value.

This might be a cause of missing update issues that happen on node failures.

xiang90 · 2018-02-06T04:09:44Z

lgtm.

@yudai can you add a test for this fix?

xiang90 · 2018-02-06T04:10:18Z

/cc @gyuho @jpbetz this needs to be backported to both 3.2 and 3.3

codecov-io · 2018-02-06T07:57:49Z

Codecov Report

Merging #9281 into master will decrease coverage by 0.22%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #9281      +/-   ##
==========================================
- Coverage      76%   75.78%   -0.23%     
==========================================
  Files         364      364              
  Lines       30231    30231              
==========================================
- Hits        22978    22911      -67     
- Misses       5657     5716      +59     
- Partials     1596     1604       +8

Impacted Files	Coverage Δ
internal/mvcc/watchable_store.go	`85.23% <0%> (-1.11%)`	⬇️
proxy/grpcproxy/register.go	`69.44% <0%> (-25%)`	⬇️
clientv3/namespace/watch.go	`87.87% <0%> (-12.13%)`	⬇️
etcdserver/api/v3rpc/lease.go	`78.37% <0%> (-8.11%)`	⬇️
internal/auth/simple_token.go	`87.03% <0%> (-6.49%)`	⬇️
pkg/adt/interval_tree.go	`87.08% <0%> (-4.51%)`	⬇️
proxy/grpcproxy/watch.go	`90.06% <0%> (-3.11%)`	⬇️
etcdserver/api/v3rpc/watch.go	`83.4% <0%> (-2.98%)`	⬇️
etcdserver/api/v3election/election.go	`64.7% <0%> (-2.95%)`	⬇️
clientv3/concurrency/election.go	`79.52% <0%> (-2.37%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

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

In case syncWatchersLoop() starts before Restore() is called, watchers already added by that moment are moved to s.synced by the loop. However, there is a broken logic that moves watchers from s.synced to s.uncyned without setting keyWatchers of the watcherGroup. Eventually syncWatchers() fails to pickup those watchers from s.unsynced and no events are sent to the watchers, because newWatcherBatch() called in the function uses wg.watcherSetByKey() internally that requires a proper keyWatchers value.

yudai · 2018-02-06T18:33:02Z

@xiang90 Added a test case as a sub test. Let me know in case you like simpler test cases.

xiang90 · 2018-02-06T18:39:25Z

internal/mvcc/watchable_store_test.go

 	}
+
+	t.Run("Normal", test(0))
+	t.Run("RunSyncWatchLoopBeforeRestore", test(time.Millisecond*120)) // longer than default waitDuration


@yudai this test will 100% fail without this patch?

@xiang90 It's hard to guarantee because it's a timing issue, but it fails at least 99% on my env.
The existing test case was failing sometimes because of random delay caused by the runtime here.

good enough.

xiang90 · 2018-02-06T18:41:31Z

I prefer to move the sub test func out of the test func. Not a huge fan of defining func scope functions. but not a big deal.

LGTM

xiang90 · 2018-02-06T18:41:37Z

defer to @gyuho

gyuho

lgtm. Thanks @yudai!

/cc @jpbetz Probably we can do patch release together with Golang security release https://groups.google.com/forum/#!topic/golang-announce/lGkem2e5WyQ. Probably this or next week.

…rigin-release-3.2 Automated cherry pick of #9281

jpbetz · 2018-02-13T19:06:25Z

cc @wojtek-t

…rade Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Bump etcd server patch version to 3.2.16 etcd 3.2.16 contains a critical fix for HA clusters: etcd-io/etcd#9281 Also, update newly added tests to use `REGISTRY` make variable. Release note: ```release-note Upgrade the default etcd server version to 3.2.16 ```

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Bump etcd server patch version to 3.2.16 etcd 3.2.16 contains a critical fix for HA clusters: etcd-io/etcd#9281 Also, update newly added tests to use `REGISTRY` make variable. Release note: ```release-note Upgrade the default etcd server version to 3.2.16 ``` Kubernetes-commit: 9aae9b58a5e2b78d27f2bd093fc1c4299bf63c36

yudai force-pushed the fix_unrestored_watchers branch from 36cfb33 to 438c7eb Compare February 6, 2018 01:36

xiang90 added kind/need-backport-v3.2 backport/v3.3 labels Feb 6, 2018

yudai force-pushed the fix_unrestored_watchers branch from 438c7eb to fcab10b Compare February 6, 2018 18:30

xiang90 reviewed Feb 6, 2018

View reviewed changes

gyuho approved these changes Feb 6, 2018

View reviewed changes

gyuho merged commit 63183f8 into etcd-io:master Feb 6, 2018

xiang90 mentioned this pull request Feb 6, 2018

test failed: TestWatchRestore #9086

Closed

jpbetz mentioned this pull request Feb 7, 2018

Automated cherry pick of #9281 #9297

Merged

jpbetz added a commit to jpbetz/etcd that referenced this pull request Feb 7, 2018

mvcc: fix watchable store test for 3.2 cherrypick of etcd-io#9281

33633da

jpbetz added a commit that referenced this pull request Feb 8, 2018

Merge pull request #9297 from jpbetz/automated-cherry-pick-of-#9281-o…

b5abfe1

…rigin-release-3.2 Automated cherry pick of #9281

jpbetz mentioned this pull request Feb 13, 2018

Bump etcd server patch version to 3.2.16 kubernetes/kubernetes#59836

Merged

jpbetz added a commit to jpbetz/etcd that referenced this pull request Mar 8, 2018

mvcc: fix watchable store test for 3.2 cherrypick of etcd-io#9281

087b9aa

This was referenced May 18, 2018

*: test watch restore in network-partitioned node, clean up fields, logging #9745

Merged

mvcc: watcherGroup chooseAll panic (skip proxy tests for now) #9765

Merged

gyuho mentioned this pull request May 24, 2018

mvcc: fix panic by allowing future revision watcher from restore operation #9775

Merged

chaochn47 mentioned this pull request Apr 7, 2023

tests: deflake TestV3WatchRestoreSnapshotUnsync #15667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mvcc: restore unsynced watchers #9281

mvcc: restore unsynced watchers #9281

yudai commented Feb 6, 2018 •

edited

Loading

xiang90 commented Feb 6, 2018

xiang90 commented Feb 6, 2018

codecov-io commented Feb 6, 2018

yudai commented Feb 6, 2018

xiang90 Feb 6, 2018

yudai Feb 6, 2018

xiang90 Feb 6, 2018

xiang90 commented Feb 6, 2018

xiang90 commented Feb 6, 2018

gyuho left a comment

jpbetz commented Feb 13, 2018

mvcc: restore unsynced watchers #9281

mvcc: restore unsynced watchers #9281

Conversation

yudai commented Feb 6, 2018 • edited Loading

xiang90 commented Feb 6, 2018

xiang90 commented Feb 6, 2018

codecov-io commented Feb 6, 2018

Codecov Report

yudai commented Feb 6, 2018

xiang90 Feb 6, 2018

Choose a reason for hiding this comment

yudai Feb 6, 2018

Choose a reason for hiding this comment

xiang90 Feb 6, 2018

Choose a reason for hiding this comment

xiang90 commented Feb 6, 2018

xiang90 commented Feb 6, 2018

gyuho left a comment

Choose a reason for hiding this comment

jpbetz commented Feb 13, 2018

yudai commented Feb 6, 2018 •

edited

Loading