cds: config for load balancer subsets #173
Conversation
Signed-off-by: Stephan Zuercher <[email protected]>
api/cds.proto
Outdated
| // { "keys": [ "stage", "hardware_type" ] } | ||
| // ]} | ||
| // Subsets may overlap. In the case of overlapping subsets, the first | ||
| // matching subset is selected. |
There was a problem hiding this comment.
Can you comment on how subsets are to be used here?
api/cds.proto
Outdated
| // endpoint metadata and selected by route and weighted cluster metadata. | ||
| message LbSubsetConfig { | ||
| // The behavior used when no endpoint subset matches the selected route's | ||
| // metadata. The options are no_fallback, any_endpoint, or default_subset. |
There was a problem hiding this comment.
Nit: vague preference to capitalize these comments to match the enum. When we auto-generate docs, it will look more consistent.
There was a problem hiding this comment.
Heh. I was following the pattern in DnsLookupFamily. Want me to fix those up too?
There was a problem hiding this comment.
Yeah. A bunch of docs were copy+paste from the v1 JSON, which may have not been case sensitive. In v2, folks are going to have to use all caps for JSON/YAML/proto.
Signed-off-by: Stephan Zuercher <[email protected]>
|
@zuercher in general this seems reasonable to me, but in this case I wonder if it would be useful to see the accompanying code PR to see if the API makes sense. I'm still a little hazy on how the matching is going to happen at decent performance without using reflection. I don't feel that strongly about it but my preference would be to see a rough draft of the code PR so that we can look at them side by side. We can keep this open and iterate on both if that works for you. |
|
Ok. |
|
See envoyproxy/envoy#1735 for the requested rough draft. |
There was a problem hiding this comment.
LGTM pending my question about LB runtime performance in the other PR. @htuch ?
There was a problem hiding this comment.
While reading this it occurred to me that there are other choices that could be made, such as "most specific subset wins" or the like, but these may imply more overhead in the LB matching algorithm. @zuercher can you comment on the rationale here?
|
Also, are the subset key lists only a convenience for the purpose of precomputing subsets ahead of time? |
|
Yes, there are choices about how to reconcile overlapping subsets that require more or less overhead. I tried to pick one that was efficient and allowed one to reason about which subset would chosen for a given input. The subset key lists call out the combinations of metadata keys that are desired for routing. This lets you specify that you might want, for example, to route on stage+version or just version, but not on stage alone. Or as a different example, that you want to route on stage+version and memory_size+version but not other combinations. Effectively it lets you limit the number of subsets be limiting the possible combinations and lets you ignore certain metadata altogether. (The latter could be achieved by filtering them from the config in the first place.) Precomputing the subsets ahead of time lets you avoid whatever locking would be necessary if they were done at the point of load balancing. I think that's preferred. We could instead compute the required subsets from the current routes. Eventually, however, I'd like to see a mechanism where the metadata values used for choosing a subset come from a request header. My use case for this is to allow a developer to force their requests to a particular endpoint (e.g., a test version they've deployed) without having to explicitly add routes. |
I have a lot of thoughts on cool stuff we can do here. See: envoyproxy/envoy#343 for tracking issue. I think the decisions made here are a good balance of flexibility and runtime performance. @htuch I'm inclined to merge. Any objections? |
|
Yeah agreed on docs. I think MD in proxy repo is probably fine but up to @zuercher if he wants to do here. |
|
I'll add some docs tomorrow. |
|
OK, will merge this one for now so we can unblock work on your other PR, look forward to looking at docs. |
Provides a configuration point for a load balancer that can divide endpoints into subsets addressable by route metadata.
See discussion in envoyproxy/envoy#1279.