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

✨ Implement delete for KubeadmControlPlane #2037

Merged
merged 3 commits into from
Jan 17, 2020
Merged

✨ Implement delete for KubeadmControlPlane #2037

merged 3 commits into from
Jan 17, 2020

Conversation

dlipovetsky
Copy link
Contributor

@dlipovetsky dlipovetsky commented Jan 10, 2020 •
edited

What this PR does / why we need it: Implement delete for KubeadmControlPlane. Part of #1756.

Special notes for reviewers:

  • I know that ⚠️ ClusterName required on Machine/MachineSet/MachineDeployment #1539 introduced Machine.Spec.ClusterName, but the code base still uses the cluster label to filter, which is why I also did this in GetMachinesForCluster.
  • The two cases for the delete unit test share some code, but could probably share more. If you feel that's important to address here, let me know.

/cc @chuckha @detiber @randomvariable

@k8s-ci-robot
Copy link
Contributor

@dlipovetsky: GitHub didn't allow me to request PR reviews from the following users: cha.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it: Implement delete for KubeadmControlPlane. Part of #1756.

Special notes for reviewers:

  • I know that ⚠️ ClusterName required on Machine/MachineSet/MachineDeployment #1539 introduced Machine.Spec.ClusterName, but the code base still uses the cluster label to filter, which is why I also did this in GetMachinesForCluster.
  • The two cases for the delete unit test share some code, but could probably share more. If you feel that's important to address here, let me know.

/cc @CHA @detiber @randomvariable

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.

@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 Jan 10, 2020
@dlipovetsky
Copy link
Contributor Author

/unassign @chadswen
/assign @chuckha

(typo!)

@k8s-ci-robot k8s-ci-robot assigned chuckha and unassigned chadswen Jan 10, 2020
@randomvariable
Copy link
Member

Looks ok to me, deferring to @chuckha

@dlipovetsky
Copy link
Contributor Author

/hold

I want to squash the fixups before merging.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@dlipovetsky
Copy link
Contributor Author

I addressed all requests for changes and marked the threads "resolved." There are a couple threads I left unresolved because they are discussions.

vincepri
vincepri reviewed Jan 12, 2020
View reviewed changes
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved
util/util.go Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Show resolved Hide resolved
controlplane/kubeadm/controllers/kubeadm_control_plane_controller.go Outdated Show resolved Hide resolved
chuckha
chuckha approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@chuckha chuckha left a comment

Choose a reason for hiding this comment

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

Once the issues are filed and commits squashed is there anything left to do?

/approve

/assign @vincepri

for lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, dlipovetsky

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 Jan 14, 2020
@moshloop
Copy link
Contributor

Not sure if this is relevant to this PR, but do we have a design/plan for downscale? I am particularly interested in how this will be done safely with etcd quorum etc..

@chuckha
Copy link
Contributor

chuckha commented Jan 14, 2020

Not sure if this is relevant to this PR, but do we have a design/plan for downscale? I am particularly interested in how this will be done safely with etcd quorum etc..

Here is the general plan: https://github.com/kubernetes-sigs/cluster-api/blob/master/docs/proposals/20191017-kubeadm-based-control-plane.md#scale-down

This was referenced Jan 14, 2020
Closed
Closed
Closed
Closed
@dlipovetsky
🏃 Use MachineList instead of Machine slice in KubeadmControlPlane
8979985 
This makes it easier to split apart "get all machines" from "filter on owned
machines" functionality.

Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>
@dlipovetsky
🏃 Add util function to list all machines for a cluster
769f14e 
Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>
@dlipovetsky
✨ Implement delete for KubeadmControlPlane
5e03d6d 
Signed-off-by: Daniel Lipovetsky <dlipovetsky@d2iq.com>
@dlipovetsky
Copy link
Contributor Author

Squashed!

@dlipovetsky
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2020
@dlipovetsky dlipovetsky mentioned this pull request Jan 14, 2020
Merged
@chuckha
Copy link
Contributor

chuckha commented Jan 16, 2020

I'm going to merge this at the end of today if there are no more review comments

@ncdc ncdc added this to the v0.3.0 milestone Jan 16, 2020
@chuckha
Copy link
Contributor

chuckha commented Jan 17, 2020

I see no additional comments

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 401bd4e into kubernetes-sigs:master Jan 17, 2020
@CecileRobertMichon CecileRobertMichon mentioned this pull request Feb 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncdc ncdc ncdc left review comments

@randomvariable randomvariable randomvariable left review comments

@vincepri vincepri vincepri left review comments

@chuckha chuckha chuckha approved these changes

@detiber detiber Awaiting requested review from detiber

Assignees

@chuckha chuckha

@randomvariable randomvariable

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
v0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dlipovetsky @k8s-ci-robot @randomvariable @moshloop @chuckha @ncdc @vincepri @chadswen