split out synthetic policies for readability#2310
split out synthetic policies for readability#2310hawkw merged 8 commits intoeliza/policy-discoveryfrom
Conversation
882717c to
2cc158d
Compare
2cc158d to
d77445b
Compare
| } else if let Some(profiles::LogicalAddr(ref addr)) = profile.addr { | ||
| policy::BackendDispatcher::BalanceP2c( | ||
| ewma, | ||
| policy::EndpointDiscovery::DestinationGet { | ||
| path: addr.to_string(), | ||
| }, |
There was a problem hiding this comment.
This case isn't meaningful because we expect the policy control to always return for anything we could balance over.
There was a problem hiding this comment.
It turns out this was load-bearing for integration tests: when profiles provided a logical name, we would proceed with balancing/discovery on that name. But this isn't how we expect things to work in practice: We want to rely on the policy controller to provide service information. If it doesn't know about a service, we expect to fallback to a forward stack.
| rx | ||
| } | ||
|
|
||
| fn synthesize_forward_policy( |
There was a problem hiding this comment.
I moved this out of client-policy because it's basically a hardcoded configuration. I wouldn't want to rely on it anywhere else but here.
hawkw
left a comment
There was a problem hiding this comment.
this looks great, thanks for taking the time to fix up the tests (i didn't want to do that). i had some very minor nits to pick but i can address some of them myself after merging, as well.
| } | ||
|
|
||
| fn synthesize_forward_policy( | ||
| name: impl ToString, |
There was a problem hiding this comment.
nit, TIOLI: i feel like this could maybe take an Arc<Meta> rather than an impl ToString --- we use hard-coded strings ("endpoint" and "fallback") for the name, so that could be a singleton rather than creating a new Arc<Meta> for every synthesized policy? not a blocker.
There was a problem hiding this comment.
keep in mind this is going to happen at most once per connection (and less frequently than that in practice).
| e.downcast_ref::<tonic::Status>() | ||
| .map(|s| s.code() == tonic::Code::NotFound) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
currently the error is always a Status at the top level, but it occurs to me that this is a bit brittle in that the error might get wrapped...maybe this should traverse a whole source chain if there is one? we can address separately though.
There was a problem hiding this comment.
oh yeah we should use the errors helper here probably but it's fine for now.
| tracing::debug!(?policy, profile = ?*profile.borrow(), "Synthesizing policy from profile"); | ||
| let (tx, rx) = watch::channel(policy); | ||
| tokio::spawn(async move { | ||
| let mut profile = profile; |
There was a problem hiding this comment.
is this necessary? profile is already mut in the function declaration...
There was a problem hiding this comment.
this was probably here first. can be removed.
| return; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
occurred to me that this future maybe ought to have a span indicating the orig_dst or something, since otherwise, the logs here are not going to tell us which policy watch any of this happened to...i can address that after merging though.
There was a problem hiding this comment.
we already know it on the parent span
There was a problem hiding this comment.
oh but we do want in_current_span here
No description provided.