enhancement/ingress: add max connections tuning #1084

frobware · 2022-04-06T16:51:07Z

This enhancement proposes a new field be added to the
IngressController API to allow the maximum number of simultaneous
connections (i.e., maxconn) be configurable by cluster
administrators.

Adds a new field to the IngressController API to allow the maximum number of simultaneous connections (i.e., maxconn) be configurable by cluster administrators: openshift/enhancements#1084

enhancements/ingress/haproxy-max-connections-tuning.md

gcs278 · 2022-04-12T04:41:18Z

enhancements/ingress/haproxy-max-connections-tuning.md

+
+HAProxy builds its data structures ahead of time. If you specify a
+large value for `spec.tuningOptions.maxConnections` then that memory
+is allocated up-front when the process starts. It is never released.


I was curious and I confirmed this as well. 500 backend leastconn config with 20,000 maxconn was about ~280mB. Increased to 500,000 maxconn and it spiked to 366mB.

gcs278

Everything looks good to me. I think we discussed this quite a bit so I'm on board with everything you documented here. The "-1" for maxConn auto feels a bit awkward as a end-user, but I see your rational for a more strongly typed parameter. I'll yield to others
that know these datatypes better.

Adds a new field to the IngressController API to allow the maximum number of simultaneous connections (i.e., maxconn) be configurable by cluster administrators: openshift/enhancements#1084

enhancements/ingress/haproxy-max-connections-tuning.md

Miciah · 2022-04-11T19:11:18Z

enhancements/ingress/haproxy-max-connections-tuning.md

+queued clients will time out; 20000 is likely too small a value for
+the majority of deployments.


Do we have any data to substantiate the "likely too small" statement?

None. I will remove the comment as it is subjective.

Miciah · 2022-04-19T02:13:04Z

enhancements/ingress/haproxy-max-connections-tuning.md

+To do this, the administrator can configure
+`spec.nodePlacement.nodeSelector` with labels that match the intended
+node, as well as configuring `spec.tuningOptions.maxConnections`.


The administrator will probably also want to increase nbthread.

Miciah · 2022-04-19T02:13:08Z

enhancements/ingress/haproxy-max-connections-tuning.md

+within the container when HAProxy starts. The nature of HAProxy's
+dynamic computation also takes into consideration what is configured
+in `haproxy.config` at that time.


The nature of HAProxy's
dynamic computation also takes into consideration what is configured
in haproxy.config at that time.

Can you elaborate on this point a little?

Miciah · 2022-04-19T02:14:33Z

enhancements/ingress/haproxy-max-connections-tuning.md

