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.

Sign up for GitHub

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

Jump to bottom

Lower the per connection buffer to 64k at the listener level #1407

Closed
davecheney opened this issue Aug 29, 2019 · 5 comments · Fixed by #5513
Closed

Lower the per connection buffer to 64k at the listener level #1407

davecheney opened this issue Aug 29, 2019 · 5 comments · Fixed by #5513
Labels
blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@davecheney
Copy link
Contributor

Envoy assigns a 1Mb buffer to to each incoming connection. It is not clear if it consumes the whole 1Mb in one go, or that is a upper limit.

Either way, I think this should be lowered to something like 64.

https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/lds.proto#envoy-api-field-listener-per-connection-buffer-limit-bytes
@davecheney davecheney added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 29, 2019
@davecheney davecheney modified the milestones: 1.0.0-beta.1, 1.0.0-rc.1 Aug 29, 2019
@davecheney davecheney modified the milestones: 1.0.0-rc.1, 1.0.0-rc.2 Sep 29, 2019
@davecheney davecheney added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Oct 11, 2019
@youngnick youngnick mentioned this issue Oct 21, 2019
Closed
@youngnick
Copy link
Member

In #1375, this is recommended to be 32k instead. Updating title accordingly.

@youngnick youngnick changed the title Consider lowering the per connection buffer to 64k Lower the per connection buffer to 64k at the listener level Oct 21, 2019
@jpeach jpeach self-assigned this Oct 22, 2019
jpeach added a commit to jpeach/contour that referenced this issue Oct 22, 2019
@jpeach
internal/envoy: default the cluster connection buffer to 32K
74c54db 
Introduce a `DefaultCluster()` convenience API that returns a Envoy
`v2.Cluster` struct that has only the Contour internal default
fields initialized. This gives us a single place where clusters get
default settings.

Add the 32K default for `per_connection_buffer_limit_bytes`.

Update all the tests to merge their Cluster spec into the default
Cluster struct. This makes the tests insensitive to changes in the
envoy Cluster defaults.

This fixes projectcontour#1407.

Signed-off-by: James Peach <jpeach@vmware.com>
@jpeach jpeach mentioned this issue Oct 22, 2019
Closed
@davecheney
Copy link
Contributor Author

Based on #1797 (review) there's a good change that this setting doesn't do what I thought it did. Moving to the backlog for more investigation.

@davecheney davecheney modified the milestones: 1.0.0-rc.2, Backlog Oct 25, 2019
@davecheney davecheney added blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. and removed good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Oct 25, 2019
@skriss skriss removed this from the Backlog milestone Jul 25, 2022
@BerGer23
Copy link

BerGer23 commented Mar 20, 2023 •
edited
Loading

Hi,
I am in the process of reviewing the Envoy configuration Contour generates for us.

Looks like one of the differences between Contour Envoy configuration and the suggestions from the Envoy team for running on the edge is the lack of defining this buffer limit for Listener & Cluster.

Searching for this led me to this ticket here.

  1. Is not overriding the Envoy default done intentionally by Contour?
  2. Since this ticket has been around for a while, could anybody tell me what's the latest on this?

Thanks in advance folks!

@skriss
Copy link
Member

skriss commented Mar 21, 2023

@BerGer23 I think it makes sense to look into using Envoy's recommended settings here, but we also need to ensure we understand the impact on existing users of changing the settings. One possible approach here would be to change the default setting, but also expose a tuneable to allow users to modify the setting to something else if needed.

Have you done/are you able to do any testing around changing this setting, and observing any perf or behavior changes?

@skriss skriss mentioned this issue Apr 10, 2023
Closed
1 task
@BerGer23
Copy link

Hi @skriss, sorry about the late answer.
I'm afraid I don't have any constructive input on this currently, neither on the performance nor behavioural side - happy to see this is actively worked on though.

@skriss skriss mentioned this issue Jun 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocked/needs-design Categorizes the issue or PR as blocked because it needs a design document. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Milestone
No milestone
5 participants
@davecheney @jpeach @skriss @BerGer23 @youngnick