[thrift_proxy] Add metadata_match to RouteAction and WeightedClusters #4402
Conversation
- add to proto definition - add BUILD dependency to MetadataMatchCriteria[Impl] - add fields to RouteEntryImplBase, add method to RouteEntry interface Signed-off-by: Brian Ramos <[email protected]>
- Keep track of parent RouteEntryImplBase into WeightedClusterEntry to support metadataMatchCriteria implementation. Use parent's metadata when none provided in WeightedClusterEntry - use route_entry's metadataMatchCriteria for Router's LoadBalanceContext metadataMatchCriteria - formatting fixes Signed-off-by: Brian Ramos <[email protected]>
- move implementation of a method into the header file. rearrange header file to make it actually work. - move BUILD deps around to depend on interface vs. implementation appropriately - update mocks to support new RouteEntry method Signed-off-by: Brian Ramos <[email protected]>
Signed-off-by: Brian Ramos <[email protected]>
- WeightedClusterEntries were being constructed before metadata_match for a RouteAction was created, resulting in the merging functionality not working. Fixed - test various cases - format everything Signed-off-by: Brian Ramos <[email protected]>
Signed-off-by: Brian Ramos <[email protected]>
| // Optional endpoint metadata match criteria. Only endpoints in the upstream | ||
| // cluster with metadata matching that set in metadata_match will be | ||
| // considered. Keys and values should be provided under the "envoy.lb" metadata key. Note that | ||
| envoy.api.v2.core.Metadata metadata_match = 3; |
There was a problem hiding this comment.
I know this got copied from the RDS route.proto, but we should probably make clear that this only applies to the subset load balancer. So like
Optional endpoint metadata match criteria used by subset load balancer.
Also, there's a dangling "Note that" at the end of the comment.
| // this will be merged with what's provided in :ref: `RouteAction.Metadata | ||
| // <envoy_api_field_config.filter.network.thrift_proxy.v2alpha1.RouteAction.metadata_match>`, | ||
| // with values here taking precedence. | ||
| envoy.api.v2.core.Metadata metadata_match = 3; |
There was a problem hiding this comment.
Same bit about subset load balancer here.
| v3.set_string_value("v3"); | ||
| HashedValue hv1(v1), hv2(v2), hv3(v3); | ||
|
|
||
| // match with multiple weighted cluster metatada criterions defined |
source/common/router/config_impl.cc
Outdated
| @@ -212,43 +212,6 @@ HashPolicyImpl::generateHash(const Network::Address::Instance* downstream_addr, | |||
| return hash; | |||
| } | |||
|
|
|||
| std::vector<MetadataMatchCriterionConstSharedPtr> | |||
There was a problem hiding this comment.
I think the extension policy says these source/common changes should go in a separate PR for ease of review.
Signed-off-by: Brian Ramos <[email protected]>
…te docs. Signed-off-by: Brian Ramos <[email protected]>
…metadata_match Signed-off-by: Brian Ramos <[email protected]>
Signed-off-by: Brian Ramos <[email protected]>
|
@rgs1 @fishcakez if you're interested |
|
Meta-commentary on this PR: what we've found with the subset load balancer in production usage as an edge proxy is that its computation of upstream state changes is quite CPU-intensive (and is done in each worker thread, so it competes with request processing). I suspect the subset load balancer may need some optimization work to be viable for Thrift service mesh usage in environments with a large number of upstreams. @rgs1 has deeper context on this issue. |
| new Envoy::Router::MetadataMatchCriteriaImpl(filter_it->second)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
would 62441f9 work with thrift without further work?
There was a problem hiding this comment.
(this relates to @brian-pane's perf comment)
| // <envoy_api_field_config.filter.network.thrift_proxy.v2alpha1.WeightedCluster.ClusterWeight.metadata_match>`, | ||
| // with values there taking precedence. Keys and values should be provided under the "envoy.lb" | ||
| // metadata key. | ||
| envoy.api.v2.core.Metadata metadata_match = 3; |
There was a problem hiding this comment.
- i don't know of any concrete plans to do so since we don't keep track of
RequestInfoin the thrift connection manager right now - it's all the same cluster/upstream management so it should just work
Description
This change adds the ability to attach metadata to the clusters and weighted clusters to allow users to filter a down to instances that match metadata criteria provided. This is used only when the subset load balancer is enabled and leverages what exists already for http metadata matching.
Changes include:
MetadataMatchCriteriaImplinto a single fileMetadataMatchCriteriaobjects from protobufsThrift::Router::RouteEntryand use in router implRisk Level: LOW
Testing: tests, new and old, pass
Docs Changes: added description for new proto fields. docs build successfully.