http: Lazily build http/tcp stacks by olix0r · Pull Request #681 · linkerd/linkerd2-proxy

olix0r · 2020-09-29T19:00:49Z

Currently, the DetectHttp layer always builds both HTTP and TCP
stacks, even though only one is needed. Furthermore, we lose the
information about which protocol was detected--it's never passed into
the http stack (and instead must be inferred from individual requests).

This change modifies the DetectHttp layer to only build inner services
as needed, after the protocol has been detected.

Furthermore, outbound caching has been moved up from the tcp balancer to
wrap the HTTP detection layer. This was necessary so that the tcp
balancer could be built instantaneously as a NewService while still
being cloneable. This means that, for each outbound target ip:port, we
will reuse HTTP and TCP stacks.

However, HTTP routing remains dependent on per-request metadata. This is
expected to change in a followup.

Currently, the `DetectHttp` layer always builds both HTTP and TCP stacks, even though only one is needed. Furthermore, we lose the information about which protocol was detected--it's never passed into the http stack (and instead must be inferred from individual requests). This change modifies the `DetectHttp` layer to only build inner services as needed, after the protocol has been detected. Furthermore, outbound caching has been moved up from the tcp balancer to wrap the HTTP detection layer. This was necessary so that the tcp balancer could be built instantaneously as a NewService while still being cloneable. This means that, for each outbound target ip:port, we will reuse HTTP and TCP stacks. However, HTTP routing remains dependent on per-request metadata. This is expected to change in a followup.

hawkw

this change looks great to me --- i commented on a few minor nits.

hawkw · 2020-09-29T20:31:05Z

linkerd/app/outbound/src/endpoint.rs

 #[derive(Copy, Clone, Debug)]
 pub struct FromMetadata;

+#[derive(Clone, Debug, Eq, PartialEq, Hash)]


nit/TIOLI: i imagine these could also be Copy?

hawkw · 2020-09-29T20:41:42Z

linkerd/app/outbound/src/endpoint.rs

    pub concrete: HttpConcrete,
 }

+#[derive(Clone, Debug, Eq, PartialEq, Hash)]


this can also be Copy, if you wanted it to.

hawkw · 2020-09-29T20:43:02Z

linkerd/app/outbound/src/lib.rs

+            .push_on_response(
+                svc::layers()
+                    .push(tcp::balance::layer(EWMA_DEFAULT_RTT, EWMA_DECAY))
+                .push(svc::layer::mk(tcp::Forward::new))
+            )


nit: some kinda weird formatting going on here?

hawkw · 2020-09-29T22:54:54Z

linkerd/proxy/http/src/detect.rs

+    http1: Option<H::Service>,
+    h2: Option<H::Service>,


nit, doesn't actually matter: naming these http1 and h2 is kinda bugging me.

It corresponds to the Version variant

hawkw · 2020-09-29T22:58:48Z

linkerd/proxy/http/src/detect.rs

+                let http1 = if let Some(svc) = self.http1.clone() {
+                    svc
+                } else {
+                    let svc = self
+                        .new_http
+                        .new_service((HttpVersion::Http1, self.target.clone()));
+                    self.http1 = Some(svc.clone());
+                    svc
+                };


nit, take it or leave it: get_or_insert_with might be more concise here

Suggested change

let http1 = if let Some(svc) = self.http1.clone() {

svc

} else {

let svc = self

.new_http

.new_service((HttpVersion::Http1, self.target.clone()));

self.http1 = Some(svc.clone());

svc

};

let http1 = self

.http1

.get_or_insert_with(|| {

self

.new_http

.new_service((HttpVersion::Http1, self.target.clone()))

})

.clone();

(i tried to format the suggested code the way i imagine rustfmt would want it)

That doesn't work due to borrows on self.

hawkw · 2020-09-29T22:59:06Z

linkerd/proxy/http/src/detect.rs

+                let h2 = if let Some(svc) = self.h2.clone() {
+                    svc
+                } else {
+                    let svc = self
+                        .new_http
+                        .new_service((HttpVersion::H2, self.target.clone()));
+                    self.h2 = Some(svc.clone());
+                    svc
+                };


could use get_or_insert_with here as well.

That doesn't work due to borrows on self.

hawkw · 2020-09-29T22:59:17Z

linkerd/proxy/http/src/detect.rs

+                let tcp = if let Some(svc) = self.tcp.clone() {
+                    svc
+                } else {
+                    let svc = self.new_tcp.new_service(self.target.clone());
+                    self.tcp = Some(svc.clone());
+                    svc
+                };


...and here.

hawkw · 2020-09-29T23:00:44Z

linkerd/app/outbound/src/lib.rs

            )
-            .into_make_service()
-            .spawn_buffer(buffer_capacity)
-            .instrument(|_: &_| info_span!("tcp"))


nit: do we still want the instrument layer here?

the tcp scope is now set in detect-http. it's probably better to stop doing that and keep them in here

