[Issue-7870] kops controller support for digital ocean #7961

srikiz · 2019-11-19T20:16:42Z

This PR addresses the issue mentioned in #7870
for digital ocean cloud provider.

Added a kops-controller that updates the node labels. Tested it and it seems to be working as expected.
Please see the attached image.

k8s-ci-robot · 2019-11-19T20:16:50Z

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-11-19T20:18:21Z

/ok-to-test

zetaab

seems good for me, please fix those comments that I mentioned

also check verify-packages output, its missing something from hack

zetaab · 2019-11-19T20:20:54Z

pkg/nodeidentity/do/identify.go

+	return string(bodyBytes), nil
+}
+
+// IdentifyNode queries OpenStack for the node identity information


openstack :)

thanks, updated.

zetaab · 2019-11-19T20:21:06Z

pkg/nodeidentity/do/identify.go

+	return token, nil
+}
+
+// New creates and returns a nodeidentity.Identifier for Nodes running on OpenStack


zetaab · 2019-11-19T20:21:17Z

pkg/nodeidentity/do/identify.go

+	"k8s.io/kops/pkg/nodeidentity"
+)
+
+// nodeIdentifier identifies a node from EC2


timoreimann

Looking good in general; left a few comments.

timoreimann · 2019-11-20T08:43:41Z

pkg/nodeidentity/do/identify.go

+func getMetadata(url string) (string, error) {
+	resp, err := http.Get(url)
+	if err != nil {
+		return "", err


We may want to annotate this error and the one one in line 112 to be able to distinguish the two; i.e., do something like

return "", fmt.Errorf("failed to get metadata URL %s: %v", url, err)

here and similarly below.

sure, updated.

timoreimann · 2019-11-20T08:46:26Z

pkg/nodeidentity/do/identify.go

+
+	instanceID := strings.TrimPrefix(providerID, "digitalocean://")
+	if strings.HasPrefix(instanceID, "/") {
+		instanceID = strings.TrimPrefix(instanceID, "/")


What's the reason we check for a leading slash here?

i think this is copied from openstack. In openstack the instanceid is in format openstack:///[id] basically this supports both two and three slashes. No idea what is the format for do

ah, interesting, thanks for sharing that bit of info @zetaab.

The provider ID is a parameter passed in by kubelet, so I think it depends on how it's configured in kops. In the managed Kubernetes product at DigitalOcean, we use the digitalocean://<id> format -- hoping that's an established convention for Kubernetes clusters running on DO infrastructure.

@srikiz you have much more familiar with the DO integration in kops than me -- could you tell (or look at an existing kops-managed cluster on DO)?

I looked into it a little more and it looks like we use the digitalocean:// format.
This is not set in the KOPS code though - looks like this is set in the cloud-provider repo - https://github.com/kubernetes/cloud-provider/blob/master/cloud.go#L103.
The associated changes to cloud-controller-manager for DO is done here -https://github.com/digitalocean/digitalocean-cloud-controller-manager/pull/36/files
I have updated the code to use the same pattern.

Thanks for digging, Sri.

timoreimann · 2019-11-20T08:48:25Z

pkg/nodeidentity/do/identify.go

+
+func (i *nodeIdentifier) getInstanceGroup(instanceID string) (string, error) {
+
+	dropletID, err := strconv.Atoi(instanceID)


To me, this piece still belongs to the provider ID parsing functionality, so I'd move it into IdentifyNode. What do you think?

timoreimann · 2019-11-20T08:48:46Z

pkg/nodeidentity/do/identify.go

+
+func (i *nodeIdentifier) getInstanceGroup(instanceID string) (string, error) {
+
+	dropletID, err := strconv.Atoi(instanceID)


The error still needs to be handled.

Updated and moved this to IdentifyNode.

timoreimann · 2019-11-20T08:49:01Z

pkg/nodeidentity/do/identify.go

+	dropletID, err := strconv.Atoi(instanceID)
+	ctx := context.TODO()
+	droplet, _, err := i.doClient.Droplets.Get(ctx, dropletID)
+


Remove blank?

timoreimann · 2019-11-20T08:49:52Z

pkg/nodeidentity/do/identify.go

+func (i *nodeIdentifier) getInstanceGroup(instanceID string) (string, error) {
+
+	dropletID, err := strconv.Atoi(instanceID)
+	ctx := context.TODO()


Does it make sense to use a timeout-based context here to make sure we do not hang too long trying to fetch the droplet?

There are lot of places where context.TODO is used. I'll create an issue to address that as a separate PR. Please suggest if that's Ok for you.

Totally okay with that 👍

timoreimann · 2019-11-20T08:51:59Z

pkg/nodeidentity/do/identify.go

+		if strings.Contains(dropletTag, dropletTagInstanceGroupName) {
+			instancegrouptag := strings.SplitN(dropletTag, ":", 2)
+			if len(instancegrouptag) < 2 {
+				return "", fmt.Errorf("failed to retrieve droplet instance group tag = %s properly", dropletTag)


How about rephrasing this to something like

return "", fmt.Errorf("failed to parse droplet tag %q: expected colon-separated key/value pair", dropletTag)

Sure, done.

timoreimann · 2019-11-20T08:52:46Z

pkg/nodeidentity/do/identify.go

+		}
+	}
+
+	return "", fmt.Errorf("Could not find tag 'kops-instancegroup' from instance metadata")


Should use errors.New here since the error message does not have any place holders.

sure, updated.

srikiz · 2019-11-24T07:29:26Z

@timoreimann @zetaab - I have incorporated all the comments. Please have a look and approve if you are Ok. Thanks !

timoreimann

LGTM. 👍

zetaab

thanks @srikiz

/lgtm
/approve

rifelpet · 2019-11-24T15:00:33Z

not sure why tide is still waiting on approved and lgtm...

/lgtm
/approve

k8s-ci-robot · 2019-11-24T15:00:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,zetaab]

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

droslean · 2019-11-24T18:02:32Z

/refresh

srikiz · 2019-11-25T02:45:50Z

/refresh

…pstream-release-1.16 Automated cherry pick of #7961: Initial work

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

k8s-ci-robot requested review from rifelpet and zetaab November 19, 2019 20:17

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 Nov 19, 2019

zetaab requested changes Nov 19, 2019

View reviewed changes

timoreimann reviewed Nov 20, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2019

srikiz added 5 commits November 24, 2019 12:56

Initial work

90f3ebe

Initial work for kops-controller to support digital ocean

7b8eb2b

Remove ununsed code

7467d1a

incorporate review comments

5ee945f

Update packages

0cc3bc0

srikiz force-pushed the DO-KOPS-Controller-Support branch from 59f3032 to 0cc3bc0 Compare November 24, 2019 07:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2019

timoreimann approved these changes Nov 24, 2019

View reviewed changes

zetaab approved these changes Nov 24, 2019

View reviewed changes

k8s-ci-robot assigned zetaab Nov 24, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 24, 2019

k8s-ci-robot assigned rifelpet Nov 24, 2019

k8s-ci-robot merged commit 5f4a731 into kubernetes:master Nov 25, 2019

srikiz deleted the DO-KOPS-Controller-Support branch November 25, 2019 18:36

srikiz mentioned this pull request Nov 27, 2019

Automated cherry pick of #7961: Initial work #8018

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 23, 2019

Merge pull request #8018 from srikiz/automated-cherry-pick-of-#7961-u…

e9aaa71

…pstream-release-1.16 Automated cherry pick of #7961: Initial work


		func (i *nodeIdentifier) getInstanceGroup(instanceID string) (string, error) {

		dropletID, err := strconv.Atoi(instanceID)

[Issue-7870] kops controller support for digital ocean #7961

[Issue-7870] kops controller support for digital ocean #7961

Conversation

srikiz commented Nov 19, 2019

k8s-ci-robot commented Nov 19, 2019

rifelpet commented Nov 19, 2019

zetaab left a comment • 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

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

srikiz commented Nov 24, 2019

timoreimann left a comment

Choose a reason for hiding this comment

zetaab left a comment

Choose a reason for hiding this comment

rifelpet commented Nov 24, 2019

k8s-ci-robot commented Nov 24, 2019

droslean commented Nov 24, 2019

srikiz commented Nov 25, 2019

zetaab left a comment •

edited

Loading