Support setting azure LB idle timeout #66045

cpuguy83 · 2018-07-10T22:23:16Z

What this PR does / why we need it:

Adds a new annotation to allow users to configure the idle timeout of
the Azure LB.

Release note:

Support configuring the Azure load balancer idle connection timeout for services

cpuguy83 · 2018-07-10T22:28:41Z

/assign @khenidak

cpuguy83 · 2018-07-10T22:33:15Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -1280,7 +1304,8 @@ func equalLoadBalancingRulePropertiesFormat(s, t *network.LoadBalancingRulePrope
 		reflect.DeepEqual(s.LoadDistribution, t.LoadDistribution) &&
 		reflect.DeepEqual(s.FrontendPort, t.FrontendPort) &&
 		reflect.DeepEqual(s.BackendPort, t.BackendPort) &&
-		reflect.DeepEqual(s.EnableFloatingIP, t.EnableFloatingIP)
+		reflect.DeepEqual(s.EnableFloatingIP, t.EnableFloatingIP) &&
+		s.IdleTimeoutInMinutes == t.IdleTimeoutInMinutes


Of course this doesn't work as expected because it's a pointer.

feiskyer · 2018-07-11T03:36:24Z

/ok-to-test

feiskyer · 2018-07-11T03:37:39Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -74,6 +74,10 @@ const (
 	// ServiceAnnotationAllowedServiceTag is the annotation used on the service
 	// to specify a list of allowed service tags separated by comma
 	ServiceAnnotationAllowedServiceTag = "service.beta.kubernetes.io/azure-allowed-service-tags"
+
+	// ServiceAnnotationLoadBalancerIdleTimeout is the annotation used on the service
+	// to specify the idle timeout for connections on the load balancer.


The timeout is in minutes?

nit: s/the idle timeout/the idle timeout in minutes/

feiskyer · 2018-07-11T03:39:56Z

LGTM in general. @cpuguy83 Could you document the timeout unit clearly?

andyzhangx · 2018-07-11T03:54:22Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -487,6 +505,11 @@ func (az *Cloud) reconcileLoadBalancer(clusterName string, service *v1.Service,
 	lbBackendPoolName := getBackendPoolName(clusterName)
 	lbBackendPoolID := az.getBackendPoolID(lbName, lbBackendPoolName)

+	lbIdleTimeout, err := getIdleTimeout(service)


so what if there is no service.beta.kubernetes.io/azure-load-balancer-idle-timeout setting, it will return nil, nil, are we going to handle that? And what if it's a negative?

We want nil, nil in this case as this will set the LB timeout to the default value. I will add a check here to see if the key even exists and only return nil when it does not, and a separate error case for when it does exist but the value is empty.

For validation of the value beyond if it converts to an int... I didn't want to do too much validation here as I expect the backend to deal with this, but I do see value in validating here since they are fairly hard values that are well documented (or at least were easy for me to find), which saves us a failed request.

This is all updated

Thanks. The range is [4,30]

cpuguy83 · 2018-07-11T18:28:00Z

/test pull-kubernetes-integration

feiskyer · 2018-07-12T01:52:29Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

@@ -74,6 +74,10 @@ const (
 	// ServiceAnnotationAllowedServiceTag is the annotation used on the service
 	// to specify a list of allowed service tags separated by comma
 	ServiceAnnotationAllowedServiceTag = "service.beta.kubernetes.io/azure-allowed-service-tags"
+
+	// ServiceAnnotationLoadBalancerIdleTimeout is the annotation used on the service
+	// to specify the idle timeout for connections on the load balancer in miutes.


typo miutes

feiskyer · 2018-07-12T01:57:10Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+				FrontendPort:         to.Int32Ptr(port.Port),
+				BackendPort:          to.Int32Ptr(port.Port),
+				EnableFloatingIP:     to.BoolPtr(true),
+				IdleTimeoutInMinutes: lbIdleTimeout,


IdleTimeoutInMinutes only applies to TCP protocol. Could you also add a check and only set it when transportProto is TCP?

Yep, also updated annotation name to clarify that.

andyzhangx

@cpuguy83
could you also write a unit test for getIdleTimeout func?

andyzhangx · 2018-07-12T02:50:10Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+		min = 4
+		max = 30
+	)
+	errInvalidTimeout := fmt.Errorf("idle timeout value must be a whole number representing minutes between %d and %d", min, max)


no need to define a var here, put it under
return nil, fmt.Errorf("idle timeout value must be a whole number representing minutes between %d and %d", min, max)

I'm using this multiple times.

andyzhangx · 2018-07-12T02:50:57Z

pkg/cloudprovider/providers/azure/azure_loadbalancer.go

+		// Return a nil here as this will set the value to the azure default
+		return nil, nil
+	}
+	if val == "" {


is it necessary to check here since below code would still parse val:

to, err := strconv.Atoi(val)

Not necessary, removing this explicit check.

cpuguy83 · 2018-07-12T17:27:33Z

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws

Adds a new annotation to allow users to configure the idle timeout of the Azure LB.

cpuguy83 · 2018-07-12T18:07:46Z

/test pull-kubernetes-integration

andyzhangx

/lgtm
would you take a another look @feiskyer ?

feiskyer · 2018-07-13T03:41:26Z

/lgtm
/approve

k8s-ci-robot · 2018-07-13T03:41:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, cpuguy83, feiskyer

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:

~~pkg/cloudprovider/providers/azure/OWNERS~~ [andyzhangx,feiskyer]

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

k8s-github-robot · 2018-07-13T06:05:07Z

Automatic merge from submit-queue (batch tested with PRs 66121, 66140, 66045). If you want to cherry-pick this change to another branch, please follow the instructions here.

rite2nikhil

Thanks, lgtm

feiskyer · 2018-07-13T23:20:35Z

@cpuguy83 PR got merged. Could you also file a PR to cloud-provider-azure and add docs for the new annotation?

cpuguy83 · 2018-07-13T23:50:01Z

kubernetes-sigs/cloud-provider-azure#37

marpaia · 2018-07-17T03:18:32Z

/sig azure

djsly · 2018-09-24T12:08:24Z

@cpuguy83 / @feiskyer anyway we could get this in 1.11 ? WE are currently blocked with a client requesting > 4min idle timeout and unfortunately, 1.12 would be too risky to push to production once it comes out.

Thanks!

…45-upstream-release-1.11 Automated cherry pick of #66045: Support setting azure LB idle timeout

k8s-ci-robot requested review from andyzhangx and feiskyer July 10, 2018 22:23

k8s-ci-robot assigned khenidak Jul 10, 2018

cpuguy83 commented Jul 10, 2018

View reviewed changes

cpuguy83 force-pushed the az_lb_timeout branch from e2e3a2a to d0efd1f Compare July 10, 2018 22:35

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 11, 2018

feiskyer reviewed Jul 11, 2018

View reviewed changes

andyzhangx reviewed Jul 11, 2018

View reviewed changes

cpuguy83 force-pushed the az_lb_timeout branch from d0efd1f to 1abce40 Compare July 11, 2018 17:36

feiskyer reviewed Jul 12, 2018

View reviewed changes

andyzhangx suggested changes Jul 12, 2018

View reviewed changes

cpuguy83 force-pushed the az_lb_timeout branch 2 times, most recently from 8cbf2ca to 8000342 Compare July 12, 2018 17:00

Support setting azure LB idle timeout

5556949

Adds a new annotation to allow users to configure the idle timeout of the Azure LB.

cpuguy83 force-pushed the az_lb_timeout branch from 8000342 to 5556949 Compare July 12, 2018 17:41

k8s-ci-robot assigned andyzhangx Jul 13, 2018

andyzhangx approved these changes Jul 13, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2018

k8s-ci-robot assigned feiskyer Jul 13, 2018

k8s-github-robot merged commit 16c5ba4 into kubernetes:master Jul 13, 2018

rite2nikhil reviewed Jul 13, 2018

View reviewed changes

cpuguy83 deleted the az_lb_timeout branch July 13, 2018 17:22

k8s-ci-robot added the sig/azure label Jul 17, 2018

feiskyer mentioned this pull request Sep 25, 2018

Automated cherry pick of #66045: Support setting azure LB idle timeout #69020

Merged

k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018

Merge pull request #69020 from feiskyer/automated-cherry-pick-of-#660…

16498a2

…45-upstream-release-1.11 Automated cherry pick of #66045: Support setting azure LB idle timeout

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting azure LB idle timeout #66045

Support setting azure LB idle timeout #66045

cpuguy83 commented Jul 10, 2018

cpuguy83 commented Jul 10, 2018

cpuguy83 Jul 10, 2018 •

edited

Loading

feiskyer commented Jul 11, 2018

feiskyer Jul 11, 2018 •

edited

Loading

feiskyer commented Jul 11, 2018

andyzhangx Jul 11, 2018

cpuguy83 Jul 11, 2018

cpuguy83 Jul 11, 2018

cpuguy83 Jul 11, 2018

feiskyer Jul 12, 2018

cpuguy83 commented Jul 11, 2018

feiskyer Jul 12, 2018

feiskyer Jul 12, 2018

cpuguy83 Jul 12, 2018

andyzhangx left a comment

andyzhangx Jul 12, 2018

cpuguy83 Jul 12, 2018

andyzhangx Jul 12, 2018

cpuguy83 Jul 12, 2018

cpuguy83 commented Jul 12, 2018

cpuguy83 commented Jul 12, 2018

andyzhangx left a comment

feiskyer commented Jul 13, 2018

k8s-ci-robot commented Jul 13, 2018

k8s-github-robot commented Jul 13, 2018

rite2nikhil left a comment

feiskyer commented Jul 13, 2018

cpuguy83 commented Jul 13, 2018

marpaia commented Jul 17, 2018

djsly commented Sep 24, 2018

Support setting azure LB idle timeout #66045

Support setting azure LB idle timeout #66045

Conversation

cpuguy83 commented Jul 10, 2018

cpuguy83 commented Jul 10, 2018

cpuguy83 Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

feiskyer commented Jul 11, 2018

feiskyer Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

feiskyer commented Jul 11, 2018

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

cpuguy83 commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx 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

cpuguy83 commented Jul 12, 2018

cpuguy83 commented Jul 12, 2018

andyzhangx left a comment

Choose a reason for hiding this comment

feiskyer commented Jul 13, 2018

k8s-ci-robot commented Jul 13, 2018

k8s-github-robot commented Jul 13, 2018

rite2nikhil left a comment

Choose a reason for hiding this comment

feiskyer commented Jul 13, 2018

cpuguy83 commented Jul 13, 2018

marpaia commented Jul 17, 2018

djsly commented Sep 24, 2018

cpuguy83 Jul 10, 2018 •

edited

Loading

feiskyer Jul 11, 2018 •

edited

Loading