bjw-s / helm-charts

A collection of Helm charts
https://bjw-s.github.io/helm-charts/
Apache License 2.0
524 stars 98 forks source link

All probes use port of first container. #296

Closed reitermarkus closed 3 months ago

reitermarkus commented 3 months ago

Details

What steps did you take and what happened:

I have a controller with two containers: server and exporter.

The server container uses custom probes. The exporter container simply enables the default probes.

The default probes of the exporter container wrongly uses the last port specified for the server container.

What did you expect to happen:

The default probes of the exporter container should use the exporter port.

Anything else you would like to add:

Additional Information:

Values can be found here:

https://github.com/reitermarkus/helm-charts/blob/b303bc9690ffa467d4a033a8abbf31bf643b04d6/charts/teamspeak/values.yaml

The startup probe fails with:

Startup probe errored: strconv.Atoi: parsing "voice": invalid syntax
reitermarkus commented 3 months ago

May be related to (or the same issue as) https://github.com/bjw-s/helm-charts/issues/293.

bjw-s commented 3 months ago

Hi 👋! Thanks for taking the time to submit this report.

The way the probe port selection has worked since app-template v2 is as follows:

In this logic there is no concept of ports being related to a specific container within a controller. So in your use case you will need to explicitly define the port that you wish to probe for the metrics container.

There are some other things I was seeing when trying to render the linked values.yaml:

- service.teamspeak.ports.http: port is required # 1
- persistence.data: Must validate one and only one schema (oneOf) # 2

Regarding error 1: https://github.com/reitermarkus/helm-charts/blob/b303bc9690ffa467d4a033a8abbf31bf643b04d6/charts/teamspeak/values.yaml#L112-L114 This needs to be a valid service values definition if you wish to keep it around (even if disabled) in your values.yaml.

Regarding error 2: https://github.com/reitermarkus/helm-charts/blob/b303bc9690ffa467d4a033a8abbf31bf643b04d6/charts/teamspeak/values.yaml#L140-L143 This needs to be a valid persistence item values definition if you wish to keep it around (even if disabled) in your values.yaml.

Finally your values somehow render two Service objects with the same name. This seems to be fixed by the same fix that fixes #293 which will be included in v3.0.4.

reitermarkus commented 3 months ago

The way the probe port selection has worked since app-template v2 is as follows:

I see, there was no probe for these in v2 at all in my case, and now that I added enabled: true it also needs a port.

There are some other things I was seeing when trying to render the linked

How exactly does validation occur, so I can catch this in CI? ct lint doesn't seem to do it.

Regarding error 1:

Right, good catch, this isn't needed anymore since there is no default http port for this service.

Regarding error 2:

I don't really want to specify any default persistence type, that should be done explicitly by the chart user. I guess I can use type: emptyDir instead?

bjw-s commented 3 months ago

How exactly does validation occur, so I can catch this in CI? ct lint doesn't seem to do it.

I have added the following file to app-template to have Helm validate the values (that should also make ct lint catch it): https://github.com/bjw-s/helm-charts/blob/main/charts/other/app-template/values.schema.json

I don't really want to specify any default persistence type, that should be done explicitly by the chart user. I guess I can use type: emptyDir instead?

Yes, I would suggest that is the best approach. It makes it so that during ct test the volume/mount are available but you aren't forcing anything specific on the end user.

reitermarkus commented 3 months ago

Thanks.

Regarding values.schema.json, do you update the version manually currently? It doesn't seem like e.g.

https://github.com/bjw-s/renovate-config/blob/3c73d7e2168372877070507a7c9bbc10c577c696/custom-managers.json5#L18-L31

works for the <dep>-<version> format as is the case for e.g. common-3.0.3.

bjw-s commented 3 months ago

I have just added https://github.com/bjw-s/helm-charts/blob/main/.github/renovate/custom-managers.json5, which will handle any version updates to the schema :) f12d6d9 (#283)

reitermarkus commented 3 months ago

Thanks, seems like I was missing registryUrlTemplate and using helmv3 instead of helm, so my custom manager wasn't working.

reitermarkus commented 3 months ago

Closing, since all questions have been cleared up.