Bumps GRPC to 1.12.0 #2649

cyli · 2018-05-25T18:25:03Z

Supercedes #2631 by bumping GRPC to 1.12.0.

This includes a bunch of fixes to our code to address breaking API changes, namely:

1.10: TLS errors are no longer surfaced when dialing with WithBlock(), so the tests need to change.
1.10: Closing a GRPC server no longer returns a transport closing error, so the tests need to change.
1.11+: transport.StreamFromContext has been removed, so our raft proxy and dispatcher can no longer use that API. transport is intended to be an internal package to GRPC anyway, so in general I think we aren't intended to use/manipulate the transports anyway.

This is based on #2646.

cyli · 2018-05-25T19:17:42Z

Oops, looks like one more issue.

--- FAIL: TestStreamRaftMessage (0.55s)
	Error Trace:	raft_test.go:978
	Error:		An error is expected but got nil.
	Messages:	Received unexpected error EOF
		

2018-05-25 18:57:11.557227 E | snap: failed to remove broken snapshot file /tmpfs/TestRaftEncryptionKeyRotationWait889009686/snap-v3-encrypted/0000000000000003-0000000000000008.snap
2018-05-25 18:57:11.809242 E | snap: failed to remove broken snapshot file /tmpfs/TestRaftEncryptionKeyRotationWait889009686/snap-v3-encrypted/0000000000000003-0000000000000009.snap
2018-05-25 18:57:12.353853 I | wal: segmented wal file /tmpfs/TestGCWAL627798653/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
2018-05-25 18:57:12.365181 I | wal: segmented wal file /tmpfs/TestGCWAL142803895/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
2018-05-25 18:57:12.376396 I | wal: segmented wal file /tmpfs/TestGCWAL383336376/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
2018-05-25 18:57:12.968345 I | wal: segmented wal file /tmpfs/TestGCWAL997199274/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
2018-05-25 18:57:12.987945 I | wal: segmented wal file /tmpfs/TestGCWAL009475841/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
2018-05-25 18:57:12.989433 I | wal: segmented wal file /tmpfs/TestGCWAL207767148/wal-v3-encrypted/0000000000000001-000000000000000d.wal is created
FAIL

I didn't change raft, so possibly something else in GRPC changed.

codecov · 2018-05-26T00:13:43Z

Codecov Report

Merging #2649 into master will increase coverage by 0.34%.
The diff coverage is 32%.

@@            Coverage Diff            @@
##           master   #2649      +/-   ##
=========================================
+ Coverage   61.85%   62.2%   +0.34%     
=========================================
  Files         134     134              
  Lines       21823   21739      -84     
=========================================
+ Hits        13499   13522      +23     
+ Misses       6869    6759     -110     
- Partials     1455    1458       +3

thaJeztah · 2018-05-26T00:51:12Z

and it's green! 🎉

anshulpundir

LGTM. Perhaps some squashing is needed on the code changes ?

1. We now call `Clone` on the tls config when cloning the MutableTLSConfig because you can't reuse a tls.Config for multiple connections, which we would be doing if something called MutableTLSCreds.Clone, which newer versions of GRPC do. 2. To prevent data races when grpc attempts to log, only set the grpc logger to discard in test init functions, rather than in TestMain functions as is documented by the `SetLoggerV2` function. Also, use `SetLoggerV2` since SetLogger is deprecated. 3. Wrap the default logger in a private struct that converts a logrus entry into a grpc LoggerV2, since the grpc Logger is deprecated. 4. Stop using transport.StreamFromContext to get the peer address when checking redirects in the raft proxy. grpc/transport is an internal package that downstream should not depend on, as the API may change (StreamFromContext for example is removed in >=1.11). peer.FromContext can get the peer address just as well. Signed-off-by: Ying Li <ying.li@docker.com>

1. The GRPC update stopped returning errors when the listener underlying a GRPC server closed, so remove that assertion from the tests. 2. Change the first error tracking in node not to check the GRPC error type for x509 errors, since it's changed between GRPC <1.10 and >=1.10, and the error message has changed as well in >=1.11. Also change the error message checking to something that works in <1.10 as well as >=1.11. Signed-off-by: Ying Li <ying.li@docker.com>

…r that can surface TLS handshake errors, which GRPC considers transient. Signed-off-by: Ying Li <ying.li@docker.com>

to reconnect. Just return an error - if an error is returned, the agent will reconnect anyway - it seems like the transport closing was a way to convince GRPC's load balancer to start reconnecting automatically. See moby#797. Signed-off-by: Ying Li <ying.li@docker.com>

cyli · 2018-05-29T19:02:28Z

@anshulpundir There are a couple I'd like to leave separate in case we need to revert any of these. The changes that I think should work regardless of GRPC version are squashed and up top.

