roundrobin: strip attributes from addresses #4024

menghanl · 2020-11-10T19:01:07Z

attributes is stripped from addresses before they are used as map keys.
SubConns are still created with the original addresses (with attributes).

dfawley · 2020-11-10T21:18:00Z

balancer/base/balancer.go

+		aNoAttrs := a
+		aNoAttrs.Attributes = nil
+		addrsSet[aNoAttrs] = struct{}{}
+		if _, ok := b.subConns[aNoAttrs]; !ok {


} else { sc.UpdateAddresses(a) }

In case the attributes change in a meaningful way (to the transport/grpc).

dfawley · 2020-11-10T22:47:32Z

balancer/base/balancer.go

+			// changed. This is a noop if the current address is the same as the
+			// old one (reflect.DeepEqual).


What does this mean? It shouldn't be a "true" nop. It should make it so these attributes are used if we ever need to reconnect.

Of course, we don't actually have any attributes that matter right now, besides the network type, which isn't something that it makes sense to change over time for the same address.

Maybe we should update the grpclb usage of Metadata for the server name to go through attributes instead so we can test this?

If reflect.DeepEqual returns true, sc.UpdateAddresses would immediately return.
reflect.DeepEqual also compares the attributes, so this early return only happens when the new addresses is exactly the same as old.

This change has nothing to do with grpclb. And we will need changes in ClientConn to handle attributes.
That includes making the attributes hashable (or comparable?), and distinguish between the attributes that matter (for connection, e.g. creds), and those that only for balancers to use (e.g. hierarchy)

reflect.DeepEqual also compares the attributes

Oh, I see what you are saying here. My mistake.

This change has nothing to do with grpclb.

No, but that's an example of a potential attribute that could affect the transport, besides network type. Changing it would allow us to test the change you just made. Actually, we could also implement a dummy handshaker to read the attributes and verify they were updated when a reconnect happens.

What I was thinking was:

Push an address from the resolver with no attributes

Connect

Push the same address from the resolver but with new attributes

Force disconnect/reconnect

Verify new attributes are in use

Added a test

dfawley · 2020-11-10T22:49:49Z

balancer/base/balancer.go

+			// changed. This is a noop if the current address is the same as the
+			// old one (reflect.DeepEqual).
+			//
+			// TODO: delete this when this balancer reads attributes.


This is what Java does now. It's unclear if we should be trying to be more aggressive about forcing a reconnect to pick up the new attributes if we know they changed.

When we know how to hash attributes, and know which part of it matters for connections, it will be easier to decide a UpdateAddresses is needed. And the SubConn would decide if it will reconnect to pick up the new attributes.

will be easier to decide a UpdateAddresses is needed

Right -- my point is we will still need to call UpdateAddresses in that case, so we won't be deleting this.

I would still say we should delete the TODO. We will update the addresses when the attributes change in a meaningful way.

balancer/roundrobin/roundrobin_test.go

dfawley · 2020-11-19T21:50:06Z

balancer/roundrobin/roundrobin_test.go

+		if len(md1) != 0 {
+			t.Fatalf("got md: %v, want empty metadata", md1)
+		}
+	case <-time.After(time.Microsecond * 100):


It's unclear which case is desired. Is this expected? If so we should Fatal always above, right?

Made the above always fatal.

balancer/roundrobin/roundrobin_test.go

…esses `attributes` is stripped from addresses before they are used as map keys. SubConns are still created with the original addresses (with attributes).

lemonlinger · 2021-02-28T12:29:33Z

Sad.... we implemented a custom resolver and picker to support load balancing by weight saved in resolver.Address.Attributes. When the picker was generated, it read weights from SubConnInfo.Address whose Attributes were stripped. Now our picker doesn't work properly.

dfawley · 2021-03-01T16:36:01Z

@lemonlinger you may want to fork the base balancer at a version that was working for you. This is a problem with the way the base balancer is designed IMO. #3425

joeblubaugh · 2021-05-24T22:34:37Z

balancer/base/balancer.go

+		// Note that this doesn't handle the case where the attribute content is
+		// different. So if users want to set different attributes to create
+		// duplicate connections to the same backend, it doesn't work. This is
+		// fine for now, because duplicate is done by setting Metadata today.


Hello @menghanl , we were using attributes to create duplicate connections to our backend because Metadata is deprecated. Is the current advice to use this deprecated API in order to create duplicate backend connections?

