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.

Sign up for GitHub

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

Jump to bottom

kata-deploy: Add Helm Chart #9880

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

kata-deploy: Add Helm Chart #9880

wants to merge 4 commits into from

Conversation

zvonkok
Copy link
Contributor

@zvonkok zvonkok commented Jun 19, 2024

For easier handling of kata-deploy we can leverage a Helm chart to get rid of all the base and overlays for the various components

@katacontainersbot katacontainersbot added the size/large Task of significant size label Jun 19, 2024
@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 19, 2024

Per default the appVersion would be set to VERSION one can override it by simply saying:

helm install --set image.tag=latest  kata-deploy-0.1.0.tgz

A default

helm install ./kata-deploy-0.1.0.tgz

would give one the latest release.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 19, 2024

The helm-chart can be eaisly hosted via github.io on kata-containers.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 19, 2024

With this we can also automate the chart publishing for each release https://github.com/helm/chart-releaser

beraldoleal
beraldoleal reviewed Jun 19, 2024
View reviewed changes
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

Hi @zvonkok , thanks a lot for this. IMO a good improvement. Just left a few questions for you.

tools/packaging/kata-deploy/helm-chart/Makefile Outdated
endef

HELM = $(shell pwd)/bin/helm
helm: ## Download helm locally if necessary.
Copy link
Member

Choose a reason for hiding this comment

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

This is installing bin/helm under the current folder. If I'm calling it from tools/packaging/kata-deploy/helm-chart there will be a bin folder there. Is this the excepted location?

It sounds a bit strange to me to have it under this location. But if we decide to keep it there, we should include this binary into the .gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to keep this there. We could use $HOME/.local/bin are we just assume that helm is installed by some other entity. I think I am going to remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this. Consider this a NOOP.

tools/packaging/kata-deploy/helm-chart/Makefile Show resolved Hide resolved
@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 20, 2024

@beraldoleal Take a look at the second commit how we can use Helm for rendering the correct yamls without changing the yamls in place and hence making the repository dirty.

FYI @fidencio @ryansavino

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 20, 2024

kata-deploy.yaml and kata-cleanup.yaml are the same manifests and scripts just with different arguments next commit will clean this up

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 20, 2024

cleanup_kata_deploy is now really simple, just uninstall the helm release if found. All the needed data is encapsulated in the deployed release.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 20, 2024

With this we should be able to remove "ALL" base/overlay kustomzie manifests.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 20, 2024

TODO follow up PRs:

  • Add Helm chart publishing to release.yaml
  • Add Helm chart for each PR build
  • Latest PR build will also upload a latest Helm chart.
  • We need a "place" where to publish our Helm charts. I would be nice to have one Helm chart per PR

ryansavino
ryansavino reviewed Jun 20, 2024
View reviewed changes
Copy link
Member

@ryansavino ryansavino left a comment

Choose a reason for hiding this comment

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

Nice. Made a few comments. I think this would help fix several issues that we brought up in the ci meeting.

Specifically:
#9793

This PR eliminates some of the longer sleep times that were mentioned in the following issues.
#9795
#9796

tests/common.bash Show resolved Hide resolved
tests/integration/kubernetes/gha-run.sh Outdated Show resolved Hide resolved
tests/integration/kubernetes/gha-run.sh Outdated Show resolved Hide resolved
@zvonkok zvonkok force-pushed the helm-chart branch 2 times, most recently from 7de731b to 395a9b7 Compare June 21, 2024 09:02
@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 21, 2024 •
edited
Loading

I've build a simple GHA job to upload a Helm chart for each PR automatically to a specific repository. In my case its zvonkok/helm-charts those are hosted via github.io.

Replace here zvonkok/helm-charts with kata-containers/helm-charts moving forward just using zvonkok/helm-charts for illustration purposes.

helm repo add kata-containers https://zvonkok.github.io/helm-charts
helm repo update
helm search repo --devel -o json 
[{"name":"kata-containers/kata-deploy","version":"3.6.0-dev+24-4d45fd9818726d7d3a37cfd4ad1281bea29b67c2","app_version":"3.6.0-dev+24-4d45fd9818726d7d3a37cfd4ad1281bea29b67c2","description":"A Helm chart for deploying Kata Containers"}]

The input.tag (pr-githash) is defined by the GHA and I am using the VERSION + the tag to create a new sermer version for Helm to consume.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 21, 2024

Additionally values.yaml would be updated for the chart to point to the correct payload

imagePullPolicy: Always
imagePullSecrets: []
image:
  reference: ghcr.io/zvonkok/kata-deploy-ci/kata-deploy
  tag: 24-b9d7f9333087e5ee789af8f37dd26df3b3e308e0
# k8s-dist can be k8s, k3s, rke2, k0s
k8sDistribution: "k8s"
env:
  debug: "false"
  shims: "clh cloud-hypervisor dragonball fc qemu qemu-coco-dev qemu-runtime-rs qemu-sev qemu-snp qemu-tdx stratovirt qemu-nvidia-gpu qemu-nvidia-gpu-snp qemu-nvidia-gpu-tdx"
  defaultShim: "qemu"
  createRuntimeClasses: "false"
  createDefaultRuntimeClass: "false"
  allowedHypervisorAnnotations: ""
  snapshotterHandlerMapping: ""
  agentHttpProxy: ""
  agentNoProxy: ""
  pullTypeMapping: ""
  hostOS: ""

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 21, 2024 •
edited
Loading

For each PR we woudl have a Helm chart kata-deploy-VERSION-dev+{{ input.tag }},tar.gz and for each release we would have a kata-deploy-VERSION.tar.gz with updated values.yaml also pushed as an artifact in the release payload.

Users can then do a helm repo update and without the --devel flag they will not see any kata-deploy-VERSION-dev+{{ input.tag }},tar.gz charts, only the release charts.

@zvonkok zvonkok force-pushed the helm-chart branch 7 times, most recently from 7f3f7bb to 53cff14 Compare June 21, 2024 21:10
@ryansavino
Copy link
Member

I was looking through the failed test runs for the amd node jobs. Let me know if you want to spend some time troubleshooting the failures together. Looks like it wasn't able to pull the kata-deploy image.

@zvonkok zvonkok force-pushed the helm-chart branch 4 times, most recently from e4fb3f1 to ef168d8 Compare June 26, 2024 12:40
@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 26, 2024

@ryansavino Found the error. Thanks for the offer :)

zvonkok
zvonkok commented Jun 26, 2024
View reviewed changes
tools/packaging/kata-deploy/helm-chart/kata-deploy/values.yaml
imagePullSecrets: []
image:
reference: quay.io/kata-containers/kata-deploy
tag: ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If tag is empty we will per default use chart.version which translates to VERSION

mkulke
mkulke reviewed Jun 27, 2024
View reviewed changes
Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

does it make sense to add a values.schema.json to the chart? In my experience this takes the edge off when working with helm charts.

@zvonkok
Copy link
Contributor Author

zvonkok commented Jun 27, 2024

@mkulke Good point, adding this to the list of follow up items:
#9924

@BbolroC
Copy link
Member

BbolroC commented Jun 27, 2024

Please rebase this PR onto main when you want to re-trigger the whole set of checks (by pushing something and etc.) as #9923 resolves the issue for the zvsi tests. Thanks.

@zvonkok
kata-deploy: Add Helm Chart
bcafe61 
For easier handling of kata-deploy we can leverage a Helm chart to get
rid of all the base and overlays for the various components

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok
ci: Use helm to deploy kata-deploy
af7335e 
Rather then modifying the kata-depoy scripts let's use Helm and
create a values.yaml that can be used to render the final templates

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok
ci: make cleanup_kata_deploy really simple
224b23b 
Remove the unneeded logic for cleanup the values are
encapsulated in the deployed helm release

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
@zvonkok
ci: add reset_runtime to cleanup
374b804 
Adding reset_cleanup to cleanup action so that it is done automatically
without the need to run yet another DS just to reset the runtime.

This is now part of the lifecycle hook when issuing kata-deploy.sh
cleanup

Signed-off-by: Zvonko Kaiser <zkaiser@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beraldoleal beraldoleal beraldoleal left review comments

@mkulke mkulke mkulke left review comments

@ryansavino ryansavino ryansavino left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
ok-to-test size/large Task of significant size
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@zvonkok @ryansavino @BbolroC @beraldoleal @mkulke @katacontainersbot