✨ clusterctl: add ClusterClass support to `clusterctl generate cluster` #5351

ykakarap · 2021-09-28T16:17:39Z

What this PR does / why we need it:

This PR adds ClusterClass support to clusterctl generate cluster command. The command does the following:

Look through all the objects in the base template to identify Clusters with topology.
For every ClusterClass referred in the base template check if the ClusterClass is already installed on the clusters.
If the cluster is not already initialized or if the ClusterClass is not installed, fetches the ClusterClass template from the repository by looking up "clusterclass-.yaml" file.
Finally appends the ClusterClass templates to output template.

TODO:

Add support for running without a cluster.

I will shortly open follow-up PRs for the following items:

Support --list-variables for cluster classes as well (Issue: support --list-variables in cluserctl generate cluster with managed topology clusters #5347)
e2e tests for clusterctl generate cluster (Issue: Add e2e tests to the enhanced clusterctl generate cluster #5348)
Support for cluster classes using --from (Issue: Explore options to handle clusterctl generate cluster --from for managed topologies #5350)

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 #5108

cmd/clusterctl/client/config.go

cmd/clusterctl/client/repository/template.go

cmd/clusterctl/client/yamlprocessor/simple_processor.go

ykakarap · 2021-10-01T19:24:52Z

/hold

This PR is rebased on top of #5373. Hold until #5373 is merged. This PR uses some APIs introduced in 5373.

fabriziopandini

A couple of minors but I think we are almost there
@sbueringer PTAL

cmd/clusterctl/client/cluster/inventory.go

cmd/clusterctl/client/clusterclass.go

ykakarap · 2021-10-12T16:02:28Z

/hold cancel
#5373 is merged and this PR is now rebased on main.

sbueringer · 2021-10-12T17:01:30Z

@ykakarap I'll try to take a look tomorrow

sbueringer

First (more or less) high-level review

Open questions from my side which have the biggest impact on the code:

Can we reference multiple ClusterClasses in a cluster template?
- As far as I know currently a cluster template always contains one Cluster and I think this assumption is pretty baked in (and I think it's a good one)
Can a ClusterClass itself be a template?
- I don't think it should be, as we're doing something wrong with patches and variables if we have to add an additional layer of templating to our ClusterClasses
Do we want to partially render a ClusterClass depending on which resources are already deployed?
- I would prefer just returning an error in this case. I think it's a better UX as it's less surprising and I don't think we should support partial ClusterClass rendering. There are no guarantees that the already deployed parts of a ClusterClass fit to the ones in the ClusterClass file.

@fabriziopandini @ykakarap

cmd/clusterctl/client/cluster/inventory.go

cmd/clusterctl/client/repository/template.go

cmd/clusterctl/client/yamlprocessor/processor.go

cmd/clusterctl/client/clusterclass.go

controllers/topology/cluster_controller.go

sbueringer

Looks good for me. A few nits, otherwise lgtm

controllers/topology/cluster_controller.go

cmd/clusterctl/client/init_test.go

cmd/clusterctl/client/repository/template_test.go

sbueringer · 2021-10-20T16:13:24Z

/lgtm

Not 100% about the apidiff. But we can't implement it without those changes. I don't remember the compatibility guarantees for those packages and can't find them atm.

…command

ykakarap · 2021-10-20T23:00:04Z

@sbueringer had to push a clean up (found some commented out debug statements). Can you please re-lgtm?

k8s-ci-robot · 2021-10-20T23:00:58Z

@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	`61f13e1`	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 · 2021-10-21T06:15:46Z

/lgtm

ykakarap · 2021-10-21T17:08:28Z

/assign @fabriziopandini

fabriziopandini · 2021-10-22T11:42:01Z

Great to see clusterctl getting ready for ClusterClass, we should demo this at the office hours!
/lgtm
/approve

k8s-ci-robot · 2021-10-22T11:42:16Z

[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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2021

k8s-ci-robot requested review from enxebre and sbueringer September 28, 2021 16:17

ykakarap force-pushed the clusterclass_clusterctl_generate branch from f8276aa to e713c88 Compare September 28, 2021 16:31

fabriziopandini reviewed Sep 29, 2021

View reviewed changes

ykakarap force-pushed the clusterclass_clusterctl_generate branch from e713c88 to 88a9fdf Compare October 1, 2021 19:23

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 Oct 1, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2021

ykakarap force-pushed the clusterclass_clusterctl_generate branch 2 times, most recently from a6cc341 to cd82387 Compare October 7, 2021 23:49

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 Oct 7, 2021

ykakarap force-pushed the clusterclass_clusterctl_generate branch from cd82387 to 44b31f0 Compare October 7, 2021 23:50

ykakarap changed the title ~~✨ [WIP] clusterctl: add cluster class support to clusterctl generate cluster~~ ✨ clusterctl: add cluster class support to clusterctl generate cluster Oct 7, 2021

ykakarap changed the title ~~✨ clusterctl: add cluster class support to clusterctl generate cluster~~ ✨ clusterctl: add cluster class support to clusterctl generate cluster Oct 7, 2021

ykakarap changed the title ~~✨ clusterctl: add cluster class support to clusterctl generate cluster~~ ✨ clusterctl: add ClusterClass support to clusterctl generate cluster Oct 7, 2021

fabriziopandini reviewed Oct 12, 2021

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2021

ykakarap force-pushed the clusterclass_clusterctl_generate branch from 44b31f0 to 74c2e58 Compare October 12, 2021 18:33

sbueringer reviewed Oct 13, 2021

View reviewed changes

ykakarap force-pushed the clusterclass_clusterctl_generate branch from 74c2e58 to 52b75c9 Compare October 14, 2021 02:38

ykakarap commented Oct 14, 2021

View reviewed changes

controllers/topology/cluster_controller.go Show resolved Hide resolved

ykakarap force-pushed the clusterclass_clusterctl_generate branch 2 times, most recently from c7465bf to ca13c66 Compare October 14, 2021 05:47

ykakarap mentioned this pull request Oct 14, 2021

🌱 e2e tests for clusterctl quickstart flow using managed topologies #5423

Merged

k8s-ci-robot requested a review from fabriziopandini October 14, 2021 13:36

ykakarap force-pushed the clusterclass_clusterctl_generate branch from ca13c66 to 30acfc5 Compare October 20, 2021 04:52

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 Oct 20, 2021

sbueringer reviewed Oct 20, 2021

View reviewed changes

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

cmd/clusterctl/client/init_test.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/repository/template_test.go Outdated Show resolved Hide resolved

ykakarap force-pushed the clusterclass_clusterctl_generate branch from 30acfc5 to 07136f7 Compare October 20, 2021 15:28

k8s-ci-robot assigned sbueringer Oct 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2021

clusterctl: add cluster class support to clusterctl generate cluster …

61f13e1

…command

ykakarap force-pushed the clusterclass_clusterctl_generate branch from 07136f7 to 61f13e1 Compare October 20, 2021 22:59

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2021

k8s-ci-robot assigned fabriziopandini Oct 21, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2021

k8s-ci-robot merged commit 2778a0a into kubernetes-sigs:main Oct 22, 2021

k8s-ci-robot added this to the v0.4 milestone Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ clusterctl: add ClusterClass support to `clusterctl generate cluster` #5351

✨ clusterctl: add ClusterClass support to `clusterctl generate cluster` #5351

ykakarap commented Sep 28, 2021 •

edited

Loading

ykakarap commented Oct 1, 2021

fabriziopandini left a comment

ykakarap commented Oct 12, 2021

sbueringer commented Oct 12, 2021

sbueringer left a comment •

edited

Loading

sbueringer left a comment

sbueringer commented Oct 20, 2021 •

edited

Loading

ykakarap commented Oct 20, 2021

k8s-ci-robot commented Oct 20, 2021

sbueringer commented Oct 21, 2021

ykakarap commented Oct 21, 2021

fabriziopandini commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

✨ clusterctl: add ClusterClass support to clusterctl generate cluster #5351

✨ clusterctl: add ClusterClass support to clusterctl generate cluster #5351

Conversation

ykakarap commented Sep 28, 2021 • edited Loading

ykakarap commented Oct 1, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

ykakarap commented Oct 12, 2021

sbueringer commented Oct 12, 2021

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Oct 20, 2021 • edited Loading

ykakarap commented Oct 20, 2021

k8s-ci-robot commented Oct 20, 2021

sbueringer commented Oct 21, 2021

ykakarap commented Oct 21, 2021

fabriziopandini commented Oct 22, 2021

k8s-ci-robot commented Oct 22, 2021

✨ clusterctl: add ClusterClass support to `clusterctl generate cluster` #5351

✨ clusterctl: add ClusterClass support to `clusterctl generate cluster` #5351

ykakarap commented Sep 28, 2021 •

edited

Loading

sbueringer left a comment •

edited

Loading

sbueringer commented Oct 20, 2021 •

edited

Loading