Skip to content

Conversation

@olix0r
Copy link
Member

@olix0r olix0r commented Sep 18, 2020

There's no functional change here, just some general cleanup
after recent changes, including:

  • The TCP balancer is no longer responsible for fallback
    forwarding. This makes the server responsible for all TCP
    forwarding. The metrics have been removed from the TCP
    balancer stack (for now).
  • Loop detection is moved to the connect stack, rather than
    the accept stack.
  • The TCP balancer tests have been simplified somewhat.
  • tcp::Forward no longer has a maker. OnReponse does
    the job just fine.

@olix0r olix0r requested a review from a team September 18, 2020 03:58
@hawkw
Copy link
Contributor

hawkw commented Sep 18, 2020

this will probably conflict with #658, so whoever merges second is gonna have to figure that out

}

/// Constructs a TCP load balancer.
pub fn build_tcp_balance<C, E, I>(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any actual change here, or did this just move around?

Copy link
Member Author

Choose a reason for hiding this comment

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

just moved to be next to tcp_connect

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like some of the parameters were also removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, fallback was extracted from tcp_balance c18b0ae

Comment on lines +602 to +604
fn is_loop(err: &(dyn std::error::Error + 'static)) -> bool {
err.is::<prevent_loop::LoopPrevented>() || err.source().map(is_loop).unwrap_or(false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/tioli: is_discovery_rejected has a debug! event in it, should this as well?

bind: listen::Bind::new(SocketAddr::new(LOCALHOST.into(), 0), None)
.with_orig_dst_addr(orig_dst.into()),
h2_settings,
h2_settings: h2::Settings::default(),
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this Default impl returns different settings than the actual default values defined in env.rs; IIRC it's derived and these are both None.

Copy link
Member Author

Choose a reason for hiding this comment

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

These settings don't matter for tests, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it's not a big deal, i just thought it was better for this to match the actual defaults everywhere, so that we don't go to write a test that does depend on some setting and get surprising results. not a blocker though.

svc::layers()
.push_failfast(dispatch_timeout)
.push_spawn_buffer_with_idle_timeout(buffer_capacity, cache_max_idle_age)
.push(metrics.stack.layer(stack_labels("tcp"))),
Copy link
Contributor

Choose a reason for hiding this comment

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

what happened to the TCP metrics layer? did that get misplaced, or is it added somewhere else that i'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mentioned in the PR description

The metrics have been removed from the TCP balancer stack (for now).

These are just stack metrics (for our own debugging, really). Will probably reintroduce this after the caching stops moving around.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yup, i can't read. carry on!

Copy link
Contributor

@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.

lgtm!

}

/// Constructs a TCP load balancer.
pub fn build_tcp_balance<C, E, I>(
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like some of the parameters were also removed?

svc::layers()
.push_failfast(dispatch_timeout)
.push_spawn_buffer_with_idle_timeout(buffer_capacity, cache_max_idle_age)
.push(metrics.stack.layer(stack_labels("tcp"))),
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yup, i can't read. carry on!

@olix0r olix0r merged commit e06982b into main Sep 18, 2020
@olix0r olix0r deleted the ver/tcp-bal-cleanup branch September 18, 2020 20:08
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Sep 18, 2020
This release fixes a recent regression in multicluster gateway
configurations that would forbid inbound gateway traffic. It also fixes
URI normalization for orig-proto-upgrade requests that do not include a
`Host` header.

---

* http: Simplify stacks and target types (linkerd/linkerd2-proxy#656)
* Make SkipDetect more generic as stack::MakeSwitch (linkerd/linkerd2-proxy#657)
* introduce tests for isolated services (linkerd/linkerd2-proxy#655)
* http: Put normalize_uri back on the stack (linkerd/linkerd2-proxy#659)
* inbound: Apply loop detection on the connect stack (linkerd/linkerd2-proxy#660)
* tracing: Elide redundant info in tracing contexts (linkerd/linkerd2-proxy#661)
* outbound: Reorganize outbound stacks (linkerd/linkerd2-proxy#662)
* app: Decouple stacks from listeners (linkerd/linkerd2-proxy#663)
* inbound: Split HTTP detection stack from TLS (linkerd/linkerd2-proxy#664)
* integration: Bundle tests in src (linkerd/linkerd2-proxy#665)
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Sep 19, 2020
This release fixes a recent regression in multicluster gateway
configurations that would forbid inbound gateway traffic. It also fixes
URI normalization for orig-proto-upgrade requests that do not include a
`Host` header.

---

* http: Simplify stacks and target types (linkerd/linkerd2-proxy#656)
* Make SkipDetect more generic as stack::MakeSwitch (linkerd/linkerd2-proxy#657)
* introduce tests for isolated services (linkerd/linkerd2-proxy#655)
* http: Put normalize_uri back on the stack (linkerd/linkerd2-proxy#659)
* inbound: Apply loop detection on the connect stack (linkerd/linkerd2-proxy#660)
* tracing: Elide redundant info in tracing contexts (linkerd/linkerd2-proxy#661)
* outbound: Reorganize outbound stacks (linkerd/linkerd2-proxy#662)
* app: Decouple stacks from listeners (linkerd/linkerd2-proxy#663)
* inbound: Split HTTP detection stack from TLS (linkerd/linkerd2-proxy#664)
* integration: Bundle tests in src (linkerd/linkerd2-proxy#665)
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.

3 participants