vendor: update gRPC, protobuf by thaJeztah · Pull Request #2452 · moby/swarmkit

thaJeztah · 2017-11-17T13:10:00Z

This updates the vendored packages for gRPC, protobuf, and re-generates (make generates)

Full diffs of the updated dependencies:

google.golang.org/grpc: grpc/grpc-go@v1.3.0...v1.7.5
github.com/gogo/protobuf: gogo/protobuf@v0.4...v0.5
github.com/golang/protobuf: golang/protobuf@7a211bc...1e59b77

NOTE: this PR is built on top of #2451, so needs to be rebased if that one is merged; I thought keeping them separate was cleaner, but we can close the other PR if combining them is okay 😄

relates to #2448, #2444

ping @stevvooe @aaronlehmann @dperny @anshulpundir PTAL

thaJeztah · 2017-11-17T14:44:58Z

Looks like I have some work to do;

# github.com/docker/swarmkit/api
api/ca.pb.go:971: undefined: metadata.FromContext
api/ca.pb.go:979: undefined: metadata.NewContext
api/ca.pb.go:1113: undefined: metadata.FromContext
api/ca.pb.go:1121: undefined: metadata.NewContext
api/control.pb.go:5846: undefined: metadata.FromContext
api/control.pb.go:5854: undefined: metadata.NewContext
api/dispatcher.pb.go:1589: undefined: metadata.FromContext
api/dispatcher.pb.go:1597: undefined: metadata.NewContext
api/health.pb.go:273: undefined: metadata.FromContext
api/health.pb.go:281: undefined: metadata.NewContext
api/health.pb.go:281: too many errors

thaJeztah · 2017-11-17T14:47:47Z

Due to this change; grpc/grpc-go@596a6ac#diff-b3a9f1c1b5d5600f4924efe942b13fb0 updating now

stevvooe · 2017-11-17T21:49:41Z

LGTM when the tests pass.

Let me know if you need help hunting down the correct context functions.

thaJeztah · 2017-11-17T22:24:19Z

