🌱 Support for ClusterClass template #1405

p-strusiewiczsurmacki-mobica · 2024-01-16T14:38:11Z

What this PR does / why we need it:

This PR adds support for ClusterClass API as discussed in #1267

Which issue(s) this PR fixes:
Fixes #1267

metal3-io-bot · 2024-01-16T14:38:22Z

Hi @p-strusiewiczsurmacki-mobica. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

p-strusiewiczsurmacki-mobica · 2024-01-16T14:39:41Z

Converting this to draft as I am yet trying to create testing environment for this change.

Rozzii · 2024-01-31T11:45:56Z

/cc @lentzi90 @zaneb @Rozzii @hardys FYI

metal3-io-bot · 2024-01-31T11:46:03Z

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

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

In response to this:

/cc @lentzi90 @zaneb @Rozzii @hardys FYI

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.

Rozzii · 2024-01-31T11:46:38Z

/cc @kashifest

lentzi90 · 2024-02-01T09:42:34Z

/ok-to-test

lentzi90

Thanks for working on this! I think the code is looking good already but I'm a bit confused about the kustomizations under examples...
Why is it split up in cluster, controlplane and machinedeployment? I would expect to have one kustomization with everything needed for the ClusterClass (Metal3MachineTemplates, Metal3ClusterTemplate, KubeadmControlPlaneTemplate, KubeadmConfigTemplates, and also Metal3DataTemplates and IPPools). Then the ClusterClass kustomization would be "self-contained" and we can have a separate cluster-template to create clusters based on this class. What do you think?

lentzi90 · 2024-02-01T10:02:04Z

examples/clusterclass/cluster/cluster.yaml

+---
+apiVersion: cluster.x-k8s.io/v1beta1
+kind: Cluster
+metadata:
+  name: ${CLUSTER_NAME}
+  namespace: ${NAMESPACE}


The cluster-template should be separate from the ClusterClass IMO, and we should not rely on the CLUSTER_NAME in the ClusterClass at all. That way it will be possible to reuse the ClusterClass for multiple clusters.

Changed that. I previously left the old names as it was easier for me to make clusterclass example similar to the non-clusterclass one as possible.
Can you check if this is what you've meant?

lentzi90 · 2024-02-01T10:03:37Z

examples/generate.sh

@@ -94,14 +97,26 @@ ENVSUBST="${SOURCE_DIR}/envsubst-go"
 curl --fail -Ss -L -o "${ENVSUBST}" https://github.com/a8m/envsubst/releases/download/v1.2.0/envsubst-"$(uname -s)"-"$(uname -m)"
 chmod +x "$ENVSUBST"

+SRC_DIR="${SOURCE_DIR}"
+REORDER_TYPE="--reorder=legacy"


Why is this needed?

Mainly --reorder=none is required for clusterclass example. By default kustomization will use reorder=legacy and will reorder definitions in the outcome file alphabetically. Now, if the definitions are ordered and ClusterClass will be in the outcome file above the Metal3 definitions (e.g. Metal3DataTemplate), during deletion with make delete-examples-clusterclass the operation will fail as k8s will first delete ClusterClass and associated Metal3 objects and will try to delete Metal3 objects once again. At least that's what's happening to me, hence reored=none was added for ClusterClass example and reorder=legacy was added for non-ClusterClass example generation.

However, I've deleted this reorder=legacy and now only reorder=none will be used and only for ClusterClass.

lentzi90 · 2024-02-01T10:05:31Z

api/v1beta1/metal3clustertemplate_webhook_test.go

+	g.Expect(c.Spec).To(Equal(Metal3ClusterTemplateSpec{}))
+}
+
+func TestMetal3ClusterTemplateValidation(t *testing.T) {


Could you add one test for when the template is invalid also?

p-strusiewiczsurmacki-mobica · 2024-02-05T18:12:31Z

Added changes required by @lentzi90

Additionally added some changes in the Tiltfile and other scripts.
Now, to test with Tilt below commands should be sufficient:

make tilt-settings-clusterclass
make tilt-up
make generate-examples-clusterclass
make deploy-examples-clusterclass

make titl-settings-clusterclass will now generate settings that should enable ClusterTopology in Cluster API.

Additionally, I was able to change the example available in metal3-dev-env and to deploy and provision cluster using ClusterClass, so I'm moving this from draft.

If anyone wants to try to test it with metal3-dev-env here's my branch with the changes.. Cluster should be provisioned if make test will be executed, but test itself will fail as I don't know how to enable ClusterTopology in the 'internal' cluster yet.

lentzi90

I would not worry about the tilt setup here honestly. If you want to work on it, fine, but isn't it easier to just use CAPI's directly?
The template and ClusterClass that you have here will work great with that already. The only thing you have to do is to add the folder to template_dirs in the tilt settings.

examples/clusterclass/class/class.yaml

lentzi90 · 2024-02-09T12:43:16Z

examples/clusterclass/class/class.yaml

+        host: ${CLUSTER_APIENDPOINT_HOST}
+        port: ${CLUSTER_APIENDPOINT_PORT}


We should make these variables in the ClusterClass or it will be impossible to create more than one cluster from it.

Done. Added variables and patches to ClusterClass and referenced those from Cluster itself. Seems to work fine regarding to my tests.
Just one question - currently Metal3ClusterTemplate is validated by the webhook for controlPlaneEndpoint definition, meaning something has always be defined there. Should this validation be omitted and we should allow empty definition (without specifying controlPlaneEndpoint`)?

Not sure I understood the question. I think the controlPlaneEndpoint is always required. How would the cluster work without it?

Currently Metal3ClusterTemplate is required to have controlPlaneEndpoint defined, e.g:

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 kind: Metal3ClusterTemplate metadata: name: example-cluster-template spec: template: spec: controlPlaneEndpoint: host: 127.0.0.1 port: 6443 noCloudProvider: true

But I think that makes it not reusable, right? Do we want to accept something like this (with no controlPlaneEndpoint):

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 kind: Metal3ClusterTemplate metadata: name: example-cluster-template spec: template: spec: noCloudProvider: true

And then controlPlaneEndpoint should be defined in Cluster resource, I think?

Ok now I understand!
I think the way to handle this is to use variables in the ClusterClass. So set it like you already did in the Metal3ClusterTemplate, but then add a variable and patch to override it. Then it would be possible to create clusters like

apiVersion: cluster.x-k8s.io/v1beta1 kind: Cluster metadata: name: my-cluster spec: ... topology: class: example-clusterclass version: v1.29.0 variables: - name: controlPlaneEndpointHost value: 192.168.0.100

examples/clusterclass/class/class.yaml

lentzi90 · 2024-02-09T12:45:39Z

examples/clusterclass/class/class.yaml

+        checksum: ${IMAGE_CHECKSUM}
+        checksumType: ${IMAGE_CHECKSUM_TYPE}
+        format: ${IMAGE_FORMAT}
+        url: ${IMAGE_URL}


These would also be nice to set through ClusterClass variables, but more as a nice to have.

examples/clusterclass/cluster/cluster.yaml

lentzi90 · 2024-02-09T12:55:33Z

hack/gen_tilt_settings.sh

@@ -44,10 +44,27 @@ get_latest_release() {
 CAPIRELEASEPATH="${CAPIRELEASEPATH:-https://api.github.com/repos/${CAPI_BASE_URL:-kubernetes-sigs/cluster-api}/releases}"
 export CAPIRELEASE="${CAPIRELEASE:-$(get_latest_release "${CAPIRELEASEPATH}" "v1.3.")}"

+# ClusterClass enable flag
+CLUSTERCLASS_ENABLE="${CLUSTERCLASS:-}"


I think we should use the same variable as CAPI does, so this should be CLUSTER_TOPOLOGY.

lentzi90 · 2024-02-09T13:01:16Z

examples/generate.sh

This way of generating manifests with kustomize and envsubst doesn't seem to really give us anything useful from what I can see. I'm fine having it here is if it useful to you but I wonder if we should not just put the ClusterClass and cluster-template in templates instead? No kustomize needed (we are anyway not doing anything useful with it) and they would be easily discovered and useful. It also matches exactly what CAPI has for CAPD.

I've deleted customization files and moved clusterclass.yaml and cluster.yaml to examples/templates/.

hack/gen_tilt_settings.sh

lentzi90 · 2024-02-19T09:41:52Z

examples/templates/clusterclass.yaml

+        clusterConfiguration:
+          apiServer:
+            extraArgs:
+              cloud-provider: baremetal


What is this cloud provider?

should be CCM, and looks like a leftover before the rename (baremetal=>metal3)

TBH, I've copied it from the existing example: https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/examples/controlplane/controlplane.yaml#L28
Should this be changed?

Not sure about what this does honestly. I think we can remove it. We don't use it in the tests at least: https://github.com/metal3-io/cluster-api-provider-metal3/blob/main/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml

OK, removed all references to cloud-provider: baremetal.

Makefile

examples/generate.sh

lentzi90 · 2024-02-19T09:48:00Z

examples/templates/clusterclass.yaml

+metadata:
+  name: example-md-metadata
+spec:
+  clusterName: example


Too bad we have the clusterName on the Metal3DataTemplate and IPPool 😞
I guess this makes it quite hard to use them from the ClusterClass? Would it make more sense to have them separate or in the cluster.yaml?

kashifest · 2024-04-10T19:03:53Z

This would need squashing of the commits I suppose, @furkatgofurov7 PTAL

furkatgofurov7 · 2024-04-11T07:04:06Z

This would need squashing of the commits I suppose, @furkatgofurov7 PTAL

@kashifest I approved it already, had a small comment but this is not a blocker for this PR.

And yes, the last 2 commits could be squashed into 1, preferably.

metal3-io-bot · 2024-04-11T11:11:35Z

@p-strusiewiczsurmacki-mobica: 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
markdownlint	`4552910`	link	true	`/test markdownlint`

Full PR test history. Your PR dashboard.

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.

p-strusiewiczsurmacki-mobica · 2024-04-11T11:40:10Z

@furkatgofurov7

And yes, the last 2 commits could be squashed into 1, preferably.

I've left 2 commits now. Is it OK, or should squash it into 1?

Also, I've just rebased the branch as there were some conflicts. Trying to run e2e tests to check if everything is correct just now.

p-strusiewiczsurmacki-mobica · 2024-04-11T12:49:10Z

Well, both this branch and main branch just kills my VM when first node is being provisioned. Need to investigate that.

EDIT: Apart from that, the clusterclass and cluster resource are created correctly, so I think the PR itself should be OK.

furkatgofurov7 · 2024-04-11T14:46:50Z

I've left 2 commits now. Is it OK, or should squash it into 1?

I am fine with as is (2 commits).

furkatgofurov7 · 2024-04-11T14:47:09Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

furkatgofurov7 · 2024-04-12T19:36:20Z

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

furkatgofurov7 · 2024-04-12T19:36:58Z

@kashifest tests are not triggered, is this a known issue or triggers have changed?

kashifest · 2024-04-15T06:36:10Z

@kashifest tests are not triggered, is this a known issue or triggers have changed?

Yeah its a known issue, CI admins are working on it, tests are getting triggered but status is not getting updated on github.

LingyanCao · 2024-04-27T15:09:33Z

May I know when could this PR be merged and when is the next release that contains this change? @p-strusiewiczsurmacki-mobica
Our product has the ClusterClass dependency of capm3. I want to raise this risk if there is schedule.
Thank you in advance!

kashifest · 2024-04-27T18:57:49Z

May I know when could this PR be merged and when is the next release that contains this change? @p-strusiewiczsurmacki-mobica Our product has the ClusterClass dependency of capm3. I want to raise this risk if there is schedule. Thank you in advance!

I think this is ready to go in , pending a rebase

chess-knight · 2024-04-30T06:18:02Z

examples/templates/clusterclass.yaml

+    spec:
+      joinConfiguration:
+        nodeRegistration:
+          name: '{{ ds.meta_data.hostname }}'


This doesn't work for me(virtualized env). Shouldn't it be '{{ ds.meta_data.local_hostname }}' or '{{ ds.meta_data.name }}'?. The question is also, what is the difference between local_hostname and name because I see both here.

We can fix it in a followup or leave it out completely from the template in a followup PR. @p-strusiewiczsurmacki-mobica would you be interested in a followup PR? For now, we take this PR in

@kashifest I'll try to take a look at this as soon as I can. However if anyone wants to work on this before I'll be able to, then go ahead. :)

Co-authored-by: Lennart Jern <[email protected]> Co-authored-by: Furkat Gofurov <[email protected]> Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>

Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>

p-strusiewiczsurmacki-mobica · 2024-04-30T11:03:36Z

May I know when could this PR be merged and when is the next release that contains this change? @p-strusiewiczsurmacki-mobica Our product has the ClusterClass dependency of capm3. I want to raise this risk if there is schedule. Thank you in advance!

I think this is ready to go in , pending a rebase

Hi, I've just rebased it :)

kashifest · 2024-04-30T11:46:32Z

/lgtm

kashifest · 2024-04-30T11:46:55Z

/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 16, 2024

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 16, 2024

p-strusiewiczsurmacki-mobica marked this pull request as draft January 16, 2024 14:38

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2024

p-strusiewiczsurmacki-mobica mentioned this pull request Jan 16, 2024

Implement the ClusterClass API #1267

Closed

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 6849485 to d8f1d12 Compare January 26, 2024 14:41

metal3-io-bot requested review from lentzi90, zaneb, Rozzii and hardys January 31, 2024 11:46

Rozzii requested a review from kashifest January 31, 2024 11:47

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 8bd749e to 8834b66 Compare January 31, 2024 11:54

metal3-io-bot 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 Jan 31, 2024

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 1, 2024

lentzi90 reviewed Feb 1, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 48e7b67 to a355040 Compare February 5, 2024 14:46

p-strusiewiczsurmacki-mobica marked this pull request as ready for review February 5, 2024 17:52

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2024

lentzi90 reviewed Feb 9, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 60b4bb7 to a8d4f82 Compare February 13, 2024 18:37

lentzi90 reviewed Feb 19, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 93f5b4f to 4552910 Compare April 11, 2024 11:10

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 4552910 to 48687d0 Compare April 11, 2024 11:36

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 48687d0 to 854ef91 Compare April 11, 2024 14:30

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2024

chess-knight mentioned this pull request Apr 24, 2024

Cluster Stacks for Metal³ SovereignCloudStack/cluster-stacks#21

Closed

2 tasks

chess-knight reviewed Apr 30, 2024

View reviewed changes

p-strusiewiczsurmacki-mobica and others added 2 commits April 30, 2024 13:02

Added support for ClusterClass

95ace1a

Co-authored-by: Lennart Jern <[email protected]> Co-authored-by: Furkat Gofurov <[email protected]> Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>

Added basic e2e tests

7e3d56c

Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>

p-strusiewiczsurmacki-mobica force-pushed the clusterclass-support branch from 854ef91 to 7e3d56c Compare April 30, 2024 11:02

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2024

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2024

metal3-io-bot merged commit e948801 into metal3-io:main Apr 30, 2024
18 checks passed

p-strusiewiczsurmacki-mobica mentioned this pull request May 16, 2024

🐛 Clusterclass e2e test fix #1671

Merged

		host: ${CLUSTER_APIENDPOINT_HOST}
		port: ${CLUSTER_APIENDPOINT_PORT}

🌱 Support for ClusterClass template #1405

🌱 Support for ClusterClass template #1405

Conversation

p-strusiewiczsurmacki-mobica commented Jan 16, 2024

metal3-io-bot commented Jan 16, 2024

p-strusiewiczsurmacki-mobica commented Jan 16, 2024

Rozzii commented Jan 31, 2024

metal3-io-bot commented Jan 31, 2024

Rozzii commented Jan 31, 2024 • edited Loading

lentzi90 commented Feb 1, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica commented Feb 5, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Apr 10, 2024

furkatgofurov7 commented Apr 11, 2024

metal3-io-bot commented Apr 11, 2024 • edited Loading

p-strusiewiczsurmacki-mobica commented Apr 11, 2024

p-strusiewiczsurmacki-mobica commented Apr 11, 2024 • edited Loading

furkatgofurov7 commented Apr 11, 2024

furkatgofurov7 commented Apr 11, 2024

furkatgofurov7 commented Apr 12, 2024

furkatgofurov7 commented Apr 12, 2024 • edited Loading

kashifest commented Apr 15, 2024 • edited Loading

LingyanCao commented Apr 27, 2024 • edited Loading

kashifest commented Apr 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-strusiewiczsurmacki-mobica commented Apr 30, 2024

kashifest commented Apr 30, 2024

kashifest commented Apr 30, 2024

Rozzii commented Jan 31, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica Feb 5, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica Feb 21, 2024 •

edited

Loading

metal3-io-bot commented Apr 11, 2024 •

edited

Loading

p-strusiewiczsurmacki-mobica commented Apr 11, 2024 •

edited

Loading

furkatgofurov7 commented Apr 12, 2024 •

edited

Loading

kashifest commented Apr 15, 2024 •

edited

Loading

LingyanCao commented Apr 27, 2024 •

edited

Loading