linki / chaoskube

chaoskube periodically kills random pods in your Kubernetes cluster.
MIT License
1.79k stars 121 forks source link

Helm chart on Kubeapps is not available #275

Open maurizio-lattuada opened 3 years ago

maurizio-lattuada commented 3 years ago

In README.md is suggested to download Chaoskube's Helm chart from Kubeapps. Unfortunately, the chart there cannot be found: the HTTP GET https://hub.kubeapps.com/api/chartsvc/v1/charts/stable/chaoskube returns a 404 there.

Would it be possible for you to check whether the Helm chart is still published there? The old repository https://github.com/helm/charts/tree/master/stable/chaoskube is now in archived state since end November.

Thank you

Perfect-Web commented 3 years ago

same here. also any helm 3 version ?

javajon commented 3 years ago

This Katacoda scenario that teaches chaoskube as well as this scenario at O'Reilly learning relies on the Helm chart. These scenarios will be updated/fixed as soon as the new chart location is made available.

ghouscht commented 3 years ago

Unfortunately I've the same issue here. Probably submitting the chart to the kuebapps Hub would be a good idea. I can help out with this if there is something missing which is "required" by the best practices of kubeapps Hub.

WyriHaximus commented 3 years ago

The stable chart repository maintainer by Helm has been deprecated so that might be it. Is there an official Helm chart for Chaoskube?

ghouscht commented 3 years ago

I don't think so, no. But why not create one now? I can file a PR with the chart if @linki is ok with merging it.

WyriHaximus commented 3 years ago

That's the thing I was hinting at :+1: . Happy to help out

ghouscht commented 3 years ago

FYI: The PR was merged, you can now use the helm chart with:

helm repo add chaoskube https://linki.github.io/chaoskube
helm repo update
helm template chaoskube/chaoskube
maurizio-lattuada commented 3 years ago

Perfect, thank you all for this wonderful outcome :+1:

javajon commented 3 years ago

Tried this:

kubectl create namespace chaoskube

helm repo add chaoskube https://linki.github.io/chaoskube

helm repo update

helm install chaoskube chaoskube/chaoskube \
  --version 0.1.0 \
  --namespace chaoskube \
  --set 'tag=v0.21.0' \
  --set 'namespaces=!kube-system' \
  --set labels=app-purpose=chaos \
  --set interval=20s

Got this:

Events:
  Type     Reason     Age                    From               Message
  ----     ------     ----                   ----               -------
  Normal   Scheduled  4m28s                  default-scheduler  Successfully assigned chaoskube/chaoskube-87f854bc9-5dxsv to node01
  Normal   Pulling    2m54s (x4 over 4m26s)  kubelet, node01    Pulling image "quay.io/linki/chaoskube:0.21.0"
  Warning  Failed     2m52s (x4 over 4m25s)  kubelet, node01    Failed to pull image "quay.io/linki/chaoskube:0.21.0": rpc error: code = Unknown desc = Error response from daemon: manifest for quay.io/linki/chaoskube:0.21.0 not found: manifest unknown: manifest unknown
  Warning  Failed     2m52s (x4 over 4m25s)  kubelet, node01    Error: ErrImagePull
  Normal   BackOff    2m38s (x6 over 4m24s)  kubelet, node01    Back-off pulling image "quay.io/linki/chaoskube:0.21.0"
  Warning  Failed     2m27s (x7 over 4m24s)  kubelet, node01    Error: ImagePullBackOff

The Issue is image tag in repo is prefixed with "v" and this line strips the "v":

image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"

Also,

In value.yaml, the default for "tag" should be "latest", not "":

# Overrides the image tag whose default is the chart appVersion.
  tag: "latest"

Also, as part of the new chart for the merge, the install instructions in the README should have new Helm instructions.

Also, chart version number went from 3.3.2 backwards to 0.1.0. Suggest the new chart to be 4.0.0, bump major number since most of the chart values are different.

linki commented 3 years ago

@javajon Thanks for trying and reporting this.

linki commented 3 years ago

@javajon I added a PR that fixed it for me: https://github.com/linki/chaoskube/pull/324

I don't think we should add tag: "latest" to the default values.yaml. This is only for when you want to override the default version defined in the Chart.yaml with something else. Let me know what you think about it.

javajon commented 3 years ago

It's best to peg it at a released version. The tag default is current blank (""), which is a covert way of defining "latest". Suggested latest as default to be self documenting. However, latest is an antipattern.

