[DigitalOcean] Add load balancer support for master HA #8237

srikiz · 2019-12-31T08:34:49Z

This PR adds load balancer support for digital ocean cloud provider.

It would add a new load balancer based on a cluster master tag.
Incorporated the new load balancer address as part of the kuber config file.
Tested with 3 masters and a load balancer and was able to access the kubernetes cluster via the load balancer address from outside.
Deleting the cluster also deletes the load balancer.
Created a new cluster with the below command

./kops create cluster --cloud=digitalocean --name=dev5.k8s.local --networking=cilium --api-loadbalancer-type=public --master-count=3 --networking=cilium --zones=tor1 --ssh-public-key=~/.ssh/id_rsa.pub -v 10 --yes

Will be creating a new PR to add documentation for DO's support to multi-master with gossip based clustering.

k8s-ci-robot · 2019-12-31T08:34:57Z

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

rifelpet · 2019-12-31T13:06:00Z

/ok-to-test

srikiz · 2020-01-07T06:10:37Z

@timoreimann - Please review when you get a chance, thanks !

timoreimann

Looking good -- left a bunch of improvement suggestions.

timoreimann · 2020-01-07T22:41:09Z

pkg/model/domodel/api_loadbalancer.go

+
+	lbSpec := b.Cluster.Spec.API.LoadBalancer
+	if lbSpec == nil {
+		// Skipping API ELB creation; not requested in Spec


ELB is probably from AWS? If so, we should just call it LB in my opinion.

Similar for several other occurrences in this PR.

sure, will do.

timoreimann · 2020-01-07T22:47:10Z

pkg/model/domodel/api_loadbalancer.go

+		return fmt.Errorf("unhandled LoadBalancer type %q", lbSpec.Type)
+	}
+
+	loadbalancerName := "api-" + strings.Replace(b.ClusterName(), ".", "-", -1)


Nit: we could extract the result of the Replace call into a variable for reusage on the next line.

Sure, done.

timoreimann · 2020-01-07T22:50:52Z

pkg/model/domodel/api_loadbalancer.go

+
+	// Create LoadBalancer for API ELB
+	var loadbalancer *dotasks.LoadBalancer
+	{


Is there a reason to have the extra scope?

To me it looks like we can get rid of it and use := to define loadbalancer directly.

Sure, updated.

timoreimann · 2020-01-07T22:53:10Z

pkg/resources/digitalocean/cloud.go

+		// Note that this must match Digital Ocean's lb name
+		klog.V(2).Infof("Querying DO to find Loadbalancers for API (%q)", cluster.Name)
+
+		loadBalaners, _, err := c.LoadBalancers().List(context.TODO(), nil)


Typo in the first variable name (missing c).

Is List() from godo and thus needs paging?

yeah, that was the godo api to retrieve all the load balancers, so needed paging.

fixed typo as well.

timoreimann · 2020-01-07T23:04:40Z

pkg/resources/digitalocean/resources.go

+	_, err := c.Client.LoadBalancers.Delete(context.TODO(), lb.ID)
+
+	if err != nil {
+		return fmt.Errorf("failed to delete load balancer with name %s", lb.Name)


Please include err in the output.

timoreimann · 2020-01-07T23:20:36Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	}
+
+	for _, loadbalancer := range loadbalancers {
+		if loadbalancer.Name == fi.StringValue(lb.Name) {


Do you know if it's also possible to look up an LB by ID? That would allow us to do a single lookup using a different godo method, with no paging necessary whatsoever.

This really helps, thanks for letting me know. Changed the logic to use LB ID instead.

We should handle the case where the names don't match, e.g., logging.

I'm not sure I understand why we need the extra check though -- can we think of a case where the ID alone is insufficient?

timoreimann · 2020-01-07T23:24:03Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	}
+
+	klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name))
+	time.Sleep(10 * 3)


changed that to 10 seconds with time.Sleep(10 * time.Second)

