✨clusterctl: move shared objects #2161

fabriziopandini · 2020-01-27T18:02:46Z

What this PR does / why we need it:
This PR refactors clusterctl in order to support move when two or move clusters are sharing some objects (like e.g. Templates or LoadBalancers).

The PR takes a generic approach to solve this problem and the move sequence is defined by processing the ownerReference chain, so we ensure that a node is moved only after its owners.

Additionally, nodes are grouped, so each group of nodes can be moved in parallel. e.g.

All the Clusters should be moved first because they have no OwenerReferences
All the MachineDeployments should be moved second because they have OwenerReferences to clusters, which are moved as part of the first group
then all the MachineSets should be moved second because they have OwenerReferences to MachineDeployments, which are moved as part of the second group
etc.

Which issue(s) this PR fixes:
rif: #1729

/area clusterctl
/assign @ncdc
/assign @vincepri

vincepri

First pass, thanks for working on this!

cmd/clusterctl/pkg/client/cluster/mover_test.go

cmd/clusterctl/pkg/client/cluster/mover.go

cmd/clusterctl/pkg/client/cluster/objectgraph.go

cmd/clusterctl/pkg/client/cluster/mover.go

detiber · 2020-01-27T20:59:14Z

cmd/clusterctl/pkg/client/cluster/mover.go

+	}
+
+	// Define the move sequence by processing the ownerReference chain, so we ensure that a node is moved only after its owners.
+	// Additionally nodes are grouped, so bulk of nodes can be moved in parallel. e.g.


Should we s/node/resource/ here? I can extrapolate that it's meant to mean a node in the graph, but it could likely cause someone not familiar with cluster api to think it means a k8s Node resource here.

I will make a pass to make it clear in the comment where we are talking about nodes in the graphs, but I would like to keep the names as it is.

cmd/clusterctl/pkg/client/cluster/mover.go

fabriziopandini · 2020-01-28T08:58:13Z

@ncdc @vincepri @detiber comments addressed, ready for another pass.

fabriziopandini · 2020-01-28T10:21:38Z

/retest

fabriziopandini · 2020-01-28T10:33:57Z

@ncdc @vincepri it seems that tests are flaking, probably due to parallel move. it will be ok for you removing the parallelism now? without "WaitForMachines", there is no real need for this additional level of complexity now...

vincepri · 2020-01-28T14:34:17Z

@fabriziopandini Go for it, we don't need it to be parallel now

fabriziopandini · 2020-01-28T16:16:42Z

@vincepri done

vincepri · 2020-01-28T16:46:35Z

@fabriziopandini

Seems linter is unhappy

cmd/clusterctl/pkg/client/cluster/mover.go:244:32: Using the variable on range scope `nodeToCreate` in function literal (scopelint)
			return o.createTargetObject(nodeToCreate, toProxy)
			                            ^
cmd/clusterctl/pkg/client/cluster/mover.go:361:32: Using the variable on range scope `nodeToDelete` in function literal (scopelint)
			return o.deleteSourceObject(nodeToDelete)

fabriziopandini · 2020-01-28T18:58:22Z

@vincepri CI is happy again 😉

fabriziopandini · 2020-01-29T10:18:45Z

/test pull-cluster-api-e2e

vincepri

/approve
/assign @ncdc

for final lgtm

k8s-ci-robot · 2020-01-29T16:30:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

cmd/clusterctl/pkg/client/cluster/objectgraph_test.go

ncdc · 2020-01-29T22:13:03Z

cmd/clusterctl/pkg/client/cluster/objectgraph_test.go

 				},
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			graph, err := getDetachedObjectGraphWihObjs(tt.fields.objs)
+			gb, err := getDetachedObjectGraphWihObjs(tt.fields.objs)


Why this change?

I'm getting a little bit obsessed by code readability 🤣
If you prefer I can rollback

cmd/clusterctl/pkg/client/cluster/objectgraph_test.go

fabriziopandini · 2020-01-30T07:17:20Z

@ncdc comment addressed

ncdc · 2020-01-30T19:47:33Z

LGTM. @fabriziopandini please squash!

fabriziopandini · 2020-01-31T07:35:40Z

@ncdc done!
@vincepri for the final lgtm

vincepri

/lgtm

k8s-ci-robot assigned ncdc and vincepri Jan 27, 2020

k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 27, 2020

k8s-ci-robot requested review from justinsb and vincepri January 27, 2020 18:03

vincepri reviewed Jan 27, 2020

View reviewed changes

ncdc reviewed Jan 27, 2020

View reviewed changes

cmd/clusterctl/pkg/client/cluster/mover.go Show resolved Hide resolved

detiber reviewed Jan 27, 2020

View reviewed changes

fabriziopandini mentioned this pull request Jan 28, 2020

Tracking issue for clusterctl v2 implementation #1729

Closed

62 tasks

fabriziopandini force-pushed the clusterctl-move-shared-objects branch from c2d2ff1 to 22be9f9 Compare January 28, 2020 17:13

ncdc added this to the v0.3.0 milestone Jan 28, 2020

vincepri approved these changes Jan 29, 2020

View reviewed changes

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

ncdc reviewed Jan 29, 2020

View reviewed changes

clusterctl move shared objects

ca66c55

fabriziopandini force-pushed the clusterctl-move-shared-objects branch from 3be299c to ca66c55 Compare January 31, 2020 06:59

vincepri approved these changes Jan 31, 2020

View reviewed changes

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

k8s-ci-robot merged commit f068437 into kubernetes-sigs:master Jan 31, 2020

fabriziopandini deleted the clusterctl-move-shared-objects branch January 31, 2020 16:30

fabriziopandini mentioned this pull request Feb 7, 2020

Define clusterctl move process #1525

Closed

✨clusterctl: move shared objects #2161

✨clusterctl: move shared objects #2161

Uh oh!

Conversation

fabriziopandini commented Jan 27, 2020

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

detiber Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabriziopandini commented Jan 28, 2020

Uh oh!

fabriziopandini commented Jan 28, 2020

Uh oh!

fabriziopandini commented Jan 28, 2020

Uh oh!

vincepri commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabriziopandini commented Jan 28, 2020

Uh oh!

vincepri commented Jan 28, 2020

Uh oh!

fabriziopandini commented Jan 28, 2020

Uh oh!

fabriziopandini commented Jan 29, 2020

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncdc Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

fabriziopandini Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabriziopandini commented Jan 30, 2020

Uh oh!

ncdc commented Jan 30, 2020

Uh oh!

fabriziopandini commented Jan 31, 2020

Uh oh!

vincepri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vincepri commented Jan 28, 2020 •

edited

Loading