ringhash: handle config updates properly #5557

easwars · 2022-08-02T19:31:21Z

Fix a few things in the config handling routine of the ringhash LB policy:

config changes in the ring size should result in a new picker with a new ring
config changes should result in the internal copy of the config being updated

RELEASE NOTES:

ringhash: fix config update processing to recreate ring and picker when min/max ring size changes

zasweq · 2022-08-02T20:42:15Z

xds/internal/balancer/ringhash/ringhash_test.go

+// picker is pushed on the channel.
+func (s) TestUpdateClientConnState_NewRingSize(t *testing.T) {
+	addrs := []resolver.Address{{Addr: testBackendAddrStrs[0]}}
+	cc, b, p0 := setupTest(t, addrs)


Nit: I know it's p0 elsewhere, but for some reason p1 and p2 make more sense in my mind than p0 and p1.

zasweq · 2022-08-02T20:52:11Z

xds/internal/balancer/ringhash/ringhash.go

+
+	// If the ring configuration has changed, we need to regenerate the ring and
+	// send a new picker.
+	var regenerateRing bool


In correspondence to your comment on my Outlier Detection PR:
regenerateRing := b.updateAddresses(s.ResolverState.Addresses)
if b.config == nil || b.config.MinRingSize != newConfig.MinRingSize || b.config.MaxRingSize != newConfig.MaxRingSize {
regenerateRing = true
}
b.config = newConfig

Done. Thanks.

zasweq · 2022-08-02T21:02:45Z

xds/internal/balancer/ringhash/ringhash.go

+	}
+
+	if regenerateRing {
+		ring, err := newRing(b.subConns, b.config.MinRingSize, b.config.MaxRingSize)


This changes logic, and I don't know if this is correct. I still think it needs to write to b.ring. Otherwise it would persist a built ring map from the previous unrelated good update? Then subsequent ResolverError calls (line 318) and UpdateSubConnState() calls (line 369) call into regeneratePicker which reads b.ring, which would not be in the correct state and "synchronized" with the UpdateClientConnState that caused the SubConn error from the address list. Please triage.

Earlier, when we were assigning to b.ring here and if err != nil, we were still calling ResolverError, but not sending a new picker. So, we would have still ended up using the old ring (from the old picker) to service RPCs. So, in that sense, i feel that the behavior has not changed. But either ways, I'm not sure if our existing behavior was right. So, I have posted on our chat room, let's see what comes out of it. Thanks.

Ok, based on the discussion in the chat room, I've change our ring creation routine newRing() to not return an error.

zasweq · 2022-08-02T21:04:56Z

xds/internal/balancer/ringhash/ringhash.go

-		return balancer.ErrBadResolverState
-	}
+	// Successful resolution; clear resolver error and return nil.
+	b.resolverErr = nil


Again, I don't know if this breaks it, but this doesn't clear it if hits line 295, whereas in master it did. Although it feels appropriate here, but please triage if this breaks anything.

This one looks totally fine to me.

zasweq · 2022-08-02T21:07:33Z

xds/internal/balancer/ringhash/ringhash_test.go

+// TestUpdateClientConnState_NewRingSize tests the scenario where the ringhash
+// LB policy receives new configuration which specifies new values for the ring
+// min and max sizes. The test verifies that a new ring is created and a new
+// picker is pushed on the channel.


Nit: we're not testing whether the picker is pushed on "the channel". We're testing whether the new picker gets forwarded to this balancer's client conn, which in this case sends on the channel for verification purposes. Would prefer rewording "is pushed on the channel" to "sent to the ClientConn".

zasweq

LGTM.

zasweq · 2022-08-03T18:33:46Z

xds/internal/balancer/ringhash/ring.go

+		// getWeightAttribute() returns 1 if the weight attribute is not found
+		// on the address. And since this function is guaranteed to be called
+		// with a non-empty subConns map, weightSum is guaranteed to be
+		// non-zero. So, we need not worry about divide by zero error here.


Sorry, super super minor nit: "need not worry about a divide"

zasweq · 2022-08-03T18:37:25Z

xds/internal/balancer/ringhash/ring.go

