Skip to content

fix(valkey): seed on the current master, not a reachable replica#687

Open
mreho wants to merge 2 commits into
helmforgedev:mainfrom
mreho:fix/valkey/masterDiscovery
Open

fix(valkey): seed on the current master, not a reachable replica#687
mreho wants to merge 2 commits into
helmforgedev:mainfrom
mreho:fix/valkey/masterDiscovery

Conversation

@mreho

@mreho mreho commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Problem

The Sentinel bootstrap seeds on the node currently reporting role:master, but the wait loop only ran that discovery when the static seed (node-0) was unreachable. After a failover, node-0 can be a live replica, so a reachability-only check can seed Sentinel from a read-only node and advertise the wrong master.

Fix

Discover the node reporting role:master among the data-node peers first, and fall back to the static seed only if none answers within the bounded budget.

Type Of Change

  • New chart
  • Existing chart change
  • Documentation only
  • CI / repository workflow

PR Governance

  • I linked an existing issue in this PR body (Resolves #NNN or Related to #NNN)
  • If this is a new chart PR, labels enhancement and type:feature are applied

Checklist

  • I created this branch from updated main
  • My PR targets main
  • My commit message and PR title follow Conventional Commits
  • I did not edit version in Chart.yaml manually
  • I updated chart docs for behavior or default changes

Site Sync (GR-007)

Local Validation

  • make validate-chart CHART=valkey TIMEOUT=1200 passed end-to-end: FULLY VALIDATED (21 layers), including k3d behavioral scenarios for default, cluster, dual-stack, existing-secret, external-secrets, metrics, networkpolicy, replication, sentinel, standalone, tls-cluster, and tls-replication.
  • make site-sync-check CHART=valkey passed on site#365.
  • make release-check REPO=charts passed with the expected GR-077 post-merge release confirmation warning.
  • make attribution-check REPO=charts passed.

Remote Validation

  • GitHub Actions are green; standards-check is skipped by fork policy and covered locally.
  • CodeRabbit completed with no actionable comments.
  • Review threads: 0 unresolved.

Issue: #633

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Sentinel entrypoint's initial bootstrap logic in the Valkey Helm chart's configmap is modified to discover the current master by repeatedly calling find_current_master instead of pinging the seed node, falling back to SEED_MASTER after bounded attempts. Corresponding Helm test assertions are updated to match the new script content.

Changes

Sentinel Bootstrap Discovery Update

Layer / File(s) Summary
Discovery loop rewrite
charts/valkey/templates/configmap.yaml
Replaces ping-based seed reachability loop with repeated find_current_master calls; falls back to SEED_MASTER and updates logging after 6 unsuccessful iterations.
Test assertions updated
charts/valkey/tests/cluster_domain_test.yaml, charts/valkey/tests/ha_extension_test.yaml
Adds/updates matchRegex assertions to validate the new find_current_master-based master assignment logic in the entrypoint script.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: seeding from the current master instead of a reachable replica.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant