Repository navigation
feat(sentinel): report Sentinel peers and masters that are not pods of the RedisFailover - #335
Merged
Merged
Conversation
The Sentinel config, the Sentinel readiness probe, the shutdown script and the Redis client each had their own copy of `mymaster`. They take the name from `DefaultSentinelMasterName` through `SentinelMasterName`, so a name per RedisFailover needs a change in one place. The generated objects do not change. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
…f the RedisFailover All RedisFailovers use one Sentinel master name, and a Sentinel accepts each peer that sends this name. A Sentinel of another RedisFailover at an old IP of an own Sentinel merges the two groups, and the groups then exchange their masters. The operator compared only counts and did not see such a merge. In Sentinel mode, each reconcile reads SENTINEL MASTER and SENTINEL SENTINELS from each running Sentinel, before the count of the masters. A peer that is no Sentinel pod and a master that is no Redis pod of the RedisFailover are unknown. An IP counts after two checks at least 10s apart. The operator logs a Warning at the start and an Info at the end, and sets the gauges unknown_sentinel_peers and sentinel_unknown_master. It changes no status, no role and no Sentinel. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
…entinel addresses The other Sentinels keep a deleted Sentinel as a peer until their reset, which can come 55s or more after the deletion. A report after 10s thus gave a Warning in each Sentinel restart and rollout. - An IP counts after two checks at least 90s apart. - The operator keeps each pod IP of the RedisFailover for 5 minutes after the last check that found it. Such an IP is not unknown. - A Sentinel in deletion without an answer does not stop the end of an address, because on a lost node it never answers. - A reconcile without the check ends the report and sets the gauges to 0. - The IPs compare in their standard form, and a dual-stack pod has both IPs. - The read uses the short timeouts without a retry, because an error only leaves the Sentinel unread for one check. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
The test of the old IP gives the address in each check, so a check without the memory of the pod IPs reports it after 90s. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G4mmyo3sqLwf23m5FE2nBE
Owner
Author
|
@codex review Generated by Claude Code |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed on the PR:
DefaultSentinelMasterName,SentinelMasterName) forsentinel.conf, the readiness probe and the shutdown script. The generated objects do not change. Correction: the first commit message says "a change in one place", butservice/redis/client.gostill uses the constant. A name per RedisFailover needs a name parameter on the Sentinel methods ofClient. Literals that can drift:example/redisfailover/custom-shutdown.yaml(lines 34 and 65);README.md(lines 280, 284 and 701);docs/migrations/4.2.0.md(line 445); the comment atapi/redisfailover/v1/types.goline 281 and in the 3 CRD yamls; the comment atservice/redis/sentinel.goline 282.SENTINEL MASTERandSENTINEL SENTINELSfrom each running Sentinel, before the count of the masters, with short timeouts. All RedisFailovers usemymaster, so the Sentinels of two RedisFailovers can merge into one group and exchange masters. The soak merged two instances for 30 minutes.redis_operator_controller_unknown_sentinel_peersandredis_operator_controller_sentinel_unknown_master. A reconcile without the check sets them to 0. The IPs compare in standard form, also for dual-stack pods. The operator changes no status, role or Sentinel.docs/logic.mdand the gauges in the README.Tests:
TestSentinelMasterNameHasOneDefinition(fails on main),TestObserveSentinelAddrs,TestObserveSentinelAddrsPrunesThePodIPs,TestSkipSentinelAddrs*,TestSentinelAddrsOnlyInSentinelMode,TestGetUnknownSentinelAddrs*,TestGetSentinelPeers*(one uses two real Sentinels, one a frozen Sentinel),TestSentinelUnknownAddrsMetrics. The new tests do not compile on main; mutations of each rule fail them. Not tested: a real cluster, and a real merge of two groups.Generated by Claude Code