Yes. Please use the deprecated Metadata field (it should be pretty stable, there's no plan to delete it). Sorry for the trouble.

We're looking forward to when the attributes can be used for this again. Thanks for the reply!

menghanl requested a review from dfawley November 10, 2020 19:01

menghanl assigned dfawley Nov 10, 2020

dfawley reviewed Nov 10, 2020

View reviewed changes

menghanl added no release notes Type: Behavior Change Behavior changes not categorized as bugs and removed no release notes labels Nov 10, 2020

menghanl added this to the 1.34 Release milestone Nov 10, 2020

menghanl force-pushed the roundrobin_strips_attributes branch from 10703ab to e0b3d58 Compare November 10, 2020 22:31

dfawley reviewed Nov 10, 2020

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Nov 12, 2020

menghanl modified the milestones: 1.34 Release, 1.35 Release Nov 17, 2020

menghanl force-pushed the roundrobin_strips_attributes branch from ea68d72 to 3d81524 Compare November 19, 2020 01:16

menghanl assigned dfawley and unassigned menghanl Nov 19, 2020

menghanl force-pushed the roundrobin_strips_attributes branch 3 times, most recently from 8227351 to 469a7d0 Compare November 19, 2020 01:26

dfawley reviewed Nov 19, 2020

View reviewed changes

dfawley assigned menghanl and unassigned dfawley Nov 19, 2020

menghanl assigned dfawley and unassigned menghanl Nov 20, 2020

dfawley approved these changes Nov 20, 2020

View reviewed changes

balancer/roundrobin/roundrobin_test.go Show resolved Hide resolved

dfawley assigned menghanl and unassigned dfawley Nov 20, 2020

menghanl added 2 commits November 30, 2020 13:52

[roundrobin_strips_attributes] roundrobin: strip attributes from addr…

6e1895a

…esses `attributes` is stripped from addresses before they are used as map keys. SubConns are still created with the original addresses (with attributes).

[roundrobin_strips_attributes] update address

2691eeb

menghanl added 4 commits November 30, 2020 13:52

[roundrobin_strips_attributes] not not implemented

a31c0d6

[roundrobin_strips_attributes] c0

cd52612

[roundrobin_strips_attributes] roundrobin test

4e295c1

[roundrobin_strips_attributes] always fatal

b67914f

menghanl force-pushed the roundrobin_strips_attributes branch from 4268a4c to b67914f Compare November 30, 2020 21:53

menghanl merged commit 4a0125a into grpc:master Nov 30, 2020

menghanl deleted the roundrobin_strips_attributes branch November 30, 2020 22:20

davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020

roundrobin: strip attributes from addresses (grpc#4024)

f3fa549

This was referenced Mar 8, 2021

Bump google.golang.org/grpc from 1.34.1 to 1.36.0 in /processor/routingprocessor jmacd/opentelemetry-collector-contrib#699

Open

Bump google.golang.org/grpc from 1.34.1 to 1.36.0 in /exporter/stackdriverexporter jmacd/opentelemetry-collector-contrib#707

Open

This was referenced Mar 9, 2021

balancer: Fix base balancer return nil attributes #4252

Closed

balancer/base: keep address attributes for pickers #4253

Merged

This was referenced Mar 12, 2021

Bump google.golang.org/grpc from 1.32.0 to 1.36.0 kisshang1993/v2ray-core#8

Closed

build(deps): bump google.golang.org/grpc from 1.26.0 to 1.36.0 concourse/dex#13

Closed

MooooonStar mentioned this pull request Mar 18, 2021

The attributes of the address is removed when passing to the picker. #4276

Closed

joeblubaugh reviewed May 24, 2021

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roundrobin: strip attributes from addresses #4024

roundrobin: strip attributes from addresses #4024

menghanl commented Nov 10, 2020

dfawley Nov 10, 2020

menghanl Nov 10, 2020

dfawley Nov 10, 2020

menghanl Nov 11, 2020

dfawley Nov 11, 2020

menghanl Nov 19, 2020

dfawley Nov 10, 2020

menghanl Nov 11, 2020

dfawley Nov 11, 2020

dfawley Nov 12, 2020

menghanl Nov 19, 2020

dfawley Nov 19, 2020

menghanl Nov 20, 2020

lemonlinger commented Feb 28, 2021 •

edited

Loading

dfawley commented Mar 1, 2021

joeblubaugh May 24, 2021

menghanl May 24, 2021

joeblubaugh May 25, 2021

		// changed. This is a noop if the current address is the same as the
		// old one (reflect.DeepEqual).

roundrobin: strip attributes from addresses #4024

roundrobin: strip attributes from addresses #4024

Conversation

menghanl commented Nov 10, 2020

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

Choose a reason for hiding this comment

lemonlinger commented Feb 28, 2021 • edited Loading

dfawley commented Mar 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemonlinger commented Feb 28, 2021 •

edited

Loading