The change to the tests to use our own custom dialer - this is a pretty ugly hack, and the GRPC devs seem to indicate that there are additional changes coming down the line for Dial and maybe the error returned by the context timeout (see TLS failures in blocking Dial calls don't provide useful error messages grpc/grpc-go#2031 (comment)). It may be easier to revert the commit than redo everything - not sure what those changes may look like.
The change where I just remove the code that deletes the transport.StreamFromContext(ctx).Close() from the dispatcher - I think this was just an optimization and is not needed, but I wasn't really sure about it. I believe if we originally couldn't close the transport, we'd just close the session and error anyway, and the agent will reconnect regardless. The original change came from agent: handle initial session message #797. Sorry to bother you @stevvooe, but does that sound correct?
The change to the test where sending a message beyond the max receivable size no longer returns an EOF because I have no idea what caused this change in behavior. The change was originally from Snapshot streaming #2458, but I can't find a corresponding change in GRPC which would change the behavior on send.

thaJeztah · 2018-05-29T19:16:50Z

manager/state/raft/raft_test.go

@@ -975,7 +975,7 @@ func TestStreamRaftMessage(t *testing.T) {

 	raftMsg := &api.StreamRaftMessageRequest{Message: msg}
 	err = stream.Send(raftMsg)
-	assert.Error(t, err, "Received unexpected error EOF")
+	assert.NoError(t, err, "Received unexpected error EOF")


Looks like the "Received unexpected error EOF" should be removed here?

i.e., just

assert.NoError(t, err)

Ah sorry, I yes, thank you.

no longer returns an EOF. Signed-off-by: Ying Li <ying.li@docker.com>

Signed-off-by: Ying Li <ying.li@docker.com>

cyli · 2018-05-31T22:59:53Z

cc @tonistiigi - Apologies for bothering you, but you were pinged for review on #797. I was wondering if you remembered enough about that to review this change: 90418ad?

#2 in my comment up above.

tonistiigi · 2018-05-31T23:53:30Z

@cyli Yes, there shouldn't be a need for that transport close.

thaJeztah

LGTM, thanks!!

dperny · 2018-06-01T18:58:40Z

LGTM, let's ship it.

cyli force-pushed the bump-grpc branch from 6897543 to 519330a Compare May 26, 2018 00:13

anshulpundir approved these changes May 29, 2018

View reviewed changes

cyli force-pushed the bump-grpc branch from 519330a to 90c7767 Compare May 29, 2018 18:58

cyli added 3 commits May 29, 2018 12:02

Change TestSecurityConfigUpdateRootCA test to use a custom GRPC diale…

6a6993f

…r that can surface TLS handshake errors, which GRPC considers transient. Signed-off-by: Ying Li <ying.li@docker.com>

cyli force-pushed the bump-grpc branch from 90c7767 to c2cd480 Compare May 29, 2018 19:02

cyli mentioned this pull request May 29, 2018

Minor GRPC cleanups #2646

Closed

thaJeztah reviewed May 29, 2018

View reviewed changes

cyli added 2 commits May 29, 2018 12:21

Sending a message beyond the max receivable message size for the server

e54ad63

no longer returns an EOF. Signed-off-by: Ying Li <ying.li@docker.com>

Vendor GRPC 1.12.0, protobuf 1.1.0, and gogo protobuf 1.0.0.

8f58755

Signed-off-by: Ying Li <ying.li@docker.com>

cyli force-pushed the bump-grpc branch from c2cd480 to 8f58755 Compare May 29, 2018 19:21

dmcgowan mentioned this pull request May 31, 2018

Update containerd to 1.1 moby/moby#36895

Merged

thaJeztah approved these changes Jun 1, 2018

View reviewed changes

nishanttotla approved these changes Jun 1, 2018

View reviewed changes

dperny merged commit edd5641 into moby:master Jun 1, 2018

cyli deleted the bump-grpc branch June 1, 2018 20:41

thaJeztah mentioned this pull request Jun 4, 2018

[WIP] Update grpc etc #2631

Closed

thaJeztah mentioned this pull request Feb 12, 2019

[18.03 backport] Use gometalinter; switch from x/net/context -> context #2827

Closed

thaJeztah mentioned this pull request Aug 19, 2019

[WIP][18.03 backport] dependency changes for newer Go versions #2881

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bumps GRPC to 1.12.0 #2649

Bumps GRPC to 1.12.0 #2649

cyli commented May 25, 2018 •

edited

Loading

cyli commented May 25, 2018 •

edited

Loading

codecov bot commented May 26, 2018 •

edited

Loading

thaJeztah commented May 26, 2018

anshulpundir left a comment

cyli commented May 29, 2018 •

edited

Loading

thaJeztah May 29, 2018 •

edited

Loading

cyli May 29, 2018

cyli Jun 1, 2018

cyli commented May 31, 2018

tonistiigi commented May 31, 2018

thaJeztah left a comment

dperny commented Jun 1, 2018 •

edited

Loading

Bumps GRPC to 1.12.0 #2649

Bumps GRPC to 1.12.0 #2649

Conversation

cyli commented May 25, 2018 • edited Loading

cyli commented May 25, 2018 • edited Loading

codecov bot commented May 26, 2018 • edited Loading

Codecov Report

thaJeztah commented May 26, 2018

anshulpundir left a comment

Choose a reason for hiding this comment

cyli commented May 29, 2018 • edited Loading

thaJeztah May 29, 2018 • edited Loading

Choose a reason for hiding this comment

cyli May 29, 2018

Choose a reason for hiding this comment

cyli Jun 1, 2018

Choose a reason for hiding this comment

cyli commented May 31, 2018

tonistiigi commented May 31, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

dperny commented Jun 1, 2018 • edited Loading

cyli commented May 25, 2018 •

edited

Loading

cyli commented May 25, 2018 •

edited

Loading

codecov bot commented May 26, 2018 •

edited

Loading

cyli commented May 29, 2018 •

edited

Loading

thaJeztah May 29, 2018 •

edited

Loading

dperny commented Jun 1, 2018 •

edited

Loading