feat: Add Valkey Sentinel & HAProxy support in High Availability setup. - #137
feat: Add Valkey Sentinel & HAProxy support in High Availability setup.#137khtee wants to merge 16 commits into
Conversation
|
Nice work! I have some notes/questions:
|
Good points! Will work on both improvements. |
|
Good job, I have some questions too, Also, would that make sense to run sentinel as a side container in the replica STS pods ? |
|
Thank you @khtee can't wait to get this merged in 🙏 |
I got confused, thinking |
@dmaes I guess when you suggest to implement HAproxy, it's for sentinel incompatible clients workloads ? |
Added following enhancements.
|
That's correct. The truly kubernetes-native way would probably be to have a sentinel-master Service, using a |
Signed-off-by: KHTee <teekahhui@hotmail.com>
Signed-off-by: KHTee <teekahhui@hotmail.com>
Co-authored-by: Dieter Maes <dieter.maes@dmaes.be> Signed-off-by: khtee <75174583+khtee@users.noreply.github.com>
Signed-off-by: KHTee <teekahhui@hotmail.com>
Allow HAProxy to retry DNS resolution during startup when pending for Valkey node to start. Essentially it does the following - Try to use the last known IP. - If none, query the libc resolver (DNS). - If that fails, resolve to none (meaning the server has no IP address yet, but HAProxy won't crash) and wait for the runtime resolver health-checks to pick up the DNS correctly. Signed-off-by: KHTee <teekahhui@hotmail.com>
|
Waiting for this to be merged. Thanks for your work! |
| # labels: | ||
| # severity: error | ||
|
|
||
| haproxy: |
There was a problem hiding this comment.
| haproxy: | |
| # Enable haproxy in front of sentinel | |
| # This is allows running redis in sentinel mode even when the client is not compatible | |
| # https://arystech.com/blog/setting-up-haproxy-with-redis-sentinel-for-high-availability-on-microk8s-kubernetes | |
| haproxy: |
fix(chart): resolve schema validation and template errors
|
Hi @khtee I’ve been testing this with ACLs enabled + TLS active and found some discovery gaps when the
I've verified that ACL + TLS now work seamlessly. I'm currently finishing validation for HAProxy with credentials and will update soon. Thanks for your contribution! |
- Updating HAProxy watcher for near-instant IP-based failover. - Refactoring init scripts to support dynamic topology and universal auth/TLS injection. - Adding smart L7 health checks in HAProxy to handle ACL-protected nodes. - Fully parameterizing Service and ConfigMap ports for end-to-end flexibility.
…notations in haproxy-deployment
…schema Valkey Sentinel: Fixing Auth/ACL gaps in TLS environments and security hardening (Enhancements for PR valkey-io#137)
| @@ -188,3 +188,34 @@ Validate replica authentication configuration | |||
| {{- end }} | |||
| {{- end -}} | |||
|
|
|||
There was a problem hiding this comment.
| {{/* | |
| Validate haproxy is used in replica mode | |
| */}} | |
| {{- define "valkey.validateHaproxyRequirements" -}} | |
| {{- if and .Values.haproxy.enabled (not .Values.replica.enabled) }} | |
| {{- fail "Haproxy is only relevant in replica mode with clients incompatible with Sentinel." }} | |
| {{- end }} | |
| {{- end -}} |
- update of
deploy_valkey.yamlto fail if haproxy is enabled in standalone mode.
| {{- include "valkey.validateHaproxyRequirements" . }} |
lazariv
left a comment
There was a problem hiding this comment.
Redis/Valkey pub/sub (SUBSCRIBE) connections are long-lived and idle by design — they block waiting for messages. HAProxy's timeout client/timeout server treats them as stale and drops them, causing clients to see ConnectionError: Connection closed by server and forcing reconnect loops.
Adding timeout tunnel to the HAProxy defaults section solves this cleanly. In HAProxy, timeout tunnel governs bidirectional connections after the initial handshake — exactly the pattern pub/sub uses. Setting it to 0 keeps SUBSCRIBE connections alive indefinitely while preserving normal client/server timeouts as a safety net for regular command connections.
Without this, users running any pub/sub workload (Socket.IO, Celery, Sidekiq, etc.) through HAProxy must either set client/server timeouts to 0 (triggering HAProxy warnings) or accept periodic disconnects.
| log global | ||
| timeout connect {{ .Values.haproxy.config.timeout.connect }} | ||
| timeout client {{ .Values.haproxy.config.timeout.client }} | ||
| timeout server {{ .Values.haproxy.config.timeout.server }} |
There was a problem hiding this comment.
| timeout server {{ .Values.haproxy.config.timeout.server }} | |
| timeout server {{ .Values.haproxy.config.timeout.server }} | |
| timeout tunnel {{ .Values.haproxy.config.timeout.tunnel }} | |
| option clitcpka | |
| option srvtcpka |
| }, | ||
| "server": { | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
| } | |
| }, | |
| "tunnel": { | |
| "type": "string" | |
| } |
| timeout: | ||
| connect: 5s | ||
| client: 1m | ||
| server: 1m |
There was a problem hiding this comment.
| server: 1m | |
| server: 1m | |
| # Timeout for long-lived bidirectional connections (e.g. pub/sub). | |
| # Set to 0 to keep pub/sub SUBSCRIBE connections alive indefinitely. | |
| tunnel: 0s |
|
I've implemented a preStop hook. I found that improper shutdowns during updates were tripping up Sentinel and risked data loss, so this change ensures a more graceful exit. I made a new pull request @khtee |
|
I've added the workloadAnnotations block to haproxy-deployment.yaml. I noticed that the upstream main branch recently introduced workloadAnnotations for other workloads, so adding it here ensures HAProxy is aligned with main once this PR is eventually merged. Do you have a rough ETA for this merge? Please let me know if there is anything else I can help review or test to get this PR over the finish line. I'm planning to adopt these changes soon and would like to have an idea of when it might be merged. Thanks! |
|
I have some reservations about the approach of running Sentinel as a sidecar within the same pod as the Valkey instance. The main concern is related to Helm upgrade workflows: when a pod is taken down during a rolling update or a Helm upgrade, the entire pod is terminated — including both the Valkey container and the Sentinel sidecar. This means we lose a Sentinel node during the upgrade process. Losing a Sentinel during an upgrade can temporarily break the quorum. For example, in a typical 3-Sentinel setup, losing one during an upgrade leaves only 2 Sentinels, which may still meet quorum — but in smaller or less redundant setups, this can prevent failover from working correctly during the exact moment it may be needed most. A more resilient architecture would deploy Sentinels as independent pods (e.g., via a separate StatefulSet), decoupled from the Valkey data pods. This ensures that Sentinel availability is not tied to the lifecycle of the data nodes, preserving quorum integrity throughout rolling updates and upgrades. |
|
@jose-10000 across all other best known redis sentinel helm charts, running in the same pod is the most common setup, and has been working just fine for most people. So I (and I think that goes for others too), never really considered using different statefulsets? |
Thanks for the context, @dmaes! Totally makes sense, I know most charts (like Bitnami) do it this way to keep things simplier. I just wanted to point out that coupling their lifecycles can be tricky for quorum during upgrades, but I'm fine keeping the current approach. Just wanted to share my two cents |
|
Any idea when will this be released? can be amazing |
Hello Team, just want to reiterate here that implementation is great and we are very much looking forward to have it released. Is there any news about it? Thanks! |
| - name: {{ $key }} | ||
| value: "{{ $val }}" |
There was a problem hiding this comment.
I suggest adding the ability to pass extraEnvs for metrics from secrets as follows, for cases where a separate ACL has been created:
extraEnvs:
REDIS_USER: metrics
REDIS_PASSWORD:
valueFrom:
secretKeyRef:
name: valkey-users
key: metrics-pwd| - name: {{ $key }} | |
| value: "{{ $val }}" | |
| - name: {{ $key }} | |
| {{- if kindIs "map" $val }} | |
| {{- toYaml $val | nindent 14 }} | |
| {{- else }} | |
| value: {{ $val | quote }} | |
| {{- end }} |
There was a problem hiding this comment.
Isn't it even cleaner to do this instead? #133
|
@khtee You could add missing sign-offs in the last e.g. 10 commits via commit amend like this and force push: |
|
Just FYI, besides the valid review comments people left on this PR, which I think should be taken into consideration, it also needs to be rebased, and some tests are failing. I also ran some local analysis and noted a few things that could be improved – I'll post the findings in another comment. I'm saying all this because I've noticed some people are expecting this to be merged, but I'm not sure if that will happen anytime soon (hope I'm wrong! 🙂) |
Hi @dmaes , I thought too running sentinel as a sidecar was the simplest/cleaner option but I'm reconsidering it since having those components in 2 different sts has some advantages. |
|
@daanvinken @Bloodraven21 It would really be great to finalize this feature (in here or a separate clean branch) so this can finally find it's way in. I am also happy to clean up if there's a commitment to review and get this merged then, avoiding additional future conflicts. Looking forwarding hearing back from you and getting HA support into the chart! |
Same. I have some improvements I'd like to submit, but I'm not sure if it's worth trying to include them in this PR or creating a separate one. |
|
first can you rebase and make it up to date second give me a exact values files to test it out on local |
please make a spearate pr and raise it will look into it |
please create a spearate one |
Feat #22