[DO-7442] Digital Ocean add consistent volume and droplet tags for multi master feature #7566

srikiz · 2019-09-11T19:06:55Z

Digital Ocean doesn't support Multi master yet. This PR introduces tags that helps in associating a specific droplet to a volume.
I have tested with by making the needed changes in etcd-manager and they are getting linked up properly. I'll be sending a PR for etcd-manager repo separately once this is merged.

I'll also be adding a new PR to add DO support for protokube's gossip cluster to add peers as part of the next step.

k8s-ci-robot · 2019-09-11T19:07:02Z

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.

timoreimann

Left a few minor comments; overall, this looks good to me but it needs another pair of 👁s from someone more familiar with kops.

Thanks!

timoreimann · 2019-09-11T20:42:39Z

pkg/model/components/etcdmanager/model.go

@@ -393,10 +393,12 @@ func (b *EtcdManagerBuilder) buildPod(etcdCluster *kops.EtcdClusterSpec) (*v1.Po
 		case kops.CloudProviderDO:
 			config.VolumeProvider = "do"

+			// DO does not support . in tags / names
+			safeClusterName := strings.Replace(b.Cluster.Name, ".", "-", -1)


nit: this could also call strings.ReplaceAll to allow dropping the last parameter (-1).

Same for similar usages of Replace within this PR.

timoreimann · 2019-09-11T20:45:04Z

pkg/model/domodel/droplets.go


+	indexCount := 0


Maybe call this masterIndexCount to make clearer that this is about masters only?

Sure, updated.

timoreimann · 2019-09-11T20:47:33Z

pkg/model/domodel/droplets.go

+			clusterTagIndex := do.TagKubernetesClusterIndex + ":" + strconv.Itoa(indexCount)
+			droplet.Tags = []string{clusterTag, clusterTagIndex}
+		} else {
+			droplet.Tags = []string{clusterTag}


Alternatively, we could invoke this line before line 67 and append the tags in line 70 like

droplet.Tags = append(droplet.Tags, clusterTagIndex)

in order to drop the else branch.

This helps, thanks. Updated.

timoreimann · 2019-09-11T20:51:58Z

pkg/model/master_volumes.go

@@ -184,11 +185,19 @@ func (b *MasterVolumeBuilder) addDOVolume(c *fi.ModelBuilderContext, name string
 		name = name[:64]
 	}

+	tags := make(map[string]string)
+	tags[do.TagNameEtcdClusterPrefix+etcd.Name] = gce.SafeClusterName(m.Name)


I wonder if we should move SafeClusterName out of the gcp package now that DO uses it as well. This would also indicate to developers that changes to the function do not affect GCP only. WDYT?

I created a new function in package do - calling it as SafeDOClusterName, otherwise, I had to change the package name to something common, and update all the GCE references to use the common package. I thought this is more safer, since the change will only be reflecting to the DO code now. Is this OK?

Yeah that makes total sense. 👍

timoreimann · 2019-09-11T20:52:52Z

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

@@ -19,9 +19,10 @@ package dotasks
 import (
 	"context"
 	"errors"
-
+	//"strconv"


Should be dropped.

timoreimann · 2019-09-11T20:56:48Z

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

@@ -108,12 +110,26 @@ func (_ *Volume) RenderDO(t *do.DOAPITarget, a, e, changes *Volume) error {
 		return nil
 	}

+	tagArray := []string{}
+
+	klog.V(2).Info("Looping DO tag arrays")


AFAIK V(2) is the standard logging level. If so, then this message might be rather verbose for that level. Maybe push it out to something higher (or drop the line if it was mostly for debugging purposes)?

Similar for line 121.

Sure dropped the line and updated the other klog to V(10)

timoreimann · 2019-09-11T21:00:29Z

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

+		// DO tags don't accept =. Separate the key and value with an ":"
+		strJoin := strings.Join(s, ":")
+		klog.V(2).Infof("DO - Join the volume tag - %s", strJoin)
+		tagArray = append(tagArray, strJoin)


We could also do a direct

tagArray = append(tagArray, fmt.Sprintf("%s:%s", k, v)

here and drop s and strJoin since we're always joining a pair only.

Sounds great, updated !

timoreimann

Two small nits, otherwise all good from my end.

timoreimann · 2019-09-12T05:26:06Z

upup/pkg/fi/cloudup/do/cloud.go

 const TagNameRolePrefix = "k8s.io/role/"
+const TagKubernetesClusterNamePrefix = "KubernetesCluster"
+
+func SafeDOClusterName(clusterName string) string {


We can drop the DO part since it's part of the do package.

timoreimann · 2019-09-12T05:26:35Z

upup/pkg/fi/cloudup/do/cloud.go

+const TagKubernetesClusterNamePrefix = "KubernetesCluster"
+
+func SafeDOClusterName(clusterName string) string {
+	// GCE does not support . in tags / names


Should say DO instead of GCE.

Thanks, updated.

srikiz · 2019-09-12T06:31:49Z

@timoreimann - The build is failing in Go 1.11 version. It looks like I have to revert ReplaceAll to use Replace instead - TravisCI builds KOPS on 1.11 Go version as well, and Go 1.11 version it didn't ReplaceAll method. It was newly introducted in 1.12.

timoreimann · 2019-09-12T07:16:14Z

@srikiz oh great point, didn't realize kops was still building for Go 1.11.

Let's stick to Remove then. 👍

…nstead of ReplaceAll

timoreimann · 2019-09-12T16:41:57Z

/assign @gambol99

rdrgmnzs · 2019-09-12T20:56:25Z

/ok-to-test

srikiz · 2019-09-13T07:06:03Z

/retest

zetaab

/lgtm

chrisz100 · 2019-09-16T13:00:55Z

/approve

k8s-ci-robot · 2019-09-16T13:01:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, 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~~ [chrisz100]

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

…66-origin-release-1.15 Automated cherry pick of #7566: Update DO objects to use tags

#7566-origin-release-1.14 Automated cherry pick of #7467: Update godo dependency to v1.19.0 version #7566: Update DO objects to use tags

srikiz added 2 commits September 6, 2019 00:01

Update DO objects to use tags

35ca8b0

Add tags for droplets and volumes

84da7d0

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 11, 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2019

k8s-ci-robot requested review from granular-ryanbonham and robinpercy September 11, 2019 19:07

timoreimann reviewed Sep 11, 2019

View reviewed changes

Address review comments

a5cda66

timoreimann approved these changes Sep 12, 2019

View reviewed changes

Additional review comments addressed, also reverted back to Replace i…

88850e2

…nstead of ReplaceAll

srikiz mentioned this pull request Sep 12, 2019

[Digital Ocean] Support for multi master KOPS cluster #7442

Closed

k8s-ci-robot assigned gambol99 Sep 12, 2019

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 Sep 12, 2019

srikiz added 2 commits September 13, 2019 12:42

Update bazel

ee18312

Update build.bazel

45e3e0d

zetaab approved these changes Sep 16, 2019

View reviewed changes

k8s-ci-robot assigned zetaab Sep 16, 2019

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

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

k8s-ci-robot merged commit 070498b into kubernetes:master Sep 16, 2019

srikiz mentioned this pull request Sep 17, 2019

[DO-7442] Fix logic for Digital Ocean volume tag kopeio/etcd-manager#254

Merged

This was referenced Sep 18, 2019

Automated cherry pick of #7566: Update DO objects to use tags #7618

Merged

Automated cherry pick of #7566: Update DO objects to use tags #7619

Closed

k8s-ci-robot added a commit that referenced this pull request Sep 18, 2019

Merge pull request #7618 from mikesplain/automated-cherry-pick-of-#75…

5b58fd5

…66-origin-release-1.15 Automated cherry pick of #7566: Update DO objects to use tags

mikesplain mentioned this pull request Sep 19, 2019

Automated cherry pick of #7467: Update godo dependency to v1.19.0 version #7566: Update DO objects to use tags #7635

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 19, 2019

Merge pull request #7635 from mikesplain/automated-cherry-pick-of-#7467-

c85f449

#7566-origin-release-1.14 Automated cherry pick of #7467: Update godo dependency to v1.19.0 version #7566: Update DO objects to use tags

srikiz deleted the DO-7442-AddTagsForCreateVolumeRequest branch September 25, 2019 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO-7442] Digital Ocean add consistent volume and droplet tags for multi master feature #7566

[DO-7442] Digital Ocean add consistent volume and droplet tags for multi master feature #7566

srikiz commented Sep 11, 2019

k8s-ci-robot commented Sep 11, 2019

timoreimann left a comment

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann Sep 11, 2019

srikiz Sep 12, 2019

timoreimann left a comment

timoreimann Sep 12, 2019

srikiz Sep 12, 2019

timoreimann Sep 12, 2019

srikiz Sep 12, 2019

srikiz commented Sep 12, 2019

timoreimann commented Sep 12, 2019

timoreimann commented Sep 12, 2019

rdrgmnzs commented Sep 12, 2019

srikiz commented Sep 13, 2019

zetaab left a comment

chrisz100 commented Sep 16, 2019

k8s-ci-robot commented Sep 16, 2019

[DO-7442] Digital Ocean add consistent volume and droplet tags for multi master feature #7566

[DO-7442] Digital Ocean add consistent volume and droplet tags for multi master feature #7566

Conversation

srikiz commented Sep 11, 2019

k8s-ci-robot commented Sep 11, 2019

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

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

srikiz commented Sep 12, 2019

timoreimann commented Sep 12, 2019

timoreimann commented Sep 12, 2019

rdrgmnzs commented Sep 12, 2019

srikiz commented Sep 13, 2019

zetaab left a comment

Choose a reason for hiding this comment

chrisz100 commented Sep 16, 2019

k8s-ci-robot commented Sep 16, 2019