Add XDS logic for cluster and endpoints #112
Conversation
This adds the logic to processes responses from the XDS server and sends back acknowlegements according to the protocol. Implementation to talk to the server isn't included. Work on #10
|
So the ci failure is from rustdoc trying to treat documentation in the generated code as tests and failing to compile since those aren't tests (that's why those modules were removed when running doctests), haven't found a workaround so far to tell rustdoc to ignore them |
| @@ -21,7 +21,7 @@ | |||
| mod udpa { | |||
| pub mod core { | |||
| pub mod v1 { | |||
| #![cfg(not(doctest))] | |||
| #![doc(hidden)] | |||
There was a problem hiding this comment.
I'm assuming this is an attempt to ignore the doc tests? 😬 I see your pain here. 🤔
There was a problem hiding this comment.
Yeah that was part of the plan, figured in any case we don't want to include them in our documentation. Unfortunately it seems that there isn't a clear workaround for this at the moment, I'm looking at instead excluding these modules and any of ours that depend on it, from doctests https://users.rust-lang.org/t/disabling-rustdoc-tests-for-module/50110/9
There was a problem hiding this comment.
Updated to exclude both the xds modules and cluster module that depends on it when running doc tests
| } | ||
|
|
||
| #[derive(Clone, Debug, Hash, Eq, PartialEq)] | ||
| pub struct Locality { |
There was a problem hiding this comment.
This is actually really interesting -- and I can see where this information could well be useful. 👍
src/xds/cluster.rs
Outdated
| */ | ||
|
|
||
| use crate::cluster::{ | ||
| Cluster as QuilkinCluster, ClusterLocalities, Endpoint, Locality, LocalityEndpoints, |
There was a problem hiding this comment.
| Cluster as QuilkinCluster, ClusterLocalities, Endpoint, Locality, LocalityEndpoints, | |
| Cluster as ProxyCluster, ClusterLocalities, Endpoint, Locality, LocalityEndpoints, |
Just a suggestion?
Just thinking the project hasn't gone through legal approval on the name, so it may get changed 😃
Also a bit more generic. WDYT?
src/xds/cluster.rs
Outdated
|
|
||
| /// Processes a CDS response and updates its cluster view if needed. | ||
| pub(in crate::xds) async fn on_cluster_response(&mut self, response: DiscoveryResponse) { | ||
| info!( |
There was a problem hiding this comment.
Should these be debug! ? or do we want to track each config change in the logs?
(Not against it, but thought I'd ask the question)
There was a problem hiding this comment.
I think I only defaulted to info!, debug! makes sense, will fix
| last_seen_cluster_load_assignment_version: Option<(String, String)>, | ||
| } | ||
|
|
||
| impl ClusterManager { |
There was a problem hiding this comment.
Just so I'm clear - this isn't plugging into anything yet, right?
There was a problem hiding this comment.
Yes, its not instantiated in code at the moment
src/xds/cluster.rs
Outdated
| &mut self, | ||
| response: DiscoveryResponse, | ||
| ) { | ||
| info!( |
There was a problem hiding this comment.
Same as above - debug! vs info! ?
| } | ||
|
|
||
| /// Processes a CDS response and updates its cluster view if needed. | ||
| pub(in crate::xds) async fn on_cluster_response(&mut self, response: DiscoveryResponse) { |
There was a problem hiding this comment.
I assume that these on_* methods are called via gRPC requests?
if so, I think it would be good to indicate that in the comments so that it's super clear where the invocations come from (especially as I know we're dealing with part code atm)
This adds the logic to processes responses from
the XDS server and sends back acknowlegements according to
the protocol. Implementation to talk to the server
isn't included.
Work on #10
This is a larger PR but the actual logic should be under 400LOC and a lot of that is wading through envoy's complicated structs to find the info we want. The rest is tests that includes handcrafting the same large and nested structs 😔 I can split it up even more if that makes sense instead.