[Bug] Ignore migrating/importing slot markers when computing slot ownership in Robin

Open
  • Is something wrong?
#68 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
22/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
go
Domain
databases, distributed-systems

Research direction

Start in internal/redis/client.go with ParseSlotRanges and SlotsFullyCovered, then read the callers in internal/health/checker.go (slotsCovered, balanced) and internal/reconciler/health_reconciler.go (remediateSlotCoverage, resolveSlotDisagreement, resolveConfigDisagreement). The maintainers have not chosen between Option A and Option B, so agree on the approach before editing. Done means the new unit and fuzz tests pass and make lint, make test-all and make fuzz succeed.

Written by the indexing model from the issue text.

Description

bug

Summary

redis.ParseSlotRanges returns an error for the whole slot field when the field contains a migrating/importing marker ([<slot>->-<nodeID>] or [<slot>-<-<nodeID>]). For example, "0-5460 [100->-abc]" fails to parse, and the valid range 0-5460 is discarded with it.

Several health-check and remediation paths handle that error by treating the node as having no slots, or by skipping it. While a slot is open, Robin can therefore misjudge slot coverage and balance. Remediation may then act on slots that already have an owner.

Related: #62 (slot range hardening). This issue was left out of #62's scope on purpose, because it changes behavior while migrations are in progress.

Background

  • Redis adds migrating/importing markers only to the line flagged as myself, that is, the node that holds the open slot. Other nodes do not show them.
  • A slot stays open while redis-cli --cluster reshard/rebalance is running. It can also stay open indefinitely if a migration is interrupted, for example when a pod restarts mid-rebalance.
  • During a migration, the slot still appears in the source node's regular ranges until CLUSTER SETSLOT <slot> NODE <target> completes it. The target shows it only as [slot-<-id]. Ignoring the markers therefore still yields the correct ownership view.
  • redis.SlotsFullyCovered (internal/redis/client.go) already relies on this: it skips [...] tokens before parsing.

Current behavior

When the health check's seed node has an open slot, its own CLUSTER NODES line contains a marker. Each caller of ParseSlotRanges then behaves as follows:

Location Function Behavior with a marker
internal/health/checker.go slotsCovered Returns false. SlotsCovered condition is a false negative (SlotsUncovered)
internal/health/checker.go balanced Returns false. SlotsBalanced condition is a false negative (SlotsUnbalanced)
internal/reconciler/health_reconciler.go remediateSlotCoverage Skips the primary. Its slots are treated as missing and CLUSTER ADDSLOTS is sent to other primaries
internal/reconciler/health_reconciler.go resolveSlotDisagreement Ignores that node's vote for slot ownership
internal/reconciler/health_reconciler.go resolveConfigDisagreement Ignores that node's vote for slot ownership
internal/reconciler/health_reconciler.go remediateExcessPrimaries ✅ Correct: checks for [ before parsing and treats the node as non-empty
internal/redis/client.go SlotsFullyCovered ✅ Correct: skips markers
Potential impact (not yet reproduced)

The health remediation pipeline runs remediateSlotCoverage before remediateClusterCheck, which is the redis-cli --cluster fix step that actually repairs open slots. If the seed has an open slot, the following sequence looks possible:

  1. slotsCovered returns false, so remediateSlotCoverage runs.
  2. The seed's own slots are reported as missing, and CLUSTER ADDSLOTS is sent to another primary.
  3. Redis replies already busy, so resolveSlotDisagreement runs and forces ownership with CLUSTER SETSLOT ... NODE while a migration is still in progress.

This sequence comes from reading the code. It still needs to be confirmed with a test.

Proposal

Option A: make ParseSlotRanges skip markers
for _, token := range tokens {
    if strings.HasPrefix(token, "[") {
        continue // migrating/importing marker, not stable ownership
    }
    rng, err := parseSlotToken(token)
    ...
}
  • Pros: smallest change. It fixes every caller at once and matches SlotsFullyCovered.
  • Cons: it changes the contract of a shared function. Callers no longer get an implicit "migration in progress" signal from the error.
Option B (recommended): explicit helper for ownership

Add a helper that filters the markers. For example, redis.ParseOwnedSlotRanges(raw string) ([]SlotRange, error), or a StripSlotMarkers(raw string) string used before ParseSlotRanges. Use it explicitly in:

  • slotsCovered
  • balanced
  • remediateSlotCoverage
  • resolveSlotDisagreement
  • resolveConfigDisagreement

ParseSlotRanges stays strict, and remediateExcessPrimaries keeps its explicit check. Each call site is reviewed one by one, and the change touches fewer places.

Optional: skip slot coverage remediation while slots are in flight

In HealthReconciler.Reconcile, skip remediateSlotCoverage when redis.HasInFlightSlots(report.ClusterNodes) is true. Open-slot repair is then left to remediateClusterCheck (--cluster fix) and recoverOpenSlots, which are designed for it. This works with either option.

Tests

Unit:

  • slotsCovered and balanced with a full, balanced topology in which the myself line carries [slot->-id] / [slot-<-id] markers. Both should return true.
  • remediateSlotCoverage with the fake clientFactory: when a primary has markers but covers its slots, ClusterAddSlots must not be called.
  • resolveSlotDisagreement and resolveConfigDisagreement: the vote of a node whose slot field has markers must be counted.
  • New helper (option B), or updated ParseSlotRanges (option A):
    • marker-only input returns no ranges;
    • mixed input returns only the stable ranges;
    • markers never hide a malformed regular token.
  • Option A only: update TestParseSlotRanges_RejectsUnsupportedTokens, which currently expects an error.
  • Fuzz: add an invariant that appending a marker never changes the parsed ownership.

Integration (envtest): not needed, because those tests do not use Redis slot data.

E2E: not strictly required. For end-to-end evidence, use an operator chaos scenario that interrupts a rebalance and leaves a slot open. Then check that Robin does not issue ADDSLOTS/SETSLOT for owned slots and that the cluster converges through --cluster fix.

Acceptance criteria

  • With an open slot on the seed node, SlotsCovered and SlotsBalanced reflect the real stable ownership.
  • remediateSlotCoverage never sends CLUSTER ADDSLOTS for slots that are owned by a node with an open slot.
  • Slot ownership voting counts nodes that have open slots.
  • Unit tests and fuzz invariants covering the above are added.
  • make lint, make test-all and make fuzz pass.
Dominant language
Go
Stars
18
Forks
0
Avg merge
11h 16m
Merged PRs (30d)
11

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from InditexTech/redkey-robin

All issues in InditexTech/redkey-robin

Similar issues

More Go issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.