This release includes changes to TCP metrics to ensure that peer identities are encoded via the `client_id` and `server_id` labels. --- * outbound: Explicitly ignore the source address for tap (linkerd/linkerd2-proxy#680) * Update proxy-api and tonic (linkerd/linkerd2-proxy#682) * http: Lazily build http/tcp stacks (linkerd/linkerd2-proxy#681) * outbound: Remove required identity from HttpLogical (linkerd/linkerd2-proxy#683) * profiles: Expose the fully_qualified_name (linkerd/linkerd2-proxy#684) * request-filter: Support altering the request type (linkerd/linkerd2-proxy#685) * tracing: Set contexts in new_service/make_service (linkerd/linkerd2-proxy#686) * discover: Allow resolution streams to terminate (linkerd/linkerd2-proxy#689) * metrics: add peer identities to all TLS metric labels (linkerd/linkerd2-proxy#687) * outbound: Return a default endpoint on reject (linkerd/linkerd2-proxy#690) * Skip endpoint resolution when profile lookup is rejected (linkerd/linkerd2-proxy#691)

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([#681]) - **retry**: Add generic backoff utilities ([#685]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([#654]) the `Either` trait bounds are particularly impactful for us. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> * <tower-rs/tower#815> * <tower-rs/tower#817> * <tower-rs/tower#818> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <[email protected]>

this commit updates our tower dependency from 0.4 to 0.5. note that this commit does not affect the `tower-service` and `tower-layer` crates, reëxported by `tower` itself. the `Service<T>` trait and the closely related `Layer<S>` trait have not been changed. the `tower` crate's utilities have changed in various ways, some of particular note for the linkerd2 proxy. see these items, excerpted from the tower changelog: - **retry**: **Breaking Change** `retry::Policy::retry` now accepts `&mut Req` and `&mut Res` instead of the previous mutable versions. This increases the flexibility of the retry policy. To update, update your method signature to include `mut` for both parameters. ([#584]) - **retry**: **Breaking Change** Change Policy to accept &mut self ([#681]) - **retry**: **Breaking Change** `Budget` is now a trait. This allows end-users to implement their own budget and bucket implementations. ([#703]) - **util**: **Breaking Change** `Either::A` and `Either::B` have been renamed `Either::Left` and `Either::Right`, respectively. ([#637]) - **util**: **Breaking Change** `Either` now requires its two services to have the same error type. ([#637]) - **util**: **Breaking Change** `Either` no longer implemenmts `Future`. ([#637]) - **buffer**: **Breaking Change** `Buffer<S, Request>` is now generic over `Buffer<Request, S::Future>.` ([#654]) see: * <tower-rs/tower#584> * <tower-rs/tower#681> * <tower-rs/tower#703> * <tower-rs/tower#637> * <tower-rs/tower#654> the `Either` trait bounds are particularly impactful for us. because this runs counter to how we treat errors (skewing towards boxed errors, in general), we temporarily vendor a version of `Either` from the 0.4 release, whose variants have been renamed to match the 0.5 interface. updating to box the inner `A` and `B` services' errors, so we satiate the new `A::Error = B::Error` bounds, can be addressed as a follow-on. that's intentionally left as a separate change, due to the net size of our patchset between this branch and #3504. * <tower-rs/tower@v0.4.x...master> * <https://github.com/tower-rs/tower/blob/master/tower/CHANGELOG.md> this work is based upon #3504. for more information, see: * linkerd/linkerd2#8733 * #3504 Signed-off-by: katelyn martin <[email protected]> X-Ref: tower-rs/tower#815 X-Ref: tower-rs/tower#817 X-Ref: tower-rs/tower#818 X-Ref: tower-rs/tower#819

olix0r added 11 commits September 29, 2020 15:15

Lazily build an HTTP service per protocol variant

11e0278

ignore detected version on inbound

d5a48ef

wip

eb80798

wip

8f9b6d7

fixup target types

e885c71

Ignore source address on outbound tap

c1eeaf7

+failfast

fd60eec

Merge branch 'main' into ver/lazy-http

6adc1ca

fixup discovery buffering for the TCP stack

71fcfa8

touchup

61522cd

olix0r requested a review from a team September 29, 2020 19:00

olix0r added 2 commits September 29, 2020 19:39

fixup test

42c628a

Merge branch 'main' into ver/lazy-http

6bfdab7

hawkw approved these changes Sep 29, 2020

View reviewed changes

olix0r added 2 commits September 29, 2020 23:13

Extract tracing contexts from DetectHttp to the stacks

3180b69

fmt

ab08c67

olix0r merged commit b03ff81 into main Sep 29, 2020

olix0r deleted the ver/lazy-http branch September 29, 2020 23:28

olix0r mentioned this pull request Oct 2, 2020

proxy: v2.113.0 linkerd/linkerd2#5037

Merged

Comments

Conversation

olix0r commented Sep 29, 2020

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants