✨ clusterctl alpha topology-dryrun #5893

ykakarap · 2021-12-30T05:01:55Z

What this PR does / why we need it:
This PR implements a alpha new command: clusterctl alpha topology-dryrun.
This command can be used to dry run ClusterClass and/or Cluster changes.

The command expects as input a file that includes the new or modified objects. Example: the file can contain a modified Cluster or a modified ClusterClass. The output lists all the Clusters and ClusterClasses that will be affected.

The following example shows the output when dry-running to create a Cluster that uses a pre-installed ClusterClass.

Special thanks to @fabriziopandini for providing guidance for this feature. 🙂

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

linux-foundation-easycla · 2021-12-30T05:01:58Z

The committers are authorized under a signed CLA.

✅ Yuvaraj Kakaraparthi (789b804)

go.mod

cmd/clusterctl/client/cluster/topology_test.go

ykakarap · 2021-12-30T05:28:20Z

/retest

ykakarap · 2021-12-30T05:28:36Z

/test pull-cluster-api-verify-main

ykakarap · 2021-12-30T05:29:35Z

/retest

ykakarap · 2021-12-30T06:03:14Z

~~The verify pipeline failure seems to be unrelated to the code changes. the failure seems to be related to the pipeline execution.~~
Resolved.

ykakarap · 2021-12-30T06:15:35Z

/retest

cmd/clusterctl/client/cluster/proxy.go

fabriziopandini

This is great work!
first pass, mostly nits

cmd/clusterctl/client/cluster/internal/dryrun/client.go

controllers/topology/cluster_controller.go

cmd/clusterctl/client/cluster/internal/dryrun/client.go

cmd/clusterctl/client/cluster/topology.go

cmd/clusterctl/client/cluster/topology_test.go

ykakarap · 2022-01-04T01:56:03Z

@fabriziopandini thank you for the review.
I am on PTO right now and will address the changes after I am back in a few days (probably next week).

sbueringer · 2022-01-07T17:43:08Z

I'll take a closer look on Monday.

controllers/topology/cluster_controller.go

cmd/clusterctl/cmd/topology.go

sbueringer

First round of review

go.mod

cmd/clusterctl/cmd/topology.go

cmd/clusterctl/client/cluster/internal/dryrun/client.go

dlipovetsky · 2022-01-12T18:56:01Z

Do create/update requests go through validation webhooks? That is, are they sent to a Kubernetes API? It looks like client/cluster/internal/dryrun/client.go uses a "fake" client.

If they do not, then I think it's worth making this explicitly a "client-side" dry-run, e.g.,

> kubectl create --help | grep dry
      --dry-run='none': Must be "none", "server", or "client". If client strategy, only print the object that would be sent, without sending it. If server strategy, submit server-side request without persisting the resource.

ykakarap

@dlipovetsky Within this dry run code we run validation and defaulting but only for Cluster and ClusterClass types. We do this not by running them through the webhooks, instead we run the validation and defaulting functions directly.

Ref: https://github.com/ykakarap/cluster-api/blob/clusterctl-dry-run/cmd/clusterctl/client/cluster/topology.go#L358-L383

Defaulting and Validation of any of the provider types is out of scope for this feature.

ykakarap

There are a few more comments I need to address.
Resolving some conversations and replying to some comments in the mean time.

cmd/clusterctl/client/cluster/internal/dryrun/client.go

cmd/clusterctl/client/topology.go

cmd/clusterctl/cmd/topology.go

ykakarap · 2022-01-17T09:51:20Z

FYI: The new size of the clusterctl binary on linux amd64 is ~60mb. Previously it was ~52mb.

cmd/clusterctl/client/cluster/internal/dryrun/client.go

cmd/clusterctl/client/cluster/assets/topology-test/existing-clusterclass-and-cluster.yaml

cmd/clusterctl/cmd/topology.go

cmd/clusterctl/client/topology.go

cmd/clusterctl/client/cluster/topology.go

sbueringer · 2022-01-17T16:42:44Z

cmd/clusterctl/client/cluster/topology.go

+		object := o.DeepCopyObject().(client.Object)
+		if err := localScheme.Convert(obj, object, nil); err != nil {
+			return errors.Wrapf(err, "failed to convert object to %s", obj.GetKind())
+		}


Q: why wo we have to do this. Couldn't we just use the obj directly?

If I'm right we might also don't need the back-conversion at the end of the loop

we cant use obj directly. The defaulter and validator expect types like Cluster and ClusterClass and that is object. Hence we use convert to basically move data from unstructured to object (typed object) and then move back to unstructured.

cmd/clusterctl/client/cluster/topology.go

cmd/clusterctl/client/cluster/internal/dryrun/client.go

killianmuldoon

First part of the review - just a lot of nits 😄

cmd/clusterctl/client/cluster/internal/dryrun/client.go

cmd/clusterctl/client/cluster/topology.go

sbueringer · 2022-01-18T14:17:17Z

Great work!!

/lgtm

killianmuldoon · 2022-01-18T14:33:41Z

/lgtm

Code looks good! I'm going to spend some time playtesting but if anything comes up I can create an issue and we can patch.

k8s-ci-robot · 2022-01-18T15:25:05Z

@ykakarap: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`dcc22db`	link	false	`/test pull-cluster-api-apidiff-main`

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.

sbueringer · 2022-01-18T15:25:42Z

/lgtm
to latest fixes

fabriziopandini · 2022-01-18T17:32:47Z

This is a super powerful feature!
Let's make it a demo in the office hours and wait for feedback and ideas for further improvements
/lgtm

vincepri · 2022-01-18T19:46:15Z

Overall this feature looks great, my only comment is about the UX for the command itself, consider to have something like

clusterctl alpha topology plan [...]

We can always follow-up for this, not a blocker for this PR, but release blocking

fabriziopandini · 2022-01-18T19:47:18Z

/approve
@ykakarap to follow up on the name change before release

k8s-ci-robot · 2022-01-18T19:47:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 30, 2021

k8s-ci-robot requested review from fabriziopandini and killianmuldoon December 30, 2021 05:02

ykakarap force-pushed the clusterctl-dry-run branch 3 times, most recently from 21028c8 to 88ec975 Compare December 30, 2021 05:14

ykakarap commented Dec 30, 2021

View reviewed changes

go.mod Show resolved Hide resolved

cmd/clusterctl/client/cluster/topology_test.go Show resolved Hide resolved

ykakarap force-pushed the clusterctl-dry-run branch from 88ec975 to 8f49f8f Compare December 30, 2021 05:27

ykakarap force-pushed the clusterctl-dry-run branch from 8f49f8f to 27cc281 Compare December 30, 2021 05:41

ykakarap commented Dec 30, 2021

View reviewed changes

cmd/clusterctl/client/cluster/proxy.go Show resolved Hide resolved

fabriziopandini reviewed Jan 3, 2022

View reviewed changes

sbueringer reviewed Jan 7, 2022

View reviewed changes

controllers/topology/cluster_controller.go Outdated Show resolved Hide resolved

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

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

sbueringer reviewed Jan 10, 2022

View reviewed changes

ykakarap commented Jan 13, 2022

View reviewed changes

ykakarap mentioned this pull request Jan 16, 2022

Replace github.com/gosuri/uitable with github.com/olekukonko/tablewriter #5941

Closed

ykakarap force-pushed the clusterctl-dry-run branch 2 times, most recently from d63aec5 to e2d4237 Compare January 16, 2022 17:25

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

DiptoChakrabarty mentioned this pull request Jan 16, 2022

🌱 Replace uitable with tablewriter in clusterctl describe cluster command #5942

Merged

fabriziopandini reviewed Jan 17, 2022

View reviewed changes

sbueringer reviewed Jan 17, 2022

View reviewed changes

killianmuldoon requested changes Jan 17, 2022

View reviewed changes

ykakarap force-pushed the clusterctl-dry-run branch from e2d4237 to 789b804 Compare January 18, 2022 13:15

sbueringer reviewed Jan 18, 2022

View reviewed changes

cmd/clusterctl/client/cluster/topology.go Outdated Show resolved Hide resolved

ykakarap force-pushed the clusterctl-dry-run branch from 789b804 to ecf198e Compare January 18, 2022 13:52

k8s-ci-robot assigned sbueringer Jan 18, 2022

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

k8s-ci-robot assigned killianmuldoon Jan 18, 2022

clusterctl topology dry-run implementation

dcc22db

ykakarap force-pushed the clusterctl-dry-run branch from ecf198e to dcc22db Compare January 18, 2022 15:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 18, 2022

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

k8s-ci-robot assigned fabriziopandini Jan 18, 2022

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

k8s-ci-robot merged commit da03567 into kubernetes-sigs:main Jan 18, 2022

k8s-ci-robot added this to the v1.1 milestone Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ clusterctl alpha topology-dryrun #5893

✨ clusterctl alpha topology-dryrun #5893

ykakarap commented Dec 30, 2021 •

edited

linux-foundation-easycla bot commented Dec 30, 2021 •

edited

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021 •

edited

ykakarap commented Dec 30, 2021

fabriziopandini left a comment

ykakarap commented Jan 4, 2022

sbueringer commented Jan 7, 2022 •

edited

sbueringer left a comment

dlipovetsky commented Jan 12, 2022 •

edited

ykakarap left a comment

ykakarap left a comment

ykakarap commented Jan 17, 2022

sbueringer Jan 17, 2022

ykakarap Jan 18, 2022 •

edited

killianmuldoon left a comment

sbueringer commented Jan 18, 2022

killianmuldoon commented Jan 18, 2022

k8s-ci-robot commented Jan 18, 2022

sbueringer commented Jan 18, 2022

fabriziopandini commented Jan 18, 2022

vincepri commented Jan 18, 2022 •

edited

fabriziopandini commented Jan 18, 2022

k8s-ci-robot commented Jan 18, 2022

✨ clusterctl alpha topology-dryrun #5893

✨ clusterctl alpha topology-dryrun #5893

Conversation

ykakarap commented Dec 30, 2021 • edited

linux-foundation-easycla bot commented Dec 30, 2021 • edited

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021

ykakarap commented Dec 30, 2021 • edited

ykakarap commented Dec 30, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

ykakarap commented Jan 4, 2022

sbueringer commented Jan 7, 2022 • edited

sbueringer left a comment

Choose a reason for hiding this comment

dlipovetsky commented Jan 12, 2022 • edited

ykakarap left a comment

Choose a reason for hiding this comment

ykakarap left a comment

Choose a reason for hiding this comment

ykakarap commented Jan 17, 2022

sbueringer Jan 17, 2022

Choose a reason for hiding this comment

ykakarap Jan 18, 2022 • edited

Choose a reason for hiding this comment

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer commented Jan 18, 2022

killianmuldoon commented Jan 18, 2022

k8s-ci-robot commented Jan 18, 2022

sbueringer commented Jan 18, 2022

fabriziopandini commented Jan 18, 2022

vincepri commented Jan 18, 2022 • edited

fabriziopandini commented Jan 18, 2022

k8s-ci-robot commented Jan 18, 2022

ykakarap commented Dec 30, 2021 •

edited

linux-foundation-easycla bot commented Dec 30, 2021 •

edited

ykakarap commented Dec 30, 2021 •

edited

sbueringer commented Jan 7, 2022 •

edited

dlipovetsky commented Jan 12, 2022 •

edited

ykakarap Jan 18, 2022 •

edited

vincepri commented Jan 18, 2022 •

edited