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

Jump to bottom

Add kops delete instance command #9784

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Add kops delete instance command #9784

merged 1 commit into from
Aug 28, 2020

Conversation

olemarkus
Copy link
Member

This command makes it possible to delete a single instance.
It is implemented mostly reusing rollingupdate code. This meant opening more of the API than I liked, but given the intricate way that RollingUpdate is implemented, it was hard to get around. Ideally some of the logic could be moved to cloudinstances but quickly ran into fi circular imports when going down that route.

@k8s-ci-robot 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 Aug 19, 2020
@olemarkus olemarkus force-pushed the kops-delete-instance branch from 611a052 to efeb402 Compare August 19, 2020 11:15
@olemarkus olemarkus force-pushed the kops-delete-instance branch from efeb402 to b4850be Compare August 20, 2020 06:39
@olemarkus
Copy link
Member Author

Moved some stuff into rollingupgrade and made the functions there private again. Feels cleaner this way.

@olemarkus olemarkus requested a review from rifelpet August 20, 2020 06:42
johngmyers
johngmyers requested changes Aug 21, 2020
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

Clever use of the rolling update code! I like it.

cmd/kops/delete_instance.go Outdated Show resolved Hide resolved
pkg/instancegroups/instancegroups.go Outdated Show resolved Hide resolved
pkg/instancegroups/instancegroups.go Outdated Show resolved Hide resolved
pkg/instancegroups/instancegroups.go
}
err := c.detachInstance(cloudMember)
if err != nil {
return fmt.Errorf("failed to detach instance: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

The most likely cause of this would be that the cloud provider doesn't support surging. It might be better to default detach to false for such cloud providers.

Copy link
Member Author

Choose a reason for hiding this comment

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

From this codes perspective, one doesn't know why it failed, so it is a bit dangerous to just carry on. I agree that one could ignore surging on cloud providers that do not support it. Maybe Cloud.DetachInstance should be its own interface that one could test for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed that this was a param passed on. Changed the functions to figure out the instance group role on its own instead.

pkg/instancegroups/instancegroups.go Outdated Show resolved Hide resolved
pkg/instancegroups/instancegroups.go
@@ -561,3 +561,21 @@ func (c *RollingUpdateCluster) deleteNode(ctx context.Context, node *corev1.Node

return nil
}

Copy link
Member

Choose a reason for hiding this comment

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

Should have doc comment. Should have added unit test(s).

@johngmyers
Copy link
Member

You might want to make the --detach flag a *bool and set the default based on whether the instance's instancegroup configuration has a nonzero maxSurge.

A better name might be --surge.

@olemarkus olemarkus force-pushed the kops-delete-instance branch from 56c1653 to 366390e Compare August 22, 2020 07:38
@olemarkus olemarkus requested a review from johngmyers August 22, 2020 08:29
moshevayner
moshevayner reviewed Aug 22, 2020
View reviewed changes
Copy link
Member

@moshevayner moshevayner left a comment

Choose a reason for hiding this comment

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

Great feature! Thanks for adding that @olemarkus 🙇

docs/cli/kops_delete.md Outdated Show resolved Hide resolved
docs/cli/kops_delete_instance.md Outdated Show resolved Hide resolved
docs/cli/kops_delete_instance.md Outdated Show resolved Hide resolved
@olemarkus
Copy link
Member Author

@MoShitrit Thanks. Fixed docs according to your comments.

@olemarkus olemarkus requested a review from moshevayner August 22, 2020 20:16
@moshevayner
Copy link
Member

@MoShitrit Thanks. Fixed docs according to your comments.

Thanks @olemarkus!
I think there's just more that was probably overlooked- #9784 (comment)
The rest looks good 😄

@olemarkus olemarkus force-pushed the kops-delete-instance branch from a58b816 to 1ab5029 Compare August 23, 2020 04:44
@olemarkus
Copy link
Member Author

Thanks for catching. Fixed.

@moshevayner
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2020
@olemarkus olemarkus requested a review from moshevayner August 27, 2020 07:52
@hakman
Copy link
Member

hakman commented Aug 28, 2020

@olemarkus to keep things consistent with other commands, this one needs --yes also.

@olemarkus olemarkus force-pushed the kops-delete-instance branch from 2a12a05 to c7f0b10 Compare August 28, 2020 06:41
Add kops delete instance command
ff6c049 
Add support for deleting instance by k8s node name

Add yes flag
@olemarkus olemarkus force-pushed the kops-delete-instance branch from c7f0b10 to ff6c049 Compare August 28, 2020 06:43
@hakman
Copy link
Member

hakman commented Aug 28, 2020

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, 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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2020
@hakman
Copy link
Member

hakman commented Aug 28, 2020

/test all

@k8s-ci-robot k8s-ci-robot merged commit e537846 into kubernetes:master Aug 28, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

@rifelpet rifelpet Awaiting requested review from rifelpet

@johngmyers johngmyers Awaiting requested review from johngmyers

@moshevayner moshevayner Awaiting requested review from moshevayner

Assignees

@hakman hakman

@moshevayner moshevayner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation area/provider/aws Issues or PRs related to aws provider area/rolling-update cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
6 participants
@olemarkus @johngmyers @moshevayner @hakman @k8s-ci-robot @rifelpet