add firstready balancer #349

demmer · 2024-05-16T04:25:00Z

Description

Implement a "first_ready" load balancer and a config to select it.

Despite how simple the code actually is, this balancer (I think!) does what (I think!) we want it to do (basically!):

When the discovery returns N targets, the base code in the balancer will attempt to establish a SubConn to each of the targets, and will maintain that subconn with keepalives and retries.
However when picking one of these for a given request, it always returns the first "ready" subconn.

This means that in practice, in steady state a webapp host will have one "hot" connection to a single vtgate, and will route all queries to it. At the same time it will maintain N-1 idle connections to other vtgates, and if the connection to the primary vtgate goes away or is interrupted, queries will get routed to one of the other ones as a backup.

AZ Affinity: Just like the case of round_robin if there aren't N hosts in the same AZ, then some amount of cross-AZ queries are inevitable. When using first_ready it can be especially bad since if the "first" ready subconn is in a different AZ then webapp will be stuck crossing AZs until discovery runs again.

In practice I think this isn't actually a problem since we should always have at least N vtgates in each AZ.

Testing

This seems to do what it's supposed to with some basic testing, including using iptables rules to temporarily black hole network traffic to the "selected" gate. After the keepalive timeout, the connection was marked as TRANSIENT_FAILURE and the query was routed to a different gate, so the app saw a slowdown but not a failure.

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

demmer · 2024-05-16T04:45:51Z

go/vt/vtgateproxy/firstready_balancer.go

