rls: Fix flaky test Test/ControlChannelConnectivityStateMonitoring

[eshitachandwani]

fixes #5468
The test flakes because

1. • Test creates a ClientConn with service config setting the LB policy to "rls"
   • RLS LB policy initializes the control channel to the RLS server. As part of this, it spawns a goroutine to monitor the control channel connectivity state changes.
   • But by the time the first RPC is successfully made, the above goroutine has not gotten a chance to run yet.
   • And at this time, the test stops the RLS server. This moves the control channel to IDLE and it is only now that the monitoring goroutine gets to run, and it has already missed the first transition to READY.

FIX : Use channel to make sure the go routine starts

2. Our current state change API is lossy because state changes can be lost between the former returning and the caller invoking GetState
   FIX : The fix is to use grpcsync.pubsub to subscribe to the state changes so that we do not loose state changes.

RELEASE NOTES: N/A

[codecov]

Codecov Report

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

Project coverage is 82.26%. Comparing base (e0d191d) to head (b25261c).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer/rls/control_channel.go	90.32%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8055      +/-   ##
==========================================
- Coverage   82.29%   82.26%   -0.04%     
==========================================
  Files         387      387              
  Lines       39065    39083      +18     
==========================================
+ Hits        32150    32152       +2     
- Misses       5584     5612      +28     
+ Partials     1331     1319      -12     

Files with missing lines	Coverage Δ
balancer/rls/control_channel.go	88.66% <90.32%> (-4.47%)	⬇️

... and 31 files with indirect coverage changes

[easwars]

Can you try 10K or 1M runs in forge before and after the fix to ensure that flakes are eliminated by the fix?

[easwars]

This is an error we don't expect to happen in practice and if it does, it indicates a severe programming error. I would be OK to add a panic here that includes the type being received.

[eshitachandwani]

Right! Done.

[easwars]

When a new subscriber is added to the pubsub, it receives the most recent message posted on the pubsub. This means that if there were N messages posted on the pubsub when a new subscriber is added, the subscriber only receives the most recently posted message. This might not be good enough for our purposes here. So, I suggest making the following changes.

• Get rid of the ccStateSubscriber. Instead store the buffer.Unbounded as a field of controlChannel.
• Initialize the unbounded buffer when the control channel is created in newControlChannel.
• Change grpc.Dial to grpc.NewClient in newControlChannel.
• Register the subscriber right after creating the ClientConn to the RLS server, but before calling Connect on it. This will ensure that the subscriber will receive every single state change on the ClientConn.
  • Implement the OnMessage method on the controlChannel type and pass it to the call to internal.SubscribeToConnectivityStateChanges

[eshitachandwani]

Done.

[easwars]

This would move to controlChannel.close.

[eshitachandwani]

Done.

[easwars]

Can we define this variable to be of the concrete type connectivity.State instead?

[eshitachandwani]

The buffer.Get() function used here as <-stateSubscriber.state.Get() returns a channel of type any we cannot do a type assertion on a channel directly. Also, we already do a type assertion in the OnMessage function , so IMO it should be okay. If we have to do the type assertion here, we will have to do it on the s value after the Get() function each time (we cna make a helper function that does get and type assertion for reuse). What would you suggest?

[easwars]

Do we need a for loop here? We know for a fact that we are in READY. So, the first time we actually read anything out of the unbounded buffer, we can be sure that we have moved out of READY. Am I missing something?

[eshitachandwani]

Right!