+//
+// Must be called with a non-empty subConns map.
+func normalizeWeights(subConns *resolver.AddressMap) ([]subConnWithWeight, float64) {
+	var weightSum float64


What do you think of keeping this uint32, and instead of having another var to convert it like in master, you convert in the denominator float64(weightsum) to avoid cast on line 119. I don't care/don't feel strongly about this but was just proposing a suggestion.

zasweq · 2022-08-03T18:38:06Z

xds/internal/balancer/ringhash/ring.go

 }

 // normalizeWeights divides all the weights by the sum, so that the total weight
 // is 1.
-func normalizeWeights(subConns *resolver.AddressMap) ([]subConnWithWeight, float64, error) {
+//
+// Must be called with a non-empty subConns map.


I'm guessing "non-empty" encapsualtes nil and len(0)?

That's right.

zasweq · 2022-08-03T18:43:15Z

xds/internal/balancer/ringhash/ringhash.go

@@ -291,6 +272,29 @@ func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) err
 		b.ResolverError(errors.New("produced zero addresses"))


This is not correct. This happens before b.subConns gets written to. Thus, line 303 won't hit guaranteed. Say you get a first update with 3 addresses, you make 3 subconns. Then, the next UpdateClientConnState comes in with 0 addresses. Then, this conditional hits, the SubConn len is still 3, and doens't set to connectivity.TransientFailure.

That's right. I moved it to be right before we go about constructing the ring. So, if we end up with zero addresses, we save the effort of making a new ring.

Bug, not approved.

easwars · 2022-08-03T23:53:22Z

Thanks for the review.

zasweq

LGTM. I found my first correctness issue ever I'm so proud of myself :D!

easwars · 2022-08-04T19:11:16Z

LGTM. I found my first correctness issue ever I'm so proud of myself :D!

Yay Yay Yay !!!

ringhash: handle config updates properly

217e9ae

easwars added the Type: Bug label Aug 2, 2022

easwars added this to the 1.49 Release milestone Aug 2, 2022

easwars requested a review from zasweq August 2, 2022 19:31

easwars assigned zasweq Aug 2, 2022

zasweq requested changes Aug 2, 2022

View reviewed changes

zasweq assigned easwars and unassigned zasweq Aug 2, 2022

easwars added 3 commits August 2, 2022 14:37

simplify the logic to determine if ring has to be regenerated

1814745

rename vars/consts

1a9db40

ring creation should never fail

fbed058

easwars assigned zasweq and unassigned easwars Aug 2, 2022

zasweq assigned easwars and unassigned zasweq Aug 3, 2022

zasweq previously approved these changes Aug 3, 2022

View reviewed changes

zasweq reviewed Aug 3, 2022

View reviewed changes

review comments

6a45d8e

easwars assigned zasweq and unassigned easwars Aug 3, 2022

zasweq approved these changes Aug 4, 2022

View reviewed changes

easwars merged commit f9409d3 into grpc:master Aug 4, 2022

easwars deleted the ringhash_config_update_fix branch August 4, 2022 19:11

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ringhash: handle config updates properly #5557

ringhash: handle config updates properly #5557

easwars commented Aug 2, 2022

zasweq Aug 2, 2022

easwars Aug 2, 2022

zasweq Aug 2, 2022

easwars Aug 2, 2022

zasweq Aug 2, 2022 •

edited

easwars Aug 2, 2022

easwars Aug 2, 2022

zasweq Aug 2, 2022

easwars Aug 2, 2022

zasweq Aug 2, 2022

easwars Aug 2, 2022

zasweq left a comment

zasweq Aug 3, 2022

easwars Aug 3, 2022

zasweq Aug 3, 2022

easwars Aug 3, 2022

zasweq Aug 3, 2022

easwars Aug 3, 2022

zasweq Aug 3, 2022

easwars Aug 3, 2022

easwars commented Aug 3, 2022

zasweq left a comment

easwars commented Aug 4, 2022

		@@ -291,6 +272,29 @@ func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) err
		b.ResolverError(errors.New("produced zero addresses"))

ringhash: handle config updates properly #5557

ringhash: handle config updates properly #5557

Conversation

easwars commented Aug 2, 2022

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

zasweq Aug 2, 2022 • edited

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

zasweq left a 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

easwars commented Aug 3, 2022

zasweq left a comment

Choose a reason for hiding this comment

easwars commented Aug 4, 2022

zasweq Aug 2, 2022 •

edited