feat(valkey): add an optional HAProxy front-end for Sentinel - #235
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>
|
Warning Review limit reached
Next review available in: 30 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 (15)
📝 WalkthroughWalkthroughThe Helm chart adds Valkey Sentinel high availability, failover handling, Sentinel services, optional HAProxy read/write routing, configuration validation, tests, examples, documentation, and chart version ChangesSentinel high availability and HAProxy routing
Sequence Diagram(s)sequenceDiagram
participant StatefulSet
participant SentinelSidecar
participant ValkeyPods
StatefulSet->>SentinelSidecar: Start Sentinel sidecar
SentinelSidecar->>ValkeyPods: Discover master and generate configuration
ValkeyPods->>SentinelSidecar: Return Sentinel health
SentinelSidecar->>ValkeyPods: Request failover before shutdown
sequenceDiagram
participant Client
participant HAProxyService
participant HAProxy
participant ValkeySentinel
participant ValkeyPods
Client->>HAProxyService: Send read or write request
HAProxyService->>HAProxy: Forward request
HAProxy->>ValkeySentinel: Check current master
ValkeySentinel->>HAProxy: Return master health
HAProxy->>ValkeyPods: Route writes to master or reads to replicas
🚥 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
valkey/templates/NOTES.txt (1)
94-104: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not advertise the load-balanced
valkeyService as a direct write endpoint in Sentinel mode.The
valkeyService selects all pods, so theSETcommands in these ranges can reach a replica and return-READONLY. If HAProxy is enabled, use...-haproxyand its write port. Otherwise, show Sentinel master discovery and do not provide a directvalkey-cli SETcommand.Also applies to: 142-146
🤖 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/NOTES.txt` around lines 94 - 104, Update the Sentinel-mode write instructions in the NOTES template so they never use the load-balanced valkey Service for SET operations. When HAProxy is enabled, reference the valkey-haproxy Service and its write port; otherwise document Sentinel master discovery without providing a direct valkey-cli SET command, while preserving the existing non-Sentinel instructions.
🧹 Nitpick comments (9)
valkey/templates/sentinel-configmap.yaml (1)
187-195: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict the permissions on
sentinel.conf.The block appends the plaintext
defaultpassword and the monitor password to$SENTINEL_CONF. The file keeps the default creation mode from the container umask, commonly world-readable.init_config.yamlsetschmod 0400on/etc/valkey/users.aclfor the same class of data. Apply the same posture here. Sentinel rewrites this file at runtime, so use0600rather than0400.🔒️ Proposed change
rm -f "$SENTINEL_CONF" + umask 077 cat > "$SENTINEL_CONF" << EOF🤖 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` around lines 187 - 195, Update the configuration-generation flow around SENTINEL_CONF to ensure the file is created or chmodded with mode 0600 after appending the plaintext credentials. Preserve Sentinel’s ability to rewrite the file and apply the permission consistently to the generated sentinel.conf.valkey/templates/statefulset.yaml (1)
46-59: 🗄️ Data Integrity & Integration | 🔵 TrivialDocument that enabling Sentinel persistence needs StatefulSet recreation.
Kubernetes rejects updates to
volumeClaimTemplateson an existing StatefulSet. A user who enablesreplica.sentinel.persistence.enabledon a running release gets a failed upgrade until the StatefulSet is deleted with--cascade=orphan. Add this note to the README or to the values comment.🤖 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/statefulset.yaml` around lines 46 - 59, Document near replica.sentinel.persistence.enabled, preferably in the values comment or README, that enabling Sentinel persistence requires recreating the existing StatefulSet because volumeClaimTemplates are immutable. Instruct users to delete the StatefulSet with --cascade=orphan before upgrading, preserving the existing release and pods.valkey/tests/sentinel_test.yaml (1)
285-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
replica.sentinel.service.enabled: false.
valkey/templates/sentinel-service.yamlline 1 gates rendering on three values. The suite coversreplica.enabledandreplica.sentinel.enabled, but never setsreplica.sentinel.service.enabledto false. That branch stays untested.♻️ Proposed additional test
- it: should not render any sentinel resource when sentinel is disabled set: replica.sentinel.enabled: false templates: - templates/sentinel-configmap.yaml - templates/sentinel-service.yaml asserts: - hasDocuments: count: 0 + + - it: should not render the sentinel service when the service is disabled + set: + replica.sentinel.service.enabled: false + template: templates/sentinel-service.yaml + asserts: + - hasDocuments: + count: 0🤖 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/tests/sentinel_test.yaml` around lines 285 - 293, Add a test case in sentinel_test.yaml covering replica.sentinel.service.enabled: false, while keeping the replica and sentinel settings enabled so the service-specific gate is exercised. Render templates/sentinel-service.yaml and assert that zero documents are produced.valkey/templates/haproxy-deployment.yaml (4)
38-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable the service account token mount for HAProxy.
HAProxy makes no Kubernetes API calls. It resolves backends through DNS, per the
resolvers kubernetessection invalkey/templates/haproxy-configmap.yaml. The Deployment inheritsserviceAccount.automount, so a chart-leveltrueprojects an API token into a network-facing proxy for no reason.Hardcode
automountServiceAccountToken: false, or add a dedicatedhaproxy.automountServiceAccountTokenvalue that defaults to false.🤖 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/haproxy-deployment.yaml` around lines 38 - 39, Update the HAProxy Deployment’s automountServiceAccountToken setting to always be false, or use a dedicated haproxy.automountServiceAccountToken value defaulting to false instead of inheriting serviceAccount.automount; leave serviceAccountName unchanged.
115-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the scheduling fallbacks consistent.
nodeSelectorat Line 115 andtolerationsat Line 127 fall back to the chart-level values.affinityat Line 119 andtopologySpreadConstraintsat Line 123 do not. A user who sets a chart-levelaffinitygets it on the Valkey pods but not on the HAProxy pods.♻️ Proposed change
- {{- with .Values.haproxy.affinity }} + {{- with .Values.haproxy.affinity | default .Values.affinity }} affinity: {{- toYaml . | nindent 8 }} {{- end }} - {{- with .Values.haproxy.topologySpreadConstraints }} + {{- with .Values.haproxy.topologySpreadConstraints | default .Values.topologySpreadConstraints }} topologySpreadConstraints: {{- toYaml . | nindent 8 }} {{- end }}Confirm that
affinityandtopologySpreadConstraintsexist at the chart level before you apply this.🤖 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/haproxy-deployment.yaml` around lines 115 - 130, Update the HAProxy deployment template’s affinity and topologySpreadConstraints blocks to fall back to the corresponding chart-level values, matching nodeSelector and tolerations; first verify those chart-level values exist, while preserving HAProxy-specific values as the preferred configuration.
79-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet explicit thresholds on the three probes.
All three probes use the Kubernetes defaults:
periodSeconds: 10,failureThreshold: 3,initialDelaySeconds: 0. Two consequences follow.The
startupProbeallows only 30 seconds before the kubelet kills the container. ThereadinessProbeneeds up to 10 seconds to add a new HAProxy replica to the Service, which extends the failover window that the README describes.A TCP probe on
valkey-writealso succeeds as soon as HAProxy binds the port, even while every backend server is stilldownbecause ofinit-state down. Readiness therefore does not reflect the ability to serve writes.♻️ Proposed change
startupProbe: tcpSocket: port: valkey-write + periodSeconds: 2 + failureThreshold: 15 livenessProbe: tcpSocket: port: valkey-write + periodSeconds: 10 + failureThreshold: 3 readinessProbe: tcpSocket: port: valkey-write + periodSeconds: 2 + failureThreshold: 3🤖 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/haproxy-deployment.yaml` around lines 79 - 87, Update the startupProbe, livenessProbe, and readinessProbe definitions in the deployment template with explicit periodSeconds, failureThreshold, and initialDelaySeconds values that meet the documented startup and failover timing. Also make readiness verify backend write availability rather than only HAProxy’s listening socket, while preserving the existing probe structure and port target where applicable.
14-19: 🧹 Nitpick | 🔵 TrivialConsider a PodDisruptionBudget and default anti-affinity for HAProxy.
HAProxy becomes the write path for the whole cluster. With
haproxy.replicas: 1, a node drain removes the write endpoint until the pod reschedules. With more replicas and no anti-affinity, the scheduler can place them all on one node.Add a PodDisruptionBudget template gated on
haproxy.enabled, and ship a defaultpodAntiAffinitywithpreferredDuringSchedulingIgnoredDuringExecutionon the HAProxy component label.🤖 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/haproxy-deployment.yaml` around lines 14 - 19, Add a PodDisruptionBudget template gated by haproxy.enabled and configure its selector to target the HAProxy component labels, preserving the chart’s replica and availability settings. In the HAProxy Deployment template, add default preferredDuringSchedulingIgnoredDuringExecution pod anti-affinity matching app.kubernetes.io/component=haproxy, while allowing user-provided affinity configuration to override or extend the default.valkey/tests/haproxy_test.yaml (2)
33-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the read backend and for the config checksum.
The suite asserts the
valkey_mastercheck sequence in detail, but never asserts thevalkey_replicassequence.valkey/templates/haproxy-configmap.yamlLines 83-88 definePING,+PONG, andfall 2. A regression that swaps the two backends, or that copiesrole:masterinto the read backend, passes the current suite.
valkey/templates/haproxy-deployment.yamlLine 35 setschecksum/config, and no test asserts it.valkey/tests/sentinel_test.yamlLines 254-258 assert the equivalent Sentinel annotation, so the coverage is inconsistent.♻️ Proposed additional tests
+ - it: should ping the read backend and tolerate one failed check + template: templates/haproxy-configmap.yaml + asserts: + - matchRegex: + path: data["haproxy.cfg"] + pattern: 'backend valkey_replicas' + - matchRegex: + path: data["haproxy.cfg"] + pattern: 'tcp-check send PING\\r\\n\n tcp-check send QUIT\\r\\n\n tcp-check expect string \+PONG' + - matchRegex: + path: data["haproxy.cfg"] + pattern: 'fall 2 rise 1 init-addr last,libc,none init-state down' + + - it: should roll the haproxy pods when the config changes + template: templates/haproxy-deployment.yaml + asserts: + - exists: + path: spec.template.metadata.annotations["checksum/config"]🤖 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/tests/haproxy_test.yaml` around lines 33 - 45, Add assertions to the HAProxy test covering the valkey_replicas backend’s PING/+PONG health-check sequence and fall 2 setting, ensuring it remains distinct from the valkey_master role check. Also assert the deployment’s checksum/config annotation, following the equivalent assertion pattern in the Sentinel tests and using the rendered ConfigMap checksum.
53-65: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the intent of
timeout tunnel 0s.This test locks in
0s, which disables the tunnel timeout. Pub/sub connections then survive, as the comment invalkey/templates/haproxy-configmap.yamlLines 36-37 explains. A half-open connection that neither peer closes also survives forever, and it consumes a slot againstmaxconn.
option clitcpkaandoption srvtcpkamitigate this, but only when the kernel keepalive defaults are shorter than the leak window. Consider a large finite default instead, for example24h, and document the pub/sub tradeoff.🤖 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/tests/haproxy_test.yaml` around lines 53 - 65, Replace the HAProxy tunnel timeout assertion in the “should keep pub/sub connections open” test with a large finite timeout such as 24h, update the corresponding timeout configuration and its comment in the haproxy ConfigMap template, and document the pub/sub persistence versus half-open connection tradeoff while preserving the client and server TCP keepalive options.
🤖 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/examples/ha-sentinel.yaml`:
- Around line 23-36: Update the auth configuration in the example to require
operator-supplied secrets instead of shipping usable default and replicator
passwords, while preserving the required ACL permissions. Add a prominent
warning near the credential settings instructing operators to replace both
values before installation and not use the example credentials in deployed
environments.
In `@valkey/README.md`:
- Around line 76-90: Update valkey/README.md lines 76-90 to state that the
valkey-sentinel Service and its endpoint are available only when
replica.sentinel.service.enabled is true, or document the per-pod Sentinel
endpoints when disabled. Update valkey/templates/NOTES.txt lines 27-32 to
conditionally print Service-based connection commands when
replica.sentinel.service.enabled is true and provide the appropriate per-pod
Sentinel alternative otherwise.
- Around line 109-113: Update the README failover documentation around the
“Failover behaviour” section and related haproxy.config.checkInterval
descriptions to distinguish Sentinel failure detection and replica promotion
from HAProxy health-check convergence and routing. Replace the unqualified
“within a few seconds” claim with wording that reflects Sentinel’s promotion
timing, then describe checkInterval only as bounding HAProxy’s post-promotion
detection, not initiating promotion or defining end-to-end failover.
- Around line 104-107: Add the `text` language identifier to the fenced code
block containing the ACL permission list in the README, changing the opening
fence to a language-tagged fence while preserving its contents.
In `@valkey/templates/_helpers.tpl`:
- Around line 243-244: Update the HAProxy configuration template around the
requireClientCertificate condition so the crt directive explicitly references a
combined certificate/private-key bundle, or uses a chart value that guarantees
both files share the same filename. Do not rely on HAProxy’s automatic
private-key lookup based on serverPublicKey.
In `@valkey/templates/haproxy-configmap.yaml`:
- Around line 52-74: Add the HAProxy `balance first` directive within the
`backend valkey_master` configuration before the server definitions, so traffic
uses the first healthy master instead of round-robin distribution when multiple
nodes pass the `role:master` check. Preserve the existing health-check and
server configuration.
In `@valkey/templates/haproxy-deployment.yaml`:
- Around line 106-111: Update the TLS Secret volume in the Helm template’s
tls.enabled block to set defaultMode to 0440 instead of 0400, allowing the
non-root HAProxy container to read required Secret files such as ca.crt.
In `@valkey/templates/init_config.yaml`:
- Around line 188-196: Update the bootstrap marker handling in the init logic to
store the observed role value, either “master” or “replica”, rather than using
mere file presence. Change the `$BOOTSTRAP_MARKER` branch to read and honor that
value, preserving MASTER only when the marker contains “master”; replicas must
not be promoted when no on-disk `replicaof` exists. Also update the marker
creation near the existing init write to record the current role for every pod.
- Around line 81-84: Update the persistence handling around SAVED_REPLICAOF to
normalize any restored replicaof/slaveof target that contains a primary pod IP
to the configured stable master FQDN before reuse. Preserve the existing command
and protocol while ensuring restored replication always targets the master’s
stable DNS name rather than an ephemeral address.
In `@valkey/templates/sentinel-configmap.yaml`:
- Around line 66-81: Replace argument-based Valkey authentication with
REDISCLI_AUTH for every affected valkey-cli call: in
valkey/templates/sentinel-configmap.yaml#L66-L81, update sentinel_cli and
valkey_cli; in `#L232-L236`, export REDISCLI_AUTH="$PASSWORD" in sentinel-ping.sh;
and in `#L280-L297`, set REDISCLI_AUTH for both local_role and sentinel failover
in sentinel-prestop.sh. Remove each corresponding -a password argument while
preserving the existing authentication conditions and commands.
- Around line 36-40: The Sentinel configuration template currently assumes both
the monitor and default ACL users exist when auth is enabled. Update
valkey.validateSentinelConfig or the sentinel script-rendering guard to require
auth.aclUsers.default in addition to the monitor user, preventing Sentinel
scripts from rendering when either dependency is missing.
- Around line 141-146: Update the discovery branch in the Sentinel configuration
setup to validate both extracted fields before assigning MASTER_HOST and
MASTER_PORT. If either discovered value is empty, retain the existing defaults
and use the bootstrap path; only apply discovered values when both are present,
ensuring the monitor directive always receives a valid host and port.
In `@valkey/templates/service.yaml`:
- Around line 34-38: Update the primary Service selector in the service template
so Sentinel mode cannot expose all pods without master-only routing: require
haproxy.enabled whenever replica.sentinel.enabled is true, or implement and
select a label tracking the current master. Preserve the existing
replica-enabled, non-Sentinel pod-name selector behavior and ensure direct
clients using the primary Service cannot send writes to replicas.
In `@valkey/templates/statefulset.yaml`:
- Around line 168-173: Update the StatefulSet and sentinel configuration
validation so terminationGracePeriodSeconds is configurable with a default of 30
seconds, and ensure it is applied to the pod spec. In
valkey.validateSentinelConfig, when replica.sentinel.preStopFailover is enabled,
fail rendering if preStopFailoverTimeoutSeconds is greater than or equal to the
effective grace period, using the existing values and validation conventions.
- Around line 232-236: Update the sentinel peer-discovery command used by
master_from_peers() to pass a bounded -t connection timeout to sentinel_cli when
querying each peer’s master address. In the StatefulSet startupProbe, increase
failureThreshold so the startup budget covers the full local Valkey wait plus
bounded peer discovery, while preserving the existing periodSeconds and
startupTimeoutSeconds-based configuration.
---
Outside diff comments:
In `@valkey/templates/NOTES.txt`:
- Around line 94-104: Update the Sentinel-mode write instructions in the NOTES
template so they never use the load-balanced valkey Service for SET operations.
When HAProxy is enabled, reference the valkey-haproxy Service and its write
port; otherwise document Sentinel master discovery without providing a direct
valkey-cli SET command, while preserving the existing non-Sentinel instructions.
---
Nitpick comments:
In `@valkey/templates/haproxy-deployment.yaml`:
- Around line 38-39: Update the HAProxy Deployment’s
automountServiceAccountToken setting to always be false, or use a dedicated
haproxy.automountServiceAccountToken value defaulting to false instead of
inheriting serviceAccount.automount; leave serviceAccountName unchanged.
- Around line 115-130: Update the HAProxy deployment template’s affinity and
topologySpreadConstraints blocks to fall back to the corresponding chart-level
values, matching nodeSelector and tolerations; first verify those chart-level
values exist, while preserving HAProxy-specific values as the preferred
configuration.
- Around line 79-87: Update the startupProbe, livenessProbe, and readinessProbe
definitions in the deployment template with explicit periodSeconds,
failureThreshold, and initialDelaySeconds values that meet the documented
startup and failover timing. Also make readiness verify backend write
availability rather than only HAProxy’s listening socket, while preserving the
existing probe structure and port target where applicable.
- Around line 14-19: Add a PodDisruptionBudget template gated by haproxy.enabled
and configure its selector to target the HAProxy component labels, preserving
the chart’s replica and availability settings. In the HAProxy Deployment
template, add default preferredDuringSchedulingIgnoredDuringExecution pod
anti-affinity matching app.kubernetes.io/component=haproxy, while allowing
user-provided affinity configuration to override or extend the default.
In `@valkey/templates/sentinel-configmap.yaml`:
- Around line 187-195: Update the configuration-generation flow around
SENTINEL_CONF to ensure the file is created or chmodded with mode 0600 after
appending the plaintext credentials. Preserve Sentinel’s ability to rewrite the
file and apply the permission consistently to the generated sentinel.conf.
In `@valkey/templates/statefulset.yaml`:
- Around line 46-59: Document near replica.sentinel.persistence.enabled,
preferably in the values comment or README, that enabling Sentinel persistence
requires recreating the existing StatefulSet because volumeClaimTemplates are
immutable. Instruct users to delete the StatefulSet with --cascade=orphan before
upgrading, preserving the existing release and pods.
In `@valkey/tests/haproxy_test.yaml`:
- Around line 33-45: Add assertions to the HAProxy test covering the
valkey_replicas backend’s PING/+PONG health-check sequence and fall 2 setting,
ensuring it remains distinct from the valkey_master role check. Also assert the
deployment’s checksum/config annotation, following the equivalent assertion
pattern in the Sentinel tests and using the rendered ConfigMap checksum.
- Around line 53-65: Replace the HAProxy tunnel timeout assertion in the “should
keep pub/sub connections open” test with a large finite timeout such as 24h,
update the corresponding timeout configuration and its comment in the haproxy
ConfigMap template, and document the pub/sub persistence versus half-open
connection tradeoff while preserving the client and server TCP keepalive
options.
In `@valkey/tests/sentinel_test.yaml`:
- Around line 285-293: Add a test case in sentinel_test.yaml covering
replica.sentinel.service.enabled: false, while keeping the replica and sentinel
settings enabled so the service-specific gate is exercised. Render
templates/sentinel-service.yaml and assert that zero documents are produced.
🪄 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: e5b2fd37-90c1-4b8b-a5d3-e72f7f171d4e
📒 Files selected for processing (20)
valkey/.helmignorevalkey/Chart.yamlvalkey/README.mdvalkey/examples/ha-sentinel.yamlvalkey/templates/NOTES.txtvalkey/templates/_helpers.tplvalkey/templates/deploy_valkey.yamlvalkey/templates/haproxy-configmap.yamlvalkey/templates/haproxy-deployment.yamlvalkey/templates/haproxy-service.yamlvalkey/templates/init_config.yamlvalkey/templates/sentinel-configmap.yamlvalkey/templates/sentinel-service.yamlvalkey/templates/service-headless.yamlvalkey/templates/service.yamlvalkey/templates/statefulset.yamlvalkey/tests/haproxy_test.yamlvalkey/tests/sentinel_test.yamlvalkey/values.schema.jsonvalkey/values.yaml
| **Services:** | ||
|
|
||
| * `valkey`: load balances across all pods, the master can be any of them | ||
| * `valkey-sentinel`: Sentinel endpoints, used by clients to resolve the current master | ||
| * `valkey-headless`: headless service for pod and Sentinel discovery | ||
|
|
||
| **Connecting:** | ||
|
|
||
| Because the master moves, clients must ask Sentinel for its address instead of connecting to a fixed pod. | ||
| Most client libraries do this for you: | ||
|
|
||
| ```python | ||
| from valkey.sentinel import Sentinel | ||
|
|
||
| sentinel = Sentinel([("valkey-sentinel", 26379)], password="...") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard Sentinel endpoint documentation on replica.sentinel.service.enabled.
The chart creates the valkey-sentinel Service only when this value is true. Both locations currently present the endpoint as unconditional.
valkey/README.md#L76-L90: state the Service prerequisite or document per-pod Sentinel endpoints.valkey/templates/NOTES.txt#L27-L32: conditionally print the Service commands and provide an alternative when the Service is disabled.
📍 Affects 2 files
valkey/README.md#L76-L90(this comment)valkey/templates/NOTES.txt#L27-L32
🤖 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 76 - 90, Update valkey/README.md lines 76-90
to state that the valkey-sentinel Service and its endpoint are available only
when replica.sentinel.service.enabled is true, or document the per-pod Sentinel
endpoints when disabled. Update valkey/templates/NOTES.txt lines 27-32 to
conditionally print Service-based connection commands when
replica.sentinel.service.enabled is true and provide the appropriate per-pod
Sentinel alternative otherwise.
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>
Sentinel moves the master between pods, which every client has to follow. Clients whose library cannot talk to Sentinel have no way to do that. Add an optional HAProxy deployment that health checks every Valkey node with INFO replication and forwards its write port only to the node answering "role:master", with a second port load balancing reads across all healthy nodes. The health check is the failover mechanism, so there is no watcher sidecar, no runtime package installation and no admin socket. * Send QUIT before the role check so a replica fails its check immediately instead of once the check timeout expires, which shortens the window in which a demoted master still receives writes. * Start every server in the down state, so a starting HAProxy cannot forward writes to a replica before the first check has run. * Pass the health check credentials through an environment variable read from the existing secret, keeping the password out of the ConfigMap. * Keep pub/sub connections alive with a tunnel timeout and TCP keepalives. * Validate the node certificates against the configured CA when TLS is on. 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: lazariv <lazariv.taras@gmail.com> Co-authored-by: Tim Karger <49390121+tkarger@users.noreply.github.com> Signed-off-by: pat-s <patrick.schratz@devxy.io>
629bd4a to
9630b55
Compare
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…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>
Review follow-ups on the HAProxy front-end. * Balance the write backend with "first" instead of the default roundrobin. If two nodes ever pass the role:master check at once, for example an old master that Sentinel has not demoted yet, roundrobin would split writes between them and everything landing on the loser would be discarded. * Mount the TLS secret with mode 0440 and give the pod an fsGroup, the files are owned by root so the non-root HAProxy user could not read them and the container never started with TLS enabled. * Take the client certificate from haproxy.tls.clientCertFile, a single file holding the certificate and its key, because that is what HAProxy expects. The previous "crt tls.serverPublicKey" pointed at a certificate without its key. Fail rendering with an explanation when the value is missing. * Describe a failover as Sentinel detection plus HAProxy convergence rather than checkInterval alone. Signed-off-by: pat-s <patrick.schratz@devxy.io>
|
Review fixes pushed, and I tested the TLS path properly this time: a kind cluster with a real CA, ACL auth and Fixed
The 186 unit tests, |
1 participant
Stacked on #234, review that one first. Only the last commit belongs to this PR.
Sentinel moves the master between pods and clients have to follow it. Clients whose library cannot talk to Sentinel have no way to do that, so this adds an optional HAProxy deployment (
haproxy.enabled) that does it for them:valkey-haproxy:6379always forwards to the current mastervalkey-haproxy:6380load balances reads across all healthy nodesHAProxy finds the master itself, by health checking every node with
INFO replicationand only routing to the one answeringrole:master. That replaces the sentinel-watcher sidecar from #137, and with it theapk add socatat startup that broke air-gapped installs,readOnlyRootFilesystemand MS Defender. No sidecar, no runtime installs, no admin socket: one container running the stock image with a read-only root filesystem as non-root.Also addressed from the #137 review:
timeout tunnelplus TCP keepalives so pub/sub connections are not dropped (@lazariv), and the health check password comes from the secret via an env var rather than being written into the ConfigMap.Testing
helm lint, 173 unit tests, rendering against k8s 1.28/1.35/1.36, andhaproxy -cagainst the rendered config with and without TLS. Verified on a kind cluster with ACL auth:role:master, read port round-robins across all 3 nodesTwo bugs found and fixed during that testing, both of which silently sent writes to a replica:
init-state down.role:masterto appear. SendingQUITbefore the expect makes the node close the connection, so it fails immediately.A short
-READONLYwindow remains possible while a recovered old master is being demoted by Sentinel. That is inherent to Sentinel and documented in the README.