Implement DigitalOcean Droplet FI Task #3707

andrewsykim · 2017-10-27T02:21:55Z

Implements cloudup fi tasks for DigitalOcean droplets. It makes a few assumptions to reduce the size of this PR, those will be addressed in future PRs.

Also does some cleanup in the DigitalOcean dns package.

chrislovecnm · 2017-10-27T05:19:01Z

hack/.packages is not in up to date. Please run:
  go list k8s.io/kops/... | grep -v vendor > hack/.packages

CI not happy

justinsb · 2017-10-27T05:15:29Z

cmd/kops/create_cluster.go

+				Name: subnetName,
+				// region and zone are the same for DO
+				Region: region,
+				Zone:   region,


Hmm... we should be able to cope without a Zone being set, but I'm guessing something breaks ?

Yes, the FindZonesForInstanceGroup function only inserts to list if Zone is set, and so this check fails. Any objections to add a region check there?

No objections, particularly if we can scope it to DO. The root issue there is that EBS volumes on AWS / PDs on GCE are single zone, and thus we need to figure out the zone. But it looks like DO volumes are regional (wow!) so it's fine to relax that check. I would want to see the existing checks for AWS & GCE continue to work (likely by a switch statement on the cloudproviderid).

justinsb · 2017-10-27T05:16:26Z

cmd/kops/create_cluster.go

+			}
+			cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet)
+		}
+		zoneToSubnetMap[region] = subnet


I'd be in favor of a region flag in future, but we can run with this...

justinsb · 2017-10-27T05:16:54Z

pkg/apis/kops/validation/legacy.go

