-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
base: main
Are you sure you want to change the base?
feat(helm): add readiness probe for memcached #15609
Conversation
06c7a8c
to
5b0fd72
Compare
5b0fd72
to
2d72d61
Compare
2d72d61
to
4227e04
Compare
@salvacorts @ashwanthgoli maybe I could get some feedback on this one 🙏 ? |
4227e04
to
ed8ebf0
Compare
@trevorwhitney, could someone from the Loki team take a look at this? That gcp warning is poking our eyes 😂 |
ed8ebf0
to
6b4539c
Compare
Still relevant, folks, would anyone be able to take a look? |
Hi @LukoJy3D, thanks for this!
from the top of the Loki repo as the docs lint is failing. |
6b4539c
to
6daf290
Compare
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 🙈 |
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 |
Hi @LukoJy3D here are some thoughts from the team: PR, my thoughts are:
memcached:
readinessProbe:
enabled: true
initialDelaySeconds: 5
periodSeconds: 5
timeoutSeconds: 3
failureThreshold: 6
successThreshold: 1
customReadinessProbe: {} This is what the bitnami/memcached chart does
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.
https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_tplvalues.tpl#L7-L24
I hope this helps! If we can implement some of the changes above I would be happy to push this forward |
6551b72
to
d9b02ee
Compare
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 |
production/helm/loki/templates/memcached/_memcached-statefulset.tpl
Outdated
Show resolved
Hide resolved
d9b02ee
to
7fba421
Compare
7fba421
to
b762446
Compare
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readinessProbe: | |
readinessProbe: | |
tcpSocket: | |
port: client |
Should be worked as well?
readinessProbe: | ||
tcpSocket: | ||
port: {{ .port }} | ||
{{- toYaml $.ctx.Values.memcached.readinessProbe | nindent 12 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readinessProbe: | |
tcpSocket: | |
port: {{ .port }} | |
{{- toYaml $.ctx.Values.memcached.readinessProbe | nindent 12 }} | |
{{- with $.ctx.Values.memcached.readinessProbe }} | |
readinessProbe: | |
{{- toYaml . | nindent 12 }} | |
{{- end }} |
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
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
deprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR