xds: make comparison of server configs in bootstrap more reliable #6112
Conversation
|
@arvindbr8 : FYI, this change could cause merge conflicts with your A53 PR. Also, I don't remember where you are storing the |
easwars
force-pushed
the
drops_with_federation
branch
from
fcf0c22 to
e71d352
Compare
easwars
force-pushed
the
drops_with_federation
branch
from
e71d352 to
cf10dac
Compare
| // Also ensure that a new LRS stream is not created. | ||
| sCtx, sCancel = context.WithTimeout(ctx, defaultTestShortTimeout) | ||
| defer sCancel() | ||
| if _, err := mgmtServer.LRSServer.LRSStreamOpenChan.Receive(ctx); err == nil { |
There was a problem hiding this comment.
This should be sCtx, right? Otherwise this waits 10 seconds?
| features := "" | ||
| if len(sc.ServerFeatures) != 0 { | ||
| features = strings.Join(sc.ServerFeatures, "-") | ||
| } |
There was a problem hiding this comment.
Since strings.Join returns "" when given an empty slice, this can be simplified to:
features := strings.Join(sc.ServerFeatures, "-")| case (sc != nil) != (other != nil): | ||
| return false |
There was a problem hiding this comment.
FWIW: if sc == nil || other == nil is equivalent given the preceding case; I'm not sure which is more readable, but I find this form with 3 !=s to be a little confusing, personally.
There was a problem hiding this comment.
I agree that sc == nil || other == nil is much easier to read than (sc != nil) != (other != nil) when considered in isolation. But when used here, the former requires carrying over of state from the previous case, because we need to remember that both are not nil to make complete sense of this statement. But the latter is more self-contained as it simply compares the nilness of one with the other.
I think I might have written it as if sc == nil || other == nil if I had written this whole method as a series of if conditions, instead of a case statement.
| // Also ensure that a new LRS stream is not created. | ||
| sCtx, sCancel = context.WithTimeout(ctx, defaultTestShortTimeout) | ||
| defer sCancel() | ||
| if _, err := mgmtServer.LRSServer.LRSStreamOpenChan.Receive(ctx); err == nil { |
| features := "" | ||
| if len(sc.ServerFeatures) != 0 { | ||
| features = strings.Join(sc.ServerFeatures, "-") | ||
| } |
| case (sc != nil) != (other != nil): | ||
| return false |
There was a problem hiding this comment.
I agree that sc == nil || other == nil is much easier to read than (sc != nil) != (other != nil) when considered in isolation. But when used here, the former requires carrying over of state from the previous case, because we need to remember that both are not nil to make complete sense of this statement. But the latter is more self-contained as it simply compares the nilness of one with the other.
I think I might have written it as if sc == nil || other == nil if I had written this whole method as a series of if conditions, instead of a case statement.
When the
clusterimplLB policy receives a config update, it checks if theload_reporting_serverconfiguration has changed. If so, it closes the existing LRS stream and creates a new one.Unfortunately this equality comparison was broken because the
bootstrap.ServerConfigstruct contains agrpc.DialOptionto store the configured credentials. The implemention of this dial option (like many others) uses a function pointer which is not very amenable to comparisons. So, even if two configuration specify the same transport credentials, the equality comparison will fail.This PR addresses this issue and ensures that the LRS stream is not closed and re-created when the actual
load_reporting_serverconfig has not changed.Summary of changes:
ChannelCredstype in the bootstrap package to representchannel_credentialsspecified as part of the server config.ServerConfigtype in the bootstrap package in the following ways:ChannelCredstype.ServerConfigFromJSON.server_featuresas part of the server config. This also needs to be used for equality comparisons.This fixes an internal P1 issue that is blocking the federation release.
Fixes #6097
RELEASE NOTES: