NE-577: Support a Configurable ROUTER_MAX_CONNECTIONS in HAproxy #735

frobware · 2022-04-07T18:52:52Z

This implements openshift/enhancements#1084.

Requires openshift/api#1161 [MERGED]

frobware · 2022-04-07T18:55:00Z

/hold

Needs the EP to be approved.
API change: openshift/api#1161

frobware · 2022-04-08T12:20:30Z

Errors:

[sig-scheduling][Early] The openshift-etcd pods should be scheduled on different nodes [Suite:openshift/conformance/parallel]

/retest

frobware · 2022-04-11T08:21:05Z

configurable_route_test.go:101: error updating ingress.status: timed out waiting for the condition

/test e2e-aws-operator

gcs278

Looks good to me.

test/e2e/maxconn_test.go

frobware · 2022-04-21T08:12:22Z

/retest

Switch from the unsupported config override to the tuning option introduced in openshift/api#1161.

frobware · 2022-05-03T11:40:19Z

/retest

ahardin-rh · 2022-05-03T12:33:39Z

/label docs-approved

sferich888 · 2022-05-05T15:08:30Z

/label px-approved

Miciah

I have a few suggestions to simplify the code, but overall I would be fine with merging this as-is.

Miciah · 2022-05-09T03:23:57Z

pkg/operator/controller/ingress/deployment_test.go

-		Raw: []byte(`{"loadBalancingAlgorithm":"random","dynamicConfigManager":"false","maxConnections":-1,"reloadInterval":15}`),
+		Raw: []byte(`{"loadBalancingAlgorithm":"random","dynamicConfigManager":"false","reloadInterval":15}`),
 	}
+	ic.Spec.TuningOptions.MaxConnections = -1


This could be moved above, here:

cluster-ingress-operator/pkg/operator/controller/ingress/deployment_test.go

Lines 430 to 434 in 6620e18

ic.Spec.TuningOptions = operatorv1.IngressControllerTuningOptions{

HeaderBufferBytes: 16384,

HeaderBufferMaxRewriteBytes: 4096,

ThreadCount: RouterHAProxyThreadsDefaultValue * 2,

}

Did you mean above? I added MaxConnections as an addition after ThreadCount.

Miciah · 2022-05-09T03:35:39Z

test/e2e/maxconn_test.go

