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

*: configure server keepalive, optimize client balancer with health check #8477

Closed
wants to merge 8 commits into from

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Aug 31, 2017

  1. Configure server gRPC keepalive parameters
  2. Optimize client endpoint switch by gray-listing transient-failed nodes

keepalive timed-out is 'connectivity.TransientFailure'
in gRPC; it keeps retrying (calling 'Balancer.Up') until
success. This is problematic in multi-endpoint balancer
with an endpoint being blackholed. Balancer can get stuck
retrying blackholed endpoint, taking several seconds to
find healthy ones.

Also gray-listing(#8463) endpoints doesn't work in following case:

# TestKVGetResetLoneEndpoint

CODE / CURRENT PINNED ADDRESS
01. clientv3.New(ep1, ep2) / NONE
02. notifyAddrs(ep1, ep2) / NONE
03. grpc.lbWatcher receives ep1, ep2 from Notify() / NONE
04. grpc.lbWatcher ADD calls resetAddrConn ep1 / NONE
05. grpc.lbWatcher ADD calls resetAddrConn ep2 / NONE

06. grpc calls resetTransport Up(ep1) / NONE
07. clientv3.Balancer Up pins ep1 / ep1

08. grpc calls resetTransport Up(ep2) / ep1
09. clientv3.Balancer Up DOES NOT pin ep2 / ep1

10. updateNotifyLoop sends b.notifyCh <- pinned ep1 / ep1

11. Stop(ep1) / ep1

12. grpc.lbWatcher receives ep1 from Notify() / ep1
13. grpc.lbWatcher DEL calls tearDown(errConnDrain) ep2 / ep1

14. Stop(ep2) / ep1

15. clientv3.Balancer Up down network I/O error on ep1 / NONE
16. Gray-list ep1

# ep1 is gray-listed, so only notify ep2
# but ep2 is also stopped
# this makes balancer stuck with ep2
17. notifyAddrs(ep2) / NONE
18. notifyAddrs(ep2) / NONE
19. grpc.lbWatcher receives ep2 from Notify() / NONE
20. grpc.lbWatcher ADD calls resetAddrConn ep2 / NONE
21. grpc.lbWatcher DEL calls tearDown(errConnDrain) ep1 / NONE
22. grpc.lbWatcher receives ep2 from Notify() / NONE

23. Restart(ep1) / NONE

24. Get(ep1,ep2) timed out / NONE

At step 17 above, we should instead notify both ep1 and ep2.
If we exclude gray-listed ep1, balancer gets stuck with stopped endpoint ep2.

Problem is:

  1. ep2 was never pinned
  2. ep2 is also stopped
  3. So gRPC fails to connect to ep2 with error "transport: Error while dialing dial unix ep2: connect: no such file or directory"
  4. There's no way to check if ep2 is down.

This PR adds additional health-check API call to discover endpoint status on endpoint notify.

@gyuho gyuho added the WIP label Aug 31, 2017
@gyuho gyuho force-pushed the keepalive-2 branch 4 times, most recently from bd621e1 to c55d608 Compare August 31, 2017 22:46
@heyitsanthony
Copy link
Contributor

If we exclude gray-listed ep1, balancer gets stuck with stopped endpoint ep2.

Is this with posting ep1 to notifyCh once the gray list deadline times out?

@gyuho
Copy link
Contributor Author

gyuho commented Sep 1, 2017

If we exclude gray-listed ep1, balancer gets stuck with stopped endpoint ep2.

Is this with posting ep1 to notifyCh once the gray list deadline times out?

That happens when we only post ep2 (ep1 is gray-listed), which is step 17.
To post ep1 again (after ep1 gray-list time-out), ep2 has to be pinned first, and then gets network I/O error, so it can trigger notifyAddrs in #8463. But this whole process takes more than 5-seconds, so Get request in TestKVGetResetLoneEndpoint times out even before ep2 gets pinned--easy to repro in slow CPU machine.

@gyuho gyuho changed the title [WIP] *: configure server keepalive, optimize client balancer [WIP] *: configure server keepalive, optimize client balancer with health check Sep 1, 2017
@gyuho gyuho force-pushed the keepalive-2 branch 5 times, most recently from 03b521f to 3fff472 Compare September 6, 2017 17:56
@gyuho gyuho changed the title [WIP] *: configure server keepalive, optimize client balancer with health check *: configure server keepalive, optimize client balancer with health check Sep 6, 2017
@gyuho gyuho removed the WIP label Sep 6, 2017
@gyuho gyuho force-pushed the keepalive-2 branch 2 times, most recently from 9a51d46 to 7a27a45 Compare September 8, 2017 16:32
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Signed-off-by: Gyu-Ho Lee <gyuhox@gmail.com>
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

balancer will have to be a separate patch before keepalive can be merged in; I'm trying to refactor this into something that can cleanly work with partition failover as well. simpleBalancer is getting too complicated to reason about

if len(addrs) == 0 { // no better alternative found
addrs = b.addrs
} else { // sort that latest failed be at the end
addrConns := make([]addrConn, 0, len(b.failed))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does grpc make any guarantees about the ordering? my understanding is it can try all the connections at once

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there's no ordering and gRPC tries all at once. Still, goroutine starts in order.
Anyway, this wasn't necessary.

@gyuho
Copy link
Contributor Author

gyuho commented Sep 9, 2017

@heyitsanthony Agree. This is getting too complicated. I will separate out server options, first.

@gyuho gyuho added the WIP label Sep 9, 2017
@fanminshi
Copy link
Member

there is something call pickFirstBalancer from grpc-go client. Has anyone took a deep look on what it does?

// pickFirst is used to test multi-addresses in one addrConn in which all addresses share the same addrConn.
// It is a wrapper around roundRobin balancer. The logic of all methods works fine because balancer.Get()
// returns the only address Up by resetTransport().
type pickFirst struct {
	*roundRobin
}

func pickFirstBalancer(r naming.Resolver) Balancer {
	return &pickFirst{&roundRobin{r: r}}
}

https://github.com/grpc/grpc-go/blob/master/balancer.go#L399-L408

@gyuho
Copy link
Contributor Author

gyuho commented Sep 13, 2017

@fanminshi pickFirstBalancer assumes the default grpc-go roundrobin balancer, so we can't use it. Maybe in the new balancer implementation grpc/grpc-go#1506.

@gyuho gyuho closed this Sep 18, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Sep 20, 2017

Update: closed in favor of #8545.

@gyuho gyuho deleted the keepalive-2 branch October 5, 2017 17:00
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