Refactor ClusterMap #785

XAMPPRocky · 2023-09-10T18:17:36Z

This PR refactors our internal storage of transmission of clusters over xDS to be more focused on endpoints rather than mimicking Envoys layout. We weren't using basically any of it, and having all of those extra fields was creating a lot of extra bytes and made the encoding logic more complicated.

This PR moves to using a simpler map of locality -> endpoints, as that was the only part we were using, and uses our own proto types for xDS discovery. We still use the core discovery request and response wrapper, it's just the types inside are now Quilkin types rather than envoy types.

We don't have a xDS benchmark, but I wrote a small micro benchmark of just the time and size of discovery responses, and there's a significant improvement to using this implementation, being roughly 30%-250% faster depending on the endpoint amounts (being faster for smaller amounts of endpoints where the message size overhead is more noticeable), and being 12% smaller.

Old

endpoints	time (seconds)	size (bytes)
0	0.001	53
100	0.005	5537
1000	0.028	55013
10000	0.117	550013
65535	0.721	3653593

New

endpoints	time (seconds)	size (bytes)
0	0.001	53
100	0.002	4910
1000	0.011	48984
10000	0.085	489984
65535	0.539	3260352

Diff

endpoints	time (faster)	size (smaller)
100	2.5x	12%
1000	2.5x	12%
10000	37%	12%
65535	33%	12%

#[test]
fn protobuf_test() {
    let measure = |count: u16| {
        let config = Config::default();
        if count != 0 {
            config.clusters.write().insert_default(crate::endpoint::LocalityEndpoints::from((0..count).map(|i| Endpoint::from((std::net::Ipv4Addr::UNSPECIFIED, i))).collect::<Vec<_>>()));
        }
        let response_creation = std::time::Instant::now();
        let response = config.discovery_request("", ResourceType::Cluster, &[]).unwrap();
        let time = response_creation.elapsed().as_secs_f64();
        let size = crate::prost::encode(&response).unwrap().len();
        println!("count: {count}, time: {time}, size: {size}");
    };

    (measure)(0);
    (measure)(100);
    (measure)(1000);
    (measure)(10000);
    (measure)(u16::MAX);
    panic!();
}

github-actions · 2023-09-10T18:17:55Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-10T18:53:43Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-10T19:03:50Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-10T19:15:18Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

markmandel · 2023-09-12T23:06:22Z

Lemme know if you want me to take a look, but also reminding you that we have:

quilkin/agones/src/lib.rs

