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

Isolate shotovers tokio runtime in integration tests #912

Closed
wants to merge 5 commits into from

Conversation

rukai
Copy link
Member

@rukai rukai commented Nov 14, 2022

Makes integration tests more robust by isolating:

  • Shotover async runtime from test runtime
  • Shotover async runtime from other shotover runtimes created at the same time or afterwards

It also simplifies our test helper logic.

An immediate practical benefit is removing the ERROR shotover_proxy::observability: Metrics HTTP server failed: Failed to bind to 0.0.0.0:9001 error log clutter that occurs every time we create a second Shotover instance after destroying the previous shotover instance.

@rukai
Copy link
Member Author

rukai commented Nov 14, 2022

Oooh the redis_int_tests::cluster_dr failure actually reveals a bug in shotover that was hidden before.

@rukai
Copy link
Member Author

rukai commented Nov 23, 2022

These are fixes needed to land this:

@rukai rukai force-pushed the isolate_shotovers_tokio_runtime branch from 6c1c0af to 10f6024 Compare December 13, 2022 03:44
@rukai
Copy link
Member Author

rukai commented Dec 13, 2022

Although the plan is to eventually remove ShotoverManager, replacing it with ShotoverProcess, it makes sense to land this PR now so that in the meantime we can ensure we dont regress on any of the work that it took to make the shutdown process good enough to make the test pass.

@rukai rukai marked this pull request as ready for review December 13, 2022 06:13
@rukai
Copy link
Member Author

rukai commented Dec 21, 2022

Superceded by #976 and #975
They will provide isolation to the majority of tests, so point in landing this anymore.

@rukai rukai closed this Dec 21, 2022
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.

None yet

2 participants