Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toxiproxy resiliency testing #39

Merged
merged 6 commits into from
Sep 27, 2024
Merged

Conversation

mrattle
Copy link
Contributor

@mrattle mrattle commented Sep 19, 2024

TL;DR - What are you trying to accomplish?

This PR introduces Toxiproxy resiliency testing to test potential errors between the client and server at the crate level.

Details - How are you making this change? What are the effects of this change?

In these tests a clean_client refers to a client that runs without any toxics applied to it, i.e. it performs as you would expect a normal client to perform. A toxic_client refers to a client that is running through the Toxiproxy service to apply various toxics used to simulate different network issues that can be encountered in normal client/server communication.

  • The with_down() method is used to test the behaviour of the crate if the memcached server happens to be down / cannot be contacted.
  • The with_limit_data() method is used to test behaviour when the connection from client -> server (i.e. upstream) is interrupted and an incomplete request / command string is sent to the server, and when the connection from server -> client (i.e. downstream) is interrupted and an incomplete response is sent back from the server.

@mrattle mrattle added the #gsd:42391 Meta protocol in async-memcached label Sep 19, 2024
@mrattle mrattle self-assigned this Sep 19, 2024
@mrattle mrattle marked this pull request as ready for review September 19, 2024 02:41
@mrattle mrattle requested a review from a team as a code owner September 19, 2024 02:41
@mrattle mrattle force-pushed the mrattle/toxiproxy_resiliency_testing branch from 3a764af to d3d0c0a Compare September 19, 2024 19:24
@mrattle mrattle changed the title Mrattle/toxiproxy resiliency testing Add toxiproxy resiliency testing Sep 20, 2024
@mrattle mrattle force-pushed the mrattle/toxiproxy_resiliency_testing branch 5 times, most recently from 245dd54 to e03c466 Compare September 26, 2024 01:15
@mrattle mrattle force-pushed the mrattle/toxiproxy_resiliency_testing branch from 307682e to 6d93b6f Compare September 26, 2024 18:25
let values = vec!["value1", "value2", "value3"];
let kv: Vec<(&str, &str)> = keys.clone().into_iter().zip(values).collect();

let mut clean_client = rt.block_on(async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need both a clean client and a toxic client? The toxic client should work fine as long as you don't use down ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the toxic client for everything until you apply a toxic condition is fine, but in order to do the checks after using an actual toxic (i.e. a closure using with_down() or with_limit_data()) you need a clean client. The toxic client is still in a compromised state at that point and trying to use it for further actions leads to out-of-bounds errors like this:

thread 'tests::test_set_multi_errors_on_downstream_with_toxic_client_via_limit_data' panicked at /Users/mark/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bytes-1.7.1/src/bytes_mut.rs:394:9:
split_to out of bounds: 8 <= 1


let (proxies, toxic_local_addr) = create_proxies_and_configs();

let toxic_local_url = "tcp://".to_string() + &toxic_local_addr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of boiler plate code here, can we turn it into some helper methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout, I'll clean this up 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a refactoring pass on this. Reconfigured everything so that it's making proper use of environment variables, methods make more sense in the context that they're being use in (e.g. no more unnecessary vecs / proxies[0] since we're only ever making one proxy per test) tests should be more parsable and it should be easier to add more tests in the future.

@mrattle mrattle force-pushed the mrattle/toxiproxy_resiliency_testing branch from 83c1582 to efbb364 Compare September 26, 2024 22:16
@mrattle mrattle merged commit 49d001f into main Sep 27, 2024
4 checks passed
@mrattle mrattle deleted the mrattle/toxiproxy_resiliency_testing branch September 27, 2024 13:25
@mrattle mrattle removed the #gsd:42391 Meta protocol in async-memcached label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants