xds: introduce generic xds clients xDS and LRS Client API signatures #8042
Conversation
purnesh42H
changed the title
purnesh42H
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8042 +/- ##
==========================================
- Coverage 82.15% 82.01% -0.14%
==========================================
Files 387 390 +3
Lines 39067 39081 +14
==========================================
- Hits 32094 32051 -43
- Misses 5643 5688 +45
- Partials 1330 1342 +12
|
purnesh42H
changed the title
purnesh42H
added
Area: xDS
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
3 times, most recently
from
042b236 to
f8f7af5
Compare
| v3statuspb "github.com/envoyproxy/go-control-plane/envoy/service/status/v3" | ||
| ) | ||
|
|
||
| // XDSClient is a full fledged client which queries a set of discovery APIs |
There was a problem hiding this comment.
"full fledged" -- please remove words that don't help with understanding what it does. It's just "a client which" ...
| * | ||
| */ | ||
|
|
||
| // Package xdsclient provides implementation of the xDS client for enabling |
| */ | ||
|
|
||
| // Package xdsclient provides implementation of the xDS client for enabling | ||
| // applications to communicate with xDS management servers. |
There was a problem hiding this comment.
"enabling applications to" is also unnecessarily wordy.
Compare to the http package's documentation:
// Package http provides HTTP client and server implementations.| // applications to communicate with xDS management servers. | ||
| // | ||
| // It allows applications to: | ||
| // - Create xDS client instance with in-memory configurations. |
| // It allows applications to: | ||
| // - Create xDS client instance with in-memory configurations. | ||
| // - Register watches for named resources. | ||
| // - Receive resources via the ADS (Aggregated Discovery Service) stream. |
| // During a race (e.g. an xDS response is received while the user is calling | ||
| // cancel()), there's a small window where the callback can be called after | ||
| // the watcher is canceled. Callers need to handle this case. | ||
| func (c *XDSClient) WatchResource(_ string, _ string, _ ResourceWatcher) (cancel func()) { |
There was a problem hiding this comment.
Better to fill in the names of the parameters otherwise this is not so helpful. Is vet hard-failing? Would changing to panic("not implemented") in the body of the function help?
There was a problem hiding this comment.
Putting //revive:disable:unused-parameter above the file seem to be do the trick. Though I have replaced all methods to `panic("unimplemented")
| } | ||
|
|
||
| // WatchResource uses xDS to discover the resource associated with the provided | ||
| // resource name. The resource type url look up the resource type |
There was a problem hiding this comment.
"The resource type url look up the" -- this isn't parsing for me.
Also please capitalize URL.
There was a problem hiding this comment.
Modified the language. Let me know if it looks better.
| // During a race (e.g. an xDS response is received while the user is calling | ||
| // cancel()), there's a small window where the callback can be called after | ||
| // the watcher is canceled. Callers need to handle this case. |
There was a problem hiding this comment.
This isn't even worth mentioning. It should be obvious that before this function returns, it may not have finished doing what it was called to do.
xds/internal/clients/config.go
Outdated
| @@ -0,0 +1,207 @@ | |||
| /* | |||
| * | |||
| * Copyright 2024 gRPC authors. | |||
There was a problem hiding this comment.
Let's make this 2025 in all these new files?
| * | ||
| */ | ||
|
|
||
| // Package lrsclient provides implementation of the LRS client for enabling |
There was a problem hiding this comment.
*an implementation.
Please take another pass of the whole PR and apply the previous comments throughout.
There was a problem hiding this comment.
Yes, I did another pass and applied called out comments and improved details in some places. PTAL.
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
from
f8f7af5 to
415b2ca
Compare
|
@dfawley i did the godoc review and made following changes (see the last commit)
|
| } | ||
|
|
||
| // NewConfig returns a new xDS client config with provided parameters. | ||
| func NewConfig(servers []clients.ServerConfig, authorities map[string]clients.Authority, node clients.Node, transport clients.TransportBuilder, resourceTypes map[string]ResourceType) Config { |
There was a problem hiding this comment.
It's unusual to have a constructor and a struct with exported fields.
Generally it's unusual to have a constructor for a config struct. I hope we won't need this at all. Let's remove it for now.
There was a problem hiding this comment.
Removed for now. I only see the use case for e2e tests but we can always do all those stuff while creating new client or from test itself.
| // be used only for old-style names without an authority. | ||
| Servers []clients.ServerConfig | ||
|
|
||
| // Authorities is a map of authority names to authority configurations. |
There was a problem hiding this comment.
What is an authority? How would the user find out? Is there a doc we can link?
There was a problem hiding this comment.
I have linked the proto link of authority on envoy in the common config where Authority struct is present. It can be navigated from here in godoc.
| // TransportBuilder is the implementation to create a communication channel | ||
| // to an xDS management server. |
There was a problem hiding this comment.
| // TransportBuilder is the implementation to create a communication channel | |
| // to an xDS management server. | |
| // TransportBuilder is used to connect to the management server. |
| // Below values will have default values but can be overridden for testing | ||
|
|
There was a problem hiding this comment.
| // Below values will have default values but can be overridden for testing |
There was a problem hiding this comment.
Removed the unexported fields intended for testing for now
| // OnCallbackProcessed is a function to be invoked by resource watcher | ||
| // implementations upon completing the processing of a callback from the xDS | ||
| // client. Failure to invoke this callback prevents the xDS client from reading | ||
| // further messages from the xDS server. | ||
| type OnCallbackProcessed func() | ||
|
|
There was a problem hiding this comment.
@easwars WDYT about deleting this as a type? The ResourceWatcher interface can just have a func() and name the parameter, and it can be documented on ResourceWatcher instead. Otherwise these types end up split in the godoc view and we have to write a lot to connect them and it adds to the total complexity of the package.
There was a problem hiding this comment.
I don't mind getting rid of this type.
There was a problem hiding this comment.
Done. Added documentation on the ResourceWatcher interface
| // Servers specifies a list of xDS servers to connect to. This field should | ||
| // be used only for old-style names without an authority. |
There was a problem hiding this comment.
Let's leave "old" and "style" out of the docstrings and describe what it is or what it is used for in specific terms instead.
There was a problem hiding this comment.
I have modified the documentation. One more thing i have added in both Authority and here about order of servers in the list for precedence. Let me know if thats okay. I remember there was a question recently about how the fallback servers are chosen. Should we mention the gRFC as well?
| // TransportBuilder is the implementation to create a communication channel | ||
| // to an xDS management server. |
There was a problem hiding this comment.
| // TransportBuilder is the implementation to create a communication channel | |
| // to an xDS management server. | |
| // TransportBuilder is used to connect to the LRS server. |
| // Config provides parameters for configuring the LRS client. | ||
| type Config struct { | ||
| // Node is the identity of the client application reporting load to the | ||
| // xDS management server. |
There was a problem hiding this comment.
| // xDS management server. | |
| // LRS server. |
| TypeURL() string | ||
|
|
||
| // TypeName identifies resources in a transport protocol agnostic way. This | ||
| // can be used for logging/debugging purposes, as well in cases where the |
There was a problem hiding this comment.
| // can be used for logging/debugging purposes, as well in cases where the | |
| // can be used for logging/debugging purposes, as well as in cases where the |
| // resource type name is to be uniquely identified but the actual | ||
| // functionality provided by the resource type is not required. |
There was a problem hiding this comment.
I'm not understanding this comment.
But..should a ResourceType actually be a struct and not an interface? It has 3 things that are really just settings.
Maybe
type ResourceType struct {
Name string
TypeURL string
// Incremental specifies that this resource is incremental, not state of the world.
// (link to https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol#variants-of-the-xds-transport-protocol)
Incremental bool
Decoder Decoder
}
type Decoder interface {
Decode(resource any, options DecodeOptions) (*DecodeResult, error)
}There was a problem hiding this comment.
Internally, our resource type implementations do have a struct which provides these three settings and really the decode functionality is the only moving piece.
What advantages do you see with a struct over the interface?
I know AllResourcesRequiredInSotW is not a great name, but Incremental might be confusing as well since we only support the SotW for all resources.
There was a problem hiding this comment.
What advantages do you see with a struct over the interface?
It's not very idiomatic to accept an interface with a bunch of methods that return constant setting values... I know I've never seen anything quite like that before, but maybe you have.
Incremental might be confusing as well since we only support the SotW for all resources
Sorry, I think I misinterpreted the setting. This is also the name that C++ uses, so we can leave that alone.
There was a problem hiding this comment.
Changed to the struct with Decoder interface
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
from
20e26f8 to
c3e96f7
Compare
xds/internal/clients/config.go
Outdated
| } | ||
|
|
||
| // Authority contains configuration for an xDS control plane authority. | ||
| // [authority]: https://www.envoyproxy.io/docs/envoy/latest/xds/core/v3/authority.proto |
There was a problem hiding this comment.
I'm not sure if we should link to this type. I'm not even sure if our authority corresponds to this type. I have never seen this proto before.
There was a problem hiding this comment.
Linked boo. Yeah the authority proto doesn't have much info. I have linked the service proto of LRS though in LRS client documentation.
xds/internal/clients/config.go
Outdated
| type Authority struct { | ||
| // XDSServers contains the list of server configurations for this authority. | ||
| // xDS client use the first available server from the list. To ensure high | ||
| // availability, list the most reliable server first. |
There was a problem hiding this comment.
The order of the servers does not affect "high availability" in any way. The order of servers reflects the order of preference of the data returned by those servers. See: https://github.com/grpc/proposal/blob/master/A71-xds-fallback.md#reservations-about-using-the-fallback-server-data
Also, maybe this file could be useful in terms of what to include in our docstrings: https://github.com/grpc/grpc/blob/master/doc/grpc_xds_bootstrap_format.md
There was a problem hiding this comment.
Changed to mention that. Thanks for correcting.
| type LRSClient struct { | ||
| } | ||
|
|
||
| // ReportLoad creates a new load reporting stream for the client. |
There was a problem hiding this comment.
We are saying that it creates a new load reporting stream, but it returns a LoadStore. How are they connected?
There was a problem hiding this comment.
I had mentioned the connection on LoadStore struct. Moved here that ReportLoad returns a LoadStore.
| "google.golang.org/grpc/xds/internal/clients" | ||
| ) | ||
|
|
||
| // A Config structure is used to configure an LRS client. After one has been |
There was a problem hiding this comment.
Go comments being with the name of the symbol for which the comment is meant for. And please skip structure.
What is LRS function here? This comment is quite confusing as it stands now.
There was a problem hiding this comment.
this format is from Doug's suggestion to follow same style as TLS #8042 (comment)
|
|
||
| // LoadStore keeps the loads for multiple clusters and services to be reported | ||
| // via LRS. It contains loads to report to one LRS server. It creates | ||
| // multiple stores for multiple servers. |
There was a problem hiding this comment.
These two sentences are contradictory:
- It contains loads to report to one LRS server.
- It creates multiple stores for multiple servers
There was a problem hiding this comment.
It was a mistake. It means the LoadStore struct keep load of only one server. For multiple servers, caller should create multiple stores. Modified the docstring
| type Config struct { | ||
| // Servers specifies a list of xDS management servers to connect to, | ||
| // including fallbacks. xDS client use the first available server from the | ||
| // list. To ensure high availability, list the most reliable server first. |
There was a problem hiding this comment.
including fallbacks doesn't provide much information. We need to be clear that this is an ordered list.
Same here about To ensure high availability, list the most reliable server first.
There was a problem hiding this comment.
removed fallbacks and modified as suggested in above comment
| // OnCallbackProcessed is a function to be invoked by resource watcher | ||
| // implementations upon completing the processing of a callback from the xDS | ||
| // client. Failure to invoke this callback prevents the xDS client from reading | ||
| // further messages from the xDS server. | ||
| type OnCallbackProcessed func() | ||
|
|
There was a problem hiding this comment.
I don't mind getting rid of this type.
| // AmbientError indicates an error occurred while trying to fetch or decode | ||
| // the associated resource. The previous version of the resource should still | ||
| // be considered valid. | ||
| AmbientError(err error, done func()) |
There was a problem hiding this comment.
Both methods need to be consistent with the type and name for the callback.
| ResourceChanged(ResourceDataOrError, OnCallbackProcessed) | ||
|
|
||
| // AmbientError indicates an error occurred while trying to fetch or decode | ||
| // the associated resource. The previous version of the resource should still |
There was a problem hiding this comment.
This should also indicate that the watcher may use this error message for better debuggability as mentioned in A88.
There was a problem hiding this comment.
// AmbientError indicates an error occurred after a resource has been // received that should not modify the use of that resource but may be // useful information about the ambient state of the XdsClient
Mentioned this which is the language from gRFC
| // resource type name is to be uniquely identified but the actual | ||
| // functionality provided by the resource type is not required. |
There was a problem hiding this comment.
Internally, our resource type implementations do have a struct which provides these three settings and really the decode functionality is the only moving piece.
What advantages do you see with a struct over the interface?
I know AllResourcesRequiredInSotW is not a great name, but Incremental might be confusing as well since we only support the SotW for all resources.
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
from
d50c656 to
def35ab
Compare
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
from
def35ab to
fef4e3d
Compare
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
3 times, most recently
from
a5aefef to
e134ea6
Compare
purnesh42H
force-pushed
the
generic-xds-client-2-interface
branch
from
e134ea6 to
e0a5652
Compare
This is the next part of generic xds clients to be usable outside of grpc which builds on top of #8024. This change is adding the user API signatures for xDS and LRS client (without implementation) to communicate with xDS management server.
POC
Internal Design
RELEASE NOTES: None