xds/federation: support federation in LRS #5128
Conversation
| var servers []*xdsServer | ||
| if err := json.Unmarshal(data, &servers); err != nil { | ||
| return fmt.Errorf("xds: json.Unmarshal(data) for field xds_servers failed during bootstrap: %v", err) | ||
| // MarshalJSON marshal the ServerConfig to json. |
| if sc == nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
When can this be nil? Would a comment for the same be useful?
There was a problem hiding this comment.
I deleted this.
In an Equal(), I was comparing String() of this config.
I changed that Equal() to check for nil first.
| var servers []*ServerConfig | ||
| if err := json.Unmarshal(v, &servers); err != nil { | ||
| return nil, fmt.Errorf("xds: json.Unmarshal(data) for field xds_servers failed during bootstrap: %v", err) | ||
| } | ||
| if len(servers) < 1 { | ||
| return nil, fmt.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any management server to connect to") | ||
| } | ||
| config.XDSServer = servers[0] |
There was a problem hiding this comment.
What do you think about having a type like this:
type xdsServers []*ServerConfig
func (xs *xdsServers) UnmarshalJSON(data []byte) error {
if err := json.Unmarshal(v, &xs); err != nil {
return nil, fmt.Errorf("xds: json.Unmarshal(data) for field xds_servers failed during bootstrap: %v", err)
}
if len(xds) < 1 {
return nil, fmt.Errorf("xds: bootstrap file parsing failed during bootstrap: file doesn't contain any management server to connect to")
}
return nil
}This can be used here and in Authority.UnmarshalJSON, thereby reducing some repeated code.
There was a problem hiding this comment.
My attempt to make it a type failed, because of infinite recursion.
I ended up making it a func.
xds/internal/xdsclient/loadreport.go
Outdated
| // | ||
| // The same options used for creating the Client will be used (including | ||
| // NodeProto, and dial options if necessary). | ||
| // ReportLoad starts an load reporting stream to the given server. All load |
There was a problem hiding this comment.
s/starts an/starts a/
s/All load report/All load reports/
| SecurityCfg: sc, | ||
| MaxRequests: circuitBreakersFromCluster(cluster), | ||
| LBPolicy: lbPolicy, | ||
| } | ||
|
|
||
| // Note that this is different from the gRFC (gRFC says to include the full |
There was a problem hiding this comment.
Nit: Add gFRC number in the comment?
| @@ -35,6 +37,18 @@ const ( | |||
| ClusterTypeAggregate | |||
| ) | |||
|
|
|||
| // ClusterLRSServerConfigType is the type of LRS server config. | |||
| type ClusterLRSServerConfigType int | |||
There was a problem hiding this comment.
I don't quite understand what this type offers over the existing EnableLRS bool field. Is this a forward looking change to support something in the future?
There was a problem hiding this comment.
Yes, this is a forward looking change.
The spec actually says this field should be the authority service config itself. But that adds complexity, because this function doesn't have access to the xdsclient or the bootstrap file.
I think making it as close to the proto message as possible is future proof.
| // from. | ||
| LoadReportingServerName *string `json:"lrsLoadReportingServerName,omitempty"` | ||
|
|
||
| // LoadReportingServerName *string `json:"lrsLoadReportingServerName,omitempty"` |
| MaxConcurrentRequests: newUint32(testMaxRequests), | ||
| Type: DiscoveryMechanismTypeEDS, | ||
| EDSServiceName: testEDSServcie, | ||
| Cluster: testClusterName, |
There was a problem hiding this comment.
Do we need any negative tests for this field?
There was a problem hiding this comment.
Not sure what you meant here.
A test with an invalid JSON?
| // full ServerConfig{URL,creds,server feature} here). This information is | ||
| // not available here, because this function doesn't have access to the | ||
| // xdsclient bootstrap information now (can be added if necessary). The | ||
| // ServerConfig will be read and populate by the CDS balancer when |
- update xdsclient's LoadReport method to take a ServerConfig instead of just a string - update clusterresolver and clusterimpl balancer config to take a full ServerConfig as LRS server - update CDS resource update LRS server field from a string to an enum (can be make an interface to support more types later) - update CDS balancer to read ServerConfig from xdsclient bootstrap and populate child policy config - cleanup bootstrap config json handling so the struct can be reused - change json unmarshal from taking a list (this is what used in bootstrap json) to taking an item
RELEASE NOTES: