Add pivot phase to clusterctl and unify pivoting approach #731

detiber · 2019-02-05T21:04:55Z

What this PR does / why we need it:

Adds a new phase for pivoting Cluster API resources
Unifies pivoting approach:
- Pivot ClusterAPI objects for all namespaces
- Adds MachineClass support to pivoting
- Scale down StatefulSets on pivot to avoid dueling controllers
- Remove ownerRefs prior to pivoting objects
- Change order of pivoting of Machine* objects to avoid race conditions
- Recursively copy objects based on ownerRefs
clusterctl delete cluster now pivots and deletes Cluster API
resources from all namespaces.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related to #554
Fixes: #448

Release note:

`clusterctl` now has an alpha phase for pivoting Cluster API resources: `clusterctl alpha phases pivot`
`clusterctl delete cluster` will now pivot Cluster API resources in all namespaces

cmd/clusterctl/phases/pivot.go

chuckha

first pass review

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

cmd/clusterctl/cmd/alpha_phase_pivot.go

cmd/clusterctl/phases/pivot.go

chuckha · 2019-02-08T17:58:44Z

cmd/clusterctl/phases/pivot.go

+			return nil, err
+		}
+
+		if out.TypeMeta.Kind == "StatefulSet" {


should we log a warning if this case is not matched saying we're ignoring something?

vincepri

Lots of great work in this PR. Left some comments.

cmd/clusterctl/phases/pivot.go

vincepri · 2019-02-13T20:51:07Z

One clarification: the PR adds a new way to pivot resources, but doesn't change the current way when not using phases, is that correct?

detiber · 2019-02-13T20:54:43Z

One clarification: the PR adds a new way to pivot resources, but doesn't change the current way when not using phases, is that correct?

No, this both adds the phase and updates the pivot logic for both the clusterctl create and clusterctl delete workflows. Previously each of the existing workflows pivoted objects slightly differently and there were periods during each that would have two controllers managing the same set of objects.

vincepri · 2019-02-13T21:00:04Z

Got it, we should update title and description then.

vincepri

Minor comments, otherwise LGTM! 🎉

cmd/clusterctl/clusterdeployer/clusterdeployer.go

cmd/clusterctl/cmd/delete_cluster.go

- Add `clusterctl alpha phases pivot` subcommand - Pivot ClusterAPI objects for all namespaces - Adds MachineClass support to pivoting - Unify pivoting approach - Scale down StatefulSets on pivot to avoid dueling controllers - Remove ownerRefs prior to pivoting objects - Change order of pivoting of Machine* objects to avoid race conditions - Recursively copy objects based on ownerRefs - `clusterctl delete cluster` now pivots and deletes Cluster API resources from all namespaces. Co-authored-by: Chuck Ha <[email protected]> Signed-off-by: Jason DeTiberus <[email protected]>

detiber · 2019-03-07T03:04:33Z

/test pull-cluster-api-test

detiber · 2019-03-07T03:10:00Z

arrgh, pull-cluster-api-test is failing on extracting kubebuilder-tools archive.
/test pull-cluster-api-test

detiber · 2019-03-07T12:37:26Z

/test pull-cluster-api-test

detiber · 2019-03-07T20:56:06Z

/test pull-cluster-api-test

vincepri · 2019-03-07T20:57:34Z

I think this might need a rebase now that fetch-tools has been removed in the other PR.

detiber · 2019-03-07T20:58:15Z

I think this might need a rebase now that fetch-tools has been removed in the other PR.

@vincepri that was only for cluster-api-provider-aws, it proved to still be in use for this repo

vincepri · 2019-03-07T21:10:26Z

Argh, I see. Hopefully it'll be fixes soon by kubebuilder folks.

detiber · 2019-03-07T21:50:17Z

@vincepri I triggered the last test to verify that it was working after hearing back from the kubebuilder team :)

vincepri

/approve

We should wait to merge it until early next week in case other folks have any more comments, wdyt?

k8s-ci-robot · 2019-03-07T21:51:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, vincepri

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

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

vincepri · 2019-03-11T15:53:25Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2019

k8s-ci-robot requested review from justinsb and roberthbailey February 5, 2019 21:05

detiber mentioned this pull request Feb 5, 2019

Add phases support to clusterctl #554

Closed

11 tasks

detiber commented Feb 6, 2019

View reviewed changes

cmd/clusterctl/phases/pivot.go Outdated Show resolved Hide resolved

chuckha reviewed Feb 8, 2019

View reviewed changes

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go Show resolved Hide resolved

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go Outdated Show resolved Hide resolved

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go Show resolved Hide resolved

chuckha reviewed Feb 8, 2019

View reviewed changes

vincepri added this to the v1alpha1 milestone Feb 10, 2019

detiber force-pushed the pivotPhase branch 2 times, most recently from b1fede7 to 049b558 Compare February 12, 2019 19:26

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 12, 2019

detiber mentioned this pull request Feb 13, 2019

Add support for adopting existing MachineSets to MachineDeployments #746

Closed

vincepri reviewed Feb 13, 2019

View reviewed changes

cmd/clusterctl/phases/pivot.go Outdated Show resolved Hide resolved

detiber force-pushed the pivotPhase branch from 07357d0 to e2ed90a Compare February 14, 2019 22:08

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 14, 2019

detiber force-pushed the pivotPhase branch from e2ed90a to 2b843c3 Compare February 15, 2019 18:18

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 22, 2019

vincepri mentioned this pull request Feb 25, 2019

Clusterctl should use alpha phases #775

Closed

roberthbailey removed this from the v1alpha1 milestone Feb 27, 2019

detiber mentioned this pull request Feb 27, 2019

cleanup resources in original cluster when pivoting #527

Closed

detiber force-pushed the pivotPhase branch 2 times, most recently from 611552e to a50ae6d Compare March 1, 2019 22:07

detiber changed the title ~~[WIP] Add pivot phase to clusterctl~~ Add pivot phase to clusterctl Mar 6, 2019

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 Mar 6, 2019

detiber force-pushed the pivotPhase branch from 59a0cc5 to a899161 Compare March 6, 2019 21:04

detiber changed the title ~~Add pivot phase to clusterctl~~ Add pivot phase to clusterctl and unify pivoting approach Mar 6, 2019

detiber force-pushed the pivotPhase branch 2 times, most recently from 1ec078c to c5fcf48 Compare March 6, 2019 21:39

vincepri approved these changes Mar 7, 2019

View reviewed changes

cmd/clusterctl/clusterdeployer/clusterdeployer.go Outdated Show resolved Hide resolved

cmd/clusterctl/cmd/delete_cluster.go Show resolved Hide resolved

detiber force-pushed the pivotPhase branch from c5fcf48 to dc21789 Compare March 7, 2019 02:33

detiber force-pushed the pivotPhase branch from dc21789 to 71d559a Compare March 7, 2019 02:59

detiber force-pushed the pivotPhase branch 2 times, most recently from 46b405e to 71d559a Compare March 7, 2019 17:06

vincepri approved these changes Mar 7, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2019

k8s-ci-robot assigned vincepri Mar 11, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2019

k8s-ci-robot merged commit fbb9787 into kubernetes-sigs:master Mar 11, 2019

justaugustus mentioned this pull request Mar 12, 2019

Bump dependencies (3/11/19) kubernetes-sigs/cluster-api-provider-azure#132

Merged

detiber deleted the pivotPhase branch April 2, 2019 17:44

detiber mentioned this pull request Apr 4, 2019

cluster api controllers are not deleted after k8s master provision finished #876

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pivot phase to clusterctl and unify pivoting approach #731

Add pivot phase to clusterctl and unify pivoting approach #731

detiber commented Feb 5, 2019 •

edited

Loading

chuckha left a comment

chuckha Feb 8, 2019

vincepri left a comment

vincepri commented Feb 13, 2019

detiber commented Feb 13, 2019

vincepri commented Feb 13, 2019

vincepri left a comment

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri left a comment

k8s-ci-robot commented Mar 7, 2019

vincepri commented Mar 11, 2019

Add pivot phase to clusterctl and unify pivoting approach #731

Add pivot phase to clusterctl and unify pivoting approach #731

Conversation

detiber commented Feb 5, 2019 • edited Loading

chuckha left a comment

Choose a reason for hiding this comment

chuckha Feb 8, 2019

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Feb 13, 2019

detiber commented Feb 13, 2019

vincepri commented Feb 13, 2019

vincepri left a comment

Choose a reason for hiding this comment

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri commented Mar 7, 2019

detiber commented Mar 7, 2019

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 7, 2019

vincepri commented Mar 11, 2019

detiber commented Feb 5, 2019 •

edited

Loading