feat(helm): add readiness probe for memcached

[LukoJy3D]

What this PR does / why we need it:

Special notes for your reviewer:
Probe values base on bitnami chart: https://github.com/bitnami/charts/blob/main/bitnami/memcached/values.yaml

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[LukoJy3D]

@salvacorts @ashwanthgoli maybe I could get some feedback on this one 🙏 ?

[LukoJy3D]

@trevorwhitney, could someone from the Loki team take a look at this? That gcp warning is poking our eyes 😂

[LukoJy3D]

Still relevant, folks, would anyone be able to take a look?

[Jayclifford345]

Hi @LukoJy3D, thanks for this!
Just kicking off the workflow to check the PR. How did the local testing go with this addition to the helm?
Would you mind us running

make helm-docs

from the top of the Loki repo as the docs lint is failing.

[LukoJy3D]

Did not run into any issues while testing this locally, all the rollouts went smoothly after. However, I'm not sure about adding this directly to the template without any additional flag, as some folks could be running a different setup where probe is not possible in this way. So either marking this as breaking or adding custom values to configure port/retries/thresholds with no defaults.

P.S. amended the commit, thus need workflow approval again 🙈

[Jayclifford345]

Hi @LukoJy3D, I wonder if that is the best route forward to make this an optional configuration, and we can provide documentation on adding the new readiness probe options. I would like to refrain from adding breaking changes for readiness probes. Let me see what the team has to say though

[Jayclifford345]

Hi @LukoJy3D here are some thoughts from the team:

PR, my thoughts are:
Instead of directly inlining the readiness probe configuration, I'd suggest adopting the same structure used in Bitnami's Helm charts, which support both a readinessProbe.enabled toggle and a customReadinessProbe override. This allows chart users to:

• Use a sane default
• Fully override it if needed (e.g., different probe mechanism or thresholds)
• Avoid rendering issues if values are missing
  Suggested values.yaml structure:

memcached:
  readinessProbe:
    enabled: true
    initialDelaySeconds: 5
    periodSeconds: 5
    timeoutSeconds: 3
    failureThreshold: 6
    successThreshold: 1
    customReadinessProbe: {}

This is what the bitnami/memcached chart does

         {{- if .Values.customReadinessProbe }}
          readinessProbe: {{- include "common.tplvalues.render" (dict "value" .Values.customReadinessProbe "context" $) | nindent 12 }}
          {{- else if .Values.readinessProbe.enabled }}
          readinessProbe: {{- include "common.tplvalues.render" (dict "value" (omit .Values.readinessProbe "enabled") "context" $) | nindent 12 }}
            tcpSocket:
              port: memcache
          {{- end }}

https://github.com/bitnami/charts/blob/main/bitnami/memcached/templates/statefulset.yaml#L179-L185

In the Loki charts memcached template, the port is named client instead of memcache, but we should really avoid hard-coding a port multiple times if at all possible.
the common.tplvalues.render function is defined as

{{/*
Renders a value that contains template perhaps with scope if the scope is present.
Usage:
{{ include "common.tplvalues.render" ( dict "value" .Values.path.to.the.Value "context" $ ) }}
{{ include "common.tplvalues.render" ( dict "value" .Values.path.to.the.Value "context" $ "scope" $app ) }}
*/}}
{{- define "common.tplvalues.render" -}}
{{- $value := typeIs "string" .value | ternary .value (.value | toYaml) }}
{{- if contains "{{" (toJson .value) }}
  {{- if .scope }}
      {{- tpl (cat "{{- with $.RelativeScope -}}" $value "{{- end }}") (merge (dict "RelativeScope" .scope) .context) }}
  {{- else }}
    {{- tpl $value .context }}
  {{- end }}
{{- else }}
    {{- $value }}
{{- end }}
{{- end -}}

https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_tplvalues.tpl#L7-L24

This approach:

• Keeps the template resilient (avoids null rendering errors)
• Allows flexibility for advanced users
• Avoids breaking changes for existing chart users

I hope this helps! If we can implement some of the changes above I would be happy to push this forward

[LukoJy3D]

Thank you for such detailed feedback, barely left any work for me 😄

I did it just as suggested. Both chunk cache and results cache would inherit these readiness probes from the Memcached block, yet tcpSocket.port is configured individually based on what is configured.

let me know if somethings missing

[jkroepke]

LGTM

[LukoJy3D]

I tested it in our K8s environment, and everything is working as expected. It adds that flexibility with named ports that we talked about previously, so @Jayclifford345, do you have any more input?

[Jayclifford345]

LGTM!

[Jayclifford345]

Hi @LukoJy3D, if all tests pass, then this should auto-merge! Thank you for your contribution and patience on this.