timoreimann · 2020-01-08T08:58:46Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	return nil, errors.New("IP Address is still empty.")
+}
+
+func is_ipv4(host string) bool {


AFAICS, another way to implement this is to invoke net.ParseIP followed by To4 while checking that we always get a non-nil result on each step. That is,

ip := net.ParseIP(host) if ip == nil { return false } return ip.To4() != nil

sure, updated.

timoreimann · 2020-01-08T08:59:16Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	return nil, errors.New("IP Address is still empty.")
+}
+
+func is_ipv4(host string) bool {


Function name style should follow Go, i.e., be called isIPv4.

timoreimann · 2020-01-08T09:00:23Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+
+	address := loadBalancer.IP
+
+	if len(address) > 0 && is_ipv4(address) {


I think you can drop the len(address) > 0 check part, it's implicitly checked by is_ipv4.

srikiz · 2020-01-11T18:27:14Z

@timoreimann - I have incorporated all the comments. Please have a look when you get a chance. Thanks !

srikiz · 2020-01-11T19:16:56Z

/test pull-kops-e2e-kubernetes-aws

timoreimann

This looks really good -- just two smallish things left.

timoreimann · 2020-01-20T21:02:02Z

pkg/resources/digitalocean/cloud.go

+		// Note that this must match Digital Ocean's lb name
+		klog.V(2).Infof("Querying DO to find Loadbalancers for API (%q)", cluster.Name)
+
+		loadBalancers, _, err := c.LoadBalancers().List(context.TODO(), nil)


Does this need paging?

yeah, I missed that. Updated it.

timoreimann · 2020-01-20T21:05:19Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	}
+
+	for _, loadbalancer := range loadbalancers {
+		if loadbalancer.Name == fi.StringValue(lb.Name) {


We should handle the case where the names don't match, e.g., logging.

I'm not sure I understand why we need the extra check though -- can we think of a case where the ID alone is insufficient?

Add load balancer fi tasks Add load balancer builder for DO Fix go imports Implement FindIPAddress functionality Add load balancer api ingress status calls Add error checks for FindIPAddress Add delete LB option Update load balancer delete logic Revert make file changes revert utils code changes Revert NewDOCloud changes Remove minor code comments Update with gomod for dependencies

srikiz · 2020-01-28T10:08:31Z

@timoreimann - I have incorporated the changes.
Please have a look when you get a chance.

timoreimann

This is looking really good, thanks for cleaning it up further.

I caught just a few more smallish thing; after those, I think we should be ready.

timoreimann · 2020-01-29T08:27:47Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+			return nil, fmt.Errorf("load balancer service get request returned error %v", err)
+		}
+
+		// do another check to double-sure we are talking to the right load balancer.


This comment looks like it's obsolete by now?

timoreimann · 2020-01-29T08:30:39Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	}
+
+	// Loadbalancer = nil if not found
+	return nil, nil


nit: if we do

if fi.StringValue(lb.ID) == "" { // Loadbalancer = nil if not found return nil, nil } cloud := c.Cloud.(*digitalocean.Cloud) lbService := cloud.LoadBalancers() loadbalancer, _, err := lbService.Get(context.TODO(), fi.StringValue(lb.ID)) [...]

then we do not need to intend the if-branch for the case where the LB ID exists. The code also seems easier to read because the check for the empty LB ID and the action we take are not far apart anymore.

(I also moved the cloud and lbService variables next to their usage -- they are not needed unless we have an ID.)

sure, that's a good suggestion. Updated accordingly.

timoreimann · 2020-01-29T08:36:31Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	loadBalancerService := cloud.LoadBalancers()
+
+	klog.V(10).Infof("Find IP address for load balancer ID=%s", fi.StringValue(lb.ID))
+	loadBalancer, _, err := loadBalancerService.Get(context.TODO(), fi.StringValue(lb.ID))


Are we certain at this point that we have an LB ID? Genuine question as I don't know the kops work flow in this context that well.

Yes @timoreimann - we are certain we have an LB ID, but not certain about the LB IP Address. FindAddress is only called after RenderDO is invoked - we create a new LB Request in RenderDO that returns a LB ID. We use this LB ID to find if the IP Address is available.

timoreimann · 2020-01-29T08:38:56Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+		return &address, nil
+	}
+
+	klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name))


Can we match the description with the error below and mention that we are sleeping so that users aren't surprised by the delay, e.g.:

const lbWaitTime = 10 * time.Second klog.Errorf("IP address for LB %s not yet available -- sleeping %s", fi.StringValue(lb.Name), lbWaitTime)

