Race detector appears to be triggered because GRPC previously did not call the Clone method on MutableTLSCreds in our configuration, but now does, and the Clone method is evidently not concurrency safe in this context.

ping @cyli ^^ feels like your area 🤗

The other failures are the same damned bug as #2452, which is that the returned error from GRPC changed again.

Yeah, the problem is once again that we try to fail on an error that otherwise is transient, and GRPC no longer as of that version exposes the error we need. Is there anyone in Docker with more experience with GRPC that might know the approach to fix this?

The other failures are the same damned bug as #2452, which is that the returned error from GRPC changed again.

Ugh.. I saw the failures and actually thought it could be (but it was late and didn't look into that)

As far as I can tell, there is no longer any way to get the root error out, which means we can't differentiate cert errors from other errors.

Boo! We should open a ticket for that upstream (breaking change.. again); do you think we should open a PR upstream? While looking at the code, I think I saw that other errors have a .Code() in the type that allowed getting an error-code (which probably is a lot better to detect what kind of error was produced)?

@dperny could this be the cause? grpc/grpc-go@7aea499#diff-3081f1aaaf958d7d5e00ccec7a62ea6f

Not sure if we hit that part, but it's changing codes.Internal to codes.Unauthenticated. If that's indeed the code we hit, then this part would ignore the error;

https://github.com/docker/swarmkit/blob/8a2b6fd64944bcef8154ced28f90aeec6abfeb04/node/node.go#L1315-L1318

Codecov Report

Merging #2631 into master will decrease coverage by 8.95%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2631      +/-   ##
==========================================
- Coverage   67.21%   58.26%   -8.96%     
==========================================
  Files         104       10      -94     
  Lines       15450     1253   -14197     
==========================================
- Hits        10384      730    -9654     
+ Misses       3998      453    -3545     
+ Partials     1068       70     -998

YOLO - pushed a commit to see if that was indeed the problem 😅

edit: nope, that didn't work; removed again

Negative, the error that's bubbling up instead of the x509 error is this:

rpc error: code = Unavailable desc = all SubConns are in TransientFailure

my understanding of what changed in GRPC is that it now integrally uses a Balancer, which hides the actual connection to the server behind a SubConn. The idea, I think, might be to have multiple open connections to various servers. I'm not sure, exactly.

The end result, though, is that calling grpc tries to get one of these SubConns to use. But, because the x509 error is considered "Transient", we get back an error that all of the SubConns are in a transient failure state (meaning we could expect them to work again at some point, except the whole point of the broken code is to catch a case where we know the system won't work again at any point).

@dperny do you see a path forward on this or need help looking into it?

Looking into the Clone issue. From https://groups.google.com/forum/#!topic/grpc-io/yCUwuHycNWk we may have to write our own custom dialer if we want to surface the X509 errors? :|

Update: So for some of the tests, we can just use our own GRPC dialer. I'm less happy about doing this in our actual code however, because we'd have to write a dialer that did the TLS handshake ourselves, and then pass the error back via some other channel.

If we don't use our own dialer for the code, then:

1. For some of our integration tests, we just will not be able assert anything about test what kind of error was returned, and we'll have to wait for a timeout instead of waiting for the test to fail immediately because GRPC will just retry on transient failure.

2. Actual behavior if a node cert expired, or there is some other error with the certificate - rather than the node failing on startup, it will keep trying to reconnect, and the user will have to look at the logs to diagnose what's wrong. We'd specifically be regressing on Joining a cluster with wrong certificate hides error moby#33380.

There is apparently a workaround in 1.11 (see grpc/grpc-go#1855 and grpc/grpc-go#1917) where if you make an RPC call instead of dialing with WithBlock(), you should generally get the correct error back. In our non-test code, we do not ever dial with WithBlock() so that seems fine, if we can upgrade to 1.11.

However, in 1.11, transport.StreamFromContext has been removed, which we depend on in our raftproxy (although it's mainly for nicer error messages) and for forcibly disconnecting a node in the dispatcher (but we can probably do that some other way).

Can we upgrade swarmkit to 1.11+ instead, or do we have to be on 1.10? If we want to stay on 1.10, then maybe we just live with the slight change in behavior, and I'll just skip checking those errors on tests I guess.

If we want to upgrade to 1.11+ (which we'd eventually need to do anyway) then I can work on trying to get around our use of transport.StreamFromContext and try to make sure our tests pass with their workaround.

Yes, in the long run, we will be updating to newer versions (and it's probably good to update more frequent so that we catch breaking changes early)

@dmcgowan - are there plans to update containerd to a more recent version, and if so, would that be a change that's acceptable for an 1.1.x patch release of containerd?

are there plans to update containerd to a more recent version, and if so, would that be a change that's acceptable for an 1.1.x patch release of containerd?

We would like to keep containerd as up to date with GRPC as possible. We don't need to backport to a release since our primary concern here is the client end, which does not need to be vendored to a release.

superseded by #2649