Adding helm charts #237
Adding helm charts #237
Conversation
|
Thank you @ingmarfjolla for this! I will review it tomorrow! |
There was a problem hiding this comment.
Awesome work by the way! This has been a long-standing thing we've wanted to do, so thank you @ingmarfjolla for picking it up!
I've made an initial pass just looking at some basics.
3 things I found missing completely though:
- There doesn't seem to be a chart for the UI project. This may need to be created manually since the UI project isn't Quarkus and there is no extension.
- There is no "uber" chart where a user could easily "helm install" the entire system. That work would need to happen in the
generate-helm-charts.shscript. Or maybe we hand-craft a "top-level" chart which references the other charts as dependencies? Just spit-balling. If we go the dependencies route, then do the depended-upon projects need to be in a referenceable repo somewhere? Just thinking out loud. - There is no chart for the monitoring stack. Should there be?
Outside of that, though, we should have a conversation about how to structure the different charts based on the JVM version matrix (java 11, java 17, native).
For example, if I were to generate all the charts for OpenShift for the rest-villains service (java 11, java 17, native), I would think that the only differences in the charts would be the image name to pull as well as the memory requests/limits on the DeploymentConfig for the app?
If that's true, then those things would be parameterized in the chart, correct? Which, to me, would mean only having a single chart but yet different values files would be an optimal solution?
Of course I'm saying all that without understanding what capabilities the helm extension actually provides. But honestly I think we should think about what the best way to organize this is, and then apply the technology to that. Maybe the helm extension needs an enhancement? Maybe we need to do some "magic" in the generate-helm-charts.sh script? But lets first figure out what the best/optimal solution is first. If we decide that a single chart with multiple values files are best, maybe using Helm Profiles would be a potential solution? (I'm not saying it is, but I was just reading those docs. Not sure I 100% understand it yet though).
@cescoffier / @agoncal what do you both think? I'm going to add you both as reviewers of this as well.
I also think there is some flawed logic in the generate-helm-charts.sh script. When I ran it locally it seems like it did not generate any java17 resources. See screenshot below.
This can be fixed though once we decide on an overall direction re: charts vs multiple values files. Same with updating all the project's documentation (README files, etc) once the design gets shaken out.
| - name: Create helm charts | ||
| shell: bash | ||
| run: scripts/generate-helm-charts.sh | ||
|
|
||
| - name: Create docker-compose resources | ||
| shell: bash | ||
| run: scripts/generate-docker-compose-resources.sh |
There was a problem hiding this comment.
Further down in the Commit generated resources task you probably need to add "**/deploy/helm/*.yml", "deploy/helm/*.yml","**/deploy/helm/*.yaml", "deploy/helm/*.yaml" to the add parameter so that all the generated resources get added & committed back to the repo.
| @@ -0,0 +1,4 @@ | |||
| --- | |||
There was a problem hiding this comment.
I'm only going to make this comment here but it does repeat inside all the projects.
The deploy/helm directory inside each project should not be checked into source control manually. It should be auto-generated by the generate-helm-charts.sh script and added to version control by the create-deploy-resources GitHub actions job.
| @@ -4,7 +4,7 @@ | |||
| <modelVersion>4.0.0</modelVersion> | |||
| <groupId>io.quarkus.sample.super-heroes</groupId> | |||
| <artifactId>event-statistics</artifactId> | |||
| <version>1.0</version> | |||
| <version>java11-latest</version> | |||
There was a problem hiding this comment.
This change should not have been committed. It is done by the generate-helm-charts.sh script, but the pom.xml is never re-committed back.
| <dependency> | ||
| <groupId>io.quarkiverse.helm</groupId> | ||
| <artifactId>quarkus-helm</artifactId> | ||
| <version>0.2.5</version> |
There was a problem hiding this comment.
We should keep the version as a property in the <properties> section.
| @@ -4,7 +4,7 @@ | |||
| <modelVersion>4.0.0</modelVersion> | |||
| <groupId>io.quarkus.sample.super-heroes</groupId> | |||
| <artifactId>rest-fights</artifactId> | |||
| <version>1.0</version> | |||
| <version>java11-latest</version> | |||
There was a problem hiding this comment.
This change should not have been committed. It is done by the generate-helm-charts.sh script, but the pom.xml is never re-committed back.
| <dependency> | ||
| <groupId>io.quarkiverse.helm</groupId> | ||
| <artifactId>quarkus-helm</artifactId> | ||
| <version>0.2.5</version> |
There was a problem hiding this comment.
We should keep the version as a property in the <properties> section.
| @@ -0,0 +1,189 @@ | |||
| #!/bin/bash -ex | |||
|
|
|||
| # Create the deploy/k8s files for each java version of each of the Quarkus services | |||
There was a problem hiding this comment.
Should be "creates the deploy/helm files...."
| -Dquarkus.knative.resources.requests.memory=$mem_request \ | ||
| -Dquarkus.knative.annotations.\"app.openshift.io/vcs-url\"=$GITHUB_SERVER_URL/$GITHUB_REPOSITORY \ | ||
| -Dquarkus.knative.annotations.\"app.openshift.io/vcs-ref\"=$github_ref_name \ | ||
| -Dquarkus.helm.version=1.0.0 \ |
There was a problem hiding this comment.
Should the version be 1.0.0? Or should it be $container_tag?
There was a problem hiding this comment.
Helm chart versions follow semantic versioning, so I just put 1.0.0 for now. If you wanted, it could be something like 1.0.0-$container-tag
There was a problem hiding this comment.
Gotcha. I just read https://helm.sh/docs/topics/charts/#charts-and-versioning and you are correct. Maybe your suggestion 1.0.0-${container_tag} is best then.
| process_quarkus_project $project $deployment_type $version_tag $javaVersion $kind | ||
| done | ||
|
|
||
| # process_ui_project $deployment_type $version_tag |
There was a problem hiding this comment.
What about the UI project? I don't see any helm chart anywhere for the UI project?
| done | ||
|
|
||
| ## Handle the monitoring | ||
| #create_monitoring |
There was a problem hiding this comment.
What about the monitoring stack? Should there be a separate chart for the monitoring stack? Should we talk about what the right solution is?
|
@ingmarfjolla I also moved this PR to draft status for now until we get closer to a more "final" review. |
| # Then add on the ui-super-heroes | ||
|
|
||
| INPUT_DIR=src/main/kubernetes | ||
| OUTPUT_DIR=deploy/k8s |
There was a problem hiding this comment.
Is this right? Why writing to the deploy/k8s directory?
There was a problem hiding this comment.
I have a lot to delete / cleanup in the generate script but i'm not writing to the deploy-k8s directory
|
hey @edeandrea thanks for taking a look! Let me answer the questions now.
This would definitely be done manually and apart of the script. Since it wasn't using the extension, I wanted to push up this draft first to get feedback on the pieces utilizing the extension since the logic for this will be separate anyway but that's next on my agenda to do.
This kind of depended on the conversation about different charts based on the JVM version matrix (java 11, java 17, native). I'd like to have an idea which way is preferred before I tried to automate that "uber" chart. I agree with your idea that a single chart with multiple values files is the cleaner way to go, so I could investigate using helm profiles. I ran into issues with it at first so i'll try to see what issues I run into and open issues in the quarkus-helm repo accordingly. The only differences (to me) seem like image name, the resource requests, and maybe some labels and annotations. So would there be a chart for each deployment (Kubernetes / Openshift / Minikube / Knative) or a single chart with all the resources and then multiple value files? that might be a little tougher since some deployment types had different resources than others
|
|
forgot to add on the topic of helm chart dependencies, Helm supports using a path to the directory instead of putting the charts in a repo: https://helm.sh/docs/helm/helm_dependency/#synopsis. I do need to do some more research because right now in the |
I figured as much
I figured that as well
My gut says this is the "right" way to go. Why duplicate everything if the only difference are some things which should be templatized? I'm curious though about what @cescoffier and @agoncal think. Maybe @ebullient or @holly-cummins have an opinion too?
Yes I think there would still need to be separate charts for the different deployment types. As you mention each type has different resources that are being deployed (i.e. Now you could argue you could use templates/functions/etc within the templates themselves to do that, but honestly I think I'd prefer to keep them simple & separate. We already have this level of separation via the Quarkus Kubernetes extensions (i.e. things in each project's
I thought so too
The script should generate descriptors for java 11, java 17, & native, as it does currently with the That being said, the script does force the compiler version down to 11 when generating for java 11 (see https://github.com/quarkusio/quarkus-super-heroes/blob/main/scripts/generate-k8s-resources.sh#L49).
I'm with you - I don't know a lot about it either. So maybe for now we do the poor man's approach, at least until we "shake out" the rest of the design (single chart, multiple values files, etc). Then if there's a way we can make it cleaner by using dependencies, maybe we do that. |
|
@cescoffier / @agoncal any thoughts on the approach here? |
| local app_generated_helm_chart="$project/target/helm/${deployment_type}/${project}-${container_tag}-${deployment_type}" | ||
| local generated_helm_dir="$project/${OUTPUT_HELM_DIR}/${deployment_type}" | ||
|
|
||
| # 1st do the build | ||
| # The build will generate all the resources for the project | ||
| do_build $project $deployment_type $version_tag $javaVersion $kind | ||
|
|
||
| rm -rf $generated_helm_dir | ||
| mkdir $generated_helm_dir | ||
|
|
||
| # Now merge the generated resources to the top level (deploy/helm) | ||
| if [[ -f "$app_generated_input_file" ]]; then | ||
| echo "Copying app generated helm ($app_generated_input_file) to $project_output_file and $all_apps_output_file" | ||
|
|
||
| cp -R $app_generated_helm_chart $generated_helm_dir | ||
|
|
||
| fi |
There was a problem hiding this comment.
The folder structure here deviates from other deployment types.
P.e. for k8s we have k8s/{version-tag}-{deployment-type}, while for helm we have helm/{deployment-type}/{chart-name}.
Proposal: Aligning a bit, generating helm/{version-tag}/{deployment-type}:
| local app_generated_helm_chart="$project/target/helm/${deployment_type}/${project}-${container_tag}-${deployment_type}" | |
| local generated_helm_dir="$project/${OUTPUT_HELM_DIR}/${deployment_type}" | |
| # 1st do the build | |
| # The build will generate all the resources for the project | |
| do_build $project $deployment_type $version_tag $javaVersion $kind | |
| rm -rf $generated_helm_dir | |
| mkdir $generated_helm_dir | |
| # Now merge the generated resources to the top level (deploy/helm) | |
| if [[ -f "$app_generated_input_file" ]]; then | |
| echo "Copying app generated helm ($app_generated_input_file) to $project_output_file and $all_apps_output_file" | |
| cp -R $app_generated_helm_chart $generated_helm_dir | |
| fi | |
| local app_generated_helm_chart="$project/target/helm/${deployment_type}/${project}-${container_tag}-${deployment_type}" | |
| local generated_helm_dir="$project/${OUTPUT_HELM_DIR}/${version_tag}/${deployment_type}" | |
| # 1st do the build | |
| # The build will generate all the resources for the project | |
| do_build $project $deployment_type $version_tag $javaVersion $kind | |
| rm -rf $generated_helm_dir | |
| mkdir -p $generated_helm_dir | |
| # Now copy the helm files into the deploy directory (deploy/helm) out of the transient target. | |
| if [[ -f "$app_generated_input_file" ]]; then | |
| echo "Copying generated helm chart ($app_generated_helm_chart) to $generated_helm_dir" | |
| cp -R $app_generated_helm_chart/* $generated_helm_dir | |
| fi |
| cp -R "rest-villains/${OUTPUT_HELM_DIR}/${deployment_type}/rest-villains-${container_tag}-${deployment_type}" "${generated_helm_dir}/${project}-${container_tag}-${deployment_type}/charts/" | ||
| cp -R "rest-heroes/${OUTPUT_HELM_DIR}/${deployment_type}/rest-heroes-${container_tag}-${deployment_type}" "${generated_helm_dir}/${project}-${container_tag}-${deployment_type}/charts/" |
There was a problem hiding this comment.
If we accep the previous change to align folder structures, then we also need to update the copy of sub-charts:
| cp -R "rest-villains/${OUTPUT_HELM_DIR}/${deployment_type}/rest-villains-${container_tag}-${deployment_type}" "${generated_helm_dir}/${project}-${container_tag}-${deployment_type}/charts/" | |
| cp -R "rest-heroes/${OUTPUT_HELM_DIR}/${deployment_type}/rest-heroes-${container_tag}-${deployment_type}" "${generated_helm_dir}/${project}-${container_tag}-${deployment_type}/charts/" | |
| mkdir -p "${generated_helm_dir}/charts/rest-villains" | |
| cp -R rest-villains/${OUTPUT_HELM_DIR}/${version_tag}/${deployment_type}/* "${generated_helm_dir}/charts/rest-villains" | |
| mkdir -p "${generated_helm_dir}/charts/rest-heroes" | |
| cp -R rest-heroes/${OUTPUT_HELM_DIR}/${version_tag}/${deployment_type}/* "${generated_helm_dir}/charts/rest-heroes" |
|
@ingmarfjolla I've been thinking a bit about this and honestly I think the route of having a single chart per deployment type with multiple values files is the right way to go. One thing I noticed though which would prevent that - when looking at the charts which were generated, it doesn't seem that the resource requests/limits are templatized in the If we are to go the single chart/multiple values files route, then that would need to be templatized as well, since the values change depending on if it is jvm vs native. Is that something that the current version of the extension does not templatize? Also another question - depending on the deployment type there may be different resources that get generated. For example, when generating for OpenShift, there are |
edeandrea
added
enhancement
|
hey @edeandrea I agree, the one chart per deployment type is the cleanest and most similar to what we'd probably see at a client or more mature org.
so out of the box it does not, but you can specify a helm expression: https://quarkiverse.github.io/quarkiverse-docs/quarkus-helm/dev/index.html#helm-expressions like this. So to templatize the labels and annotations you would need to add that. When i did play around with this feature I had to add it to the app properties, I had a few issues using the maven command located in the build-helm script but it did work, just up to preferences if you were ok adding stuff to the app properties.
this was one of the reasons I just went with the multiple chart approach, helm gives you flags to enable resources, so in your Openshift vs Kubernetes example the values.yaml might have something like "ingress.enabled" and you can set it to true for Kubernetes and false for Openshift and then it will just create a route instead (this is hypothetical it'll probably need more thought). It looks like the extension supports that here: https://quarkiverse.github.io/quarkiverse-docs/quarkus-helm/dev/index.html#conditionally-enable-disable-resources but I myself haven't used it yet. I'm not sure how to deal with the copying and pasting portion though in terms of the script. Probably something like "if we're doing the kubernetes deployment type, only copy the ingress) or something like that. @someth2say wanted to add some commits/take over the pr, so please add your thoughts too! |
Depends on what we are adding. We add plenty of kubernetes/minishift/knative stuff there, so if there is stuff specific for helm then so be it. Other things (like the resource limits/requests), we input as
I wasn't just talking about when deploying the resources. If you look for example at one of the I assume if we wanted to follow that pattern (which is what setting |
|
I think maybe the terminology is tripping me up a bit, so
when you say that you meant like a chart for rest-villains, with an |
|
When I say
I would envision a single chart with multiple values files, like But it seems like we can't go that route because the templates that are inside the chart will also change based on the deployment target. For example, if we generated a chart for But if we generated a chart for Similarly, if we generated a chart for knative, the app would be deployed as a knative service, which would have a different structure and be in a different yaml. Does that make sense? Given that, I think we have to generate a different chart per deployment target, like Maybe if we go that route then we can still have multiple values files, but the values files would be the java version? So inside |
|
yes, that does make sense. that will still work, but each value file will just have different flags enabled no? so in this example : https://quarkiverse.github.io/quarkiverse-docs/quarkus-helm/dev/index.html#conditionally-enable-disable-resources couldn't you just have : be the default for |
The conditionally enabling-or-disabling resources are used when you install a chart. What I'm saying is that at build time, the kubernetes version will have an ingress.yaml but the openshift version will not even have that file in its chart. Similar for other resource types. The "structure" of what the chart looks like I think is different based on the deployment target. Just take a look at https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-villains/deploy/k8s/java17-openshift.yml vs https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-villains/deploy/k8s/java17-kubernetes.yml vs https://github.com/quarkusio/quarkus-super-heroes/blob/main/rest-villains/deploy/k8s/java17-knative.yml for example. There are very different resource types in there. It sounds like what you're saying is that there would be a single chart that would be like a conglomerate chart that had all resources for every deployment type, but then use the conditional stuff to control what actually gets deployed when you install the chart? That might start to get kind of messy and involve many different conditionals, since the conditionals would have to know which resource type the resource should be? In the On the other hand, if we have separate charts per deployment type, each chart would correctly define the structure of the app for that deployment target. Then really the only things that would change would be the image to use (java 11/17/native) and the resource requests/limits, and some other labels/annotations. The structure of the application wouldn't change. Feel free to continue disagreeing with me though :) And if what I'm saying isn't clicking maybe I can work up a quick example to demonstrate. |
|
Just as a quick illustration I put this together. The values in the values files are gibberish and mostly meaningless, but more looking at it from structure. I haven't verified much in this chart. There's lots and lots of gibberish in the values files and I'm not sure where its coming from. This is merely to illustrate structure.
As you can see the actual templates (& their contents) are significantly different for the openshift vs kubernetes vs knative deployment types. |
|
@edeandrea just by chance I reached the same conclusion as you did: each service might have a chart per "deployment" (kubernetes, openshift...), and a values file per "kind" (native, java11, java17). I started my own fork with this idea at ingmarfjolla#1 Indeed, this is still far from being complete, but it´s a start.
I also opted by adding the limits/requests in the
Actually, this resonates a lot with my current experience. I tried to walk the way of having a single uber-chart, but it was really a mess. |
So what would the sane default be, since it varies greatly depending on the type of application (JVM vs native). For the k8s resources generation we make that determination in the |
|
Superceded by #291 |
Hi, just wanted to put this as a draft here to get some feedback. The script at generate-helm-charts is similar to the generate-k8s-resource script in its flow.
I was having trouble creating different values files for each deployment type which is why at the moment there are separate directories for each deployment type + java version etc. Having a separate values file for different deployment types would condense it but I wasn't sure how to approach the logic of the builds / copying after so I might need some help if thats the direction that's preferred.
Thanks !
Closes #130