tikv / tikv/pd

region labeler: cancellation responsiveness and etcd write fencing follow-ups

Open
#11,104 0 comments 0 reactions 0 assignees View on GitHub
type/enhancement
Dominant language
Go
Stars
1.2k
Forks
783
Avg merge
5d 21h
Merged PRs (30d)
36

Description

### Background

Follow-up from an internal fork PR ([tidbcloud/pd-cse#569](https://github.com/tidbcloud/pd-cse/pull/569), a region labeler retry/atomic-publish rework closing [tidbcloud/pd-cse#563](https://github.com/tidbcloud/pd-cse/issues/563)). During review, a few issues came up that are real but out of scope for that PR because they require bigger, cross-cutting changes rather than a local fix in `pkg/schedule/labeler`. Filing them upstream since they apply to `tikv/pd` in general, not just the fork.

Also tracked internally as [tidbcloud/pd-cse#578](https://github.com/tidbcloud/pd-cse/issues/578).

### 1. Range-list rebuild is not cancellation-aware

`buildRangeList` (called once from `mustLoadRules`, backing `BuildRangeListLocked`) walks the fully-loaded rule set and builds/sorts the whole `rangelist.List` with no way to observe `ctx.Done()` partway through.

At very large rule counts (the million-rule scale motivating tidbcloud/pd-cse#563), if cancellation (shutdown / leader handoff) arrives while this build is running, `RegionLabeler.Close()` still has to wait for the entire rebuild to finish before its `wg.Wait()` returns — this build has no way to bail out early.

In practice this only affects the *first* successful load of a given labeler instance (it runs once, before the labeler is ever marked ready, so it never blocks an in-flight rule-serving request), so it's a bounded one-shot cost rather than an amplifying issue — but for very large rule sets it could still meaningfully delay shutdown/leader handoff.

Possible direction: give `rangelist.Builder`/`Build()` an optional cancellation check, or chunk the build with periodic `ctx.Err()` checks.

Ref: https://github.com/tidbcloud/pd-cse/pull/569#discussion_r3679626670

### 2. Region-label page reads don't respect the labeler's context (up to ~10s shutdown stall)

`loadAllRules` checks `l.ctx.Err()` between pages, but the actual etcd read for a page goes through `etcdutil.EtcdKVGet`, which builds its own timeout via `context.WithTimeout(c.Ctx(), DefaultRequestTimeout)` (10s) — `c.Ctx()` is the etcd client's own long-lived context, completely disconnected from `l.ctx`.

So if a single page read stalls (slow/partitioned etcd, etc.) right as cancellation is requested, the between-page check added in tidbcloud/pd-cse#569 can't help — `Close()` can still block for up to the full 10s `DefaultRequestTimeout` waiting for that one read to return, instead of returning promptly on cancellation.

Fixing this properly means threading a caller context through `kv.Base`/`kv.Txn`'s `Load`/`LoadRange` (currently context-free by design, used across ~14 files), which is a bigger, cross-cutting storage-layer change — not something to do as a side effect of the labeler PR.

Ref: https://github.com/tidbcloud/pd-cse/pull/569#discussion_r3701222493

### 3. No fencing token on etcd writes (residual write from a departing leader could race a fresh bootstrap scan)

Raised in the context of "could revision-inconsistent reads during the labeler's paginated bootstrap scan cause a mixed/stale published state?". Within a single leader term the answer is no: `SaveRegionRule`/`DeleteRegionRule` are only ever called from `labeler.go` itself, and every call site checks `IsReady()` first, so nothing can write to the region-label keyspace while a labeler in that process is scanning it.

However, that argument only holds *within one leader term*. It doesn't cover this narrower scenario: a request is accepted by the leader (passes the `redirector` middleware's `IsLeader()` check) and is mid-flight to etcd; leadership then transfers before that write actually commits; the new leader starts its own bootstrap scan; the old leader's write finally lands mid-scan. `RunInTxn`/`etcdTxn`'s commit path (`pkg/storage/kv/etcd_kv.go`) has no `Cmp` conditions tied to leadership/lease/term for this write path (conditions are only populated when a preceding `Load()` call is present, which this path doesn't do), so there's no fencing token guarding against a stale leader's in-flight write landing late.

This is **not specific to the region labeler or that fork PR** — it's a general characteristic of how PD commits etcd writes today (no lease/term-based fencing on the write path). The window is narrow (bounded by how long a request can stay in-flight past a leadership change) and any resulting staleness is self-correcting on the next read/write of that key, so this is low severity in practice, but it's a real gap worth tracking as its own hardening item rather than something to patch inside the labeler.

Possible direction: attach a lease or term-comparison condition to writes that need this guarantee, or accept the current window as a documented trade-off.

Ref: https://github.com/tidbcloud/pd-cse/pull/569#discussion_r3701151254

### Scope

None of these block tidbcloud/pd-cse#569 — they're documented, deliberate deferrals. Filing this upstream so they have a home if/when they become worth prioritizing.

Contributor guide

Open the contributing guide

Research direction

Start by reading pkg/schedule/labeler, especially buildRangeList, mustLoadRules, BuildRangeListLocked, loadAllRules, and RegionLabeler.Close(). Then trace kv.Base/kv.Txn context handling and the commit path in pkg/storage/kv/etcd_kv.go; done requires an agreed design and implementation for cancellation and write fencing, with scope and validation defined before coding.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.