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
ringhash: handle config updates properly #5557
Changes from 4 commits
217e9ae
1814745
1a9db40
fbed058
6a45d8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ | |
package ringhash | ||
|
||
import ( | ||
"fmt" | ||
"math" | ||
"sort" | ||
"strconv" | ||
|
@@ -64,12 +63,12 @@ type ringEntry struct { | |
// | ||
// To pick from a ring, a binary search will be done for the given target hash, | ||
// and first item with hash >= given hash will be returned. | ||
func newRing(subConns *resolver.AddressMap, minRingSize, maxRingSize uint64) (*ring, error) { | ||
// | ||
// Must be called with a non-empty subConns map. | ||
func newRing(subConns *resolver.AddressMap, minRingSize, maxRingSize uint64) *ring { | ||
// https://github.com/envoyproxy/envoy/blob/765c970f06a4c962961a0e03a467e165b276d50f/source/common/upstream/ring_hash_lb.cc#L114 | ||
normalizedWeights, minWeight, err := normalizeWeights(subConns) | ||
if err != nil { | ||
return nil, err | ||
} | ||
normalizedWeights, minWeight := normalizeWeights(subConns) | ||
|
||
// Normalized weights for {3,3,4} is {0.3,0.3,0.4}. | ||
|
||
// Scale up the size of the ring such that the least-weighted host gets a | ||
|
@@ -106,30 +105,29 @@ func newRing(subConns *resolver.AddressMap, minRingSize, maxRingSize uint64) (*r | |
for i, ii := range items { | ||
ii.idx = i | ||
} | ||
return &ring{items: items}, nil | ||
return &ring{items: items} | ||
} | ||
|
||
// 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. | ||
func normalizeWeights(subConns *resolver.AddressMap) ([]subConnWithWeight, float64) { | ||
var weightSum float64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
keys := subConns.Keys() | ||
if len(keys) == 0 { | ||
return nil, 0, fmt.Errorf("number of subconns is 0") | ||
} | ||
var weightSum uint32 | ||
for _, a := range keys { | ||
weightSum += getWeightAttribute(a) | ||
} | ||
if weightSum == 0 { | ||
return nil, 0, fmt.Errorf("total weight of all subconns is 0") | ||
weightSum += float64(getWeightAttribute(a)) | ||
} | ||
weightSumF := float64(weightSum) | ||
ret := make([]subConnWithWeight, 0, len(keys)) | ||
min := float64(1.0) | ||
for _, a := range keys { | ||
v, _ := subConns.Get(a) | ||
scInfo := v.(*subConn) | ||
nw := float64(getWeightAttribute(a)) / weightSumF | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, super super minor nit: "need not worry about a divide" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
nw := float64(getWeightAttribute(a)) / weightSum | ||
ret = append(ret, subConnWithWeight{sc: scInfo, weight: nw}) | ||
if nw < min { | ||
min = nw | ||
|
@@ -142,7 +140,7 @@ func normalizeWeights(subConns *resolver.AddressMap) ([]subConnWithWeight, float | |
// where an address is added and then removed, the RPCs will still pick the | ||
// same old SubConn. | ||
sort.Slice(ret, func(i, j int) bool { return ret[i].sc.addr < ret[j].sc.addr }) | ||
return ret, min, nil | ||
return ret, min | ||
} | ||
|
||
// pick does a binary search. It returns the item with smallest index i that | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -259,28 +259,9 @@ func (b *ringhashBalancer) updateAddresses(addrs []resolver.Address) bool { | |
|
||
func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) error { | ||
b.logger.Infof("Received update from resolver, balancer config: %+v", pretty.ToJSON(s.BalancerConfig)) | ||
if b.config == nil { | ||
newConfig, ok := s.BalancerConfig.(*LBConfig) | ||
if !ok { | ||
return fmt.Errorf("unexpected balancer config with type: %T", s.BalancerConfig) | ||
} | ||
b.config = newConfig | ||
} | ||
|
||
// Successful resolution; clear resolver error and ensure we return nil. | ||
b.resolverErr = nil | ||
if b.updateAddresses(s.ResolverState.Addresses) { | ||
// If addresses were updated, no matter whether it resulted in SubConn | ||
// creation/deletion, or just weight update, we will need to regenerate | ||
// the ring. | ||
var err error | ||
b.ring, err = newRing(b.subConns, b.config.MinRingSize, b.config.MaxRingSize) | ||
if err != nil { | ||
b.ResolverError(fmt.Errorf("ringhash failed to make a new ring: %v", err)) | ||
return balancer.ErrBadResolverState | ||
} | ||
b.regeneratePicker() | ||
b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker}) | ||
newConfig, ok := s.BalancerConfig.(*LBConfig) | ||
if !ok { | ||
return fmt.Errorf("unexpected balancer config with type: %T", s.BalancerConfig) | ||
} | ||
|
||
// If resolver state contains no addresses, return an error so ClientConn | ||
|
@@ -291,6 +272,29 @@ func (b *ringhashBalancer) UpdateClientConnState(s balancer.ClientConnState) err | |
b.ResolverError(errors.New("produced zero addresses")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return balancer.ErrBadResolverState | ||
} | ||
|
||
// If addresses were updated, whether it resulted in SubConn | ||
// creation/deletion, or just weight update, we need to regenerate the ring | ||
// and send a new picker. | ||
regenerateRing := b.updateAddresses(s.ResolverState.Addresses) | ||
|
||
// If the ring configuration has changed, we need to regenerate the ring and | ||
// send a new picker. | ||
if b.config == nil || b.config.MinRingSize != newConfig.MinRingSize || b.config.MaxRingSize != newConfig.MaxRingSize { | ||
regenerateRing = true | ||
} | ||
b.config = newConfig | ||
|
||
if regenerateRing { | ||
// Ring creation is guaranteed to not fail because we call newRing() | ||
// with a non-empty subConns map. | ||
b.ring = newRing(b.subConns, b.config.MinRingSize, b.config.MaxRingSize) | ||
b.regeneratePicker() | ||
b.cc.UpdateState(balancer.State{ConnectivityState: b.state, Picker: b.picker}) | ||
} | ||
|
||
// Successful resolution; clear resolver error and return nil. | ||
b.resolverErr = nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one looks totally fine to me. |
||
return nil | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing "non-empty" encapsualtes nil and len(0)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right.