[octavia-ingress-controller]feat: add configurable listener timeout via ingress annotation #2435

sakshi-1505 · 2023-10-15T17:43:58Z

What this PR does / why we need it:

This PR basically adds the ability to configure timeout via ingress annotations in ingress-controller.

Which issue this PR fixes(if applicable):
fixes #2243

Special notes for reviewers:

Release note:

- Added support for configurable timeouts via ingress annotation in octavia-ingress-controller

k8s-ci-robot · 2023-10-15T17:44:08Z

Hi @sakshi-1505. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

sakshi-1505 · 2023-10-15T17:47:14Z

@pierreprinetti @dulek @jichenjc Request you to please review the above PR & let me know if I thought in the right redirection or not. I would say it was pretty much fun to navigate through the code of complete open stack & then finding all the different binary it produces and then investigating the octavia-controller's code.

jichenjc · 2023-10-16T02:15:11Z

/ok-to-test

pierreprinetti · 2023-10-16T13:09:18Z

pkg/ingress/controller/controller.go

+	timeoutClientData := getIntFromIngressAnnotation(ing, IngressAnnotationTimeoutClientData, 50000)
+	timeoutMemberConnect := getIntFromIngressAnnotation(ing, IngressAnnotationTimeoutMemberConnect, 5000)
+	timeoutMemberData := getIntFromIngressAnnotation(ing, IngressAnnotationTimeoutMemberData, 50000)
+	timeoutTCPInspect := getIntFromIngressAnnotation(ing, IngressAnnotationTimeoutTCPInspect, 0)


I think that one requirement for this change should be: if the new annotations are not set in config, the Ingress behaves exactly as it was before this patch.

I am worried that setting an explicit default timeout changes the default behaviour. If no annotation is set, TimeoutXxx fields in listeners.CreateOpts should be left nil as they were previously.

Understandable, thanks @pierreprinetti I have made the requested changes

pierreprinetti

I am worried that in case the annotation input is malformed, it's very hard to diagnose an anomalous behaviour.

I think there is at least one case we should take into consideration: the case where the annotation is not properly formatted as an int (for example: a rogue character, maybe a space, slipped in). strconv.Atoi would fail parsing and the Ingress would be created with the default Octavia setting, without any notice to the user.

At a minimum, I think that we should log if the parsing of an annotation failed, with a log line that states exactly which annotation failed parsing.

What do you think?

pierreprinetti · 2023-10-16T13:27:24Z

pkg/ingress/controller/controller.go

@@ -1017,6 +1034,19 @@ func getStringFromIngressAnnotation(ingress *nwv1.Ingress, annotationKey string,
 	return defaultValue
 }

+// maybeGetIntFromIngressAnnotation searches a given Ingress for a specific annotationKey and either returns the
+// annotation's value or a specified defaultSetting


please update the docstring: it still refers to a default setting

pierreprinetti · 2023-10-16T13:28:53Z

pkg/ingress/controller/controller.go

+	if annotationValue, ok := ingress.Annotations[annotationKey]; ok {
+		returnValue, err := strconv.Atoi(annotationValue)
+		if err != nil {
+			return nil


Please add logging here

WRT the logging level: I'd keep it consistent with this:

cloud-provider-openstack/pkg/openstack/loadbalancer.go

Line 693 in 60ad70c

klog.V(4).Infof("Found a Service Annotation: %v = %v", annotationKey, returnValue)

Note that this getBool function also has a logging issue, and I think @FavourEva is on it.

I am not using the getBool function, but yeah I got the context what you are referring to.

pierreprinetti · 2023-10-16T13:38:01Z

pkg/ingress/controller/controller.go

+	// IngressAnnotationTimeoutMemberConnet is the timeout for memer connect in ms
+	IngressAnnotationTimeoutMemberConnect = "octavia.ingress.kubernetes.io/timeout-member-connect"
+
+	// IngressAnnotationTimeoutMemberConnet is the timeout for memer connect in ms


Maybe for completeness you could mention what's the default value (I suppose it's an Octavia setting?)

Actually in my previous implementation, I already had assigned default values to each of this parameter. I read the defaultValues from here in the octavia documentations

--timeout-client-data <timeout> Frontend client inactivity timeout in milliseconds. Default: 50000. --timeout-member-connect <timeout> Backend member connection timeout in milliseconds. Default: 5000. --timeout-member-data <timeout> Backend member inactivity timeout in milliseconds. Default: 50000. --timeout-tcp-inspect <timeout> Time, in milliseconds, to wait for additional TCP packets for content inspection. Default: 0.

Should I just document default values here?

I'd point to he Octavia config argument, so that the user knows where to look

It's not an octavia config argument but it's available for LB, when the controller starts to create listener it doesn't pass this value post so that LoadBalancer default values can get applied, so basically we are passing this in CreateOpts to let override the defaults available in LB. I will still add the docs of the CLI configs https://docs.openstack.org/octavia/latest/configuration/configref.html

sakshi-1505 · 2023-10-16T17:39:52Z

@pierreprinetti please re-review

pkg/ingress/controller/controller.go

pierreprinetti · 2023-10-17T11:54:01Z

pkg/ingress/controller/controller.go

+		}
+		return &returnValue
+	}
+	klog.V(4).Infof("Could not find a Service Annotation; falling back to default setting for annotation %v", annotationKey)


