Cloudinstances refactor #9799

olemarkus · 2020-08-23T06:45:17Z

@johngmyers what do you think about an approach like this?

olemarkus · 2020-08-23T07:06:46Z

/retest
/cc @johngmyers

pkg/cloudinstances/cloud_instance_group.go

pkg/cloudinstances/cloud_instance.go

johngmyers · 2020-08-23T21:23:15Z

pkg/cloudinstances/cloud_instance.go

+const CloudInstanceStatusReady = "Ready"
+
+// CloudInstanceStatusNotJoined means the instance has not yet joined the cluster
+const CloudInstanceStatusNotJoined = "NotJoined"


Whether or not an instance has joined is independent of whether it needs update and/or is detached. It can also be determined by the absence of the Node field. So you might consider excluding this value from this level of the API.

I suppose you are right. It gets a bit awkward either way. If we were to remove this, it would mean that we'd have ready nodes that have not yet joined the cluster. But not being able to detect detached nodes that are absent from the cluster would be worse.
Clould be that "ready" is not the right term for this status. Maybe it should be "UpToDate" or "Running" or something is better.

I think you're right that "Ready" is the wrong term for this. I believe with one of your other changes it could be in this state even when it's pending.

I think I have a slight preference for "UpToDate". Another possibility might be "Current".

Also, consider whether instancegroups.AdjustNeedUpdate() needs to change the Status field.

pkg/instancegroups/rollingupdate_test.go

johngmyers · 2020-08-23T22:02:29Z

pkg/cloudinstances/cloud_instance_group.go

 	} else {
 		klog.V(8).Infof("unable to find node for instance: %s", instanceId)
 		cm.Status = CloudInstanceStatusNotJoined
 	}
 	return nil
 }

-// NewDetachedCloudInstance creates a new CloudInstance for a detached instance
-func (c *CloudInstanceGroup) NewDetachedCloudInstance(instanceId string, nodeMap map[string]*v1.Node) error {


This change is incorrect. It means a detached instance that hasn't joined won't be recognized as detached.

pkg/cloudinstances/cloud_instance.go

olemarkus · 2020-08-24T12:58:16Z

Only adds the data I wanted for aws for now. Can add for openstack in #9762. That would require some other PRs to go in first anyway.

/retest

zetaab

from my point of view this looks good.

could @johngmyers check that everything is in place

/lgtm

johngmyers · 2020-08-27T03:38:42Z

pkg/cloudinstances/cloud_instance_group.go

 	node := nodeMap[instanceId]
 	if node != nil {
 		cm.Node = node
+		cm.Status = status


I think this needs to be moved out of the surrounding if block. Otherwise Status will be the empty string for non-joined instances.

johngmyers · 2020-08-27T03:44:24Z

pkg/instancegroups/rollingupdate_test.go

@@ -1397,8 +1397,13 @@ func TestRollingUpdateMaxSurgeAllNeedUpdateMaxAlreadyDetached(t *testing.T) {

 	groups := make(map[string]*cloudinstances.CloudInstanceGroup)
 	makeGroup(groups, c.K8sClient, cloud, "node-1", kopsapi.InstanceGroupRoleNode, 7, 7)
+	groups["node-1"].NeedUpdate[0].Status = cloudinstances.CloudInstanceStatusDetached


This changes the test, partially invalidating it. The test was designed to have only instances 1 and 3 detached in order to exercise the code that ensures those get terminated last. (Though the TODO comment mentions that the verification of that wasn't written.)

The name of the test is TestRollingUpdateMaxSurgeAllNeedUpdateMaxAlreadyDetached so I assumed all needs updates and two were already detached.
What would be the correct values here then?

0 should be CloudInstanceStatusNeedsUpdate

pkg/resources/digitalocean/cloud.go

k8s-ci-robot · 2020-08-30T19:29:24Z

@olemarkus: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-e2e-cni-kuberouter	`e0a2fd9`	link	`/test pull-kops-e2e-cni-kuberouter`
pull-kops-e2e-cni-calico	`e0a2fd9`	link	`/test pull-kops-e2e-cni-calico`
pull-kops-e2e-cni-amazonvpc	`e0a2fd9`	link	`/test pull-kops-e2e-cni-amazonvpc`
pull-kops-e2e-cni-cilium	`e0a2fd9`	link	`/test pull-kops-e2e-cni-cilium`
pull-kops-verify-terraform	`e0a2fd9`	link	`/test pull-kops-verify-terraform`
pull-kops-e2e-k8s-containerd	`e0a2fd9`	link	`/test pull-kops-e2e-k8s-containerd`
pull-kops-verify-hashes	`e0a2fd9`	link	`/test pull-kops-verify-hashes`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

…e representation Apply suggestions from code review Co-authored-by: John Gardiner Myers <[email protected]>

johngmyers · 2020-08-30T20:37:51Z

pkg/instancegroups/instancegroups.go

@@ -321,7 +321,11 @@ func (c *RollingUpdateCluster) patchTaint(ctx context.Context, node *corev1.Node
 	return err
 }

+<<<<<<< HEAD


Unresolved merge conflict

Bah. Yesterday's push got stuck. Pushed now.

johngmyers

/lgtm
/approve

johngmyers · 2020-09-01T05:37:41Z

pkg/cloudinstances/cloud_instance.go

+	// CloudInstanceGroup is the managing CloudInstanceGroup
+	CloudInstanceGroup *CloudInstanceGroup
+	// Status indicates if the instance has joined the cluster and if it needs any updates.
+	Status string


Perhaps this should be its own type CloudInstanceStatus?

k8s-ci-robot · 2020-09-01T05:44:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, olemarkus

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

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/alicloud Issues or PRs related to alicloud provider labels Aug 23, 2020

k8s-ci-robot requested review from gjtempleton and liranp August 23, 2020 06:45

olemarkus changed the title ~~Cloudinstances refactor~~ WIP Cloudinstances refactor Aug 23, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2020

k8s-ci-robot requested a review from johngmyers August 23, 2020 07:06

johngmyers requested changes Aug 23, 2020

View reviewed changes

olemarkus mentioned this pull request Aug 24, 2020

Add command for listing cloud instances #9762

Merged

olemarkus force-pushed the cloudinstances-refactor branch from 9a4139a to 4bbf6de Compare August 24, 2020 07:20

olemarkus changed the title ~~WIP Cloudinstances refactor~~ Cloudinstances refactor Aug 24, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2020

olemarkus requested a review from johngmyers August 24, 2020 13:07

zetaab approved these changes Aug 26, 2020

View reviewed changes

k8s-ci-robot assigned zetaab Aug 26, 2020

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

johngmyers requested changes Aug 27, 2020

View reviewed changes

k8s-ci-robot added area/api area/channels area/documentation area/nodeup labels Aug 27, 2020

olemarkus force-pushed the cloudinstances-refactor branch from e0a2fd9 to 58a9cbb Compare August 27, 2020 16:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 27, 2020

olemarkus force-pushed the cloudinstances-refactor branch from 58a9cbb to df30b8c Compare August 27, 2020 16:45

olemarkus requested a review from johngmyers August 27, 2020 17:35

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

olemarkus force-pushed the cloudinstances-refactor branch from df30b8c to c431f7e Compare August 30, 2020 19:23

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

Refactor cloudinstancegroupmember in a more independent cloud instanc…

0ec7168

…e representation Apply suggestions from code review Co-authored-by: John Gardiner Myers <[email protected]>

johngmyers requested changes Aug 30, 2020

View reviewed changes

olemarkus force-pushed the cloudinstances-refactor branch from c431f7e to 0ec7168 Compare August 31, 2020 05:19

johngmyers reviewed Sep 1, 2020

View reviewed changes

k8s-ci-robot assigned johngmyers Sep 1, 2020

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

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

k8s-ci-robot merged commit e11146c into kubernetes:master Sep 1, 2020

k8s-ci-robot added this to the v1.19 milestone Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloudinstances refactor #9799

Cloudinstances refactor #9799

olemarkus commented Aug 23, 2020

olemarkus commented Aug 23, 2020

johngmyers Aug 23, 2020

olemarkus Aug 24, 2020

johngmyers Aug 24, 2020

johngmyers Aug 23, 2020

olemarkus commented Aug 24, 2020

zetaab left a comment

johngmyers Aug 27, 2020

johngmyers Aug 27, 2020

olemarkus Aug 27, 2020

johngmyers Aug 27, 2020

k8s-ci-robot commented Aug 30, 2020 •

edited

Loading

johngmyers Aug 30, 2020

olemarkus Aug 31, 2020

johngmyers left a comment

johngmyers Sep 1, 2020

k8s-ci-robot commented Sep 1, 2020

Cloudinstances refactor #9799

Cloudinstances refactor #9799

Conversation

olemarkus commented Aug 23, 2020

olemarkus commented Aug 23, 2020

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

olemarkus commented Aug 24, 2020

zetaab 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

k8s-ci-robot commented Aug 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 1, 2020

k8s-ci-robot commented Aug 30, 2020 •

edited

Loading