On Wed, Mar 24, 2021, 9:00 AM Martin Linkhorst @.***> wrote:

@javajon https://github.com/javajon I added a PR that fixed it for me:

324 https://github.com/linki/chaoskube/pull/324

I don't think we should add tag: "latest" to the default values.yaml. This is only for when you want to override the default version defined in the Chart.yaml with something else. Let me know what you think about it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/linki/chaoskube/issues/275#issuecomment-805799539, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIEDPGCFBXKNHJHRPKNLPTTFHO6HANCNFSM4UQD3IVA .

linki commented 3 years ago

@javajon I see, thanks for the explanation.

When I look at the code it looks like given the "" tag value it will use the chart's appVersion (the released version) rather than latest. Isn't that what we want by default?

javajon commented 3 years ago

With the default tag as "" then what version of Chaoskube will be used? The missing value "" seems very indeterministic, not self-documenting, and can easily break idempotency over time. Why not specify a precise recent version of ChaosKube that the chart was tested with? If others want an older or more recent version they can purposefully deviate from the default. With a specific version, you are communicating that the chart has been tested and verified with the specific version, such as Chaoskube 0.20.0. Good public charts tend to set the default tag to a definitive recent version that the project sanctions as a working version with the chart. For instance, the Bitnami group who support a RabbitMQ HA chart follow good practice by specifying an exact tag for the recent RabbitMQ container tag: 3.8.14-debian-10-r0. See RabbitMQ default values for their chart. https://github.com/bitnami/charts/blob/master/bitnami/rabbitmq/values.yaml

Also, IMHO the Chaoskube project public artifacts such as containers and charts should not have a prefix of "v" as it confuses the issue and its superfluous information in the tag. For instance, the redis containers https://hub.docker.com/_/redis?tab=tags&page=1&ordering=last_updated don't have the "v" prefix, like most other containers and charts.

-

On Wed, Mar 24, 2021 at 1:27 PM Martin Linkhorst @.***> wrote:

@javajon https://github.com/javajon I see, thanks for the explanation.

When I look at the code https://github.com/linki/chaoskube/blob/master/chart/chaoskube/templates/deployment.yaml#L28 it looks like given the "" tag value it will use the chart's appVersion (the released version) rather than latest. Isn't that what we want by default?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/linki/chaoskube/issues/275#issuecomment-806018175, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIEDPDBUP65IHDBI5O5JVTTFIOJLANCNFSM4UQD3IVA .

javajon commented 3 years ago

As part of this Helm chart installation update, it should be registered on the chaos list at Artifacthub.io.

javajon commented 3 years ago

Updated the chart install at Katacoda scenario for Chaoskube. The chart installs and ChaosKube appears healthy as seen in step 2. However, the Pod killing is no longer performing its duties as seen in step 4. Try it for yourself and please advise.

javajon commented 3 years ago

@linki

When I look at the code it looks like given the "" tag value it will use the chart's appVersion (the released version) rather than latest. Isn't that what we

True, however, if the chart was tested for a specific version, that version should be pegged to the chart. Charts can easily break that implicitly reference latest or "" and the underlying chaosengine updates and no tests were performed on the chart. If the version of chaoskube changes the chart image.tag should remain the same, idempotent. If you retest the chart with a new chaoskube version, at that point the image.tag default value should be explicitly bumped. Leaving it as "" leaves you open to breakages.

If you notice in the Katacoda scenario both the chart and chaoskube versions are specific to protect the scenario from breaking with future changes. Idempotency protection.

mkilchhofer commented 3 years ago

Using "" as image.tag default is perfectly fine. This is good practice in helm. Also this is how you start when you create a new chart based on helms skeleton:

$ helm create foobar
Creating foobar
$ cat foobar/templates/deployment.yaml | grep image:
          image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"

This can be easily fixed in Chart.yaml

diff --git a/chart/chaoskube/Chart.yaml b/chart/chaoskube/Chart.yaml
index 357a04c..ed96376 100644
--- a/chart/chaoskube/Chart.yaml
+++ b/chart/chaoskube/Chart.yaml
@@ -14,4 +14,4 @@ maintainers:
   - name: Thomas Gosteli
     url: https://github.com/ghouscht
 version: 0.1.0
-appVersion: 0.21.0
+appVersion: v0.21.0