Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Sep 16, 2019
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
35ca8b0
Update DO objects to use tags
srikiz Sep 5, 2019
84da7d0
Add tags for droplets and volumes
srikiz Sep 11, 2019
a5cda66
Address review comments
srikiz Sep 12, 2019
88850e2
Additional review comments addressed, also reverted back to Replace i…
srikiz Sep 12, 2019
ee18312
Update bazel
srikiz Sep 13, 2019
45e3e0d
Update build.bazel
srikiz Sep 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions pkg/model/components/etcdmanager/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Same for similar usages of Replace within this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


config.VolumeTag = []string{
fmt.Sprintf("kubernetes.io/cluster=%s", b.Cluster.Name),
do.TagNameEtcdClusterPrefix + etcdCluster.Name,
do.TagNameRolePrefix + "master=1",
fmt.Sprintf("%s=%s", do.TagKubernetesClusterNamePrefix, safeClusterName),
do.TagKubernetesClusterIndex,
}
config.VolumeNameTag = do.TagNameEtcdClusterPrefix + etcdCluster.Name

Expand Down
17 changes: 13 additions & 4 deletions pkg/model/domodel/droplets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@ limitations under the License.
package domodel

import (
"strings"

"k8s.io/kops/pkg/model"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/do"
"k8s.io/kops/upup/pkg/fi/cloudup/dotasks"
"strconv"
"strings"
)

// DropletBuilder configures droplets for the cluster
Expand All @@ -44,8 +45,9 @@ func (d *DropletBuilder) Build(c *fi.ModelBuilderContext) error {
sshKeyFingerPrint := splitSSHKeyName[len(splitSSHKeyName)-1]

// replace "." with "-" since DO API does not accept "."
clusterTag := "KubernetesCluster:" + strings.Replace(d.ClusterName(), ".", "-", -1)
clusterTag := do.TagKubernetesClusterNamePrefix + ":" + strings.Replace(d.ClusterName(), ".", "-", -1)

indexCount := 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated.

// In the future, DigitalOcean will use Machine API to manage groups,
// for now create d.InstanceGroups.Spec.MinSize amount of droplets
for _, ig := range d.InstanceGroups {
Expand All @@ -61,7 +63,14 @@ func (d *DropletBuilder) Build(c *fi.ModelBuilderContext) error {
droplet.Size = fi.String(ig.Spec.MachineType)
droplet.Image = fi.String(ig.Spec.Image)
droplet.SSHKey = fi.String(sshKeyFingerPrint)
droplet.Tags = []string{clusterTag}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps, thanks. Updated.

}

userData, err := d.BootstrapScript.ResourceNodeUp(ig, d.Cluster)
if err != nil {
Expand Down
9 changes: 9 additions & 0 deletions pkg/model/master_volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"k8s.io/kops/upup/pkg/fi/cloudup/aliup"
"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
"k8s.io/kops/upup/pkg/fi/cloudup/do"
"k8s.io/kops/upup/pkg/fi/cloudup/dotasks"
"k8s.io/kops/upup/pkg/fi/cloudup/gce"
"k8s.io/kops/upup/pkg/fi/cloudup/gcetasks"
Expand Down Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes total sense. 👍

tags[do.TagKubernetesClusterIndex] = gce.SafeClusterName(m.Name)

// We always add an owned tags (these can't be shared)
tags[do.TagKubernetesClusterNamePrefix] = gce.SafeClusterName(b.Cluster.ObjectMeta.Name)

t := &dotasks.Volume{
Name: s(name),
Lifecycle: b.Lifecycle,
SizeGB: fi.Int64(int64(volumeSize)),
Region: s(zone),
Tags: tags,
}

c.AddTask(t)
Expand Down
4 changes: 3 additions & 1 deletion upup/pkg/fi/cloudup/do/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ import (
"k8s.io/kops/upup/pkg/fi"
)

const TagNameEtcdClusterPrefix = "k8s.io/etcd/"
const TagKubernetesClusterIndex = "k8s-index"
const TagNameEtcdClusterPrefix = "etcdCluster-"
const TagNameRolePrefix = "k8s.io/role/"
const TagKubernetesClusterNamePrefix = "KubernetesCluster"

func NewDOCloud(region string) (fi.Cloud, error) {
return digitalocean.NewCloud(region)
Expand Down
30 changes: 17 additions & 13 deletions upup/pkg/fi/cloudup/dotasks/droplet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ package dotasks
import (
"context"
"errors"

//"strconv"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"github.com/digitalocean/godo"

"k8s.io/klog"
"k8s.io/kops/pkg/resources/digitalocean"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/do"
Expand Down Expand Up @@ -142,21 +143,24 @@ func (_ *Droplet) RenderDO(t *do.DOAPITarget, a, e, changes *Droplet) error {
newDropletCount = expectedCount - actualCount
}

var dropletNames []string
for i := 0; i < newDropletCount; i++ {
dropletNames = append(dropletNames, fi.StringValue(e.Name))
_, _, err = t.Cloud.Droplets().Create(context.TODO(), &godo.DropletCreateRequest{
Name: fi.StringValue(e.Name),
Region: fi.StringValue(e.Region),
Size: fi.StringValue(e.Size),
Image: godo.DropletCreateImage{Slug: fi.StringValue(e.Image)},
PrivateNetworking: true,
Tags: e.Tags,
UserData: userData,
SSHKeys: []godo.DropletCreateSSHKey{{Fingerprint: fi.StringValue(e.SSHKey)}},
})

if err != nil {
klog.Errorf("Error creating droplet with Name=%s", fi.StringValue(e.Name))
return err
}
}

_, _, err = t.Cloud.Droplets().CreateMultiple(context.TODO(), &godo.DropletMultiCreateRequest{
Names: dropletNames,
Region: fi.StringValue(e.Region),
Size: fi.StringValue(e.Size),
Image: godo.DropletCreateImage{Slug: fi.StringValue(e.Image)},
PrivateNetworking: true,
Tags: e.Tags,
UserData: userData,
SSHKeys: []godo.DropletCreateSSHKey{{Fingerprint: fi.StringValue(e.SSHKey)}},
})
return err
}

Expand Down
18 changes: 17 additions & 1 deletion upup/pkg/fi/cloudup/dotasks/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ package dotasks

import (
"context"

"github.com/digitalocean/godo"
"strings"

"k8s.io/klog"
"k8s.io/kops/pkg/resources/digitalocean"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/do"
Expand All @@ -35,6 +36,7 @@ type Volume struct {

SizeGB *int64
Region *string
Tags map[string]string
}

var _ fi.CompareWithID = &Volume{}
Expand Down Expand Up @@ -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")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, v := range e.Tags {
s := []string{k, v}

// 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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great, updated !

}

volService := t.Cloud.Volumes()
_, _, err := volService.CreateVolume(context.TODO(), &godo.VolumeCreateRequest{
Name: fi.StringValue(e.Name),
Region: fi.StringValue(e.Region),
SizeGigaBytes: fi.Int64Value(e.SizeGB),
Tags: tagArray,
})

return err
}

Expand Down