Cross-Zone Load Balancing for API ELB #6958

austinmoore- · 2019-05-13T20:52:24Z

Adds configuration (spec.api.loadBalancer.crossZoneLoadBalancing) to enable cross-zone load balancing on the API ELB. See the added docs for details.

k8s-ci-robot · 2019-05-13T20:52:31Z

Hi @austinmoore-. 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.

austinmoore- · 2019-05-13T21:07:11Z

/assign @KashifSaadat

granular-ryanbonham · 2019-05-14T04:47:14Z

upup/pkg/fi/cloudup/awstasks/loadbalancer_attributes.go

@@ -166,6 +166,9 @@ func (_ *LoadBalancer) modifyLoadBalancerAttributes(t *awsup.AWSAPITarget, a, e,
 	if e.ConnectionSettings != nil && e.ConnectionSettings.IdleTimeout != nil {
 		request.LoadBalancerAttributes.ConnectionSettings.IdleTimeout = e.ConnectionSettings.IdleTimeout
 	}
+	if e.CrossZoneLoadBalancing != nil && e.CrossZoneLoadBalancing.Enabled != nil {


Should this be an else off the if statement on line 136?

Ah yep 👍

KashifSaadat

One minor comment, thanks for the great PR :)

KashifSaadat · 2019-05-16T14:38:48Z

pkg/model/awsmodel/api_loadbalancer.go

@@ -153,6 +153,13 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 			Tags: tags,
 		}