Lines 418 to 425 in d8e93b6

    
           // Output the events and logs for each pod that matches this label selector. 
        
           // Useful for determining why something is failing in CI without having to run a cluster. 
        
           // Requires quilkin::test_utils::enable_log("agones=debug"); to enable debug logging within 
        
           // the test 
        
           pub async fn debug_pods(client: &Client, labels: String) { 
        
               debug!(labels, "🪓 Debug output for Selector"); 
        
               let pods: Api<Pod> = client.namespaced_api(); 
        
               let events: Api<Event> = client.namespaced_api();

In case you want to try debugging in here.

github-actions · 2023-09-13T06:59:00Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-13T07:31:56Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

XAMPPRocky · 2023-09-13T08:39:01Z

In case you want to try debugging in here.

I added it but it doesn't seem to be working? 🤔

markmandel · 2023-09-13T23:47:44Z

agones/src/xds.rs

@@ -57,6 +57,7 @@ mod tests {
    /// for this test, we should only have single Agones integration test in this suite, since they
    /// could easily collide with each other.
    async fn agones_token_router() {
+        quilkin::test_utils::enable_log("agones=debug");


Depending on what aspect you want to debug, you will nee to also include a call to debug_pods(client: &Client, labels: String) to actually get logs and events about those pods.

For example, debug_pods(client, "role=proxy") will output debug information about the proxy instances. You can see the labels they have here:

quilkin/agones/src/xds.rs

Line 374 in c0d5ca5

let labels = BTreeMap::from([("role".to_string(), "proxy".to_string())]);

(This would be good to add to the doc on debug_pods)

quilkin/agones/src/lib.rs

Lines 418 to 425 in d8e93b6

// Output the events and logs for each pod that matches this label selector.

// Useful for determining why something is failing in CI without having to run a cluster.

// Requires quilkin::test_utils::enable_log("agones=debug"); to enable debug logging within

// the test

pub async fn debug_pods(client: &Client, labels: String) {

debug!(labels, "🪓 Debug output for Selector");

let pods: Api<Pod> = client.namespaced_api();

let events: Api<Event> = client.namespaced_api();

)

Also looking at the build log, it's not failing because something failed to come up (as far as I can tell anyway), it's failing here:

quilkin/agones/src/xds.rs

Line 201 in c0d5ca5

assert!(failed, "Packet should have failed");

So the existing debug_pod statements aren't going to help in this moment - they have to be conditionally put in.

github-actions · 2023-09-18T15:24:59Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

markmandel · 2023-09-18T22:57:38Z

agones/src/xds.rs

@@ -181,7 +182,7 @@ filters:
            .as_mut()
            .map(|annotations| annotations.remove(token_key).unwrap());
        gameservers.replace(name.as_str(), &pp, &gs).await.unwrap();
-
+        debug_pods(client, "role=proxy");


My suggestion might be to change line 201 from

assert!(failed, "Packet should have failed");

to something like:

if failed { // proxies debug_pods(client, "role=proxy"); // xds server debug_pods(client, "role=xds"); panic!("Packet should have failed"); }

Then the condition can always stay in the test in case of future debugging requirements.

github-actions · 2023-09-19T07:33:11Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-19T08:02:33Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

markmandel · 2023-09-19T19:47:03Z

Ah, parse_ipv6_endpoint has localities in the config still

quilkin/src/config.rs

Lines 412 to 424 in de49889

    
           fn parse_ipv6_endpoint() { 
        
               let config: Config = serde_json::from_value(json!({ 
        
                   "version": "v1alpha1", 
        
                   "clusters":{ 
        
                       "default":{ 
        
                           "localities": [{ 
        
                               "endpoints": [{ 
        
                                   "address": "[2345:0425:2CA1:0000:0000:0567:5673:24b5]:25999" 
        
                               }], 
        
                           }] 
        
                       } 
        
                   } 
        
               }))

github-actions · 2023-09-20T10:15:49Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T10:41:17Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T11:16:30Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T12:37:03Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T13:14:09Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T13:33:09Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T13:47:52Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T14:29:45Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions · 2023-09-20T17:54:33Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

quilkin-bot · 2023-09-20T18:13:23Z

Build Succeeded 🥳

Build Id: a2976a9a-4d56-4064-9187-c4149131653b

The following development images have been built, and will exist for the next 30 days:

us-docker.pkg.dev/quilkin/ci/quilkin:0.7.0-dev-8e4fdf3

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/785/head:pr_785 && git checkout pr_785
cargo build

XAMPPRocky requested review from markmandel, No-ops, rezvaneh and vallfors as code owners September 10, 2023 18:17

github-actions bot added the size/xl label Sep 10, 2023

XAMPPRocky force-pushed the ep/refactor-cluster-map branch from af7ec10 to fb10779 Compare September 10, 2023 18:53

XAMPPRocky force-pushed the ep/refactor-cluster-map branch from fb10779 to 380f3f0 Compare September 10, 2023 19:03

XAMPPRocky force-pushed the ep/refactor-cluster-map branch from 380f3f0 to cf5acd4 Compare September 10, 2023 19:15

markmandel reviewed Sep 13, 2023

View reviewed changes

markmandel reviewed Sep 18, 2023

View reviewed changes

XAMPPRocky force-pushed the ep/refactor-cluster-map branch from 6a156e1 to 935558d Compare September 19, 2023 07:32

Refactor ClusterMap

c2400bb

XAMPPRocky force-pushed the ep/refactor-cluster-map branch from 1f4d587 to c2400bb Compare September 20, 2023 14:29

XAMPPRocky enabled auto-merge (squash) September 20, 2023 14:49

markmandel approved these changes Sep 20, 2023

View reviewed changes

markmandel disabled auto-merge September 20, 2023 17:54

markmandel enabled auto-merge (squash) September 20, 2023 17:54

Merge branch 'main' into ep/refactor-cluster-map

8e4fdf3

markmandel merged commit 1d2bec6 into main Sep 20, 2023
5 checks passed

markmandel deleted the ep/refactor-cluster-map branch September 20, 2023 18:28

markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ClusterMap #785

Refactor ClusterMap #785

XAMPPRocky commented Sep 10, 2023 •

edited

Loading

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

markmandel commented Sep 12, 2023

github-actions bot commented Sep 13, 2023

github-actions bot commented Sep 13, 2023

XAMPPRocky commented Sep 13, 2023

markmandel Sep 13, 2023

markmandel Sep 13, 2023

github-actions bot commented Sep 18, 2023

markmandel Sep 18, 2023

XAMPPRocky Sep 19, 2023

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

markmandel commented Sep 19, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

quilkin-bot commented Sep 20, 2023

	// Output the events and logs for each pod that matches this label selector.
	// Useful for determining why something is failing in CI without having to run a cluster.
	// Requires quilkin::test_utils::enable_log("agones=debug"); to enable debug logging within
	// the test
	pub async fn debug_pods(client: &Client, labels: String) {
	debug!(labels, "🪓 Debug output for Selector");
	let pods: Api<Pod> = client.namespaced_api();
	let events: Api<Event> = client.namespaced_api();

Refactor ClusterMap #785

Refactor ClusterMap #785

Conversation

XAMPPRocky commented Sep 10, 2023 • edited Loading

Old

New

Diff

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

github-actions bot commented Sep 10, 2023

markmandel commented Sep 12, 2023

github-actions bot commented Sep 13, 2023

github-actions bot commented Sep 13, 2023

XAMPPRocky commented Sep 13, 2023

markmandel Sep 13, 2023

Choose a reason for hiding this comment

markmandel Sep 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 18, 2023

markmandel Sep 18, 2023

Choose a reason for hiding this comment

XAMPPRocky Sep 19, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

markmandel commented Sep 19, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

github-actions bot commented Sep 20, 2023

quilkin-bot commented Sep 20, 2023

XAMPPRocky commented Sep 10, 2023 •

edited

Loading