[release-4.15] OCPBUGS-36208: Implement connect timeout tuning option

[alebedev87]

Backport of #1035 and #1084 combied into a single PR like it was supposed to be from the very beginning.

[openshift-ci-robot]

@alebedev87: This pull request references Jira Issue OCPBUGS-36208, which is valid.

7 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.15.z) matches configured target version for branch (4.15.z)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
• release note text is set and does not match the template
• dependent bug Jira Issue OCPBUGS-28673 is in the state Closed (Done-Errata), which is one of the valid states (VERIFIED, RELEASE PENDING, CLOSED (ERRATA), CLOSED (CURRENT RELEASE), CLOSED (DONE), CLOSED (DONE-ERRATA))
• dependent Jira Issue OCPBUGS-28673 targets the "4.16.0" version, which is one of the valid target versions: 4.16.0, 4.16.z
• bug has dependents

Requesting review from QA contact:
/cc @lihongan

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Backport of #1035 and #1084 combied into a single PR like it was supposed to be from the very beginning.

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 openshift-eng/jira-lifecycle-plugin repository.

[lihongan]

/retest

[alebedev87]

/hold

Waiting for the API change.

[candita]

/assign @gcs278
/assign @rfredette

[alebedev87]

/unhold

openshift/api#1937 merged.

[alebedev87]

/retest

[lihongan]

/label qe-approved
/label cherry-pick-approved

verified with pre-merge testing

$ oc get clusterversion
NAME      VERSION                                                   AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.15.0-0.ci.test-2024-07-08-094335-ci-ln-dvh3fst-latest   True        False         24m     Cluster version is 4.15.0-0.ci.test-2024-07-08-094335-ci-ln-dvh3fst-latest

$ oc explain ingresscontroller.spec.tuningOptions.connectTimeout
GROUP:      operator.openshift.io
KIND:       IngressController
VERSION:    v1

FIELD: connectTimeout <string>

DESCRIPTION:
    ConnectTimeout defines the maximum time to wait for a connection attempt to
    a server/backend to succeed. 
     This field expects an unsigned duration string of decimal numbers, each
    with optional fraction and a unit suffix, e.g. "300ms", "1.5h" or "2h45m".
    Valid time units are "ns", "us" (or "µs" U+00B5 or "μs" U+03BC), "ms",
    "s", "m", "h". 
     When omitted, this means the user has no opinion and the platform is left
    to choose a reasonable default. This default is subject to change over time.
    The current default is 5s.

$ oc -n openshift-ingress-operator patch ingresscontroller/default -p '{"spec":{"tuningOptions":{"connectTimeout":"15s"}}}' --type=merge
ingresscontroller.operator.openshift.io/default patched
$ oc -n openshift-ingress get deployment/router-default -oyaml | grep TIMEOUT -A1
        - name: ROUTER_DEFAULT_CONNECT_TIMEOUT
          value: 15s

$ oc -n openshift-ingress rsh router-default-644fd5c559-22nzr
sh-4.4$ env | grep TIMEOUT
ROUTER_DEFAULT_CONNECT_TIMEOUT=15s
sh-4.4$ 
sh-4.4$ grep "timeout connect" haproxy.config
  timeout connect 15s

[gcs278]

One nit pick about vendor commit, but everything else looks good to me.

I have one suggestion for the future, feel free to take or leave it: Andy has got me into the habit of providing paper trail of manual backports/cherry-picks. I think it's quite helpful to see the steps taken.

Providing info such as the commands you ran, the merge conflicts and summary of the resolution, and any hiccups along the way is super helpful. Here's a more complicated example: openshift/router#585. I like to recreate manual backports to see how merges were resolved.

I did recreate this backport and besides the expected go.mod merge conflicts, I noticed the merge conflict in util_test.go, that looks good to me.

[gcs278]

nit What about adding the vendoring in a separate commit, similar to the PR we are cherry-picking #1035?

BTW I usually put the vendoring commit first, so that the implementation isn't a broken commit, I think #1035 is reversed.

[alebedev87]

Personally, I see the benefit of adding the vendored files into a separate commit only if this facilitates the review process (a lot of updates in vendor/). For the building process, most of the binaries we build use -mod=vendor therefore expecting the vendor directory to contain all the changes needed. Putting the vendor commit first may result into a broken commit too (e.g. api crd package move).

[alebedev87]

However for the backporting, a separate vendor commit can indeed be helpful. It can reduce the burden of cherry picking dependencies from a different branch. So, for this PR, I agree it makes sense. Let me extract it.

[alebedev87]

/retest

[gcs278]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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 [gcs278]

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

[lihongan]

/retest

[alebedev87]

TestInternalLoadBalancer failed. Checking.

[alebedev87]

Seems like I'm hitting https://issues.redhat.com/browse/OCPBUGS-13106.

/retest

[openshift-ci]

@alebedev87: 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-sigs/prow repository. I understand the commands that are listed here.

[candita]

This backport is needed to fix an omission from the original feature implementation of the timeout tuning RFE in release 4.15. The ROUTER_DEFAULT_CONNECT_TIMEOUT router variable is already present in the router config, and this backport exposes the variable within the API. Since this PR fixes an omission from the original feature, it is not expected to pose any additional risk than any of the other timeouts that were added.

/label backport-risk-assessed

[openshift-ci-robot]

@alebedev87: Jira Issue OCPBUGS-36208: All pull requests linked via external trackers have merged:

• openshift/api#1937
• openshift/cluster-ingress-operator#1097

Jira Issue OCPBUGS-36208 has been moved to the MODIFIED state.

Details

In response to this:

Backport of #1035 and #1084 combied into a single PR like it was supposed to be from the very beginning.

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-ingress-operator-container-v4.15.0-202407102237.p0.g28bcd5f.assembly.stream.el9 for distgit ose-cluster-ingress-operator.
All builds following this will include this PR.

[openshift-merge-robot]

Fix included in accepted release 4.15.0-0.nightly-2024-07-11-014354