feat(valkey): add Valkey Sentinel for automatic failover - #234
Conversation
Replication mode alone cannot recover from a master failure: the master is always pod-0 and clients keep writing to it until an operator intervenes. Run a valkey-sentinel container next to the Valkey server in every pod, so that the Sentinels monitor each other and promote a replica automatically when the master stops responding. Enabled with replica.sentinel.enabled. * Stop pinning the valkey service selector to pod-0 when Sentinel is enabled, the master can be any pod after a failover. * Preserve the replication target Sentinel wrote via CONFIG REWRITE across restarts, instead of resetting every pod to "pod-0 is master". * Rediscover the current master on Sentinel startup by asking the running peers first and the local Valkey node second, so that a restarted Sentinel never tries to demote the real master. * Ask Sentinel to fail over in a preStop hook when the terminating pod is the master, so a rolling update does not leave clients on a dying master. * Secure the Sentinel port and the Sentinel gossip with the default user credentials whenever auth is enabled. * Drop a stray tab in the statefulset volumeClaimTemplates that made the rendered manifests unparseable for strict YAML parsers. Co-authored-by: KHTee <teekahhui@hotmail.com> Co-authored-by: Jose Dominguez <jdominguez.ops@gmail.com> Co-authored-by: yoannrt <yoann.rousseau@gmail.com> Co-authored-by: Dieter Maes <dieter.maes@dmaes.be> Co-authored-by: Tim Karger <49390121+tkarger@users.noreply.github.com> Co-authored-by: lazariv <lazariv.taras@gmail.com> Signed-off-by: pat-s <patrick.schratz@devxy.io>
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Helm chart adds Valkey Sentinel high availability with configurable monitoring, authentication, failover, persistence, services, sidecars, validation, bootstrap logic, documentation, examples, and tests. ChangesValkey Sentinel high availability
Sequence Diagram(s)sequenceDiagram
participant StatefulSet
participant SentinelSidecar
participant ValkeyInit
participant ValkeyNode
participant SentinelPeers
StatefulSet->>SentinelSidecar: Start Sentinel sidecar
SentinelSidecar->>SentinelPeers: Discover monitored master
SentinelSidecar->>ValkeyNode: Generate sentinel.conf
ValkeyInit->>ValkeyNode: Restore or assign replication target
SentinelSidecar->>SentinelPeers: Monitor Valkey node
StatefulSet->>SentinelSidecar: Run pre-stop failover
SentinelSidecar->>SentinelPeers: Request master failover
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
valkey/README.md (1)
43-45: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the empty
**IMPORTANT**label.The content that followed this label moved out of the Replication Mode section. The label now renders as a bold word with no body.
📝 Proposed change
-**IMPORTANT** - **Services:**🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@valkey/README.md` around lines 43 - 45, Remove the standalone **IMPORTANT** label immediately before the Services section in the README, leaving the Services content and surrounding documentation unchanged.
🧹 Nitpick comments (2)
valkey/examples/ha-sentinel.yaml (1)
29-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
onflag fromreplicator.permissions.The chart emits the ACL rule as
user <name> on >password <permissions>, as shown invalkey/values.yamlline 238. Thedefaultuser above and the examples invalues.yamlomiton. Keep the same form here so users do not copy a duplicated flag.♻️ Proposed change
permissions: >- - on ~* &* +multi +exec +ping +info +role +subscribe +publish +slaveof + ~* &* +multi +exec +ping +info +role +subscribe +publish +slaveof +replicaof +config|rewrite +client|setname +client|kill +script|kill +psync +replconf🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@valkey/examples/ha-sentinel.yaml` around lines 29 - 36, Remove the leading “on” token from the replicator.permissions ACL rule while preserving all existing key patterns and command permissions, matching the format used by the chart’s ACL rendering and other examples.valkey/templates/service.yaml (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe service keeps write semantics that it no longer provides in Sentinel mode.
Removing the pod-0 pin is correct, because the master moves after a failover. The service still carries
app.kubernetes.io/component: primaryat line 7, andNOTES.txtstill presents it under "WRITE Operations". Clients that follow the label or the notes will send writes to replicas and receive-READONLY.Consider setting a different component label in Sentinel mode, so that consumers of the label do not treat this service as master-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@valkey/templates/service.yaml` around lines 34 - 38, Update the service metadata around the component label and the Sentinel configuration condition so Sentinel mode no longer advertises the service as primary/master-only. Preserve the primary component label for non-Sentinel deployments, and align the service label with the behavior documented for Sentinel consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@valkey/README.md`:
- Around line 104-107: Update the fenced code block containing the ACL
permission list in the README to specify the text language, changing the opening
fence to use text while preserving the block contents.
In `@valkey/templates/_helpers.tpl`:
- Around line 219-224: Update the Sentinel monitor-user resolution in the
auth-enabled block around $monitorUser and valkey.validateSentinelConfig so
auth.aclConfig-only configurations do not dereference a missing auth.aclUsers
map. Add the same nil-safe guard before reading auth.aclUsers, or ensure the
validator emits the existing explicit missing-user failure before this lookup,
while preserving the current fallback and fail message for defined ACL users.
In `@valkey/templates/init_config.yaml`:
- Around line 181-196: The sentinel bootstrap decision in init.sh must not let
pod 0 become MASTER solely because local PVC state is missing after failover.
Before the POD_INDEX master branch, reuse sentinel-start.sh’s peer-discovery
mechanism to query a reachable Sentinel and, when it reports a promoted master,
set REPLICATION_TARGET to replicaof that address; only fall back to pod 0 as
MASTER when discovery finds no existing master.
- Around line 169-179: Update the listening-port configuration to use
.Values.service.port instead of a hardcoded 6379, preserving the existing
TLS/non-TLS directive ordering. Apply this in valkey/templates/init_config.yaml
lines 169-179 and update the corresponding hardcoded port references in
valkey/templates/sentinel-configmap.yaml lines 75-81 and 279-285 so Sentinel
uses the same configured port.
In `@valkey/templates/NOTES.txt`:
- Around line 26-32: Update the Sentinel connection examples in NOTES.txt to
branch on .Values.replica.sentinel.service.enabled: use the existing service
hostname and sentinel.service.port when enabled, and use the headless service
hostname with .Values.replica.sentinel.port when disabled. Apply the same
conditional endpoints to both the valkey-cli command and the Python Sentinel
example.
In `@valkey/templates/sentinel-configmap.yaml`:
- Around line 86-101: The master_from_peers function must validate sentinel_cli
output before accepting it: require exactly two stdout lines containing a valid
master host and port, and reject empty, malformed, or error-reply output when
the peer is not configured for MASTER_SET. Only log, echo, and return success
for an unambiguous validated address; otherwise continue checking the remaining
peers.
In `@valkey/templates/sentinel-service.yaml`:
- Around line 21-23: Update the loadBalancerSourceRanges rendering in the
sentinel service template to serialize the list with Helm’s toYaml helper,
preserving valid YAML sequence syntax and the existing conditional block.
In `@valkey/templates/statefulset.yaml`:
- Around line 19-25: Add an upgrade note under the Sentinel section in
valkey/README.md documenting that enabling Sentinel may change immutable
StatefulSet fields, including podManagementPolicy and persistence
volumeClaimTemplates, so users must delete the StatefulSet with --cascade=orphan
before helm upgrade to allow existing pods and PVCs to be re-adopted.
---
Outside diff comments:
In `@valkey/README.md`:
- Around line 43-45: Remove the standalone **IMPORTANT** label immediately
before the Services section in the README, leaving the Services content and
surrounding documentation unchanged.
---
Nitpick comments:
In `@valkey/examples/ha-sentinel.yaml`:
- Around line 29-36: Remove the leading “on” token from the
replicator.permissions ACL rule while preserving all existing key patterns and
command permissions, matching the format used by the chart’s ACL rendering and
other examples.
In `@valkey/templates/service.yaml`:
- Around line 34-38: Update the service metadata around the component label and
the Sentinel configuration condition so Sentinel mode no longer advertises the
service as primary/master-only. Preserve the primary component label for
non-Sentinel deployments, and align the service label with the behavior
documented for Sentinel consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6fd83c9-180e-40e7-9759-3b120b5dc905
📒 Files selected for processing (16)
valkey/.helmignorevalkey/Chart.yamlvalkey/README.mdvalkey/examples/ha-sentinel.yamlvalkey/templates/NOTES.txtvalkey/templates/_helpers.tplvalkey/templates/deploy_valkey.yamlvalkey/templates/init_config.yamlvalkey/templates/sentinel-configmap.yamlvalkey/templates/sentinel-service.yamlvalkey/templates/service-headless.yamlvalkey/templates/service.yamlvalkey/templates/statefulset.yamlvalkey/tests/sentinel_test.yamlvalkey/values.schema.jsonvalkey/values.yaml
The bootstrap marker was written on every pod, so a pod that lost its valkey.conf while keeping the marker, for example when the init container was interrupted between removing the old config and writing the new one, read the marker as proof that it used to be the master and came back as a second writable master. Drop the marker and derive the role from the previous config instead, which already carries the answer: a config with a replicaof directive means replica, a complete config without one means master, and no config at all means this is a first boot that falls back to the pod index. Assemble the config in a temporary file and move it into place at the end, so a config on disk is always complete and the distinction holds. Also only disable protected mode on Sentinel when auth is disabled, where peers could not connect otherwise, and say so in values.yaml. Signed-off-by: pat-s <patrick.schratz@devxy.io>
|
Thanks bots. Pushed a fix for two of the three findings: Bootstrap marker split-brain (Greptile, P1) — real, fixed. The marker is gone. The role now comes from the previous config, which already carries the answer: a Verified on a kind cluster by wiping Unauthenticated Sentinel port (Greptile, P1) — Nil Also took the MD040 suggestion. |
…argv Review follow-ups on the Sentinel support. * Ask the running Sentinels for the current master when a pod starts without a usable config. A pod that lost its volume previously fell back to the pod index, so pod-0 came back as a second writable master after a failover. * Render the Valkey listen port from service.port instead of a hardcoded 6379, which made a custom service.port disagree with the container port. * Pass credentials to valkey-cli through REDISCLI_AUTH so they no longer show up in the process list of the pod. * Reject an incomplete answer from a peer Sentinel, an error reply would otherwise be written into the monitor directive and stop Sentinel from starting. Bound the peer queries with a timeout and widen the startup probe to cover discovery. * Set terminationGracePeriodSeconds and refuse a preStop failover timeout that would be cut short by SIGKILL. * Allow replica.podManagementPolicy to be pinned, so an existing release can enable Sentinel without recreating the immutable StatefulSet, and document the recreate for anyone who takes the default. * Render loadBalancerSourceRanges as a YAML list, and point the notes at the headless service when the Sentinel service is disabled. Signed-off-by: pat-s <patrick.schratz@devxy.io>
|
Second round of review fixes pushed. Verified on a kind cluster with ACL auth. Fixed
Not changing
167 unit tests, |
|
thanks @pat-s for the pr can you provide me an example values.yaml for testing |
Signed-off-by: pat-s <patrick.schratz@gmail.com>
|
@Bloodraven21 sure — here is a complete example using an existing Secret (recommended): First create the three credentials. The Sentinel password is deliberately separate from the Valkey application and replication passwords: kubectl create secret generic valkey-users \
--from-literal=default='replace-with-a-strong-app-password' \
--from-literal=replicator='replace-with-a-strong-replication-password' \
--from-literal=sentinel='replace-with-a-strong-sentinel-password'Save this as replica:
enabled: true
replicas: 2 # 1 master + 2 replicas; minimum for Sentinel quorum
persistence:
size: 1Gi
replicationUser: replicator
sentinel:
enabled: true
quorum: 2
passwordKey: sentinel
auth:
enabled: true
usersExistingSecret: valkey-users
aclUsers:
default:
permissions: "~* &* +@all"
replicator:
permissions: >-
~* &* +multi +exec +ping +info +role +subscribe +publish +slaveof
+replicaof +config|rewrite +client|setname +client|kill +script|kill
+psync +replconfInstall from the PR checkout with: helm install valkey ./valkey -f values-sentinel.yamlThe chart also contains an inline-password example at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@valkey/values.yaml`:
- Around line 356-361: Update the comment beside replica.sentinel.password and
passwordKey to state that the existing secret’s sentinel key takes priority,
while the inline password is used as a fallback when that key is absent. Remove
the wording that claims the inline value is ignored, while preserving the
explanation that Sentinel credentials are independent from Valkey ACLs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef0b6a6e-2254-4105-875c-464b85033776
📒 Files selected for processing (13)
valkey/README.mdvalkey/examples/ha-sentinel.yamlvalkey/templates/NOTES.txtvalkey/templates/_helpers.tplvalkey/templates/init_config.yamlvalkey/templates/secret.yamlvalkey/templates/sentinel-configmap.yamlvalkey/templates/sentinel-service.yamlvalkey/templates/statefulset.yamlvalkey/tests/secret_test.yamlvalkey/tests/sentinel_test.yamlvalkey/values.schema.jsonvalkey/values.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- valkey/templates/NOTES.txt
- valkey/templates/statefulset.yaml
- valkey/templates/sentinel-service.yaml
- valkey/templates/sentinel-configmap.yaml
- valkey/values.schema.json
- valkey/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@valkey/templates/sentinel-configmap.yaml`:
- Line 204: Update the SENTINEL_PASSHASH assignment in the Sentinel
configuration template to use printf with a %s format for SENTINEL_PASSWORD
before piping it to sha256sum, replacing echo -n while preserving the existing
hash extraction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cd8aa689-6463-4a4e-9d10-97e969d63384
📒 Files selected for processing (10)
valkey/README.mdvalkey/examples/ha-sentinel.yamlvalkey/templates/_helpers.tplvalkey/templates/init_config.yamlvalkey/templates/secret.yamlvalkey/templates/sentinel-configmap.yamlvalkey/tests/secret_test.yamlvalkey/tests/sentinel_test.yamlvalkey/values.schema.jsonvalkey/values.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- valkey/values.schema.json
- valkey/tests/secret_test.yaml
- valkey/templates/secret.yaml
- valkey/values.yaml
- valkey/examples/ha-sentinel.yaml
- valkey/tests/sentinel_test.yaml
- valkey/templates/init_config.yaml
- valkey/templates/_helpers.tpl
- valkey/README.md
| {{- if .Values.auth.enabled }} | ||
| # Sentinel has a dedicated identity. Valkey application credentials must | ||
| # never gain Sentinel administration privileges as a side effect. | ||
| SENTINEL_PASSHASH=$(echo -n "$SENTINEL_PASSWORD" | sha256sum | cut -f 1 -d " ") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'SENTINEL_PASSHASH=.*echo -n|SENTINEL_PASSHASH=.*printf' \
valkey/templates/sentinel-configmap.yaml
for password in 'plain-password' '-n' 'pass\word'; do
expected="$(printf '%s' "$password" | sha256sum | cut -d ' ' -f 1)"
actual="$(sh -c 'echo -n "$1"' sh "$password" | sha256sum | cut -d ' ' -f 1)"
printf 'password=%s expected=%s actual=%s\n' "$password" "$expected" "$actual"
doneRepository: valkey-io/valkey-helm
Length of output: 493
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files with SENTINEL_PASSHASH:\n'
rg -n -C 3 'SENTINEL_PASSHASH|SENTINEL_PASSWORD' valkey/templates/sentinel-configmap.yaml || true
printf '\nChart/base shell availability check (no repo execution):\n'
command -v bash sh awk sed sha256sum || true
python3 - <<'PY'
import hashlib, re
from pathlib import Path
p = Path('valkey/templates/sentinel-configmap.yaml')
text = p.read_text()
print(re.search(r'SENTINEL_PASSHASH=\$\(.*?\| sha256sum', text, re.S))
for line in text.splitlines():
if 'SENTINEL_PASSHASH=' in line:
print(line.strip())
PYRepository: valkey-io/valkey-helm
Length of output: 2479
Hash the Sentinel password with printf.
echo -n can alter password bytes for option-like strings and backslashes, so the Sentinel ACL hash can differ from SENTINEL_PASSWORD. Use printf '%s' before hashing.
Proposed fix
-SENTINEL_PASSHASH=$(echo -n "$SENTINEL_PASSWORD" | sha256sum | cut -f 1 -d " ")
+SENTINEL_PASSHASH=$(printf '%s' "$SENTINEL_PASSWORD" | sha256sum | cut -f 1 -d " ")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SENTINEL_PASSHASH=$(echo -n "$SENTINEL_PASSWORD" | sha256sum | cut -f 1 -d " ") | |
| SENTINEL_PASSHASH=$(printf '%s' "$SENTINEL_PASSWORD" | sha256sum | cut -f 1 -d " ") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@valkey/templates/sentinel-configmap.yaml` at line 204, Update the
SENTINEL_PASSHASH assignment in the Sentinel configuration template to use
printf with a %s format for SENTINEL_PASSWORD before piping it to sha256sum,
replacing echo -n while preserving the existing hash extraction.
Signed-off-by: pat-s <patrick.schratz@gmail.com>
2 participants
Supersedes #137, rebased onto current
mainwith the review feedback applied. Sentinel only; the HAProxy front-end follows in a separate PR.Replication mode cannot recover from a master failure: the master is always pod-0 and clients keep writing to it until someone intervenes. This runs
valkey-sentinelnext to the server in every pod, so a replica is promoted automatically. Off by default (replica.sentinel.enabled).Beyond #137:
valkeyservice no longer pins its selector to pod-0 when Sentinel is enabled, otherwise the write endpoint still points at a demoted replica after a failover.CONFIG REWRITEsurvives restarts, instead of every pod resetting to "pod-0 is master".preStophook fails over before a master pod is terminated, so rolling updates do not strand clients on a dying master.auth.enabledis set, with no extra configuration.Testing
helm lint, 158 unit tests, and rendering against k8s 1.28/1.35/1.36. Verified on a kind cluster with ACL auth usingvalkey/examples/ha-sentinel.yaml:+switch-masterbefore the pod goes away@Bloodraven21 the example values file above is the exact one used for those tests.
One drive-by fix: a stray tab in
statefulset.yamlmade rendered manifests unparseable for strict YAML parsers.Credit to @khtee, @jose-10000, @yoannrt, @tkarger, @lazariv and @dmaes, whose work in #137 this builds on.