client/resource_group: enforce one active controller per process - #11132
client/resource_group: enforce one active controller per process#11132YuhaoZhang00 wants to merge 5 commits into
Conversation
Several process-global integrations around the resource group controller assume a unique controller owner: the resource-control Prometheus collectors carry no controller identity (one controller's shutdown resets the global status gauge and cleans up shared label sets), the constructor overwrites the process-global trace-log flag, and client-go installs the controller into a single global interceptor pointer. The public API still allowed multiple controllers to be constructed and started in one process, leaving this unsupported state reachable by future callers. Make the single-controller model an explicit lifecycle contract: - NewResourceGroupController reserves a process-wide ownership slot before any side effect and fails a second acquisition with ErrClientResourceGroupControllerAlreadyExists; the slot is released on every failed initialization path. - Stop is now idempotent, also valid on a controller that was never started, and releases ownership exactly once. Only the current owner can release the slot, so a stale controller's Stop cannot release a newer controller's slot. A stopped controller can no longer be started. - Existing multi-controller integration tests are converted to sequential coverage, and new contract tests cover rejection, replacement, release on failed initialization, and concurrent acquire/stop. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
added
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
contribution
|
Hi @YuhaoZhang00. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
ChangesResource group controller ownership and lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change enforces a single active controller per process with explicit cleanup and idempotent stopping; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceGroupsController
participant RunLoop
participant controllerOwnership
Client->>ResourceGroupsController: NewResourceGroupController()
ResourceGroupsController->>controllerOwnership: reserve and bind ownership
Client->>ResourceGroupsController: Start()
ResourceGroupsController->>RunLoop: launch lifecycle loop
Client->>ResourceGroupsController: Stop()
ResourceGroupsController->>RunLoop: cancel and wait
RunLoop->>ResourceGroupsController: clean process-global metrics
ResourceGroupsController->>controllerOwnership: release ownership
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/XXL
Address findings from an independent review of the ownership contract: - Serialize Start against Stop with a lifecycle mutex. Previously a Stop racing a Start could observe a nil loopCancel, release the ownership slot, and let Start launch a run loop that no longer owns the slot; its eventual exit would reset the process-global status gauge under a replacement controller. Confirmed with the race detector. - Clean the process-global metric state in Stop as well, covering controllers that were used but never started (or used again after their run loop exited), which previously left stale series behind for the replacement controller. - Release the ownership reservation via defer so a panic on a construction path cannot leak the slot. - Add contract tests: concurrent Start/Stop, ownership retained after context cancellation, rejection before any network I/O, and metric cleanup when stopping an unstarted controller. - Test hygiene: add idempotent deferred Stop backstops so a mid-test failure does not leave the slot held and poison later tests, and a nil guard in the service limit suite teardown. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
Follow-ups from a second independent review pass: - Refuse a second Start of the same controller. Previously it replaced loopCtx/loopCancel and launched a duplicate run loop whose context could never be canceled, racing the first loop's unsynchronized loopCtx reads. - Gate the process-global metric cleanup on current ownership, so stopping a controller allocated outside the supported API can no longer reset the status gauge series that belong to the legitimate owner. The ownership slot was already protected by the owner check in release; the metrics now get the same protection. - Add deferred Stop backstops to the five remaining suite tests that stopped their controller only inline at the end of the test. With the ownership guard in place, a mid-test failure would otherwise keep the slot held and fail every later test in the suite with ErrClientResourceGroupControllerAlreadyExists, masking the root cause. Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
… reused Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/resource_group/controller/ownership_test.go (1)
85-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stale-owner case does not execute
release.
c1.Stop()andc2.Stop()at Lines 88-89 are no-ops.stopOncealready fired for both controllers, so theStopbody never runs again andownership.releaseis never called with a stale controller. The assertion at Lines 90-91 passes, but it does not exercise the owner-identity guard inownership.release.
TestForeignControllerStopDoesNotDisturbOwnercovers that guard. Consider adjusting the comment so a future reader does not treat these lines as the guard's coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/resource_group/controller/ownership_test.go` around lines 85 - 92, Update the comment above the c1.Stop and c2.Stop calls to clarify that these calls are no-ops and do not exercise the stale-owner guard in ownership.release; identify TestForeignControllerStopDoesNotDisturbOwner as the test covering that behavior, while preserving the existing assertions and test flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 2488-2492: Update the Eventually condition around
GetActiveResourceGroup in the resource-group test to return false when meta is
nil before accessing RUSettings. Preserve the existing FillRate comparison for
non-nil metadata so transiently unloaded groups retry instead of panicking.
---
Nitpick comments:
In `@client/resource_group/controller/ownership_test.go`:
- Around line 85-92: Update the comment above the c1.Stop and c2.Stop calls to
clarify that these calls are no-ops and do not exercise the stale-owner guard in
ownership.release; identify TestForeignControllerStopDoesNotDisturbOwner as the
test covering that behavior, while preserving the existing assertions and test
flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66096889-8270-408d-bb98-25a8fb626299
📒 Files selected for processing (8)
client/errs/errno.goclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_group/controller/ownership.goclient/resource_group/controller/ownership_test.goclient/resource_group/controller/request_source_metrics_test.gotests/integrations/mcs/resourcemanager/resource_manager_test.gotests/integrations/mcs/resourcemanager/service_limit_test.go
| group := genGroupByKeyspace(keyspace) | ||
| testutil.Eventually(re, func() bool { | ||
| meta := c.GetActiveResourceGroup(group.Name) | ||
| return meta.RUSettings.RU.Settings.FillRate == fillRate | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard meta against nil inside the Eventually condition.
GetActiveResourceGroup returns nil when the group is not loaded. Line 2491 dereferences meta with no nil check. If the group is momentarily absent, the condition panics instead of retrying, and the failure output hides the real cause.
Lines 2501-2503 already compare the same call against nil, so the nil case is expected elsewhere in this loop.
🛡️ Proposed nil guard
group := genGroupByKeyspace(keyspace)
testutil.Eventually(re, func() bool {
meta := c.GetActiveResourceGroup(group.Name)
- return meta.RUSettings.RU.Settings.FillRate == fillRate
+ return meta != nil && meta.RUSettings.RU.Settings.FillRate == fillRate
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| group := genGroupByKeyspace(keyspace) | |
| testutil.Eventually(re, func() bool { | |
| meta := c.GetActiveResourceGroup(group.Name) | |
| return meta.RUSettings.RU.Settings.FillRate == fillRate | |
| }) | |
| group := genGroupByKeyspace(keyspace) | |
| testutil.Eventually(re, func() bool { | |
| meta := c.GetActiveResourceGroup(group.Name) | |
| return meta != nil && meta.RUSettings.RU.Settings.FillRate == fillRate | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go` around lines
2488 - 2492, Update the Eventually condition around GetActiveResourceGroup in
the resource-group test to return false when meta is nil before accessing
RUSettings. Preserve the existing FillRate comparison for non-nil metadata so
transiently unloaded groups retry instead of panicking.
Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
1 participant
What problem does this PR solve?
Issue Number: close #11080
Current TiDB production code creates one
ResourceGroupsControllerper process, and several process-global integrations (Prometheus collectors without controller identity, theenableControllerTraceLogflag, client-go's global resource-control interceptor) already assume a unique controller owner. However,pd/clientdid not enforce this: the public API allowed multiple controllers to be constructed and started in one process, leaving an unsupported state reachable by future callers.What is changed and how does it work?
This PR implements the pd/client side of the contract described in #11080. The TiDB companion changes (Domain.Close release, initialization error cleanup, bootstrap handoff serialization) are implemented in pingcap/tidb#70456 (tracked by pingcap/tidb#70455). The TiDB changes are compatible with the current pd/client version and do not depend on this PR landing first; conversely, once TiDB picks up this PR through a client upgrade, the explicit release in Domain.Close becomes load-bearing for Domain replacement scenarios.
Notes for reviewers:
loadServerConfigso that a rejected second acquisition performs no network I/O and does not overwrite the process-global trace-log flag.Stopno longer returns an error for a controller that was never started; it releases ownership and succeeds idempotently, so callers have a single cleanup path regardless of how far initialization progressed.Startstops the run loop but does not release ownership;Stopmust be called explicitly.serviceLimitTestSuite.TearDownTestrelied on context cancellation only and now callsStop.TestWatchWithSingleGroupByKeyspaceandTestResourceGroupControllerConfigChangedare split into sequential phases;TestLoadAndWatchWithDifferentKeyspaceis replaced with sequential per-keyspace coverage, deliberately dropping unsupported same-process multi-controller coexistence coverage while retaining keyspace isolation coverage.Check List
Tests
Side effects
ResourceGroupsControllerin the same process now returnsErrClientResourceGroupControllerAlreadyExistsuntil the previous one is stopped. No known production caller constructs more than one controller per process.Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests