Add grpc_resolver using external discovery service

[guanw]

Signed-off-by: Jude Wang judew@uber.com

Which problem is this PR solving?

• Allow custom load balancing for gRPC [agent]  #1497

Short description of the changes

• Add grpc resolver which listens to available host/port from discovery.notifier and updates list of collector servers for agent to connect.

[yurishkuro]

partial review, but as I'm in meetings for the next N hours, wanted to give preliminary feedback

[guanw]

saw this error twice with travis

Local build "make test-ci" works fine. Is there a way to configure memory limit on travis?

[yurishkuro]

btw, why do you need to change Gopkg.lock?

[guanw]

btw, why do you need to change Gopkg.lock?

hmmm. There seems to be discrepency error in travis yesterday but now that I checked again it was gone somehow. Will revert it back.

[codecov]

Codecov Report

Merging #1498 into master will decrease coverage by <.01%.
The diff coverage is 98.86%.

@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
- Coverage   98.79%   98.79%   -0.01%     
==========================================
  Files         189      190       +1     
  Lines        8986     9058      +72     
==========================================
+ Hits         8878     8949      +71     
- Misses         84       85       +1     
  Partials       24       24

Impacted Files	Coverage Δ
cmd/agent/app/reporter/grpc/flags.go	100% <100%> (ø)	⬆️
cmd/agent/app/reporter/grpc/builder.go	100% <100%> (ø)	⬆️
pkg/discovery/grpcresolver/grpc_resolver.go	98.57% <98.57%> (ø)

---

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 2aacfaf...0f8b179. Read the comment docs.

[guanw]

Regarding the waitGroup. I think it makes sure we exit watcher goroutine before we exit the main program, so generally a good cleanup order? Otherwise the the main program could potentially exit before watcher finishes.

[guanw]

@yurishkuro Any further comments?

[yurishkuro]

this should be called with backendCount, not connections. Or even better - it should be outside of the loop, since you're not restarting servers (which is why I suggested making it part of startTestServers, since the expectation of that function is that "servers are ready"). And you don't need to use testc with the load balancer for this health check.

[guanw]

@yurishkuro I'm not clear why we don't need to use testc for the health check. If we don't do a initial load balancing and make sure every server has been reached at least once, what's the point of having makeSureConnectionsUp function?

[yurishkuro]

I'm not clear why we don't need to use testc for the health check.

we do need to use a connection, but it can be a different connection constructed without load balancer, since you are giving it explicit peer.

[guanw]

I'm not clear why we don't need to use testc for the health check.

we do need to use a connection, but it can be a different connection constructed without load balancer, since you are giving it explicit peer.

Are you referring to testc.EmptyCall? I think we are populating peer instead of "giving". The peers will only be decided by resolver and minPeers we passed to it.

[guanw]

I'm not clear why we don't need to use testc for the health check.

we do need to use a connection, but it can be a different connection constructed without load balancer, since you are giving it explicit peer.

Are you referring to testc.EmptyCall? I think we are populating peer instead of "giving". The peers will only be decided by resolver and minPeers we passed to it.

@yurishkuro Any comment on my last reply?

[yurishkuro]

testc is created with your custom load balancer. A plain connection without load balancer is sufficient for server liveness checks. Connection with LB is only needed in the actual unit test.

[guanw]

testc is created with your custom load balancer. A plain connection without load balancer is sufficient for server liveness checks. Connection with LB is only needed in the actual unit test.

I'm not certain that's going to work. I tested the resolver without roundrobin balancer(i.e without grpc.WithBalancerName(roundrobin.Name)) the testc will just call the same peer everytime(confirm by populating). A plain connection without load balancer is sufficient to check if server is live, but without every peer being called at least once, calling roundrobin at second step could still miss some peers(tested and I could see for example {minPeers 5, connections 5} it could end up calling 4 out of 5 peers instead of 5 out of 5).

Also taking a look at TestBackendsRoundRobin in https://github.com/grpc/grpc-go/blob/master/balancer/roundrobin/roundrobin_test.go, I still think building connection for all server peers is necessary everytime we build a new grpc client connection.

[yurishkuro]

You have two rounds of network calls:

• in the first, you ping each server directly (by giving the connection a Peer) to check it's up. You don't need load balancing on the connection in this case. Therefore, this whole logic can be encapsulated in the startServers() function
• in the second round you're actually unit testing the LB, so you start a new connection that uses the LB and run unit test with it

[guanw]

You have two rounds of network calls:

• in the first, you ping each server directly (by giving the connection a Peer) to check it's up. You don't need load balancing on the connection in this case. Therefore, this whole logic can be encapsulated in the startServers() function
• in the second round you're actually unit testing the LB, so you start a new connection that uses the LB and run unit test with it

But still, load balancing at step 2 does not necessarily work even if the first step works. Cause now they are from two different connections? I tested the idea here but it some times still hits only 4 out of 5.

[yurishkuro]

Step 1 has nothing to do with step 2, it's preparation of servers and a check that they are alive. If step 2 fails after that (using a different connection) then it's either a bug in the LB or in the test, and makes it even more sense to use different connection vs. step 1.

[guanw]

Step 1 has nothing to do with step 2, it's preparation of servers and a check that they are alive. If step 2 fails after that (using a different connection) then it's either a bug in the LB or in the test, and makes it even more sense to use different connection vs. step 1.

I tested against the original grpc-go roundrobin test they make by swapping the "making connection up" part with directly calling peers, it would fail. I posted the question on their github page and will continue on this once they replied.

[yurishkuro]

can you link that issue here?

[guanw]

grpc/grpc-go#2808

[yurishkuro]

Ah, the explanation in that ticket makes sense - if you create a new connection for the LB test, you'd still need to ensure that it is connected to each peer before you can execute the test. We should add this to the comments in the test.

[guanw]

sg. Will add some comment in.