I also extracted the wait time into a const so that we can reuse it in the actual time.Sleep call.

sure, updated.

timoreimann · 2020-01-29T08:39:50Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+		return &address, nil
+	}
+
+	klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name))


Should we do a klog.Warnf here? The delay should be expected.

timoreimann · 2020-01-29T08:40:23Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name))
+	time.Sleep(10 * time.Second)
+
+	klog.V(10).Infof("Sleeping for 10 seconds")


I'd drop this one given my improvement suggestion for the message above.

sure, done.

timoreimann · 2020-01-29T08:42:29Z

upup/pkg/fi/cloudup/dotasks/loadbalancer.go

+	return ip.To4() != nil
+}
+
+// terraformVolume represents the digitalocean_volume resource in terraform


This one looks like a trailing comment that shouldn't be here?

sure, updated.

timoreimann

This looks great to me, thanks a ton Sri.

LGTM (though it still needs approval from a kops maintainer / Kubernetes org member).

pkg/model/domodel/api_loadbalancer.go

rifelpet · 2020-02-02T15:47:54Z

The E2E failures are unrelated, so we can move forward with this regardless. Thanks!

/test pull-kops-e2e-kubernetes-aws

/lgtm
/approve

k8s-ci-robot · 2020-02-02T15:48:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet, srikiz

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

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

rifelpet · 2020-02-03T04:24:25Z

/test pull-kops-e2e-kubernetes-aws

…pstream-release-1.17 Automated cherry pick of #8237: Initial changes for load balancer task

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 31, 2019

k8s-ci-robot requested review from joshbranham and robinpercy December 31, 2019 08:35

srikiz force-pushed the DO-AddLoadBalancer branch from 3dac7a1 to 54292fb Compare December 31, 2019 10:18

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 Dec 31, 2019

timoreimann reviewed Jan 8, 2020

View reviewed changes

srikiz force-pushed the DO-AddLoadBalancer branch from 54292fb to d856dac Compare January 10, 2020 14:10

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 11, 2020

timoreimann reviewed Jan 20, 2020

View reviewed changes

srikiz added 5 commits January 27, 2020 15:03

Fix few review comments

d48bb7a

Update more review comments

ddf2a90

Address all review comments

38513b3

Incorporate review comments

e440397

srikiz force-pushed the DO-AddLoadBalancer branch from 1c5c54f to e440397 Compare January 28, 2020 08:54

timoreimann reviewed Jan 29, 2020

View reviewed changes

Incorporate further review comments

d8a9470

timoreimann approved these changes Feb 2, 2020

View reviewed changes

rifelpet reviewed Feb 2, 2020

View reviewed changes

pkg/model/domodel/api_loadbalancer.go Show resolved Hide resolved

k8s-ci-robot assigned rifelpet Feb 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2020

k8s-ci-robot merged commit 4c6b874 into kubernetes:master Feb 3, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 3, 2020

srikiz mentioned this pull request May 23, 2020

Automated cherry pick of #8237: Initial changes for load balancer task #9168

Merged

k8s-ci-robot added a commit that referenced this pull request May 25, 2020

Merge pull request #9168 from srikiz/automated-cherry-pick-of-#8237-u…

4d8c8ba

…pstream-release-1.17 Automated cherry pick of #8237: Initial changes for load balancer task

srikiz deleted the DO-AddLoadBalancer branch October 25, 2021 17:38


		address := loadBalancer.IP

		if len(address) > 0 && is_ipv4(address) {

[DigitalOcean] Add load balancer support for master HA #8237

[DigitalOcean] Add load balancer support for master HA #8237

Conversation

srikiz commented Dec 31, 2019

k8s-ci-robot commented Dec 31, 2019

rifelpet commented Dec 31, 2019

srikiz commented Jan 7, 2020

timoreimann 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikiz commented Jan 11, 2020

srikiz commented Jan 11, 2020

timoreimann 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

srikiz commented Jan 28, 2020

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Jan 29, 2020 • edited Loading

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

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

rifelpet commented Feb 2, 2020

k8s-ci-robot commented Feb 2, 2020

rifelpet commented Feb 3, 2020

timoreimann Jan 29, 2020 •

edited

Loading