Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cluster class support #10

Merged
merged 21 commits into from
Apr 16, 2024
Merged

Cluster class support #10

merged 21 commits into from
Apr 16, 2024

Conversation

65278
Copy link
Collaborator

@65278 65278 commented Nov 29, 2023

Description of changes:
We have a ClusterClass. It supports all features that all templates support (and then some).

Testing performed:
I have tested most setups by deploying them, but it'd really help if everybody chimed in for testing. I've updated docs/Usage.md

Discussion:
The ClusterClass works by essentially adding ProxmoxMachineSpec to the Cluster data structure. From this, the ClusterClass makes patches to ProxmoxMachineTemplates, which then get packaged into MachineDeployments (or ControlPlanes). As such it delivers every feature you could pack into a ProxmoxMachineSpec.

Limitations:
I can not template things for which there are no templates. This pertains to ipam pool refs and clusterResourceSets. I've made CNIs work by matching labels.
I can not control the naming of certain resources, leading to a lot of redundancy in names generated.

Comments I have archived from avorima. All of them are fixed:
A few things:

  • Your problem with the required fields comes from the wrong declaration of the field. Use a pointer and omitempty and you won't have to fix the test.
  • Another thing is the controller. You mention it has a job, but I don't see any implementation. Is a controller really required here?
  • Regarding the kubernetesVersion field. https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/write-clusterclass mentions that .spec.topology.version is already used to set the k8s version on the KCP and MD. Did that not work?
  • The SSH keys and VIP interface seem a bit out of place in the "nodeCloneSpec" as they're really more KCP and KC inputs.

Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

16.7% 16.7% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
16.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@65278
Copy link
Collaborator Author

65278 commented Feb 21, 2024

I've addressed all the points in Avorima's review (good review Avorima, thank you. Sorry it took ages to get here).

  • machineSpec and KCP items are split (although I'm willing to talk about a different datastructure here)
  • removed the kubernetesVersion field. I implemented it to be similar to our templates, but it serves no purpose, really
  • removed the ProxmoxClusterTemplate controller as we never instantiate it anyway. I'm uncertain what it would actually achieve. I think we could remove the ProxmoxClusterTemplate patching with removing ipv4/ipv6 from the clusterclass. This has the added advantage of making the PR essentially nocode (except for the datastructure changes).
  • I made ProxmoxClusterCloneSpec a pointer so it can be Nil, and can thus be omitted, which means that tests do not need to be reworked. I've reverted the tests.

Apart of that, I've split machineSpecs so you can define individual machine setups for ControlPlane, WorkerNodes and LoadBalancers. If you extend the ClusterClass, you can even implement your own machines as they're a map[string]ProxmoxMachineSpec{}.

I still need to add ipvs support to KubeAdm, or else LoadBalancers will not work. Also, taints and tolerations for LoadBalancers are missing, so they will run as regular nodes.

Copy link

@wikkyk wikkyk merged commit e60da87 into main Apr 16, 2024
8 checks passed
@wikkyk wikkyk deleted the cluster_class_support branch April 16, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants