clusterimpl: update picker synchronously on config update

[aranjans]

#5469 recommends an audit of existing LB policies to ensure that they update their pickers synchronously upon receipt of a configuration update. clusterimpl does not update picker synchronously before because of multiple reasons, one is on reciept of configuration update, process of switching the child policies was aysnchronous, and second is it was not waiting for the child policy config to be update first, before returning from UpdateClientConnState.

What does this PR do?
This PR ensures that every time configuration update is triggered, picker is updated synchronously.

RELEASE NOTES:

• clusterimpl: update picker synchronously on receipt of configuration update.

[codecov]

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.83%. Comparing base (67e47fc) to head (64521e0).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
xds/internal/balancer/clusterimpl/clusterimpl.go	86.36%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7652      +/-   ##
==========================================
- Coverage   81.86%   81.83%   -0.04%     
==========================================
  Files         361      361              
  Lines       27822    27834      +12     
==========================================
+ Hits        22777    22778       +1     
- Misses       3849     3856       +7     
- Partials     1196     1200       +4     

Files with missing lines	Coverage Δ
xds/internal/balancer/clusterimpl/clusterimpl.go	89.77% <86.36%> (-1.78%)	⬇️

... and 16 files with indirect coverage changes

[arjan-bal]

nit: Can invert this condition to return early, reducing one level of indentation.

if b.inhibitPickerUpdates {
    return
}
// update picker.

[arjan-bal]

Both update state and UpdateClientConnState are using the same serializer. So when the updateClientConnState callback is being executed in the serializer, not other callback (including the one scheduled by UpdateState) can be executed until updateClientConnState has returned. So inhibiting updates seems redundant.

[easwars]

Arjan is right. Since the same serializer is used to handle updates from the parent and handle updates from the child, we will not be able to handle updates from the child until we return from updateClientConnState. This means that we need to refactor things out a little bit.

The childState field is the only shared field that is accessed from UpdateState. So, I think we might have to do the following:

• add a mutex that guards access to childState and inhibitPickerUpdates, and change existing methods to grab this mutex when they need to access these fields
• do not run UpdateState in the serializer, run it inline

Let me know what you think. Please don't take my word as is. Please ensure that the above approach would actually work.

[aranjans]

Thanks @easwars for your thoughts. I agree that currently since UpdateState, UpdateClientConnState and few other methods are using the same serializer, it will be executed one after the other. And hence child config can't be handled before updateClientConnState.
I agree that we could make the UpdateState asynchronous by running it inline and not use serializer for it.

[easwars]

Since you haven't assigned the PR back to me, I'm assuming that you are still working on it. I skimmed through it and it was still not going what I expect it to do.

[aranjans]

You're right. I was still making changes, hence i didn't assigned back to you.

[easwars]

clusterimpl has a single child if I'm not mistaken?