Add custom balancer to always remove subConns #15701

kisunji · 2022-12-06T23:38:50Z

Description

Remove the need to call UpdateState twice by changing the pick_first balancer's behavior to close existing connections (almost) every time.

Testing & Reproduction steps

No net-new behavior; existing test TestClientConnPool_IntegrationWithGRPCResolver_Rebalance is still passing.

Manually tested with frequent rebalancing

PR Checklist

updated test coverage
external facing docs updated
not a security concern

agent/grpc-internal/balancer/balancer.go

agent/grpc-internal/balancer/pickfirst.go

boxofrad

Nice work, @kisunji 🙌🏻

I'm hoping to have the 1.15+ balancer (PR in enterprise repository) ready to merge in the next couple of days. Should we merge yours into main and then have my PR remove it, or somehow just merge yours into the 1.13 and 1.14 release branches?

rboyer · 2022-12-09T17:00:00Z

agent/grpc-internal/balancer/pickfirst.go

+//
+// [1]: https://github.com/grpc/grpc-go/blob/v1.49.x/pickfirst.go
+
+/*


Is this just present here for reference, or is it executed?
If the former you could add a build tag to make it more obvious +build reference-only for example.

It's executed unfortunately

kisunji · 2022-12-09T17:03:09Z

Nice work, @kisunji 🙌🏻

I'm hoping to have the 1.15+ balancer (PR in enterprise repository) ready to merge in the next couple of days. Should we merge yours into main and then have my PR remove it, or somehow just merge yours into the 1.13 and 1.14 release branches?

Thanks! I think I should merge to main first and backport so we have traceability if someone happens to git log -S for it.

agent/grpc-internal/balancer/pickfirst.go

kisunji · 2022-12-09T21:33:22Z

agent/grpc-internal/balancer/pickfirst.go

+	}
+
+	if b.subConn != nil {
+		b.cc.UpdateAddresses(b.subConn, state.ResolverState.Addresses)


Context for reviewers: this line does not close a connection if the active subConn is in the list of state.ResolverState.Addresses. This is the behavior that resolver.go tried to work around by calling UpdateState with a single address to close the connection (unless it happened to be the active one) and again to re-add the rest of the addresses.

huikang · 2022-12-16T03:41:11Z

I am new to load balancing of gRPC and having two basic question

Given most of the customized pick_first policy is copied from grpc, do we have a plan to send the improvement to upstream gRPC?
I also noticed the WARN message contains a nil pointer (not sure where it happens)

Err: connection error: desc = "transport: Error while dialing dial tcp <nil>->192.168.1.101:8300: operation was canceled"

will the PR resolve this nil pointer?

kisunji · 2022-12-16T15:11:56Z

I am new to load balancing of gRPC and having two basic question

Given most of the customized pick_first policy is copied from grpc, do we have a plan to send the improvement to upstream gRPC?

I also noticed the WARN message contains a nil pointer (not sure where it happens)
Err: connection error: desc = "transport: Error while dialing dial tcp <nil>->192.168.1.101:8300: operation was canceled"
will the PR resolve this nil pointer?

It's not an improvement but a patch to get around gRPC behavior; I'm not sure if it's valuable upstream for others
Are you seeing that in this branch? I'm not sure how that error string is formatted but I assume <nil> means that there was no initial connection. Will leave this branch running overnight and see if I encounter it.

vercel · 2022-12-16T15:12:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
consul-ui-staging	🔄 Building (Inspect)		Dec 19, 2022 at 3:14PM (UTC)

huikang · 2022-12-16T15:20:30Z

Are you seeing that in this branch? I'm not sure how that error string is formatted but I assume means that there was no initial connection. Will leave this branch running overnight and see if I encounter it.

@kisunji , thanks for the explanation.

The error message containing was observed in the main. I will give this branch a try today.

kisunji · 2022-12-16T17:28:47Z

While testing I encountered rare WARN logs at startup and occasionally if I leave a cluster running overnight. After some debugging I raised a separate issue #15821 which I consider out of scope for now. This PR should eliminate the biggest source of WARN logs but not all.

david-yu · 2022-12-16T17:32:36Z

@kisunji Could we add labels to backport to 1.14, 1.13 and 1.12?

agent/grpc-internal/balancer/custombalancer.go

The new balancer is a patched version of gRPC's default pick_first balancer which removes the behavior of preserving the active subconnection if a list of new addresses contains the currently active address.

…e/1.12.x (#15834) * Add custom balancer to always remove subConns (#15701) The new balancer is a patched version of gRPC's default pick_first balancer which removes the behavior of preserving the active subconnection if a list of new addresses contains the currently active address.

…e/1.13.x (#15835) * Add custom balancer to always remove subConns (#15701) The new balancer is a patched version of gRPC's default pick_first balancer which removes the behavior of preserving the active subconnection if a list of new addresses contains the currently active address.

kisunji commented Dec 6, 2022

View reviewed changes

agent/grpc-internal/balancer/balancer.go Outdated Show resolved Hide resolved

kisunji force-pushed the kisunji/patched-pick-first-balancer branch 4 times, most recently from a79dbc3 to 4b43407 Compare December 7, 2022 17:28

kisunji commented Dec 7, 2022

View reviewed changes

agent/grpc-internal/balancer/pickfirst.go Outdated Show resolved Hide resolved

kisunji marked this pull request as ready for review December 7, 2022 17:50

kisunji requested a review from boxofrad December 7, 2022 17:51

kisunji force-pushed the kisunji/patched-pick-first-balancer branch from cc1a028 to eed6393 Compare December 7, 2022 19:52

boxofrad approved these changes Dec 9, 2022

View reviewed changes

kisunji force-pushed the kisunji/patched-pick-first-balancer branch from b5f20f6 to 0c4d7f5 Compare December 9, 2022 16:58

rboyer reviewed Dec 9, 2022

View reviewed changes

kisunji requested a review from banks December 9, 2022 17:17

rboyer reviewed Dec 9, 2022

View reviewed changes

agent/grpc-internal/balancer/pickfirst.go Outdated Show resolved Hide resolved

kisunji requested a review from mkeeler December 9, 2022 21:20

kisunji commented Dec 9, 2022

View reviewed changes

kisunji force-pushed the kisunji/patched-pick-first-balancer branch from 03a0aa5 to c2b35e7 Compare December 16, 2022 15:12

kisunji added backport/1.12 labels Dec 16, 2022

kisunji force-pushed the kisunji/patched-pick-first-balancer branch from c2b35e7 to aa2d5fd Compare December 19, 2022 15:14

kisunji enabled auto-merge (squash) December 19, 2022 15:18

Add custom balancer to always remove subConns

c37343d

Chris S. Kim added 7 commits December 19, 2022 11:59

Refactor to embed pickfirstBalancer

27a0ffb

Cleanup updateAddrsLocked

ee5841d

Update license comments

35f7288

Add comments

e8be8e4

Remove duplicate init

44d5c20

Rename balancer to prevent overlap

5420edd

Add changelog

ec1b87a

kisunji force-pushed the kisunji/patched-pick-first-balancer branch from aa2d5fd to ec1b87a Compare December 19, 2022 16:59

kisunji merged commit 831680d into main Dec 19, 2022

kisunji deleted the kisunji/patched-pick-first-balancer branch December 19, 2022 17:39

kisunji mentioned this pull request Dec 19, 2022

New WARN in 1.10.0 caused by shuffling the servers in the gRPC ClientConn pool #10603

Closed

kisunji commented Dec 19, 2022

View reviewed changes

agent/grpc-internal/balancer/custombalancer.go Show resolved Hide resolved

kisunji mentioned this pull request Dec 19, 2022

grpc custom balancer - UpdateClientConnState Break - instead of returning early #15844

Merged

hc-github-team-consul-core mentioned this pull request Jan 18, 2023

Backport of grpc custom balancer - UpdateClientConnState Break - instead of returning early into release/1.14.x #16002

Merged

kisunji mentioned this pull request Jun 22, 2023

Investigation: grpc WARN logs "addrConn.createTransport failed to connect to..." #17842

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom balancer to always remove subConns #15701

Add custom balancer to always remove subConns #15701

kisunji commented Dec 6, 2022 •

edited

Loading

boxofrad left a comment

rboyer Dec 9, 2022

kisunji Dec 9, 2022

kisunji commented Dec 9, 2022

kisunji Dec 9, 2022

huikang commented Dec 16, 2022

kisunji commented Dec 16, 2022

vercel bot commented Dec 16, 2022 •

edited

Loading

huikang commented Dec 16, 2022

kisunji commented Dec 16, 2022

david-yu commented Dec 16, 2022

Add custom balancer to always remove subConns #15701

Add custom balancer to always remove subConns #15701

Conversation

kisunji commented Dec 6, 2022 • edited Loading

Description

Testing & Reproduction steps

PR Checklist

boxofrad left a comment

Choose a reason for hiding this comment

rboyer Dec 9, 2022

Choose a reason for hiding this comment

kisunji Dec 9, 2022

Choose a reason for hiding this comment

kisunji commented Dec 9, 2022

kisunji Dec 9, 2022

Choose a reason for hiding this comment

huikang commented Dec 16, 2022

kisunji commented Dec 16, 2022

vercel bot commented Dec 16, 2022 • edited Loading

huikang commented Dec 16, 2022

kisunji commented Dec 16, 2022

david-yu commented Dec 16, 2022

kisunji commented Dec 6, 2022 •

edited

Loading

vercel bot commented Dec 16, 2022 •

edited

Loading