@@ -80,9 +80,6 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error {
 	switch kops.CloudProviderID(c.Spec.CloudProvider) {
 	case kops.CloudProviderBareMetal:
 		requiresSubnets = false
-		if c.Spec.NetworkCIDR != "" {
-			return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal")


justinsb · 2017-10-27T05:18:16Z

pkg/model/domodel/droplets.go

+var _ fi.ModelBuilder = &DropletBuilder{}
+
+func (d *DropletBuilder) Build(c *fi.ModelBuilderContext) error {
+	sshKeyName, err := d.SSHKeyName()


We could tweak SSHKeyName to return something you don't have to parse if that would help...

Noted! Didn't want to creep too much into other packages but this seems like a good future optimization!

justinsb · 2017-10-27T05:19:48Z

pkg/model/domodel/droplets.go

+	splitSSHKeyName := strings.Split(sshKeyName, "-")
+	sshKeyFingerPrint := splitSSHKeyName[len(splitSSHKeyName)-1]
+
+	// replace "." with "-" since DO API does not accept "."


GCE has a similar issue, and has SafeClusterName - you might want to create a similar function for DO, depending on how often you end up using it!

ahh, that's useful noted!

justinsb · 2017-10-27T05:22:07Z

pkg/model/domodel/droplets.go

+		droplet.Name = fi.String(name)
+		// during alpha support we only allow 1 region
+		// validation for only 1 region is done at this point
+		droplet.Region = fi.String(d.Cluster.Spec.Subnets[0].Region)


FYI, single-region is actually pretty deep in the k8s design today

justinsb · 2017-10-27T05:22:59Z

pkg/resources/digitalocean/dns/dns.go

-	return nil
-}
-
-func handleResponse(resp *godo.Response) error {


I take it that godo now handles this for you?

Yes! That was actually always the case, just only recently found out :)

justinsb · 2017-10-27T05:27:16Z

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

+var _ fi.CompareWithID = &Droplet{}
+
+func (d *Droplet) CompareWithID() *string {
+	return d.ID


CompareWithID is used to identify two objects that are the same thing, in particular when an object refers to another object. e.g. If Image was actually an object here, we might have two Droplet objects that both set a stub Image, and we might define a task for that Image, and we want to match all 3. CompareWithID is what is matched to match them.

The reason we care about reconciling the duplicates is primarily for dependency analysis, so we can know to run the Image task before the Droplet task in this example.

So in this case, you probably want to compare by Name, because that is what you would set in a stub Droplet I would guess - in particular you're unlikely to know the ID. But in practice, it probably doesn't matter anyway, unless you're actually creating stub Droplets in another task.

justinsb · 2017-10-27T05:29:07Z

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

+
+func (_ *Droplet) RenderDO(t *do.DOAPITarget, a, e, changes *Droplet) error {
+	if a != nil {
+		return nil


It can be a good idea to do something like this:

if a != nil && changes != nil { empty := &Disk{} if !reflect.DeepEqual(empty, changes) { return fmt.Errorf("Cannot apply changes to Disk: %v", changes) } }

Or at least warn that you're not updating e.g. the Image, even if it isn't an error. But this is the hardest bit (e.g. if someone changes the image, what do you want to do), so I think it's fine to build this up relatively incrementally as well..

justinsb · 2017-10-27T05:30:23Z

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

+		if changes.Size != nil {
+			return fi.CannotChangeField("Size")
+		}
+		if changes.Image != nil {


Ah - well there we go - they can't change the image 👍

justinsb · 2017-10-27T05:32:40Z

Looks like you added a package so you need to run go list k8s.io/kops/... | grep -v vendor > hack/.packages, at least CI thinks so! (and we should probably reconsider that in light of the bazel progress, although adding packages is relatively rare).

Code LGTM - I added some comments primarily to try to explain what some of the things are used for, but I think we should get this in and iterate (once CI is happy!)

/lgtm

k8s-github-robot · 2017-10-27T13:49:20Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @andrewsykim @justinsb

justinsb · 2017-10-27T14:55:29Z

/lgtm

Thanks @andrewsykim !

k8s-github-robot · 2017-10-27T14:55:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-27T14:57:39Z

/test all [submit-queue is verifying that this PR is safe to merge]

andrewsykim · 2017-10-27T14:58:28Z

I'll have a follow up PR soon with the improvements you suggested!

k8s-github-robot · 2017-10-27T15:30:57Z

Automatic merge from submit-queue.

andrewsykim · 2017-10-31T02:41:57Z

ref: #2150

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 27, 2017

andrewsykim force-pushed the droplet branch from ce68c28 to b5e8eb1 Compare October 27, 2017 04:38

justinsb reviewed Oct 27, 2017

View reviewed changes

k8s-ci-robot assigned justinsb Oct 27, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2017

Implement DigitalOcean Droplet FI Task

45bf2cb

andrewsykim force-pushed the droplet branch from b5e8eb1 to 45bf2cb Compare October 27, 2017 13:48

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2017

k8s-github-robot merged commit a4d6895 into kubernetes:master Oct 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement DigitalOcean Droplet FI Task #3707

Implement DigitalOcean Droplet FI Task #3707

andrewsykim commented Oct 27, 2017

chrislovecnm commented Oct 27, 2017

justinsb Oct 27, 2017

andrewsykim Oct 27, 2017

justinsb Oct 27, 2017

justinsb Oct 27, 2017

andrewsykim Oct 27, 2017

justinsb Oct 27, 2017

justinsb Oct 27, 2017

andrewsykim Oct 27, 2017

justinsb Oct 27, 2017

andrewsykim Oct 27, 2017

justinsb Oct 27, 2017

justinsb Oct 27, 2017

andrewsykim Oct 27, 2017

justinsb Oct 27, 2017

justinsb Oct 27, 2017

justinsb Oct 27, 2017

justinsb commented Oct 27, 2017 •

edited

Loading

k8s-github-robot commented Oct 27, 2017

justinsb commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

andrewsykim commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

andrewsykim commented Oct 31, 2017

Implement DigitalOcean Droplet FI Task #3707

Implement DigitalOcean Droplet FI Task #3707

Conversation

andrewsykim commented Oct 27, 2017

chrislovecnm commented Oct 27, 2017

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

justinsb commented Oct 27, 2017 • edited Loading

k8s-github-robot commented Oct 27, 2017

justinsb commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

andrewsykim commented Oct 27, 2017

k8s-github-robot commented Oct 27, 2017

andrewsykim commented Oct 31, 2017

justinsb commented Oct 27, 2017 •

edited

Loading