region labeler: cancellation responsiveness and etcd write fencing follow-ups
- 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
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