+
+	var subConn balancer.SubConn
+	for sc := range info.ReadySCs {
+		subConn = sc


Note that we might want to actually pick randomly from the set of ready subconns rather than just taking the first one in the map.

I'm frankly not sure it matters in practice since the order should be basically nondeterministic anyway, and this is downstream from the already randomized shuffle from discovery, but in case it's possible that something like the order in which vtgates are started up somehow influences this, then maybe there's extra value in doing so?

I think we want this to be deterministic in the input - we assume that Build will be called only when there is a change in the ready list, but maybe there are circumstances where nothing has changed but it gets re-invoked. In those situations we don't want to jump around vtgates. (This is different from round_robin which explicitly does want to choose a different starting point each time, and has code to do that).

Thinking further - we only want to pick a new gate if the currently picked one is no longer ready. So the Nth subconn changing state isn't something we should act on. We should pick the same subconn if it's still in the ready list, which is tricky because we don't know what we last picked.

Without this, any vtgate subconn state change causes us to pick a new gate. We'd need some stable, but host-specific, way of deterministically choosing the same gate from a list (like sorting by hash of <conn-id>-<hostname>.

If deterministic order matters, we can't rely on the ordering of this map, since map ordering is not guaranteed (in past versions of go it was intentionally shuffled). We would need to take the output of this map and sort it deterministically. If we don't care about order, then this is fine, but we'll have to be aware of the fact that the target may or may not change during every discovery.

Great points! -- I pushed a rework where we actually simplify the model so that the same struct implements both the Builder and the Picker interfaces (golang is really nice sometimes!). This way we will stick to the current conn as long as it's READY.

jscheinblum · 2024-05-16T13:57:57Z

go/vt/vtgateproxy/firstready_balancer.go

+type frPickerBuilder struct{}
+
+func (*frPickerBuilder) Build(info base.PickerBuildInfo) balancer.Picker {
+	logger.Infof("firstreadyPicker: Build called with info: %v", info)


worth normalizing on names, we have: first_ready, firstready, firstreadyPicker all in labels in this file

good point. done.

henryr · 2024-05-16T15:02:38Z

go/vt/vtgateproxy/firstready_balancer.go

+	var subConn balancer.SubConn
+	for sc := range info.ReadySCs {
+		subConn = sc


am I missing something clever by not just returning info.ReadSCs[0]?

It's a map, the values aren't indexible or sorted deterministically

henryr · 2024-05-16T15:03:37Z

go/vt/vtgateproxy/vtgateproxy.go

 	grpcclient.RegisterGRPCDialOptions(func(opts []grpc.DialOption) ([]grpc.DialOption, error) {
-		return append(opts, grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`)), nil
+		log.Infof("registering %s load balancer", *balancerType)
+		return append(opts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, *balancerType))), nil


we should sanitize the input here - I'm not sure what the behaviour of gRPC is if you ask it for a balancer it doesn't know about.

good point. done.

henryr · 2024-05-16T15:05:56Z

go/vt/vtgateproxy/firstready_balancer.go

+
+	var subConn balancer.SubConn
+	for sc := range info.ReadySCs {
+		subConn = sc


I think we want this to be deterministic in the input - we assume that Build will be called only when there is a change in the ready list, but maybe there are circumstances where nothing has changed but it gets re-invoked. In those situations we don't want to jump around vtgates. (This is different from round_robin which explicitly does want to choose a different starting point each time, and has code to do that).

henryr · 2024-05-16T15:16:46Z

go/vt/vtgateproxy/firstready_balancer.go

+	// created. The slice is immutable. Each Get() will do a round robin
+	// selection from it and return the selected SubConn.


round robin selection - The C&P is showing 😆

henryr · 2024-05-16T15:23:06Z

go/vt/vtgateproxy/firstready_balancer.go

+
+	var subConn balancer.SubConn
+	for sc := range info.ReadySCs {
+		subConn = sc


Thinking further - we only want to pick a new gate if the currently picked one is no longer ready. So the Nth subconn changing state isn't something we should act on. We should pick the same subconn if it's still in the ready list, which is tricky because we don't know what we last picked.

Without this, any vtgate subconn state change causes us to pick a new gate. We'd need some stable, but host-specific, way of deterministically choosing the same gate from a list (like sorting by hash of <conn-id>-<hostname>.

rjlaine · 2024-05-20T21:46:49Z

go/vt/vtgateproxy/firstready_balancer.go

+)
+
+// Name is the name of first_ready balancer.
+const Name = "first_ready"


this will become a public package level constant, so vtgateproxy.Name will become "first_ready" which might be confusing. (PascalCase names are automatically public in go)

rjlaine · 2024-05-20T21:51:21Z

go/vt/vtgateproxy/firstready_balancer.go

+// Name is the name of first_ready balancer.
+const Name = "first_ready"
+
+var logger = grpclog.Component("firstready")


This will also become a package level variable, although at least private. But any file that references logger in the vtgateproxy package will reference this instance.

good point - removed

rjlaine

Nice, this all looks good to me. Let's give it a spin!

* add firstready balancer * fail if an invalid balancer is picked * maintain the currentConn even if another one becomes ready * remove unnecessary globals * normalize the names

demmer commented May 16, 2024

View reviewed changes

jscheinblum reviewed May 16, 2024

View reviewed changes

henryr reviewed May 16, 2024

View reviewed changes

rjlaine reviewed May 20, 2024

View reviewed changes

demmer added 3 commits May 21, 2024 09:15

add firstready balancer

4f0cbcc

fail if an invalid balancer is picked

878bb5e

maintain the currentConn even if another one becomes ready

60dc5df

demmer force-pushed the vtgateproxy-firstready-balancer branch from ee7e879 to 60dc5df Compare May 21, 2024 16:35

demmer added 2 commits May 21, 2024 09:36

remove unnecessary globals

94197b5

normalize the names

23393aa

rjlaine approved these changes May 21, 2024

View reviewed changes

demmer merged commit 91cc200 into vtgateproxy May 21, 2024
151 of 240 checks passed

demmer deleted the vtgateproxy-firstready-balancer branch May 21, 2024 16:54

dedelala pushed a commit that referenced this pull request Jul 30, 2024

add firstready balancer (#349)

54cbaed

* add firstready balancer * fail if an invalid balancer is picked * maintain the currentConn even if another one becomes ready * remove unnecessary globals * normalize the names

dedelala pushed a commit that referenced this pull request Nov 12, 2024

add firstready balancer (#349)

ad571f0

* add firstready balancer * fail if an invalid balancer is picked * maintain the currentConn even if another one becomes ready * remove unnecessary globals * normalize the names

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add firstready balancer #349

add firstready balancer #349

demmer commented May 16, 2024 •

edited

Loading

demmer May 16, 2024

henryr May 16, 2024

henryr May 16, 2024

rjlaine May 16, 2024 •

edited

Loading

demmer May 21, 2024

jscheinblum May 16, 2024

demmer May 21, 2024

henryr May 16, 2024

rjlaine May 16, 2024

henryr May 16, 2024

demmer May 21, 2024

henryr May 16, 2024

henryr May 16, 2024

demmer May 21, 2024

henryr May 16, 2024

rjlaine May 20, 2024 •

edited

Loading

demmer May 21, 2024

rjlaine May 20, 2024

demmer May 21, 2024

rjlaine left a comment

		// created. The slice is immutable. Each Get() will do a round robin
		// selection from it and return the selected SubConn.

add firstready balancer #349

add firstready balancer #349

Conversation

demmer commented May 16, 2024 • edited Loading

Description

Testing

Related Issue(s)

Checklist

Deployment Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjlaine May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjlaine May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjlaine left a comment

Choose a reason for hiding this comment

demmer commented May 16, 2024 •

edited

Loading

rjlaine May 16, 2024 •

edited

Loading

rjlaine May 20, 2024 •

edited

Loading