+)
+
+func TestTunableMaxConnectionsValidValues(t *testing.T) {
+	updateMaxConnections := func(t *testing.T, client client.Client, timeout time.Duration, maxConnections int32, c *operatorv1.IngressController) error {


Seems you only use c as a way to get the name of the object and then as a place to store the object that you get from the API, which is subtle. What do you think about replacing the *operatorv1.IngressController parameter with a types.NamespacedName parameter and using a local variable to store the object from the API?

Would it make more sense just to use kclient and time.Minute instead of parameterizing the client and timeout?

I want the caller to specify how long to wait for. And I'd rather not have a mismatch of globals and discrete parameter values.

Seems you only use c as a way to get the name of the object and then as a place to store > the object that you get from the API, which is subtle. What do you think about replacing the > *operatorv1.IngressController parameter with a types.NamespacedName parameter and using a >local variable to store the object from the API?

Done.

Miciah · 2022-05-09T03:39:19Z

test/e2e/maxconn_test.go

+	conditions := []operatorv1.OperatorCondition{
+		{Type: operatorv1.IngressControllerAvailableConditionType, Status: operatorv1.ConditionTrue},
+		{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+		{Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+	}
+	t.Logf("waiting for ingresscontroller %v", icName)
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, conditions...); err != nil {


Suggested change

conditions := []operatorv1.OperatorCondition{

{Type: operatorv1.IngressControllerAvailableConditionType, Status: operatorv1.ConditionTrue},

{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},

{Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionFalse},

}

t.Logf("waiting for ingresscontroller %v", icName)

if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, conditions...); err != nil {

t.Logf("waiting for ingresscontroller %v", icName)

if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableConditionsForPrivateIngressController...); err != nil {

Miciah · 2022-05-09T03:49:42Z

pkg/operator/controller/ingress/deployment_test.go

-		Raw: []byte(`{"loadBalancingAlgorithm":"source","dynamicConfigManager":"true","maxConnections":40000}`),
+		Raw: []byte(`{"loadBalancingAlgorithm":"source","dynamicConfigManager":"true"}`),
 	}
+	ic.Spec.TuningOptions.MaxConnections = 40000


TestDesiredRouterDeploymentSpecAndNetwork feels like it should be at least two separate tests, but that can be a further cleanup for another day.

$ go get github.com/openshift/api@1469fac35c7515f007526e51f519eef7339a1bbb $ go mod vendor $ go mod tidy

frobware · 2022-05-09T10:57:55Z

@Miciah I addressed your feedback with my latest push.

/hold cancel

frobware · 2022-05-09T12:48:47Z

FAIL: TestHTTPCookieCapture

/test e2e-aws-operator

frobware · 2022-05-09T13:06:43Z

/test e2e-upgrade

Miciah · 2022-05-09T13:23:51Z

/lgtm

openshift-ci · 2022-05-09T13:30:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, 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,frobware]

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

frobware · 2022-05-09T14:01:44Z

/skip

openshift-ci · 2022-05-09T15:04:32Z

@frobware: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-single-node	`2c8f12e`	link	false	`/test e2e-aws-single-node`

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.

frobware changed the title ~~haproxy maxconn~~ Allow maxConnections tuning option Apr 7, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2022

frobware changed the title ~~Allow maxConnections tuning option~~ Implement maxConnections tuning option Apr 7, 2022

openshift-ci bot requested review from alebedev87 and knobunc April 7, 2022 18:57

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

frobware force-pushed the haproxy-maxconn branch 2 times, most recently from 9ce070e to 0471f55 Compare April 8, 2022 08:35

frobware force-pushed the haproxy-maxconn branch from 0471f55 to 97411cd Compare April 8, 2022 15:03

frobware force-pushed the haproxy-maxconn branch from 0ec78a9 to ebcbaee Compare April 11, 2022 14:14

gcs278 reviewed Apr 12, 2022

View reviewed changes

test/e2e/maxconn_test.go Show resolved Hide resolved

frobware mentioned this pull request Apr 12, 2022

Fix TestHostNetworkEndpointPublishingStrategy #738

Closed

frobware force-pushed the haproxy-maxconn branch 6 times, most recently from 124d543 to 9932463 Compare April 20, 2022 10:51

frobware force-pushed the haproxy-maxconn branch from 9932463 to e50a904 Compare April 21, 2022 19:54

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2022

frobware force-pushed the haproxy-maxconn branch from 04289c0 to f3c8b33 Compare April 25, 2022 12:16

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2022

frobware force-pushed the haproxy-maxconn branch 4 times, most recently from a7ba4cd to c086bda Compare April 27, 2022 11:58

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Apr 29, 2022

frobware force-pushed the haproxy-maxconn branch from 523c0c0 to 586775e Compare April 29, 2022 10:46

ingress: change deployment to use spec.tuningOptions.MaxConnections

62e8849

Switch from the unsupported config override to the tuning option introduced in openshift/api#1161.

frobware force-pushed the haproxy-maxconn branch from 586775e to 6620e18 Compare April 29, 2022 12:14

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label May 3, 2022

frobware changed the title ~~Implement maxConnections tuning option~~ https://issues.redhat.com/browse/NE-577 May 5, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label May 5, 2022

frobware changed the title ~~https://issues.redhat.com/browse/NE-577~~ NE-577: Support a Configurable ROUTER_MAX_CONNECTIONS in HAproxy May 5, 2022

Miciah reviewed May 9, 2022

View reviewed changes

frobware added 4 commits May 9, 2022 11:54

test/e2e: add test for tuningOptions.MaxConnections

bcbad4b

Drop maxConnections from unsupportedConfigOverrides

c6a01f4

vendor: openshift/api#1161

8d52365

$ go get github.com/openshift/api@1469fac35c7515f007526e51f519eef7339a1bbb $ go mod vendor $ go mod tidy

Regenerate CRD

2c8f12e

frobware force-pushed the haproxy-maxconn branch from 6620e18 to 2c8f12e Compare May 9, 2022 10:57

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2022

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-merge-robot merged commit 972f09b into openshift:master May 9, 2022

Miciah mentioned this pull request May 10, 2022

NE-408: Allow configuring ELB connection idle timeout #451

Merged

frobware deleted the haproxy-maxconn branch September 5, 2022 15:03

	ic.Spec.TuningOptions = operatorv1.IngressControllerTuningOptions{
	HeaderBufferBytes: 16384,
	HeaderBufferMaxRewriteBytes: 4096,
	ThreadCount: RouterHAProxyThreadsDefaultValue * 2,
	}

NE-577: Support a Configurable ROUTER_MAX_CONNECTIONS in HAproxy #735

NE-577: Support a Configurable ROUTER_MAX_CONNECTIONS in HAproxy #735

Uh oh!

Conversation

frobware commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frobware commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frobware commented Apr 8, 2022

Uh oh!

frobware commented Apr 11, 2022

Uh oh!

gcs278 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

frobware commented Apr 21, 2022

Uh oh!

frobware commented May 3, 2022

Uh oh!

ahardin-rh commented May 3, 2022

Uh oh!

sferich888 commented May 5, 2022

Uh oh!

Miciah 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!

frobware May 9, 2022 • 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!

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!

frobware commented May 9, 2022

Uh oh!

frobware commented May 9, 2022

Uh oh!

frobware commented May 9, 2022

Uh oh!

Miciah commented May 9, 2022

Uh oh!

openshift-ci bot commented May 9, 2022

Uh oh!

frobware commented May 9, 2022

Uh oh!

openshift-ci bot commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

frobware commented Apr 7, 2022 •

edited

Loading

frobware commented Apr 7, 2022 •

edited

Loading

frobware May 9, 2022 •

edited

Loading