+		// Only set if specified so we don't change existing logic
+		if lbSpec.CrossZoneLoadBalancing != nil {


Small test:

Deploy with crossZoneLoadBalancing unset; by default the value on the ELB is set to false

Set the above in kops spec with value true; this is successfully set to true in the ELB Spec

Revert to original configuration (not explicitly setting in the LoadBalancerSpec to false); no change to the ELB configuration (the value remains true on the ELB resource)

Just a small suggestion that if crossZoneLoadBalancing is unset in the LoadBalancerSpec, the ELB property is explicitly set to false (maintains existing behaviour), so the above potential use-case is satisfied.

Yeah, you're correct, it remains true. I'll make that change

KashifSaadat · 2019-05-16T14:41:17Z

/ok-to-test

justinsb · 2019-06-21T01:10:58Z

This looks good - thanks @austinmoore-

I think at some stage in the future we might well want to make this the default, but we'd have to have a big release note for that so we can hold off for now :-) The other option is just to make it the default for new clusters that use an ELB.

justinsb · 2019-06-21T01:09:16Z

tests/integration/update_cluster/api_elb_cross_zone/in-v1alpha2.yaml

@@ -0,0 +1,96 @@
+apiVersion: kops/v1alpha2


It's often easier to throw this into "complex" or another existing test (and it's OK to do so, at least when it's a whole test for a single field)

justinsb · 2019-06-21T01:11:17Z

/approve
/lgtm

k8s-ci-robot · 2019-06-21T01:11:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: austinmoore-, justinsb

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~~ [justinsb]

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

austinmoore- · 2019-06-21T14:53:06Z

/retest

rifelpet · 2019-06-23T21:39:40Z

cmd/kops/integration_test.go

@@ -80,6 +80,11 @@ func TestComplex(t *testing.T) {
 	runTestAWS(t, "complex.example.com", "complex", "legacy-v1alpha2", false, 1, true, nil)
 }

+// TestCrossZone tests that the the cross zone setting on the API ELB is set properly
+func TestCrossZone(t *testing.T) {
+	runTestAWS(t, "crosszone.example.com", "api_elb_cross_zone", "v1alpha2", false, 1, true, nil)


The function signature for runTestAWS recently changed in the master branch. You'll want to rebase your branch and add the new boolean argument. Alternatively you can follow justinsb's advice and add the setting to the existing complex integration test.

austinmoore- · 2019-07-24T14:28:33Z

/retest

…_OVERRIDES

austinmoore- · 2019-07-31T20:03:46Z

@justinsb or @rifelpet can you do another quick review and lgtm for the rebase?

rifelpet · 2019-07-31T20:16:32Z

/lgtm

Thanks @austinmoore- !

rifelpet · 2019-07-31T23:53:16Z

I'm hoping that #7348 will fix these job failures

rifelpet · 2019-08-01T00:41:05Z

/retest

Cherry pick of #6958 onto release-1.14

This should cause each node of the load-balancer to distribute traffic across *all* worker nodes. Without it, each LB node (one per AZ) only distributes traffic to worker nodes in the same AZ. This is a less resilient configuration. In testing, applying this change was extremely quick, and caused no downtime to the test ingress, with a load test running while the change was applied. More info: * kubernetes/kops#6958 * https://gist.github.com/digitalronin/6a131807ff90b2835fd9d53092bde44b * https://mojdt.slack.com/archives/C57UPMZLY/p1604482776372900 * https://console.aws.amazon.com/support/home#/case/?displayId=7467429111&language=en

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2019

k8s-ci-robot requested review from rdrgmnzs and robinpercy May 13, 2019 20:53

k8s-ci-robot assigned KashifSaadat May 13, 2019

granular-ryanbonham reviewed May 14, 2019

View reviewed changes

KashifSaadat suggested changes May 16, 2019

View reviewed changes

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 May 16, 2019

austinmoore- force-pushed the api-elb-cross-zone branch from e200407 to 2b1e236 Compare May 20, 2019 21:23

justinsb reviewed Jun 21, 2019

View reviewed changes

k8s-ci-robot assigned justinsb Jun 21, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019

rifelpet reviewed Jun 23, 2019

View reviewed changes

austinmoore- added 6 commits July 26, 2019 12:21

Expose --lifecycle-overrides flag as an env var called KOPS_LIFECYCLE…

938dc11

…_OVERRIDES

Add configuration to set cross-zone load balancing on api load balancer

d59ac28

Add integration test for cross-zone-load-balancing

66a8d1d

Add docs for API ELB cross-zone configuration

08957a0

Cleanup

c18f30a

Default CrossZoneLoadBalancing to false if not set

4485b11

Fix test after rebase

29b4662

austinmoore- force-pushed the api-elb-cross-zone branch from 2b1e236 to 29b4662 Compare July 26, 2019 16:29

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 26, 2019

k8s-ci-robot assigned rifelpet Jul 31, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2019

k8s-ci-robot merged commit c705116 into kubernetes:master Aug 1, 2019

austinmoore- deleted the api-elb-cross-zone branch August 1, 2019 14:29

justinsb mentioned this pull request Aug 1, 2019

Cherry pick of #6958 onto release-1.14 #7362

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 1, 2019

Merge pull request #7362 from justinsb/cherrypick_6958_release-1.14

c268fcb

Cherry pick of #6958 onto release-1.14

digitalronin mentioned this pull request Nov 4, 2020

Enable cross-zone load-balancing ministryofjustice/cloud-platform-terraform-kops#8

Merged

rifelpet mentioned this pull request Feb 3, 2021

[Question / Feature Request] AWS NLB - EIP Allocations #10706

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cross-Zone Load Balancing for API ELB #6958

Cross-Zone Load Balancing for API ELB #6958

austinmoore- commented May 13, 2019

k8s-ci-robot commented May 13, 2019

austinmoore- commented May 13, 2019

granular-ryanbonham May 14, 2019

austinmoore- May 14, 2019

KashifSaadat left a comment

KashifSaadat May 16, 2019

austinmoore- May 20, 2019 •

edited

Loading

KashifSaadat commented May 16, 2019

justinsb commented Jun 21, 2019

justinsb Jun 21, 2019

justinsb commented Jun 21, 2019

k8s-ci-robot commented Jun 21, 2019

austinmoore- commented Jun 21, 2019

rifelpet Jun 23, 2019

austinmoore- commented Jul 24, 2019

austinmoore- commented Jul 31, 2019

rifelpet commented Jul 31, 2019

rifelpet commented Jul 31, 2019

rifelpet commented Aug 1, 2019

Cross-Zone Load Balancing for API ELB #6958

Cross-Zone Load Balancing for API ELB #6958

Conversation

austinmoore- commented May 13, 2019

k8s-ci-robot commented May 13, 2019

austinmoore- commented May 13, 2019

granular-ryanbonham May 14, 2019

Choose a reason for hiding this comment

austinmoore- May 14, 2019

Choose a reason for hiding this comment

KashifSaadat left a comment

Choose a reason for hiding this comment

KashifSaadat May 16, 2019

Choose a reason for hiding this comment

austinmoore- May 20, 2019 • edited Loading

Choose a reason for hiding this comment

KashifSaadat commented May 16, 2019

justinsb commented Jun 21, 2019

justinsb Jun 21, 2019

Choose a reason for hiding this comment

justinsb commented Jun 21, 2019

k8s-ci-robot commented Jun 21, 2019

austinmoore- commented Jun 21, 2019

rifelpet Jun 23, 2019

Choose a reason for hiding this comment

austinmoore- commented Jul 24, 2019

austinmoore- commented Jul 31, 2019

rifelpet commented Jul 31, 2019

rifelpet commented Jul 31, 2019

rifelpet commented Aug 1, 2019

austinmoore- May 20, 2019 •

edited

Loading