Adding k3s support in helm chart

[unixbird]

Normally you would have to edit the DaemonSet to add the k3s containerd path so that the cluster collector would get metrics properly but with this you can enable k3s in the values.yml to do it for you.

I tested this on a brand new k3s instance and it works for me with data in the checkmk dashboard.

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[unixbird]

I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA.

[martinhv]

As this change also adds support for RKE2, please also add it and let's rename it for more clarity to:
k3s/rke2-compatibilityMode

[sjentzsch]

I would advise against using special characters like - or / in the variable names in Helm ... plain camelCase would be best!

[unixbird]

I agree with this as the user would most likely have to escape it to use it (I'm guessing). Should I rename it to k3sbackend or rancherbackend? Not sure what to rename it to

[martinhv]

Hey Zach!
Thanks for your first contribution.
Please check the comments I added.

[martinhv]

For the future: ideally the commits individually should speak for themselves. E.g. updating a filename becomes very obvious when you see that the commit only touched the one filename. In this case, I would have recommended one commit which includes both changes and use the title of this PR for the commit :-)

[martinhv]

Why do you use {{- if $.Values.k3s.enabled }} instead of {{- if .Values.k3s.enabled }} ?
I am no Helm template expert, but it loosk rather unusual.

[sjentzsch]

You have to use $ if you are inside a loop or iteration (as here introduced with with statement), in order to access variables on global level; because inside a loop using . you would access variables inside the local context.

But nonetheless, why are we adding it inside the context in the first place? For me this reads like, for each additionalArg we add, if k3s is enabled, also add --containerd ... but we would like to only add it once, no?

[martinhv]

Yes, it would be better to do this outside the context.
We only need to add --containerd once, if the user is using k3s or rke2

[unixbird]

So it does just add it once and there was a weird thing I had happen to me where it actually added the other additionalArgs 2 times instead of once. The version you see here doesn't do that (at least for me). It just adds 1 containerd arg line after the original additionalArgs.

[sjentzsch]

Two general questions on this:

1. Do we even want to add another flag? Users can already add the arg by adding - "--containerd=/run/k3s/containerd/containerd.sock" to the Helm value nodeCollector.cadvisor.additionalArgs. It's already a list of additional args that can be added. We could add

      # uncomment to add support for k3s/RKE2, see https://github.com/k3s-io/k3s/issues/2831
      #- "--containerd=/run/k3s/containerd/containerd.sock"

though to the default samples, so it's ready to be uncommented. @unixbird you were mentioning "you would have to edit the DaemonSet to add the k3s containerd path" - were you aware of nodeCollector.cadvisor.additionalArgs?

2. According to cAdvisor metrics stopped working correctly in K3s 1.20 k3s-io/k3s#2831 (comment) with latest 1.24 patch release onwards this should not be required anymore for k3s newer versions. Did not test myself; which version were you using @unixbird when testing?

[unixbird]

@sjentzsch I just tested this with 1.27.5 and the cluster collector fails without using those additional args.

I was thinking of the same thing but Martin thought of something else to try out and I like it a little better than what I originally had. Going to edit it into this PR for review.

[unixbird]

Added the changes that @martinhv suggested to my main branch. This allows you to customize the location of the containerd runtime if needed.

[martinhv]

Works for me. Will forward to a dev.