Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[etcd] Upgrade etcd client library to 3.4.3 to fix data race in tests #2101

Merged
merged 7 commits into from
Jan 12, 2020

Conversation

robskillington
Copy link
Collaborator

What this PR does / why we need it:

Fixes the data races we were seeing in unit tests. The earliest release that includes the fix is on the 3.4 release so we are upgrading to 3.4.3.

Our "TestClient" unit test was experiencing data race in package src/cluster/client/etcd.

=== RUN   TestClient                                                                                                                                                                                                                                                                                                                                               [142/880]
2020-01-12 08:23:42.066828 I | integration: launching 4035819850191458261 (unix://localhost:40358198501914582610)
2020-01-12 08:23:42.075830 I | etcdserver: name = 4035819850191458261
2020-01-12 08:23:42.075883 I | etcdserver: data dir = /var/folders/_g/hmzqg1j12hb08_d3jn26tnsm0000gn/T/etcd849792472
2020-01-12 08:23:42.075927 I | etcdserver: member dir = /var/folders/_g/hmzqg1j12hb08_d3jn26tnsm0000gn/T/etcd849792472/member
2020-01-12 08:23:42.075959 I | etcdserver: heartbeat = 10ms
2020-01-12 08:23:42.075988 I | etcdserver: election = 100ms
2020-01-12 08:23:42.076012 I | etcdserver: snapshot count = 0
2020-01-12 08:23:42.076057 I | etcdserver: advertise client URLs = unix://127.0.0.1:2100230805
2020-01-12 08:23:42.076091 I | etcdserver: initial advertise peer URLs = unix://127.0.0.1:2100130805
2020-01-12 08:23:42.076131 I | etcdserver: initial cluster = 4035819850191458261=unix://127.0.0.1:2100130805
2020-01-12 08:23:42.157559 I | etcdserver: starting member 5c2b4ba308bb1e8c in cluster c82e8fb8629e9da3
2020-01-12 08:23:42.157650 I | raft: 5c2b4ba308bb1e8c became follower at term 0
2020-01-12 08:23:42.157708 I | raft: newRaft 5c2b4ba308bb1e8c [peers: [], term: 0, commit: 0, applied: 0, lastindex: 0, lastterm: 0]
2020-01-12 08:23:42.157742 I | raft: 5c2b4ba308bb1e8c became follower at term 1
2020-01-12 08:23:42.192919 W | auth: simple token is not cryptographically signed
2020-01-12 08:23:42.220864 I | etcdserver: set snapshot count to default 100000
2020-01-12 08:23:42.220919 I | etcdserver: starting server... [version: 3.2.28, cluster version: to_be_decided]
2020-01-12 08:23:42.221446 I | etcdserver: 5c2b4ba308bb1e8c as single-node; fast-forwarding 9 ticks (election ticks 10)
2020-01-12 08:23:42.221720 E | etcdserver: cannot monitor file descriptor usage (cannot get FDUsage on darwin)
2020-01-12 08:23:42.222390 I | etcdserver/membership: added member 5c2b4ba308bb1e8c [unix://127.0.0.1:2100130805] to cluster c82e8fb8629e9da3
==================
WARNING: DATA RACE
Write at 0x000002b54290 by goroutine 121:
  github.com/m3db/m3/vendor/github.com/coreos/etcd/etcdserver/api/v3rpc.Server.func1()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/grpclog/loggerv2.go:68 +0x264
  sync.(*Once).doSlow()
      /usr/local/Cellar/go/1.13.4/libexec/src/sync/once.go:66 +0x100
  sync.(*Once).Do()
      /usr/local/Cellar/go/1.13.4/libexec/src/sync/once.go:57 +0x68
  github.com/m3db/m3/vendor/github.com/coreos/etcd/etcdserver/api/v3rpc.Server()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/grpc.go:62 +0xaaf
  github.com/m3db/m3/vendor/github.com/coreos/etcd/integration.(*member).Launch()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/integration/cluster.go:775 +0xe8c
  github.com/m3db/m3/vendor/github.com/coreos/etcd/integration.(*cluster).Launch.func1()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/integration/cluster.go:178 +0x38

Previous read at 0x000002b54290 by goroutine 117:
  github.com/m3db/m3/vendor/google.golang.org/grpc.(*ccBalancerWrapper).UpdateBalancerState()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/grpclog/grpclog.go:45 +0xe2
  github.com/m3db/m3/vendor/google.golang.org/grpc.(*balancerWrapper).HandleSubConnStateChange()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/balancer_v1_wrapper.go:247 +0x3dd
  github.com/m3db/m3/vendor/google.golang.org/grpc.(*ccBalancerWrapper).watcher()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/balancer_conn_wrappers.go:120 +0x43c

Goroutine 121 (running) created at:
  github.com/m3db/m3/vendor/github.com/coreos/etcd/integration.(*cluster).Launch()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/integration/cluster.go:177 +0xe3
  github.com/m3db/m3/vendor/github.com/coreos/etcd/integration.NewClusterV3()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/integration/cluster.go:982 +0x11c
  github.com/m3db/m3/src/cluster/client/etcd.testNewETCDFn()
      /Users/r/go/src/github.com/m3db/m3/src/cluster/client/etcd/client_test.go:366 +0xb6
  github.com/m3db/m3/src/cluster/client/etcd.TestClient()
      /Users/r/go/src/github.com/m3db/m3/src/cluster/client/etcd/client_test.go:121 +0x1df
  testing.tRunner()
      /usr/local/Cellar/go/1.13.4/libexec/src/testing/testing.go:909 +0x199

Goroutine 117 (running) created at:
  github.com/m3db/m3/vendor/google.golang.org/grpc.newCCBalancerWrapper()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/balancer_conn_wrappers.go:108 +0x23d
  github.com/m3db/m3/vendor/google.golang.org/grpc.DialContext()
      /Users/r/go/src/github.com/m3db/m3/vendor/google.golang.org/grpc/clientconn.go:449 +0x9aa
  github.com/m3db/m3/vendor/github.com/coreos/etcd/clientv3.(*Client).dial()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/clientv3/client.go:352 +0x285
  github.com/m3db/m3/vendor/github.com/coreos/etcd/clientv3.newClient()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/clientv3/client.go:421 +0x6d3
  github.com/m3db/m3/src/cluster/client/etcd.newClient()
      /Users/r/go/src/github.com/m3db/m3/vendor/github.com/coreos/etcd/clientv3/client.go:80 +0x244
  github.com/m3db/m3/src/cluster/client/etcd.(*csclient).etcdClientGen.func1()
      /Users/r/go/src/github.com/m3db/m3/src/cluster/client/etcd/client.go:268 +0x8c
  github.com/m3db/m3/src/x/retry.(*retrier).attempt()
      /Users/r/go/src/github.com/m3db/m3/src/x/retry/retry.go:113 +0x95
  github.com/m3db/m3/src/x/retry.(*retrier).Attempt()
      /Users/r/go/src/github.com/m3db/m3/src/x/retry/retry.go:98 +0x4b
  github.com/m3db/m3/src/cluster/client/etcd.(*csclient).etcdClientGen()
      /Users/r/go/src/github.com/m3db/m3/src/cluster/client/etcd/client.go:266 +0x313

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

Copy link
Collaborator

@benraskin92 benraskin92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending green build

Copy link
Collaborator

@schallert schallert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending CI. @vdarulis @andrewmains12 this might require bumping some deps internally @ Uber as well.

@codecov
Copy link

codecov bot commented Jan 12, 2020

Codecov Report

Merging #2101 into master will increase coverage by 19.2%.
The diff coverage is 94.7%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2101      +/-   ##
=========================================
+ Coverage      53%   72.3%   +19.2%     
=========================================
  Files         988    1008      +20     
  Lines       97296   86653   -10643     
=========================================
+ Hits        51649   62713   +11064     
+ Misses      41073   19741   -21332     
+ Partials     4574    4199     -375
Flag Coverage Δ
#aggregator 82% <ø> (+24.1%) ⬆️
#cluster 85.7% <ø> (-14.3%) ⬇️
#collector 64.8% <ø> (+16%) ⬆️
#dbnode 79.5% <ø> (-20.5%) ⬇️
#m3em 73.2% <ø> (+6.2%) ⬆️
#m3ninx 73.9% <ø> (-26.1%) ⬇️
#m3nsch 51.1% <ø> (-26.9%) ⬇️
#metrics 17.6% <ø> (-82.4%) ⬇️
#msg 74.9% <ø> (+0.1%) ⬆️
#query 68.5% <100%> (-1.1%) ⬇️
#x 83.2% <94.5%> (-16.8%) ⬇️

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 7f2ee5b...9ee137a. Read the comment docs.

@robskillington robskillington merged commit 36263fe into master Jan 12, 2020
@robskillington robskillington deleted the r/upgrade-etcd-client-library-fix-data-races branch January 12, 2020 16:53
@vdarulis
Copy link
Collaborator

Thanks @schallert - been eyeing to do this as well. Just curious, how does this even work without bumping grpc dependency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants