test: replace `profile_test!` macro with builder by hawkw · Pull Request #1705 · linkerd/linkerd2-proxy

hawkw · 2022-05-24T22:05:54Z

Currently, the service profiles integration test uses a very complex
declarative macro to generate test bodies. This approach has some
downsides:

All of the test code is contained inside a macro. This means that
rustfmt will not reformat it, and (in some cases) compilation
errors can be unclear.
The macro-generated code is relatively inflexible. This makes it
difficult to add new tests that need to do slightly different things.
Adding new configurations to the macro is complex. If we want to add
some new thing that can also be passed in to change the generated
code, we have to modify a bunch of macro match arms, which is
annoying and time-consuming.

This branch rewrites the profiles integration tests to use a
builder-style design instead of a macro. This way, the repetitive
boilerplate necessary to set up the tests is still factored out, and can
still be overridden by passing in different parameters to the builder,
but instead of having a giant macro generate the test, the test itself
is just normal Rust code.

The builder spins up the test proxy and servers, and returns the test
client and metrics client to the test itself, and the test can now do
whatever it wants with them, rather than having to pass bits of test
code into a macro. This is much nicer, because the ests are just normal
tests now. Additionally, the builder can accept an arbitrary server
configuration, which will be necessary for some new tests I'm working on
in a separate branch.

I've also changed the profiles integration test so that most of the
tests are run with both HTTP/1 and HTTP/2 clients and servers. This is
still accomplished using macros, but now, the macro is much simpler. The
tests themselves are factored out into functions that take either an
HTTP/1 or HTTP/2 server, and all the macro does is generate wrapper
functions that call the actual test function with the appropriate server
version. This way, the actual test code is never inside of a macro
expansion.

I'd like to change more of the integration tests to work similarly to
these, removing other cases where a macro is used to generate test
boilerplate, and replacing it with a builder. Also, and more importantly,
this change is necessary for some additional tests I'll be adding in a
separate PR.

Signed-off-by: Eliza Weisman <[email protected]>

olix0r · 2022-05-25T05:21:38Z

linkerd/app/integration/src/tests/profiles.rs

+            let counter = AtomicUsize::new(0);
+            let counter2 = AtomicUsize::new(0);
+            let counter3 = AtomicUsize::new(0);


I know this isn't new code but it's a bit weird that these endpoints alternate between failure and success without any explicit control

yeah, i also don't love this, but i didn't want to change the actual test behavior in this PR. we could maybe do something where the server has a mpsc in it that lets the test code say "do this, and then do this", but that's more complicated...

linkerd/app/integration/src/tests/profiles.rs

Signed-off-by: Eliza Weisman <[email protected]>

Currently, the service discovery integration tests are generated using a giant macro that expands to all of the different tests. The macro is used to run the tests with both HTTP/1 (with and without absolute URIs) and HTTP/2 clients/servers. This approach has some downsides: 1. All of the test code is contained inside a macro. This means that `rustfmt` will not reformat it, and (in some cases) compilation errors can be unclear. 2. The macro-generated code is relatively inflexible. This makes it difficult to add new tests that need to do slightly different things. 3. Adding new configurations to the macro is complex. If we want to add some new thing that can also be passed in to change the generated code, we have to modify a bunch of macro match arms, which is annoying and time-consuming. This PR changes these tests similarly to the way #1705 modified the profiles integration tests. Rather than generating the tests themselves inside the macro, we define version-agnostic test bodies in a `cross_version` module, which accept the HTTP version configuration as an argument. Now, a much simpler macro is used to just generate simple test wrappers that call into the cross-version test definitions. This means that none of the actual test code is inside of a macro expansion. Depends on #1705 Signed-off-by: Eliza Weisman <[email protected]>

This release includes only minor internal changes and dependency updates. --- * build(deps): bump once_cell from 1.11.0 to 1.12.0 (linkerd/linkerd2-proxy#1701) * build(deps): bump tj-actions/changed-files from 20.1 to 20.2 (linkerd/linkerd2-proxy#1700) * Shellscript housekeeping (linkerd/linkerd2-proxy#1702) * Update README to mention just and devcontainers (linkerd/linkerd2-proxy#1703) * build(deps): bump jemallocator from 0.3.2 to 0.5.0 (linkerd/linkerd2-proxy#1708) * build(deps): bump prost from 0.10.3 to 0.10.4 (linkerd/linkerd2-proxy#1710) * build(deps): bump prost-build from 0.10.3 to 0.10.4 (linkerd/linkerd2-proxy#1709) * ci: Lint markdown files (linkerd/linkerd2-proxy#1707) * test: replace `profile_test!` macro with builder (linkerd/linkerd2-proxy#1705) * build(deps): bump tj-actions/changed-files from 20.2 to 21 (linkerd/linkerd2-proxy#1712) * dev: Fix the `just docker` recipe (linkerd/linkerd2-proxy#1713) * build(deps): bump prettyplease from 0.1.10 to 0.1.11 (linkerd/linkerd2-proxy#1714) * justfile: generate a default docker tag name (linkerd/linkerd2-proxy#1716) * build(deps): bump tj-actions/changed-files from 21 to 22.2 (linkerd/linkerd2-proxy#1727) * build(deps): bump petgraph from 0.6.1 to 0.6.2 (linkerd/linkerd2-proxy#1717) * build(deps): bump clang-sys from 1.3.2 to 1.3.3 (linkerd/linkerd2-proxy#1718) * build(deps): bump flate2 from 1.0.23 to 1.0.24 (linkerd/linkerd2-proxy#1719) * build(deps): bump hyper from 0.14.18 to 0.14.19 (linkerd/linkerd2-proxy#1720) * build(deps): bump miniz_oxide from 0.5.1 to 0.5.3 (linkerd/linkerd2-proxy#1722) * build(deps): bump bumpalo from 3.9.1 to 3.10.0 (linkerd/linkerd2-proxy#1728) * build(deps): bump async-trait from 0.1.53 to 0.1.56 (linkerd/linkerd2-proxy#1729) * build(deps): bump parking_lot from 0.12.0 to 0.12.1 (linkerd/linkerd2-proxy#1730) * build(deps): bump indexmap from 1.7.0 to 1.8.2 (linkerd/linkerd2-proxy#1732) * http-box: add `EraseResponse` middleware (linkerd/linkerd2-proxy#1723) * test: don't generate discovery tests in a macro (linkerd/linkerd2-proxy#1711) * stack: add `ProxyService` for composing a `Proxy` with a `Service` (linkerd/linkerd2-proxy#1726) * build(deps): bump tokio-stream from 0.1.8 to 0.1.9 (linkerd/linkerd2-proxy#1733) * build(deps): bump tokio-macros from 1.7.0 to 1.8.0 (linkerd/linkerd2-proxy#1736) * build(deps): bump tokio from 1.18.2 to 1.19.2 (linkerd/linkerd2-proxy#1737) * Add 'group' labels to describe policy resources (linkerd/linkerd2-proxy#1738) * build(deps): bump tokio-util from 0.7.2 to 0.7.3 (linkerd/linkerd2-proxy#1735) * Move policy protobuf handling into the `server-policy` crate (linkerd/linkerd2-proxy#1739) * build(deps): bump http from 0.2.7 to 0.2.8 (linkerd/linkerd2-proxy#1740) * Implement `Deref` for address newtypes (linkerd/linkerd2-proxy#1741) * Rename `Route*` to `ProfileRoute*` (linkerd/linkerd2-proxy#1742) * Rename target types from Route to ProfileRoute (linkerd/linkerd2-proxy#1743) Signed-off-by: Oliver Gould <[email protected]>

This release includes only minor internal changes and dependency updates. --- * build(deps): bump once_cell from 1.11.0 to 1.12.0 (linkerd/linkerd2-proxy#1701) * build(deps): bump tj-actions/changed-files from 20.1 to 20.2 (linkerd/linkerd2-proxy#1700) * Shellscript housekeeping (linkerd/linkerd2-proxy#1702) * Update README to mention just and devcontainers (linkerd/linkerd2-proxy#1703) * build(deps): bump jemallocator from 0.3.2 to 0.5.0 (linkerd/linkerd2-proxy#1708) * build(deps): bump prost from 0.10.3 to 0.10.4 (linkerd/linkerd2-proxy#1710) * build(deps): bump prost-build from 0.10.3 to 0.10.4 (linkerd/linkerd2-proxy#1709) * ci: Lint markdown files (linkerd/linkerd2-proxy#1707) * test: replace `profile_test!` macro with builder (linkerd/linkerd2-proxy#1705) * build(deps): bump tj-actions/changed-files from 20.2 to 21 (linkerd/linkerd2-proxy#1712) * dev: Fix the `just docker` recipe (linkerd/linkerd2-proxy#1713) * build(deps): bump prettyplease from 0.1.10 to 0.1.11 (linkerd/linkerd2-proxy#1714) * justfile: generate a default docker tag name (linkerd/linkerd2-proxy#1716) * build(deps): bump tj-actions/changed-files from 21 to 22.2 (linkerd/linkerd2-proxy#1727) * build(deps): bump petgraph from 0.6.1 to 0.6.2 (linkerd/linkerd2-proxy#1717) * build(deps): bump clang-sys from 1.3.2 to 1.3.3 (linkerd/linkerd2-proxy#1718) * build(deps): bump flate2 from 1.0.23 to 1.0.24 (linkerd/linkerd2-proxy#1719) * build(deps): bump hyper from 0.14.18 to 0.14.19 (linkerd/linkerd2-proxy#1720) * build(deps): bump miniz_oxide from 0.5.1 to 0.5.3 (linkerd/linkerd2-proxy#1722) * build(deps): bump bumpalo from 3.9.1 to 3.10.0 (linkerd/linkerd2-proxy#1728) * build(deps): bump async-trait from 0.1.53 to 0.1.56 (linkerd/linkerd2-proxy#1729) * build(deps): bump parking_lot from 0.12.0 to 0.12.1 (linkerd/linkerd2-proxy#1730) * build(deps): bump indexmap from 1.7.0 to 1.8.2 (linkerd/linkerd2-proxy#1732) * http-box: add `EraseResponse` middleware (linkerd/linkerd2-proxy#1723) * test: don't generate discovery tests in a macro (linkerd/linkerd2-proxy#1711) * stack: add `ProxyService` for composing a `Proxy` with a `Service` (linkerd/linkerd2-proxy#1726) * build(deps): bump tokio-stream from 0.1.8 to 0.1.9 (linkerd/linkerd2-proxy#1733) * build(deps): bump tokio-macros from 1.7.0 to 1.8.0 (linkerd/linkerd2-proxy#1736) * build(deps): bump tokio from 1.18.2 to 1.19.2 (linkerd/linkerd2-proxy#1737) * Add 'group' labels to describe policy resources (linkerd/linkerd2-proxy#1738) * build(deps): bump tokio-util from 0.7.2 to 0.7.3 (linkerd/linkerd2-proxy#1735) * Move policy protobuf handling into the `server-policy` crate (linkerd/linkerd2-proxy#1739) * build(deps): bump http from 0.2.7 to 0.2.8 (linkerd/linkerd2-proxy#1740) * Implement `Deref` for address newtypes (linkerd/linkerd2-proxy#1741) * Rename `Route*` to `ProfileRoute*` (linkerd/linkerd2-proxy#1742) * Rename target types from Route to ProfileRoute (linkerd/linkerd2-proxy#1743) Signed-off-by: Oliver Gould <[email protected]> Signed-off-by: zhlsunshine <[email protected]>

Currently, gRPC responses are classified for metrics based on the value of the `grpc-status` header in either the initial `HEADERS` frame *or* in the `TRAILERS` frame that terminates the response body. However, this is *not* the case for retries. Because we don't want to buffer the entire response body before forwarding it to the client (introducing latency, and potentially breaking client behavior in the case of long-running streams), we make retry decisions for gRPC based only on `grpc-status` in the initial `HEADERS` frame. Unfortunately, some gRPC implementations (such as Akka) will send the `grpc-status` header in a `TRAILERS` frame even when the body is empty (see linkerd/linkerd2#7701). In that case, Linkerd won't retry the request, even though it wouldn't actually have to wait to buffer a long body before making a retry decision. This branch resolves this issue by changing the retry implementation so that the proxy will wait for a single additional frame after the first `HEADERS` frame before making a retry decision. If the next frame is a `TRAILERS` frame that terminates the response stream, we can include the value of the `grpc-status` in that `TRAILERS` frame in the retry decision. Otherwise, if the next frame is a `DATA` frame, we buffer only a single frame, give up on trying to retry, and forward that frame to the client, followed by the rest of the body. Unfortunately, this change is a bit complicated, as it was necessary to introduce the ability for retries to modify the response body as well as the request body. This then implies that we need to be able to erase response body types as well, when they may differ based on whether or not retries are enabled for a route. I tried a number of different ways of structuring this change, such as passing an additional `Layer` into the `Retry` service that wraps the service constructed for retries, but this was kind of a mess. Ultimately, I felt like the cleanest way of doing this was changing the `linkerd_retry::PrepareRequest` trait to a `PrepareRetry` trait that can also modify response bodies. I'm open to suggestions for a nicer way of structuing this change. I also considered the alternative of giving up on `tower::retry` entirely and writing our own retry service from scratch, but I wanted to try to avoid that if it was possible. Depends on #1705 and #1726. Fixes linkerd/linkerd2#7701. Signed-off-by: Eliza Weisman <[email protected]>

hawkw added 3 commits May 24, 2022 15:04

test: replace profiles test macro with builder

bbc95dd

Signed-off-by: Eliza Weisman <[email protected]>

run all (applicable) tests for h1 and h2

1d06cfd

Signed-off-by: Eliza Weisman <[email protected]>

warning cleanup

3165ce1

Signed-off-by: Eliza Weisman <[email protected]>

hawkw added the rust Pull requests that update Rust code label May 24, 2022

hawkw self-assigned this May 24, 2022

hawkw requested a review from a team as a code owner May 24, 2022 22:05

hawkw mentioned this pull request May 24, 2022

retry gRPC requests are immediately terminated by trailers #1706

Merged

olix0r reviewed May 25, 2022

View reviewed changes

review feedback

9df578a

Signed-off-by: Eliza Weisman <[email protected]>

hawkw requested a review from olix0r May 25, 2022 16:07

olix0r approved these changes May 25, 2022

View reviewed changes

hawkw mentioned this pull request May 25, 2022

test: don't generate discovery tests in a macro #1711

Merged

hawkw merged commit 55bfdac into main May 25, 2022

hawkw deleted the eliza/profile-test-builder branch May 25, 2022 19:00

olix0r mentioned this pull request Jun 9, 2022

proxy: v2.180.0 linkerd/linkerd2#8640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: replace `profile_test!` macro with builder#1705

test: replace `profile_test!` macro with builder#1705
hawkw merged 4 commits intomainfrom
eliza/profile-test-builder

hawkw commented May 24, 2022

Uh oh!

olix0r May 25, 2022

Uh oh!

hawkw May 25, 2022

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hawkw commented May 24, 2022

Uh oh!

olix0r May 25, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw May 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants