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

✨ Add preparation of clusterclass #141

Merged

Conversation

farodin91
Copy link
Contributor

What is the purpose of this pull request/Why do we need it?

Issue #, if available:

Description of changes:

Special notes for your reviewer:

Checklist:

  • Documentation updated
  • Unit Tests added
  • E2E Tests added
  • Includes emojis

@lubedacht
Copy link
Contributor

Check out this example from Cluster API

You seem to be missing a couple of things, e.g. clusterclass.spec.workers

Have you been able to create different cluster with different configurations from your ClusterClass template? If yes can you maybe provide some docs on how you did it?

I've checked out your branch, because I have never used ClusterClass before and wanted to test it. I couldn't really figure out how to do it, therefore I dived a bit into the documentation.

https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/write-clusterclass#clusterclass-with-patches

If I'm not wrong here, you are mixing up multiple things. We need to work with JSON patches.

jsonPatches:
      - op: add
        path: /spec/template....
        valueFrom:
          variable: xxxx

Therefore, you cannot have environment variables in the clusterclass template
(except for maybe CLUSTER_CLASS_NAME and NAMESPACE, used with clusterctl generate yaml)
e.g.

Instead of:

kind: IonosCloudMachineTemplate
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
metadata:
  name: "${CLUSTER_CLASS_NAME}-template"
  namespace: '${NAMESPACE}'
spec:
  template:
    spec:
      numCores: ${IONOSCLOUD_MACHINE_NUM_CORES}
      memoryMB: ${IONOSCLOUD_MACHINE_MEMORY_MB}
      disk:
        image:
          id: ${IONOSCLOUD_MACHINE_IMAGE_ID}

You would then need a patch for that. Something like

kind: IonosCloudMachineTemplate
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
metadata:
  name: "${CLUSTER_CLASS_NAME}-template"
  namespace: '${NAMESPACE}'
spec:
  template: {}
...
  patches:
  - name: workerMachineType
    definitions:
    - selector:
        apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
        kind: IonosCloudMachineTemplate
        matchResources:
          machineDeploymentClass:
            names:
            - XXXX
      jsonPatches:
      - op: add
        path: /spec/template/spec/numCores
        valueFrom:
          variable: numCPUs
     - op: add
        path: /spec/template/spec/memoryMB
        valueFrom:
          variable: memoryMB
     - op: add
       path: /spec/template/spec/disk/image/id
       valueFrom:
         variable: imageID
...

Copy link
Collaborator

@65278 65278 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, nice work.
I haven't tested this, only read it, and all my comments amount to nitpicking, so from my side this is okay, but it's not my place to greenlight this. My only qualification here is also having written a clusterclass.

templates/clusterclass-template.yaml Outdated Show resolved Hide resolved
templates/clusterclass-template.yaml Outdated Show resolved Hide resolved
templates/clusterclass-template.yaml Outdated Show resolved Hide resolved
templates/clusterclass-template.yaml Outdated Show resolved Hide resolved
templates/clusterclass-template.yaml Show resolved Hide resolved
@farodin91
Copy link
Contributor Author

I've added a fixed some values and added a bash script for basic testing.

Followup PRs can add:

  • end to end testing
  • quickstart guide for cluster class

templates/clusterclass-template.yaml Outdated Show resolved Hide resolved
hack/test-cluster-class.sh Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
templates/cluster-template-topology.yaml Outdated Show resolved Hide resolved
@farodin91
Copy link
Contributor Author

@lubedacht Everything should be fixed.

@farodin91 farodin91 force-pushed the add-preparation-of-clusterclass branch from 59c4735 to 5143199 Compare June 12, 2024 13:46
hack/test-cluster-class.sh Outdated Show resolved Hide resolved
hack/test-cluster-class.sh Outdated Show resolved Hide resolved
hack/test-cluster-class.sh Outdated Show resolved Hide resolved
@lubedacht lubedacht added this to the v0.3.0 milestone Jun 13, 2024
Copy link

sonarcloud bot commented Jun 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.5% Duplication on New Code

See analysis details on SonarCloud

@jriedel-ionos jriedel-ionos merged commit 2b47bae into ionos-cloud:main Jun 21, 2024
9 checks passed
Mattes83 pushed a commit to Mattes83/cluster-api-provider-ionoscloud that referenced this pull request Jun 26, 2024
**What is the purpose of this pull request/Why do we need it?**

**Issue #, if available:**

**Description of changes:**

**Special notes for your reviewer:**

**Checklist:**
- [x] Documentation updated
- [ ] Unit Tests added
- [ ] E2E Tests added
- [x] Includes
[emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning)

---------

Signed-off-by: Jan Jansen <[email protected]>
Mattes83 pushed a commit to Mattes83/cluster-api-provider-ionoscloud that referenced this pull request Jun 27, 2024
**What is the purpose of this pull request/Why do we need it?**

**Issue #, if available:**

**Description of changes:**

**Special notes for your reviewer:**

**Checklist:**
- [x] Documentation updated
- [ ] Unit Tests added
- [ ] E2E Tests added
- [x] Includes
[emojis](https://github.com/kubernetes-sigs/kubebuilder-release-tools?tab=readme-ov-file#kubebuilder-project-versioning)

---------

Signed-off-by: Jan Jansen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants