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

Wait for proxy thread to terminate in integration tests #625

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

tustvold
Copy link
Contributor

Whilst implementing #601 I encountered non-determinism due to the way the integration tests don't wait for termination of the proxy thread. The changes in this PR make join_servers explicitly signal shutdown, then wait on a oneshot channel for the thread to signal draining has completed, before finally joining the thread. Simply joining the thread after signaling shutdown is insufficient as this will block the "primary" tokio runtime preventing graceful shutdown of HTTP2 connections, which rely on the clients running in the "primary" tokio runtime to be responsive.

Unfortunately the proxy appears to intentionally wait for TCP upstreams to terminate first, which requires explicit termination of the TCP client in these tests.

@tustvold tustvold requested a review from a team August 15, 2020 13:03
Signed-off-by: Raphael Taylor-Davies <r.taylordavies@googlemail.com>
@tustvold
Copy link
Contributor Author

CI seemed to take a rather long time, so ran some tests locally

With the change, and the tests already built

Executed in   16.26 secs   fish           external 
   usr time   64.59 secs   64.27 millis   64.52 secs 
   sys time    3.39 secs    9.85 millis    3.38 secs 

On main, with the tests already built

Executed in   16.75 secs   fish           external 
   usr time   64.57 secs  1010.00 micros   64.57 secs 
   sys time    3.64 secs  363.00 micros    3.64 secs 

So at least on my machine it doesn't seem to measurably impact the time the tests take to execute

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks!

@olix0r
Copy link
Member

olix0r commented Aug 18, 2020

Thanks @tustvold!

@olix0r olix0r merged commit 162ff8f into linkerd:main Aug 18, 2020
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Aug 26, 2020
This release includes internal changes to the service discovery system,
especially when discovering control plane components (like the
destination and identity controllers). Now, the proxy attempts to
balance requests across all pods in each control plane service. This
requires control plane changes to use "headless" services so that SRV
records are exposed. When the control plane services have a `clusterIP`
set, the proxy falls back to using normal A-record lookups.

---

* tracing: add richer verbose spans to http clients (linkerd/linkerd2-proxy#622)
* trace: update tracing dependencies (linkerd/linkerd2-proxy#623)
* Remove `Resolution` trait (linkerd/linkerd2-proxy#606)
* Update proxy-identity to edge-20.8.2 (linkerd/linkerd2-proxy#627)
* Add build arg for skipping identity wrapper (linkerd/linkerd2-proxy#624)
* Wait for proxy thread to terminate in integration tests (linkerd/linkerd2-proxy#625)
* Remove scrubbing for unused headers (linkerd/linkerd2-proxy#628)
* Split orig-proto tests out of discovery tests (linkerd/linkerd2-proxy#629)
* Re-enable outbound timeout test (linkerd/linkerd2-proxy#630)
* profiles: perform profile resolution for IP addresses (linkerd/linkerd2-proxy#626)
* Move resolve api to async-stream (linkerd/linkerd2-proxy#599)
* Decouple discovery buffering from endpoint conversion (linkerd/linkerd2-proxy#631)
* resolve: Add a Reset state (linkerd/linkerd2-proxy#633)
* resolve: Eagerly fail resolutions (linkerd/linkerd2-proxy#634)
* test: replace `net2` dependency with `socket2` (linkerd/linkerd2-proxy#635)
* dns: Run DNS resolutions on the main runtime (linkerd/linkerd2-proxy#637)
* Load balance requests to the control plane (linkerd/linkerd2-proxy#594)
* Unify control plane client construction (linkerd/linkerd2-proxy#638)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Aug 26, 2020
This release includes internal changes to the service discovery system,
especially when discovering control plane components (like the
destination and identity controllers). Now, the proxy attempts to
balance requests across all pods in each control plane service. This
requires control plane changes to use "headless" services so that SRV
records are exposed. When the control plane services have a `clusterIP`
set, the proxy falls back to using normal A-record lookups.

---

* tracing: add richer verbose spans to http clients (linkerd/linkerd2-proxy#622)
* trace: update tracing dependencies (linkerd/linkerd2-proxy#623)
* Remove `Resolution` trait (linkerd/linkerd2-proxy#606)
* Update proxy-identity to edge-20.8.2 (linkerd/linkerd2-proxy#627)
* Add build arg for skipping identity wrapper (linkerd/linkerd2-proxy#624)
* Wait for proxy thread to terminate in integration tests (linkerd/linkerd2-proxy#625)
* Remove scrubbing for unused headers (linkerd/linkerd2-proxy#628)
* Split orig-proto tests out of discovery tests (linkerd/linkerd2-proxy#629)
* Re-enable outbound timeout test (linkerd/linkerd2-proxy#630)
* profiles: perform profile resolution for IP addresses (linkerd/linkerd2-proxy#626)
* Move resolve api to async-stream (linkerd/linkerd2-proxy#599)
* Decouple discovery buffering from endpoint conversion (linkerd/linkerd2-proxy#631)
* resolve: Add a Reset state (linkerd/linkerd2-proxy#633)
* resolve: Eagerly fail resolutions (linkerd/linkerd2-proxy#634)
* test: replace `net2` dependency with `socket2` (linkerd/linkerd2-proxy#635)
* dns: Run DNS resolutions on the main runtime (linkerd/linkerd2-proxy#637)
* Load balance requests to the control plane (linkerd/linkerd2-proxy#594)
* Unify control plane client construction (linkerd/linkerd2-proxy#638)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants