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

WIP release-3.3: test grpc-go 1.22.1 #10952

Closed
wants to merge 10 commits into from

Conversation

hexfusion
Copy link
Contributor

This PR is an effort to resolve #9956 for release-3.3.

mbrannock and others added 4 commits July 29, 2019 18:20
gRPC has moved the transport package to an internal-only directory. This
eliminates direct use of the transport package in the stress test in
favor of the error code from gRPC that represents a connection problem.

https://godoc.org/google.golang.org/grpc/internal/transport is the new
location for the package, which says it's not intended to be imported
directly. Instead, the maintainers suggested to use the code Unavailable
to detect a connection problem.

This change slightly reorganizes the stresser test error handling.
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion hexfusion added the WIP label Jul 29, 2019
gyuho and others added 2 commits July 31, 2019 00:20
Otherwise, grpc.DialContext would just return before
connection is up.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion hexfusion force-pushed the vendor_bump branch 3 times, most recently from 9884cd5 to 9cc5474 Compare August 4, 2019 01:00
@hexfusion hexfusion changed the title WIP release-3.3: test grpc-go 1.20.1 and bump go to 1.11.12 WIP release-3.3: test grpc-go 1.22.1 Aug 4, 2019
@hexfusion
Copy link
Contributor Author

hexfusion commented Aug 4, 2019

Status update unit tests pass when GRPC_GO_REQUIRE_HANDSHAKE=off is defined. This comment seems to outline the issue I am seeing grpc/grpc-go#2636 (comment).

"While in the new behavior where HANDSHAKE is required, the client won't sent the request until the HANDSHAKE is done. This results in a deadlock." This seems to be resolved by https://github.com/grpc/grpc-go/pull/2904/commits which will be part of 1.23.

Still getting a race/failure with Watch I hope to clear this up soon. My hope is to be able to release a gRPC update only in 3.3 fixing #9956 for release-3.3 which would not change the actual balancer. Then when we feel confident in the new balancer backport. @gyuho thoughts?

@hexfusion
Copy link
Contributor Author

=== RUN   TestWatchErrConnClosed
--- FAIL: TestWatchErrConnClosed (0.12s)
    watch_test.go:671: expected rpc error: code = Canceled desc = grpc: the client connection is closing, got grpc: the client connection is closing
=== RUN   TestWatchAfterClose
==================
WARNING: DATA RACE
Write at 0x00c0000e3251 by goroutine 444:
  testing.(*common).FailNow()
      /usr/local/go/src/testing/testing.go:589 +0x4c
  testing.(*common).Fatal()
      /usr/local/go/src/testing/testing.go:628 +0x7c
  github.com/coreos/etcd/clientv3/integration.TestWatchAfterClose()
      /go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/clientv3/integration/watch_test.go:709 +0x374
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162

Previous write at 0x00c0000e3251 by goroutine 95:
  testing.(*common).FailNow()
      /usr/local/go/src/testing/testing.go:589 +0x4c
  testing.(*common).Fatalf()
      /usr/local/go/src/testing/testing.go:634 +0x94
  github.com/coreos/etcd/clientv3/integration.TestWatchAfterClose.func1()
      /go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/clientv3/integration/watch_test.go:703 +0x1e9

Goroutine 444 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:878 +0x659
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1119 +0xa8
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1117 +0x4ee
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1034 +0x2ee
  github.com/coreos/etcd/clientv3/integration.TestMain()
      /go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/clientv3/integration/main_test.go:15 +0x38
  main.main()
      _testmain.go:366 +0x221

Goroutine 95 (finished) created at:
  github.com/coreos/etcd/clientv3/integration.TestWatchAfterClose()
      /go/src/github.com/coreos/etcd/gopath/src/github.com/coreos/etcd/clientv3/integration/watch_test.go:700 +0x249
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:827 +0x162
==================
--- FAIL: TestWatchAfterClose (3.04s)
    watch_test.go:703: expected rpc error: code = Canceled desc = grpc: the client connection is closing, got context canceled
    watch_test.go:709: wc.Watch took too long
    testing.go:771: race detected during execution of test

@hexfusion
Copy link
Contributor Author

hexfusion commented Aug 4, 2019

Looks like cmux [1] could be causing Watch failures. I will test using grpcl := m.MatchWithWriters(cmux.HTTP2MatchHeaderFieldSendSettings("content-type", "application/grpc")) as outlined by cmux limitations [2]

[1] grpc/grpc-go#2636 (comment)
[2] https://github.com/soheilhy/cmux#limitations

@hexfusion hexfusion force-pushed the vendor_bump branch 2 times, most recently from bdeab15 to 471358d Compare August 5, 2019 16:13
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion hexfusion force-pushed the vendor_bump branch 7 times, most recently from d8c6b30 to 99c6dd5 Compare August 5, 2019 23:10
Signed-off-by: Sam Batschelet <sbatsche@redhat.com>
@hexfusion
Copy link
Contributor Author

All tests pass I will raise a new clean version to backport.

@hexfusion hexfusion closed this Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants