resource_group: ModifyResourceGroup never re-syncs burst limit when patching an already-cached group
- 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
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