sakshi-1505 · 2023-10-17T14:27:25Z

@pierreprinetti Please review again

Signed-off-by: sakshi-1505 <[email protected]>

pierreprinetti

Looks good to me.
Please run go fmt ./... before we ping a maintainer again for review

sakshi-1505 · 2023-10-17T14:42:38Z

Looks good to me.
Please run go fmt ./... before we ping a maintainer again for review

Done, it was just a matter of few seconds before I squashed it 🙈

sakshi-1505 · 2023-10-17T15:33:25Z

please do check again @pierreprinetti

pierreprinetti · 2023-10-17T15:50:43Z

/lgtm
/assign jichenjc

jichenjc · 2023-10-17T23:52:06Z

/approve

thanks for the update~

k8s-ci-robot · 2023-10-17T23:52:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jichenjc]

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

dulek · 2023-10-24T16:38:00Z

pkg/ingress/controller/openstack/octavia.go

+			TimeoutClientData:    timeoutClientData,
+			TimeoutMemberData:    timeoutMemberData,
+			TimeoutMemberConnect: timeoutMemberConnect,
+			TimeoutTCPInspect:    timeoutTCPInspect,


@sakshi-1505: This is missing the update case, these properties can be updated, just like the AllowedCIDRs option is in lines 368-371. Can you update that too?

…rnetes#2435) Signed-off-by: sakshi-1505 <[email protected]>

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 15, 2023

k8s-ci-robot requested review from dulek and mdbooth October 15, 2023 17:44

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 16, 2023

pierreprinetti reviewed Oct 16, 2023

View reviewed changes

sakshi-1505 force-pushed the add_configurable_timeout branch 2 times, most recently from d41e20a to 3c1ca51 Compare October 16, 2023 13:19

sakshi-1505 requested a review from pierreprinetti October 16, 2023 13:19

sakshi-1505 force-pushed the add_configurable_timeout branch from 3c1ca51 to 7105c66 Compare October 16, 2023 13:20

pierreprinetti reviewed Oct 16, 2023

View reviewed changes

sakshi-1505 force-pushed the add_configurable_timeout branch from 8a7870e to 1372b72 Compare October 16, 2023 17:39

sakshi-1505 requested a review from pierreprinetti October 16, 2023 17:39

jichenjc reviewed Oct 17, 2023

View reviewed changes

pkg/ingress/controller/controller.go Outdated Show resolved Hide resolved

pierreprinetti suggested changes Oct 17, 2023

View reviewed changes

sakshi-1505 force-pushed the add_configurable_timeout branch from 1b2c36a to 20ef5e8 Compare October 17, 2023 14:27

feat: add configurable listerner timeout via ingress annotation

6a25117

Signed-off-by: sakshi-1505 <[email protected]>

pierreprinetti reviewed Oct 17, 2023

View reviewed changes

sakshi-1505 force-pushed the add_configurable_timeout branch from 20ef5e8 to 6a25117 Compare October 17, 2023 14:42

k8s-ci-robot assigned jichenjc and pierreprinetti Oct 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2023

k8s-ci-robot merged commit d0bedda into kubernetes:master Oct 18, 2023

dulek reviewed Oct 24, 2023

View reviewed changes

dulek mentioned this pull request Nov 6, 2023

[octavia-ingress-controller] Update listener timeouts #2468

Merged

mandre pushed a commit to shiftstack/cloud-provider-openstack that referenced this pull request Feb 6, 2024

feat: add configurable listerner timeout via ingress annotation (kube…

e32f146

…rnetes#2435) Signed-off-by: sakshi-1505 <[email protected]>

mandre pushed a commit to shiftstack/cloud-provider-openstack that referenced this pull request Feb 7, 2024

feat: add configurable listerner timeout via ingress annotation (kube…

afd5156

…rnetes#2435) Signed-off-by: sakshi-1505 <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[octavia-ingress-controller]feat: add configurable listener timeout via ingress annotation #2435

[octavia-ingress-controller]feat: add configurable listener timeout via ingress annotation #2435

sakshi-1505 commented Oct 15, 2023

k8s-ci-robot commented Oct 15, 2023

sakshi-1505 commented Oct 15, 2023

jichenjc commented Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti left a comment

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023 •

edited

Loading

sakshi-1505 Oct 16, 2023

sakshi-1505 commented Oct 16, 2023

pierreprinetti Oct 17, 2023

sakshi-1505 commented Oct 17, 2023

pierreprinetti left a comment

sakshi-1505 commented Oct 17, 2023

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

jichenjc commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

dulek Oct 24, 2023

[octavia-ingress-controller]feat: add configurable listener timeout via ingress annotation #2435

[octavia-ingress-controller]feat: add configurable listener timeout via ingress annotation #2435

Conversation

sakshi-1505 commented Oct 15, 2023

k8s-ci-robot commented Oct 15, 2023

sakshi-1505 commented Oct 15, 2023

jichenjc commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreprinetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshi-1505 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshi-1505 commented Oct 16, 2023

Choose a reason for hiding this comment

sakshi-1505 commented Oct 17, 2023

pierreprinetti left a comment

Choose a reason for hiding this comment

sakshi-1505 commented Oct 17, 2023

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

jichenjc commented Oct 17, 2023

k8s-ci-robot commented Oct 17, 2023

Choose a reason for hiding this comment

sakshi-1505 Oct 16, 2023 •

edited

Loading