I replaced for metadata.FromContext(ctx) with metadata.FromIncomingContext(ctx), and metadata.NewContext() with metadata.NewOutgoingContext() (based on grpc/grpc-go@596a6ac#diff-b3a9f1c1b5d5600f4924efe942b13fb0),
but reading the changelog, grpc/grpc-go#1157 added separation of "incoming" and "outgoing" contexts (previously they were merged), so likely have to change some yes, as CI is failing:

time="2017-11-17T15:15:20Z" level=info msg="dialing to target with scheme: \"\"" module=grpc 
time="2

I'll do some digging to see where the context is changed, so the other one should be used 👍

codecov · 2017-11-21T13:55:22Z

Codecov Report

Merging #2452 into master will decrease coverage by 0.02%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
- Coverage   61.78%   61.76%   -0.03%     
==========================================
  Files         134      134              
  Lines       21829    21821       -8     
==========================================
- Hits        13488    13478      -10     
- Misses       6887     6899      +12     
+ Partials     1454     1444      -10

nishanttotla · 2017-11-21T18:56:34Z

@thaJeztah are you still working on this?

thaJeztah · 2017-11-21T19:22:40Z

I was looking this afternoon to see if I could find where changes are needed, but didn't spot it directly 😓

Current failure is:

ok  	github.com/docker/swarmkit/manager/state/raft	75.762s	coverage: 70.8% of statements
time="2017-11-21T14:08:30Z" level=info msg="dialing to target with scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="could not get resolver for scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="dialing to target with scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="could not get resolver for scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="dialing to target with scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="could not get resolver for scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="dialing to target with scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=info msg="could not get resolver for scheme: \"\"" module=grpc 
time="2017-11-21T14:08:30Z" level=warning msg="no certificate expiration specified, using default" cluster.id=ris4q0hasft4l7oz06y5zym7t method="(*Server).UpdateRootCA" module=ca testHasExternalCA=false 
--- FAIL: TestCanRemoveMember (23.14s)
	Error Trace:	cluster_test.go:353
	Error:		Received unexpected error context deadline exceeded
		

	Error Trace:	cluster_test.go:355
	Error:		Expected nil, but got: &api.RaftMember{RaftID:0xd0b32a9451660d6, NodeID:"s95109a4smlcinl8vkjjyolv4", Addr:"127.0.0.1:37953", Status:api.RaftMemberStatus{Leader:false, Reachability:1, Message:""}}
		

	Error Trace:	cluster_test.go:356
	Error:		Not equal: 3 (expected)
			        != 2 (actual)
		

FAIL
coverage: 76.0% of statements
FAIL	github.com/docker/swarmkit/manager/state/raft/membership	23.241s
make: *** [coverage] Error 1

thaJeztah · 2017-11-21T19:40:51Z

+	if rb == nil {
+		// TODO(bar) return error when DNS becomes the default (implemented and
+		// registered by DNS package).
+		grpclog.Infof("could not get resolver for scheme: %q", target.Scheme)


Info messages come from here; they're just "info", so may not be related to CI failing (although missing DNS could probably cause the timeout)?

Locally, tests pass (but same warnings are printed);

Not sure why we're having these issues lately. I've restarted the CI.

LOL, now it's even worse; definitely possible it's related to this PR, but don't have a clue yet where the problem would be

crosbymichael · 2018-05-09T21:04:06Z

Can we get some maintainers looking at this problem? We are trying to fix this but there are some larger issues that swarm it panicing over version bumps.

dmcgowan · 2018-05-10T20:59:35Z

We need to get moby/moby#36895 into the next release, what can we do to help get this unblocked?

nishanttotla · 2018-05-11T20:49:34Z

We'll do this on priority.

Restarting CI one more time to judge if the failure is due to flakiness, but will look into the diff to try and figure out.

dperny · 2018-05-11T20:49:51Z

@nishanttotla it needs to be rebased, just in case.

thaJeztah · 2018-05-11T20:58:06Z

rebased

nishanttotla · 2018-05-11T21:04:49Z

@thaJeztah it seems like you'll have to remove vendor/ folder and re-run vndr once to fix the CI for the current error. I think this is a CI issue related to the latest version of vndr.

thaJeztah · 2018-05-11T21:07:55Z

Hm, let me try

anshulpundir · 2018-05-11T22:32:44Z

+	errStr := fmt.Sprintf("grpc: received message larger than max (%d vs. %d)", raftMsg.Size(), transport.GRPCMaxMsgSize)
 	s, _ := status.FromError(err)
-	assert.Equal(t, codes.Internal, s.Code())
+	assert.Equal(t, codes.ResourceExhausted, s.Code())


Is this change needed along with the other changes ?

I think so (it was a while back that I opened this PR);I think they changed these errors

dperny · 2018-05-14T19:36:38Z

i'm pouring through the logs on these test failures and i'm pretty confused. it seems to me that something may have changed in the internals of grpc, but i can't figure out what it would have been.

thaJeztah · 2018-05-14T19:54:04Z

I did notice there are more recent versions since I created this PR; would it be worth updating to a more recent version?

dperny · 2018-05-14T19:54:36Z

I'm unsure. It's certainly worth trying, to see if it will quietly start passing again. I'm digging into the root cause right now.

anshulpundir · 2018-05-14T20:34:29Z

agreed. it makes sense to try upgrading to the latest version @thaJeztah

dperny · 2018-05-14T20:46:00Z

I figured it out, 1 sec, verifying the fix.

dperny · 2018-05-14T21:29:52Z

@thaJeztah I pushed to your branch a fix, I hope you don't mind. The problem was that we were string matching an error message, but the message changed.

thaJeztah · 2018-05-14T21:41:58Z

+	}
+
+	// additionally, the error we're looking for is an internal error
+	if grpcStatus.Code() != codes.Internal {


Wondering; does it return a specific .Code() that could be useful for detection (in addition to string-matching)?

thaJeztah · 2018-05-14T21:49:04Z

-// If this is not set, gRPC uses the default 4MB.
+// MaxMsgSize returns a ServerOption to set the max message size in bytes the server can receive.
+// If this is not set, gRPC uses the default limit. Deprecated: use MaxRecvMsgSize instead.
 func MaxMsgSize(m int) ServerOption {


@dperny Just saw this was marked deprecated; perhaps you can update https://github.com/docker/swarmkit/blob/8aa9c33bcdff9ea38fc79e0b1d054199917513f3/manager/manager.go#L272, which is using the deprecated function

thaJeztah · 2018-05-14T21:56:58Z

+		if err := t.framer.fr.WriteWindowUpdate(0, delta); err != nil {
 			t.Close()
-			return nil, connectionErrorf(true, err, "transport: %v", err)
+			return nil, connectionErrorf(true, err, "transport: failed to write window update: %v", err)


Looks indeed that ConnectionError doesn't have any additional information 😞

thaJeztah

LGTM 👍

anshulpundir

LGTM, but a squash is needed.

thaJeztah · 2018-05-15T12:20:54Z

but a squash is needed

commits looked sensibly grouped, but not a maintainer

anshulpundir · 2018-05-15T12:26:52Z

Update grpc, protobuf
Regenerate protobuf
Update tests for changes in gRPC
Fix breakage resulting from error string matching
Update deprecated grpc function

I think 4, 5 should be squashed into 1. I don't think these commits mean much individually.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Fixes an issue resulting from string matching the error result of grpc methods. Adds comments documenting the fix, as well as explaining how to avoid this issue in the future. Also updates manager/manager.go to use the function MaxRecvMsgSize, instead of the deprecated equivalent MaxMsgSize. Signed-off-by: Drew Erny <drew.erny@docker.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2018-05-15T15:29:31Z

rebased and squashed

thaJeztah · 2018-05-15T15:30:48Z

Oh, I may have mis-interpreted; you want the changes into the same commit as the revendor? (in moby/moby we usually keep local changes separate from the vendor commit)

anshulpundir · 2018-05-15T16:35:08Z

(in moby/moby we usually keep local changes separate from the vendor commit)

I guess that makes sense. Thanks for squashing the last two!

thaJeztah force-pushed the update-protobuf-grpc branch 2 times, most recently from c130a68 to 23df0f5 Compare November 17, 2017 15:03

thaJeztah force-pushed the update-protobuf-grpc branch from 23df0f5 to d94498c Compare November 21, 2017 13:55

thaJeztah commented Nov 21, 2017

View reviewed changes

thaJeztah force-pushed the update-protobuf-grpc branch 2 times, most recently from 8c97b32 to 079fc3c Compare December 6, 2017 10:07

thaJeztah mentioned this pull request Apr 17, 2018

Don't use wrappers for grpc metadata #2610

Merged

thaJeztah force-pushed the update-protobuf-grpc branch from 4f9ecee to ee0bb7a Compare April 17, 2018 21:08

dmcgowan mentioned this pull request Apr 18, 2018

Update containerd to 1.1 moby/moby#36895

Merged

thaJeztah force-pushed the update-protobuf-grpc branch from ee0bb7a to e788cf4 Compare May 11, 2018 20:57

thaJeztah force-pushed the update-protobuf-grpc branch 2 times, most recently from e3b3cb8 to 2a14d4a Compare May 11, 2018 21:36

anshulpundir reviewed May 11, 2018

View reviewed changes

dperny force-pushed the update-protobuf-grpc branch from 3ed989f to 8dc087b Compare May 14, 2018 21:36

thaJeztah commented May 14, 2018

View reviewed changes

anshulpundir approved these changes May 15, 2018

View reviewed changes

thaJeztah and others added 4 commits May 15, 2018 17:27

Update grpc, protobuf

59f5b65

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Regenerate protobuf

61c4bd2

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Update tests for changes in gRPC

58cea3d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the update-protobuf-grpc branch from 3cac027 to 8a2b6fd Compare May 15, 2018 15:29

anshulpundir merged commit b39ef49 into moby:master May 15, 2018

thaJeztah deleted the update-protobuf-grpc branch May 15, 2018 16:57

thaJeztah mentioned this pull request May 15, 2018

Bump swarmkit to 0fe3f12bdbfee0d47216de0118c569b1042e1588 moby/moby#37059

Closed

dperny mentioned this pull request May 18, 2018

[WIP] Update grpc etc #2631

Closed

thaJeztah mentioned this pull request Aug 19, 2019

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

Closed

Uh oh!

Conversation

thaJeztah commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Nov 17, 2017

Uh oh!

thaJeztah commented Nov 17, 2017

Uh oh!

stevvooe commented Nov 17, 2017

Uh oh!

thaJeztah commented Nov 17, 2017

Uh oh!

codecov Bot commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nishanttotla commented Nov 21, 2017

Uh oh!

thaJeztah commented Nov 21, 2017

Uh oh!

thaJeztah Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishanttotla Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

crosbymichael commented May 9, 2018

Uh oh!

dmcgowan commented May 10, 2018

Uh oh!

nishanttotla commented May 11, 2018

Uh oh!

dperny commented May 11, 2018

Uh oh!

thaJeztah commented May 11, 2018

Uh oh!

nishanttotla commented May 11, 2018

Uh oh!

thaJeztah commented May 11, 2018

Uh oh!

anshulpundir May 11, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 11, 2018

Choose a reason for hiding this comment

Uh oh!

dperny commented May 14, 2018

Uh oh!

thaJeztah commented May 14, 2018

Uh oh!

dperny commented May 14, 2018

Uh oh!

anshulpundir commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dperny commented May 14, 2018

Uh oh!

dperny commented May 14, 2018

Uh oh!

thaJeztah May 14, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 14, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah May 14, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

anshulpundir left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 15, 2018

thaJeztah commented Nov 17, 2017 •

edited

Loading

codecov Bot commented Nov 21, 2017 •

edited

Loading

thaJeztah Nov 21, 2017 •

edited

Loading

anshulpundir commented May 14, 2018 •

edited

Loading

anshulpundir commented May 15, 2018 •

edited

Loading