+router deployment roll out the new pods.
+
+```sh
+$ oc patch ingresscontroller/default --type=merge --patch '{"spec":{"tuningOptions":{"maxConnections":-1}}}' -n openshift-ingress-operator


I hope curious administrators will use a test ingresscontroller and not the default one.

enhancements/ingress/haproxy-max-connections-tuning.md

Miciah · 2022-04-19T02:20:51Z

enhancements/ingress/haproxy-max-connections-tuning.md

+2. If you want an exact value then set a value that is improbably
+   large, heed the warning and the suggested fix, then configure a


I'm a little confused by the wording here. Is the "improbably large" value the one that the user actual wants to set? Is heeding the warning and suggested fix the same step as configuring the tuned profile?

Adds a new field to the IngressController API to allow the maximum number of simultaneous connections (i.e., maxconn) be configurable by cluster administrators: openshift/enhancements#1084

This enhancement proposes a new field be added to the IngressController API to allow the maximum number of simultaneous connections (i.e., `maxconn`) be configurable by cluster administrators.

Co-authored-by: Miciah Dashiel Butler Masters <[email protected]>

frobware · 2022-04-29T12:22:42Z

@Miciah I addressed your feedback. Please could you take another look.

Miciah · 2022-05-09T03:18:33Z

/lgtm
/approve

openshift-ci · 2022-05-09T03:18:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2022-05-09T03:40:44Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2022-05-09T03:56:50Z

The linter has a new requirement:

 enhancements/ingress/haproxy-max-connections-tuning.md missing "### Workflow Description"

This requirement is redundant because you already have detailed use-cases, and the requirement wasn't there when the PR was posted, so I'll try to override it.

/override ci/prow/markdownlint

openshift-ci · 2022-05-09T03:57:15Z

@Miciah: Overrode contexts on behalf of Miciah: ci/prow/markdownlint

Details

In response to this:

The linter has a new requirement:
enhancements/ingress/haproxy-max-connections-tuning.md missing "### Workflow Description" 
This requirement is redundant because you already have detailed use-cases, and the requirement wasn't there when the PR was posted, so I'll try to override it.

/override ci/prow/markdownlint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-05-09T03:57:16Z

@frobware: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from dtantsur and jwforres April 6, 2022 16:51

frobware mentioned this pull request Apr 6, 2022

types/ingress: add new tuning option: maxConnections openshift/api#1161

Merged

frobware mentioned this pull request Apr 7, 2022

NE-577: Support a Configurable ROUTER_MAX_CONNECTIONS in HAproxy openshift/cluster-ingress-operator#735

Merged

gcs278 reviewed Apr 8, 2022

View reviewed changes

enhancements/ingress/haproxy-max-connections-tuning.md Show resolved Hide resolved

gcs278 reviewed Apr 12, 2022

View reviewed changes

Miciah reviewed Apr 14, 2022

View reviewed changes

Miciah reviewed Apr 19, 2022

View reviewed changes

enhancements/ingress/haproxy-max-connections-tuning.md Outdated Show resolved Hide resolved

Miciah reviewed Apr 19, 2022

View reviewed changes

frobware force-pushed the haproxy-max-connections branch from 459ab3f to d5f8bf1 Compare April 27, 2022 12:57

frobware and others added 6 commits April 28, 2022 13:19

enhancement/ingress: add max connections tuning

21a9745

This enhancement proposes a new field be added to the IngressController API to allow the maximum number of simultaneous connections (i.e., `maxconn`) be configurable by cluster administrators.

Update enhancements/ingress/haproxy-max-connections-tuning.md

b29aee1

Co-authored-by: Miciah Dashiel Butler Masters <[email protected]>

Update enhancements/ingress/haproxy-max-connections-tuning.md

0129f20

Co-authored-by: Miciah Dashiel Butler Masters <[email protected]>

Remove spurious claim that 2000 is too small for maxconn

1581556

Mention tuning threadCount

762a7b9

Elaborate on how maxconn is computed

b017026

frobware added 4 commits April 28, 2022 13:19

Add latest godoc from openshift/api#1161

080ade1

Updated with latest maxConnections api godoc

1d47bc1

Clarify mitigation section for large unsupported maxsock values

08bdc28

Add tables detailing memory growth

be5b059

frobware force-pushed the haproxy-max-connections branch 2 times, most recently from d558dd7 to 5cf782d Compare April 28, 2022 19:21

lint

dec25f2

frobware force-pushed the haproxy-max-connections branch from 5cf782d to dec25f2 Compare April 29, 2022 10:21

frobware added 2 commits April 29, 2022 11:43

more lint

2c4c53e

lint lint lint; 3 times a charm

6aa3417

openshift-ci bot assigned Miciah May 9, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2022

openshift-merge-robot merged commit 1193f96 into openshift:master May 9, 2022

		queued clients will time out; 20000 is likely too small a value for
		the majority of deployments.

		2. If you want an exact value then set a value that is improbably
		large, heed the warning and the suggested fix, then configure a

enhancement/ingress: add max connections tuning #1084

enhancement/ingress: add max connections tuning #1084

Uh oh!

Conversation

frobware commented Apr 6, 2022

Uh oh!

Uh oh!

gcs278 Apr 12, 2022

Choose a reason for hiding this comment

Uh oh!

gcs278 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Miciah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

frobware Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Miciah Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

frobware commented Apr 29, 2022

Uh oh!

Miciah commented May 9, 2022

Uh oh!

openshift-ci bot commented May 9, 2022

Uh oh!

openshift-bot commented May 9, 2022

Uh oh!

Miciah commented May 9, 2022

Uh oh!

openshift-ci bot commented May 9, 2022

Uh oh!

openshift-ci bot commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gcs278 left a comment •

edited

Loading