Feat: Generate helm charts #291
Conversation
someth2say
added
enhancement
|
Thanks @someth2say for this! Apologies for not getting to the review sooner. We have Red Hat summit going on this week so that will take up the majority of my time this week. I will get to this on Friday if that's ok? Also you mentioned that this supercedes #237 - am I ok to close that one? |
|
One thing I do notice off the bat is that in the |
|
@edeandrea yes I think we're good to close that PR, I can close it now. @someth2say thank you for taking this over! is there any chance I can find a helm chart that gets generated? I don't see any. I also remember @edeandrea mentioning that having separate scripts for the k8s resources and helm charts might be better from a maintenance standpoint, but I think the script looks good to me with both combined (and I think makes it so that the build doesn't need to be duplicated) |
Mmmm... not sure that is true. I retained the conditional setting the Maybe am I missing some point?
👍
The charts will be generated by the GH workflow Nevertheless, I just realized that the workflow seems to have not been running for a while:
Yes, that was also my first approach. |
Maybe I missed something as I only took a quick look :) I'll have a more thorough look on Friday.
It should run on each commit to main. I'll need to figure out why it's not. I feel like it did run way more recently. Another thing for my Todo list on Friday. You may need to alter the workflow to add any new
Yes @someth2say and I discussed this @ingmarfjolla after we realized we could reuse the same Quarkus build for the helm charts. |
|
The It's done that way because it's templatized as to which branch to commit to. It is reused on all the different branches and accepts parameters from the calling workflow. |
There was a problem hiding this comment.
Hey @someth2say! Nice work!
I've gone through this and have made some comments in-line.
I also noticed that there is no chart for the ui-super-heroes nor the monitoring stuff. Were those forgotten?
For the monitoring stuff, you will probably have to hand-craft a chart inside the /monitoring folder (something like /monitoring/helm, similar to what we do for docker-compose (/monitoring/docker-compose) and k8s (/monitoring/k8s).
The generate-k8s-resources.sh script then creates the various monitoring descriptors within /deploy/docker-compose and /deploy/k8s. The helm charts will have to have the deployment type variants (knative, kubernetes, minikube, openshift) as well.
Similar for the ui-super-heroes. You'll probably have to hand-craft a chart following the same pattern(s) used for the k8s descriptors.
Also, I don't see that any of the documentation (each individual project README nor the main repo README) has been updated with any info about the helm charts nor how to use them. I would think in the main README as well as in each individual project in the Deploying to Kubernetes sections there should be a sub-section about using the Helm charts.
I also ran the generate-k8s-resources.sh script and noticed a few things:
- The note on the PR mentioned there was no "uber" chart yet, but after running I do see a
/deploy/helmdirectory with stuff in there. What is that stuff that's in there?
-
For each app in the
deploy/helm/$deploymentTypedirectory there is only a single values file. Didn't we talk about having multiple values files, one for each combination of java version + kind? It was a while ago, so maybe we changed our minds, but thats how I thought it was supposed to be. -
After running in each project I notice there is a
$project/deploy/helm/$deploymentType/chartsdirectory which is empty. Is this expected?
-
After running in each project I notice there is a
$project/deploy/helm/$deploymentType/README.mdfile which mentions how to install the chart
Those instructions are not correct. The correct command should be
helm install -f values.yaml chart-name . -
I tried to install each individual chart in this order:
rest-villainsrest-heroesevent-statisticsrest-fights
When installing the
rest-fightschart, I got this error:╰─ helm install -f values.yaml rest-fights . Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: RoleBinding "default_view" in namespace "eric-deandrea-dev" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "rest-fights"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "eric-deandrea-dev"
I think this is because both
event-statisticsandrest-fightsboth have definitions for the kafka/apicurio stuff since they both use it. There probably needs to be adjustments in the charts for both those apps to only deploy those resources if they don't already exist. When wekubctl apply -fits essentially a no-op becausekubectl apply -fwill ignore resources that are identical. Helm does not.
scripts/generate-k8s-resources.sh
Outdated
| if [ "${DEBUG}" = true ]; then | ||
| # Order the resources for testing purposes | ||
| echo "DEBUG: Sorting kubernetes resources at $project_k8s_file" | ||
| jbang yamlsort@someth2say -yamlpath "kind" -yamlpath "metadata.name" -i "${project_k8s_file}" > "${project_k8s_file}.sort"; |
There was a problem hiding this comment.
Where is jbang or helm installed? Users who run this script locally may not have either of them installed, and CI certainly does not have them already installed.
Do the resources really need to be sorted?
There was a problem hiding this comment.
Where is jbang or helm installed?
Yes, the user might not have jbang or heml.
This piece of code is only to be executed in "debug" mode. Debug mode is designed only for developers (should we better name it "development mode"?) in order to debug the script.
If the developer do not have jbang and helm available, then they will not be able to use debug mode.
Do the resources really need to be sorted?
For the script to run in CI, or simply to generate the resources, then no, we don´t need the resources sorted.
But for debugging, it is very useful.
What we are doing is, when in debug mode, we use helm template with the appropriate values file to generate the k8s resources.
Then we sort BOTH the k8s resources generated by helm templateand the k8s resources generated by the quarkus-kubernetes extension.
Once we have both resources generated, we can directly compare the files (p.e. with diff) to validate both resources are exactly the same.
With this approach I can confirm the helm charts are generating all the expected resources with the expected values.
There was a problem hiding this comment.
Makes sense! I think we should have some documentation somewhere then that informs the user about these dependencies if they want to use debug mode.
There was a problem hiding this comment.
Absolutelly :)
Next step, updating the docs.
|
|
||
| # Execute templates into a k8s-like resources file. | ||
| # This is optional, and only enabled for testing purposes | ||
| if [ "${DEBUG}" = true ]; then |
There was a problem hiding this comment.
I'm not expert in bash, but as per https://stackoverflow.com/questions/2953646/how-can-i-declare-and-use-boolean-variables-in-a-shell-script, it seems [ and = is the correct way.
I tested in my side with DEBUG=true ./scripts/generate-k8s-resources.sh and it works as expected.
No, not forgotten, but still pending :)
Yes, documentation still pending.
That was an error in the script. I gave the uber-chart a try, but then I forgot to remove the code doing that.
Those files you see are generated by the quarkus-helm extension. The values you refer to are in the
I think that yes, it is expected.
This file is also generated by quarkus-helm.
🤔 I would need more guidance here. |
|
@someth2say any chance you can maybe push the helm charts to another git folder somewhere so I can try installing it on Openshift? and see if i get this error (though it seems you already may have got the root cause): |
Just do |
I use Red Hat Developer Sandbox (https://red.ht/dev-sandbox) :) Thats where I tried the Its more than just the If you look at any of the k8s descriptors in When you |
|
so testing on openshift 4.13, I got a similar error with the resources relating to ownership but not exactly what you saw : i did it in the order of rest-villains -> rest-heroes-> rest-fights-> and then event-statistics. For helm only 1 chart can manage the resource, so it might not be an issue when you do the uber chart if its handpicked but I'm not sure how to handle it in this case when it's automatically generated |
Yeah the order you do it will affect the error you see, but generally the problem is because both So yes you are right @ingmarfjolla that the uber chart will be a problem, but so will deploying each project individually (as you & I have tried to do). We need to figure out a way around this. Is there an option in helm itself to say "hey skip this resource if it already exists"? I'm sure something like this is not a new problem, as there are plenty of things that get deployed that are shared amongst applications. Any kind of messaging platform (kafka/etc) would be a perfect example. |
|
based on this issue: helm/helm#4824 there are a couple of workarounds. It looks like we could do something with the helm lookup function like this: but I haven't used it and i'm not sure if's the most elegant solution since I'm not sure how many duplicated resources there are and idk if you want to wrap every resource in a function like that |
|
you can do conditionals also, which might be an easier solution but would also need some hand picking / manual intervention. So something like values.yaml: service.yaml (for the event statistics): sorry if I'm being confusing/not explaining well enough |
I am thinking a bit about this, and I don´t think we could (easily) use helm functions.
Talking from the perspective of a newby: Maybe we should step back, and ask ourselves "We do we have duplicated resources?" |
I think the reasoning goes back to one of the original designs/"guiding principles" of the entire system in the first place - make it stupid simple for anyone to deploy it without requiring manual changes/editing files/etc. Just a single command and boom, things are deployed, either individual services or the entire system. You want to deploy the fights service and all its dependencies? Single |
So far I just updated the rest-villains. The rest of the services will come later.
Performance boost to the k8s script.
- Temporary disable copying helm charts to the super-heroes/deploy directory. - Removed unneeded image definition in value files. - Use helm.version property in pom files - Added helm resources to the commit in create-deploy-resources.yml - Reordered base loop for clarity
someth2say
force-pushed
the
feature/generate-helm-charts
branch
from
4273d36 to
f4b5883
Compare
edeandrea
force-pushed
the
main
branch
7 times, most recently
from
2207b61 to
e55a5b9
Compare
This PR proposes an initial skeleton for creating HELM charts for the projects.
It addresses the problem by:
quarkus-helmextensionSimple cases, such as
versionor memory can be set using helm value mappings.Other complex cases, such as image names or imagestream names need using helm expressions.
scripts/generate-k8s-resources.shfile to enable the generation.Collateraly, some refactoring applied to simplify logging and debugging.
This PR have two pending features:
. Add the downstream projects to the charts for
rest-fights. Generate an "uber-chart" able to deploy all projects at once, given a single values file.
Nonetheless, those can be addressed in later PRS.
Worth mentioning the
DEBUGmode of the script.When enabled (by setting the
DEBUGenvvar totrue) it does the following:. Applies the helm chart with all the value files, generating the resources for all combinations. Those are generated in
$project/deploy/helm/generated. Uses
jbang yamlsort@someth2sayin order to sort both the generated resources and the resources at$project/deploy/k8sWith those, we can easiy compare the files in
generatedwith the files ink8sto make sure both are almost identical (expected differences in timestamps and the order of some attributes).This PR superseedes #237.
I tried to retain previous commits so contributors are honoured.