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

🌱 Enhance Tilt integration with CAPO using a ClusterClass template #1833

Merged

Conversation

maxrantil
Copy link
Contributor

@maxrantil maxrantil commented Jan 18, 2024

What this PR does / why we need it:
This PR introduces a ClusterClass template to enhance Tilt integration with CAPO, leveraging the existing CAPI setup. It simplifies cluster creation.

Which issue(s) this PR fixes
Fixes #1767

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 18, 2024
Copy link

netlify bot commented Jan 18, 2024

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit 9a71402
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65b9371edb44ef0008efb83c
😎 Deploy Preview https://deploy-preview-1833--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot
Copy link
Contributor

Hi @maxrantil. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 18, 2024
@lentzi90
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot 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 Jan 18, 2024
@mdbooth
Copy link
Contributor

mdbooth commented Jan 18, 2024

Sounds interesting! Could we have a few lines in the documentation on how to use it?

@maxrantil
Copy link
Contributor Author

@mdbooth absolutely! I just wanted @lentzi90 feedback before I did the final version + documentation.

Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Alright let's see if I understood this.

  • openstack_env.rc reads the e2e_conf.yaml and sets environment variables based on that
  • update_tilt-settings.sh adds substitutions to the tilt settings that would be used to render the template when creating a Cluster from the tilt GUI

Is that correct?

I'm a bit torn on this. On the one hand it is nice to take the settings directly from the e2e config. I could have multiple different configs and easily generate clusters based on them. And we don't have to keep the e2e config in sync with anything else.

On the other hand, this means that the ClusterClass is basically unusable without an e2e config and/or script to render it. That is not so nice. For this reason I would like to put some default values for at least some of the variables. For example, I suggest putting default values for the image, network, DNS servers, failure domain and machine flavors. For the image I think we should also use a ClusterClass variable to set the version as part of the name.

Finally, I think it would be very nice to be able to use a pre-existing secret instead of generating it as part of the template. This is the hardest variable to fill and we cannot really provide a default, so it makes more sense then to expect the user to provide it as a secret already in Kubernetes.

templates/cluster-template-clusterclass.yaml Outdated Show resolved Hide resolved
templates/README.txt Outdated Show resolved Hide resolved
@maxrantil maxrantil force-pushed the clusterclass-tilt-capo/max branch 2 times, most recently from 8133f49 to 52ea4a7 Compare January 19, 2024 12:10
@maxrantil
Copy link
Contributor Author

  • openstack_env.rc reads the e2e_conf.yaml and sets environment variables based on that

    • update_tilt-settings.sh adds substitutions to the tilt settings that would be used to render the template when creating a Cluster from the tilt GUI

Is that correct?

Yes, that is correct.

On the other hand, this means that the ClusterClass is basically unusable without an e2e config and/or script to render it.

The script will be unusable without the e2e config but your can always set the variables yourself so the ClusterClass can still be useful if you do that. I now made the script fail if the e2e_conf is not present.

For example, I suggest putting default values for the image, network, DNS servers, failure domain and machine flavors. For the image I think we should also use a ClusterClass variable to set the version as part of the name.

Okey I did as you suggested except for for external_network_id which will be different on different setups. I also put defaults to 1 for WORKER_MACHINE_COUNT & OPENSTACK_CONTROL_PLANE_MACHINE_FLAVOR.

Finally, I think it would be very nice to be able to use a pre-existing secret instead of generating it as part of the template. This is the hardest variable to fill and we cannot really provide a default, so it makes more sense then to expect the user to provide it as a secret already in Kubernetes.

This part I am now fully understanding. I added some lines in the README.txt file in $REPO_ROOT/templates/ on how you just source the env.rc file to get the secret. Isn't it so that you could bring your own if you like anyway?

p.s. I know I will need to trim the README a little after all is done. Question: Is the README the right place for that or should it be in any of the files in $REPO_ROOT/docs/

@lentzi90
Copy link
Contributor

The script will be unusable without the e2e config but your can always set the variables yourself so the ClusterClass can still be useful if you do that. I now made the script fail if the e2e_conf is not present.

I expressed myself badly. What I mean is that it would be incredibly inconvenient to use the ClusterClass. Think about the clouds.yaml file that is needed. Who in their right mind has it stored base64 encoded? They would need to encode it and then put it in a variable so that we can then render the template which requires this variable to be present.
In this scenario it makes much more sense to create that secret directly with kubectl create secret instead.

This part I am now fully understanding. I added some lines in the README.txt file in $REPO_ROOT/templates/ on how you just source the env.rc file to get the secret. Isn't it so that you could bring your own if you like anyway?

Not sure I follow? As long as the secret is part of the template I don't see how I could bring my own? It would get overwritten by the secret in the template, and I would still need to have a value set for that environment variable. I could possibly overwrite it after I apply the template but that doesn't seem nice either.

p.s. I know I will need to trim the README a little after all is done. Question: Is the README the right place for that or should it be in any of the files in $REPO_ROOT/docs/

I think it could fit better in development.md

@maxrantil
Copy link
Contributor Author

Think about the clouds.yaml file that is needed. Who in their right mind has it stored base64 encoded? They would need to encode it and then put it in a variable so that we can then render the template which requires this variable to be present.
In this scenario it makes much more sense to create that secret directly with kubectl create secret instead.

Isn't everybody having a clouds.yaml file? To get it encoded you just need to run source $REPO_ROOT/tempates/env.rc <clouds.yaml path> <$OS_CLOUD> . I could take that secret part of the template away but now it follows the same structure as the earlier templates which is good i think.

Thanks for all the clarification about different kinds of secrets. I have been a little confused about that part myself that is also why I have been doing it the same way as the earlier templates.

@lentzi90
Copy link
Contributor

Isn't everybody having a clouds.yaml file? To get it encoded you just need to run source $REPO_ROOT/tempates/env.rc <clouds.yaml path> <$OS_CLOUD> . I could take that secret part of the template away but now it follows the same structure as the earlier templates which is good i think.

Yes everybody should have a clouds.yaml. But not everybody runs linux, not everybody has yq installed, not everybody has CAPO cloned so they can easily source this file.

This is not a structure we want to have in all templates IMO. It works well for automated CI but it is not a nice for end users.
I'm hoping we could achieve two things here

  1. Convenient defaults. If it is possible to provide a default value do it. Then we have as few required variables as possible. I suggest we base the defaults on the e2e tests so they will at least fit with the devstack setup we use in CI. Then developers can reuse that without changing values.
  2. Flexibility in tooling. End users are not expected to use our scripts and they should not have to, but they may want to use our templates. They may manually create secrets, they may create them through External Secrets or some other method. We should enable them to do this while still using our templates.

@maxrantil
Copy link
Contributor Author

Okey I get you and I agree fully. I didn't understand that those earlier templates where there for CI usage. I will modify it so there will only be 2-5 variables to set for the user instead of running any scripts for setting variables.

@maxrantil maxrantil force-pushed the clusterclass-tilt-capo/max branch 3 times, most recently from ba51671 to 35c26ee Compare January 23, 2024 19:49
@maxrantil maxrantil changed the title WIP: 🌱 Enhance Tilt integration with CAPO using a ClusterClass template and scripts 🌱 Enhance Tilt integration with CAPO using a ClusterClass template and scripts Jan 23, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2024
@maxrantil maxrantil changed the title 🌱 Enhance Tilt integration with CAPO using a ClusterClass template and scripts 🌱 Enhance Tilt integration with CAPO using a ClusterClass template Jan 23, 2024
@maxrantil maxrantil force-pushed the clusterclass-tilt-capo/max branch 2 times, most recently from 8aacafd to 2c9c6e6 Compare January 26, 2024 07:02
This section provides a guide to create a Kubernetes cluster using ClusterClass. It covers the process of setting up environment variables, creating a Kind cluster, configuring a secret, applying ClusterClass and deploying the cluster.

### Setting Environment Variables

Copy link
Contributor

Choose a reason for hiding this comment

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

don't know whether we want to do this here as we should have doc guide you in doing setup of those envar?
https://github.com/kubernetes-sigs/cluster-api-provider-openstack/tree/main/docs

or anyghing special for clusterclass?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. The main difference and benefit here is that we are adding a ClusterClass that is geared specifically at development. It has defaults that fit the devstack setup so that as few variables as possible are needed. I think we need to point this out @maxrantil
I'll add a few suggestions

Copy link
Contributor Author

@maxrantil maxrantil Jan 29, 2024

Choose a reason for hiding this comment

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

I tried to explain it more clear in the documentation now. See what you think.

docs/book/src/development/development.md Outdated Show resolved Hide resolved
This section provides a guide to create a Kubernetes cluster using ClusterClass. It covers the process of setting up environment variables, creating a Kind cluster, configuring a secret, applying ClusterClass and deploying the cluster.

### Setting Environment Variables

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. The main difference and benefit here is that we are adding a ClusterClass that is geared specifically at development. It has defaults that fit the devstack setup so that as few variables as possible are needed. I think we need to point this out @maxrantil
I'll add a few suggestions

docs/book/src/development/development.md Outdated Show resolved Hide resolved
docs/book/src/development/development.md Outdated Show resolved Hide resolved
docs/book/src/development/development.md Outdated Show resolved Hide resolved
templates/clusterclass-dev-test.yaml Outdated Show resolved Hide resolved
templates/clusterclass-dev-test.yaml Outdated Show resolved Hide resolved
@maxrantil maxrantil force-pushed the clusterclass-tilt-capo/max branch 2 times, most recently from 6fd4c39 to 4e7b443 Compare January 29, 2024 09:47
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

Awesome! I'm happy with this now 🙂
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90, maxrantil

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2024
@lentzi90
Copy link
Contributor

/test pull-cluster-api-provider-openstack-e2e-test

Copy link
Contributor

@EmilienM EmilienM left a comment

Choose a reason for hiding this comment

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

Minor changes.

Copy link
Contributor

@EmilienM EmilienM left a comment

Choose a reason for hiding this comment

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

I finally managed to get tilt working, thanks for that nice tutorial !
I find it super useful. I had minor comments, otherwise LGTM.

Thanks again

@maxrantil maxrantil force-pushed the clusterclass-tilt-capo/max branch 2 times, most recently from 85e15e6 to f5c9bb9 Compare January 30, 2024 17:46
Signed-off-by: Max Rantil <max.rantil@est.tech>
@mdbooth
Copy link
Contributor

mdbooth commented Jan 30, 2024

This is wonderful. If you have time we'll want to have the new templates generated from our existing kustomize or they'll start to drift. I wouldn't want to hold it up for that, though.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2024
@k8s-ci-robot k8s-ci-robot merged commit 64fd897 into kubernetes-sigs:main Jan 30, 2024
9 checks passed
@EmilienM
Copy link
Contributor

@maxrantil I wanted to thank you for your contribution. This really helped me to get into Tilt (and probably changed my life 🤣) !

@maxrantil
Copy link
Contributor Author

Thank you all for your lovely feedback <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop User-Friendly ClusterClass for Enhanced Tilt-CAPO Integration
6 participants