search

linkerd / linkerd2

Ultralight, security-first service mesh for Kubernetes. Main repo for Linkerd 2.x.
https://linkerd.io
Apache License 2.0
10.69k stars 1.28k forks source link

proxy: Consider using `tokio-test` #2802

Open hawkw opened 5 years ago

hawkw commented 5 years ago

The proxy currently has a number of tests that rely on having the thread sleep for an amount of time. Now that the tokio-test crate, which provides mocking utilities for tokio-timer, has been released, we might want to look into rewriting these tests to use mocked timers.

There may be other test utilities in tokio-test that can simplify other tests in the proxy test suite as well.

zaharidichev commented 5 years ago

@hawkw I am quite willing to take a stab at that. However I am facing an issue that is quite ambigious (to me at least). I tried adding the tokio-test crate and running one of the telemtry tests with a mock clock. However I do get a problem at some point indicating that some timer was dropped before it should be dropped:

ERROR 2019-05-20T16:27:03Z: linkerd2_proxy::app::errors: unexpected error: timer is shutdown

Any help with that one will be greatly appreaciated. I have the branch to reproduce that right here: https://github.com/linkerd/linkerd2-proxy/compare/master...zaharidichev:zd/tokio-test?expand=1

My client::get returns 502. Interestingly the metrics client does not error out.

hawkw commented 5 years ago

Hi @zaharidichev, I think I have an idea of what the problem is here. tokio-test's mock timer creates a new mock timer and sets it as the default. However, the proxy's integration tests (the ones that live in the tests/ dir) create whole proxies and run them, and the proxy constructs and manages its own runtime. The proxy's runtime constructs its own timer, so the mock timer is not used by the proxy.

In order to make the integration tests work with mock timers, we'd need to modify the Proxy struct to allow passing in a custom timer, and have the proxy use that timer for its runtime. We'd also want to make sure that all the test support machinery (the mock servers, etc) are also using the same mock timer.

Since this is probably a decent amount of work, I might start by rewriting any proxy unit tests (the ones in src/) that use timers1 to use the mock timer first, to learn how to use the mock timer API, and then change the proxy so that a mock timer can be passed in for the integration tests.

1It's possible there may not actually be any unit tests that use timers, though --- I can't remember OTTOMH. If that's the case we'll just have to start by making the necessary changes for the integration tests.

zaharidichev commented 5 years ago

@hawkw thanks a lot for the useful explanation. I will take a closer look and give this a shot.

hawkw commented 5 years ago

@zaharidichev do let me know if you have any questions about how to actually make this change!