Skip to content

feat(helm): add readiness probe for memcached #15609

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.

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

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

Conversation

LukoJy3D
Copy link
Contributor

@LukoJy3D LukoJy3D commented Jan 6, 2025

What this PR does / why we need it:
image

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 LukoJy3D requested a review from a team as a code owner January 6, 2025 15:57
@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 06c7a8c to 5b0fd72 Compare January 6, 2025 18:19
@github-actions github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Jan 6, 2025
@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 5b0fd72 to 2d72d61 Compare January 6, 2025 18:22
@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 2d72d61 to 4227e04 Compare February 13, 2025 11:15
@LukoJy3D
Copy link
Contributor Author

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

@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 4227e04 to ed8ebf0 Compare February 19, 2025 07:45
@LukoJy3D
Copy link
Contributor Author

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

@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from ed8ebf0 to 6b4539c Compare June 6, 2025 06:33
@LukoJy3D
Copy link
Contributor Author

LukoJy3D commented Jun 6, 2025

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

@Jayclifford345
Copy link
Contributor

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 LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 6b4539c to 6daf290 Compare June 6, 2025 09:31
@LukoJy3D
Copy link
Contributor Author

LukoJy3D commented Jun 6, 2025

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
Copy link
Contributor

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
Copy link
Contributor

Jayclifford345 commented Jun 11, 2025

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

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 18, 2025
@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 6551b72 to d9b02ee Compare June 18, 2025 12:11
@LukoJy3D
Copy link
Contributor Author

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

@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from d9b02ee to 7fba421 Compare June 30, 2025 14:02
@pull-request-size pull-request-size bot added size/S and removed size/M labels Jun 30, 2025
@LukoJy3D LukoJy3D force-pushed the feat/add_readiness_probe_for_memcached branch from 7fba421 to b762446 Compare June 30, 2025 14:05
@@ -3204,6 +3204,12 @@ memcached:
fsGroup: 11211
# -- The name of the PriorityClass for memcached pods
priorityClassName: null
# -- Readiness probe for memcached pods (probe port defaults to container port)
readinessProbe:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
readinessProbe:
readinessProbe:
tcpSocket:
port: client

Should be worked as well?

Comment on lines +127 to +130
readinessProbe:
tcpSocket:
port: {{ .port }}
{{- toYaml $.ctx.Values.memcached.readinessProbe | nindent 12 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
readinessProbe:
tcpSocket:
port: {{ .port }}
{{- toYaml $.ctx.Values.memcached.readinessProbe | nindent 12 }}
{{- with $.ctx.Values.memcached.readinessProbe }}
readinessProbe:
{{- toYaml . | nindent 12 }}
{{- end }}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm helm-maintainers size/S type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories
3 participants