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

Add support for configurable envoy proxy concurrency #1277

Merged
merged 7 commits into from
Jun 16, 2022

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Jun 14, 2022

Changes proposed in this PR:

  • Adds a new flag to connect-inject deployment to take in a default concurrency value for every envoy sidecar.
  • Adds support for a new annotation on connect injected pods consul.hashicorp.com/consul-envoy-proxy-concurrency which will override the default for any given pod.

Notes to reviewers:

  • The default setting of 2 was chosen via a brief discussion with @blake and @david-yu but I'm open to suggestions if we have more information. Additional reading on this can be had at https://blog.envoyproxy.io/envoy-threading-model-a8d44b922310 where they encourage setting the value very low for sidecars and high for edge proxies.
  • I'm also open to suggestions on naming conventions for the various flags, I realize putting the word envoy into the name is a break from our typical conventions, but also sidecar in this context is muddy (consul-sidecar vs envoy proxy sidecar) and I wanted it to be clear that it was an envoy specific flag.
  • I chose not to just force the user to add --concurrency 42 to their connectInject.sidecarProxy.extraArgs because I thought it was a bit messy to have to edit strings in deployments vs just adding a new annotation to your deployment or set a default via helm.

How I've tested this PR:
unit tests + acceptance tests

How I expect reviewers to test this PR:
👀

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@kschoche kschoche self-assigned this Jun 14, 2022
@kschoche kschoche added the type/enhancement New feature or request label Jun 14, 2022
@kschoche kschoche marked this pull request as ready for review June 15, 2022 17:47
@kschoche kschoche requested review from a team, curtbushko and thisisnotashwin and removed request for a team June 15, 2022 17:47
@kschoche kschoche changed the title [wip] Add support for configurable envoy proxy concurrency Add support for configurable envoy proxy concurrency Jun 15, 2022
Copy link
Contributor

@curtbushko curtbushko 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! I only had some minor questions/nits but I think it all comes down to naming.

I prefer sidecar instead of envoy for consistency reasons. I think it is concurrency that is the word that makes it confusing as I immediately thought of replicas when I read the summary.

Would thread-concurrency make it clearer? -default-sidecar-proxy-thread-concurrency?

Naming is hard. 🥲

CHANGELOG.md Show resolved Hide resolved
control-plane/connect-inject/envoy_sidecar.go Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

NICE

@kschoche kschoche requested a review from curtbushko June 16, 2022 15:12
@kschoche kschoche force-pushed the configurable_envoy_concurrency branch from 04825a5 to c49dd14 Compare June 16, 2022 16:01
@kschoche kschoche merged commit 875ecb7 into main Jun 16, 2022
@kschoche kschoche deleted the configurable_envoy_concurrency branch June 16, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants