cluster_resolver: wait until the configs have been propagated to all the children #7562
Conversation
aranjans
force-pushed
the
aranjans_cluster_resolver_pckr_update
branch
from
626ab22
to
a3da044
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7562 +/- ##
==========================================
+ Coverage 81.57% 81.82% +0.25%
==========================================
Files 354 361 +7
Lines 27076 27819 +743
==========================================
+ Hits 22088 22764 +676
- Misses 3798 3854 +56
- Partials 1190 1201 +11
|
aranjans
force-pushed
the
aranjans_cluster_resolver_pckr_update
branch
from
a3da044
to
fdcdc5b
Compare
purnesh42H
changed the title
| @@ -306,6 +310,11 @@ func (b *clusterResolverBalancer) run() { | |||
| switch update := u.(type) { | |||
| case *ccUpdate: | |||
| b.handleClientConnUpdate(update) | |||
| if update.done != nil { | |||
| // Close the channel to convey to the consumers of updateCh | |||
| // that child policies config are updated inline. | |||
There was a problem hiding this comment.
// Close the channel to signal to consumers of updateCh
// that the child policies' configuration has been updated.
| @@ -445,3 +446,64 @@ func (s) TestOutlierDetectionConfigPropagationToChildPolicy(t *testing.T) { | |||
| t.Fatalf("Timeout when waiting for child policy to receive its configuration") | |||
| } | |||
| } | |||
|
|
|||
| // Test verifies that LB waits for child policy configuration update | |||
There was a problem hiding this comment.
// TestChildPolicyConfigUpdateBlocking ensures that the load balancer
// blocks and waits for the child policy configuration to be updated
// in response to configuration changes.
There was a problem hiding this comment.
@aranjans need to change the comment too along with test name
|
|
||
| // Test verifies that LB waits for child policy configuration update | ||
| // inline on receipt of configuration update. | ||
| func (s) TestChildPoliciesConfigUpdatedInline(t *testing.T) { |
There was a problem hiding this comment.
name suggestion: TestChildPolicyConfigUpdateBlocking
| // Test verifies that LB waits for child policy configuration update | ||
| // inline on receipt of configuration update. | ||
| func (s) TestChildPoliciesConfigUpdatedInline(t *testing.T) { | ||
| // Override the newConfigHook to ensure picker was updated. |
There was a problem hiding this comment.
// Override the ClientConnUpdateHook to track when the picker is updated.
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
| if err := managementServer.Update(ctx, resources); err != nil { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
nit: "Failed to update resources on management server: %v", err
| t.Fatalf("EmptyCall() failed: %v", err) | ||
| } | ||
|
|
||
| // Close the listener and ensure that the config is updated. |
There was a problem hiding this comment.
// Ensure that the child policy config update is processed.
| // Close the listener and ensure that the config is updated. | ||
| lis.Close() | ||
| select { | ||
| case <-clientConnUpdateDone: |
There was a problem hiding this comment.
suggestion: can add a comment in success block // Success: config update was processed within the expected time.
|
I've started an internal discussion thread to discuss what would be the correct fix for this. Please bring your thoughts to that thread. |
@easwars Should we still keep this PR open or approach will change non trivially from here? |
|
I think the approach will change non-trivially from here, so we can close this for now. Thanks. |
What does this PR do?
This PR ensures that every time configuration update is triggered, it waits for child policy configuration update inline.
Approach:
Added a channel to
ccUpdateof which that channel will be closed after child policies are updated. Same is verified in the test by having aClientConnUpdateHook()which is called after the above mentioned channel is closed, which ensures child policy was updated synchronously.RELEASE NOTES: