tikv / tikv/pd

resource_group: ModifyResourceGroup never re-syncs burst limit when patching an already-cached group

Open
#11,111 1 comment 0 reactions 0 assignees View on GitHub
needs-type
Dominant language
Go
Stars
1.2k
Forks
783
Avg merge
5d 21h
Merged PRs (30d)
36

Description

### Context

Found while auditing burst-limit sync visibility for #10873 (unrelated to that PR's async-loading changes - this predates it).

`ModifyResourceGroup` (pkg/mcs/resourcemanager/server/manager.go) patches an existing group's settings via `krgm.modifyResourceGroup`, which calls `curGroup.PatchSettings(group)` directly on the already-cached object using the object's own lock. It then calls `publishResourceGroupMutation`, whose `fn` callback only returns a non-nil `synced` group when `cur.groups[grouppb.Name] != patched` (i.e. the cache holds a *different* object - typically only true across a leadership change or when the cached entry was a placeholder).

### The gap

In the common case (no leadership change, the group was already the live cached object), `patched == cur.groups[grouppb.Name]`, so `synced` stays `nil`, and `publishResourceGroupMutation` never calls `syncBurstabilityWithServiceLimit` at all for this modify.

`PatchSettings` can change a group's raw burst-limit setting (e.g. from a bounded value to the unbounded/burstable sentinel) without touching `overrideBurstLimit`. If that happens while an active keyspace service limit was previously not applied to this group (because it didn't need it while bounded), the group is now unbounded in the cache but its burst override is never (re-)synced - a concurrent token request can read it as unlimited and bypass the active service limit, and nothing ever corrects this until some other operation happens to touch the group's burst state.

### Suggested fix

Same shape as the fix already applied to `upsertResourceGroupFromRaw`'s equivalent in-place-update branch in #10873: merge `PatchSettings`/`patchSettingsLocked` and the burst-limit sync into a single `rg.Lock()` critical section (reusing `applyBurstabilitySyncLocked`), instead of running them as two independent, unsynchronized steps.

### Suggested next step

Add a regression test pinning this (e.g. modify a bounded group to an unbounded setting under an active service limit, with no leadership change, and assert the override is applied), then apply the same locked-merge fix.

Contributor guide

Open the contributing guide

Research direction

Start in pkg/mcs/resourcemanager/server/manager.go, tracing ModifyResourceGroup, PatchSettings, publishResourceGroupMutation, and the existing upsertResourceGroupFromRaw update path referenced in #10873. Add a regression test for modifying a bounded group to an unbounded setting under an active service limit without a leadership change. Done means the override is applied and the test demonstrates the in-place update is synchronized safely.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.