resource group: starter mode support degrade to default RU group#68105
resource group: starter mode support degrade to default RU group#68105ystaticy wants to merge 23 commits into
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
Hi @ystaticy. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe domain runaway controller now supports conditional degraded-mode and VIP retry options, with tests covering fallback and error behavior. Starter configuration captures the runtime pod namespace from explicitly supplied additional parameters. ChangesRunaway controller and Starter configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TiDBServer
participant DomainController
participant ResourceGroupProvider
TiDBServer->>TiDBServer: parse Starter pod namespace
TiDBServer->>DomainController: initialize with Starter runtime metadata
DomainController->>ResourceGroupProvider: request resource group
ResourceGroupProvider-->>DomainController: return group or error
DomainController-->>TiDBServer: apply fallback settings or propagate error
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/domain/runaway_test.go (2)
109-113: Prefer field-level assertions over full proto struct equality.Asserting the whole
ResourceGroupmakes the test fragile to unrelated default-field changes. Assert the fallback-specific fields (name/mode/fill-rate/burst-limit) instead.As per coding guidelines, keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.♻️ Suggested change
group, err := controllerWithFallback.GetResourceGroup("test-group") require.NoError(t, err) - require.Equal(t, &rmpb.ResourceGroup{ - Name: "test-group", - Mode: rmpb.GroupMode_RUMode, - RUSettings: newDefaultDegradedRUSettings(), - }, group) + require.NotNil(t, group) + require.Equal(t, "test-group", group.GetName()) + require.Equal(t, rmpb.GroupMode_RUMode, group.GetMode()) + require.NotNil(t, group.GetRUSettings()) + require.NotNil(t, group.GetRUSettings().GetRU()) + require.NotNil(t, group.GetRUSettings().GetRU().GetSettings()) + require.Equal(t, int64(defaultDegradedRUFillRate), group.GetRUSettings().GetRU().GetSettings().GetFillRate()) + require.Equal(t, int64(defaultDegradedRUBurstLimit), group.GetRUSettings().GetRU().GetSettings().GetBurstLimit())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/domain/runaway_test.go` around lines 109 - 113, The test currently asserts full equality against a ResourceGroup proto which is fragile; instead update the assertion to check only the fallback-specific fields: verify group.Name == "test-group", group.Mode == rmpb.GroupMode_RUMode, and that group.RUSettings.FillRate and group.RUSettings.BurstLimit match the expected values produced by newDefaultDegradedRUSettings() (or assert those specific numeric values), leaving other proto fields untouched to avoid broad test churn.
64-66: Return a closed channel from theWatchstub for deterministic teardown.An unclosed channel can leave background consumers blocked in some implementations. Returning a pre-closed channel makes this stub safer and deterministic.
As per coding guidelines, keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.♻️ Suggested change
func (s *resourceGroupProviderStub) Watch(context.Context, []byte, ...opt.MetaStorageOption) (chan []*meta_storagepb.Event, error) { - return make(chan []*meta_storagepb.Event), nil + ch := make(chan []*meta_storagepb.Event) + close(ch) + return ch, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/domain/runaway_test.go` around lines 64 - 66, The Watch stub in resourceGroupProviderStub should return a pre-closed channel to avoid hanging background consumers: in the Watch method (resourceGroupProviderStub.Watch) create a channel of type chan []*meta_storagepb.Event, close it immediately, and then return that closed channel with nil error so teardown is deterministic and safe.pkg/domain/runaway.go (1)
60-64: Tighten VIP namespace matching to avoid accidental opt-in.
strings.Contains(os.Getenv("NAMESPACE"), "vip")also matches names likenonvip. A stricter token/prefix/suffix match would avoid enabling retry policy unintentionally.♻️ Suggested refinement
func newResourceGroupsControllerOptions() []rmclient.ResourceControlCreateOption { opts := []rmclient.ResourceControlCreateOption{ rmclient.WithMaxWaitDuration(runaway.MaxWaitDuration), rmclient.WithDegradedModeWaitDuration(defaultDegradedModeWaitTimeout), rmclient.WithDegradedRUSettings(newDefaultDegradedRUSettings()), } - if strings.Contains(os.Getenv("NAMESPACE"), "vip") { + ns := strings.ToLower(os.Getenv("NAMESPACE")) + if ns == "vip" || strings.HasPrefix(ns, "vip-") || strings.HasSuffix(ns, "-vip") || strings.Contains(ns, "-vip-") { opts = append(opts, rmclient.WithWaitRetryInterval(vipWaitRetryInterval), rmclient.WithWaitRetryTimes(vipWaitRetryTimes), ) } return opts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/domain/runaway.go` around lines 60 - 64, The current check uses strings.Contains(os.Getenv("NAMESPACE"), "vip") which unintentionally matches names like "nonvip"; change the condition in the block that appends rmclient.WithWaitRetryInterval and rmclient.WithWaitRetryTimes to use a stricter match (e.g., exact match, prefix/suffix, or token boundary) against the NAMESPACE environment value instead of Contains; modify the condition surrounding opts = append(...) so it checks os.Getenv("NAMESPACE") == "vip" or uses strings.HasPrefix/HasSuffix or a regex like `(^|-)vip(-|$)` to only opt-in true VIP namespaces while retaining the same appending of rmclient.WithWaitRetryInterval(vipWaitRetryInterval) and rmclient.WithWaitRetryTimes(vipWaitRetryTimes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/domain/runaway_test.go`:
- Around line 109-113: The test currently asserts full equality against a
ResourceGroup proto which is fragile; instead update the assertion to check only
the fallback-specific fields: verify group.Name == "test-group", group.Mode ==
rmpb.GroupMode_RUMode, and that group.RUSettings.FillRate and
group.RUSettings.BurstLimit match the expected values produced by
newDefaultDegradedRUSettings() (or assert those specific numeric values),
leaving other proto fields untouched to avoid broad test churn.
- Around line 64-66: The Watch stub in resourceGroupProviderStub should return a
pre-closed channel to avoid hanging background consumers: in the Watch method
(resourceGroupProviderStub.Watch) create a channel of type chan
[]*meta_storagepb.Event, close it immediately, and then return that closed
channel with nil error so teardown is deterministic and safe.
In `@pkg/domain/runaway.go`:
- Around line 60-64: The current check uses
strings.Contains(os.Getenv("NAMESPACE"), "vip") which unintentionally matches
names like "nonvip"; change the condition in the block that appends
rmclient.WithWaitRetryInterval and rmclient.WithWaitRetryTimes to use a stricter
match (e.g., exact match, prefix/suffix, or token boundary) against the
NAMESPACE environment value instead of Contains; modify the condition
surrounding opts = append(...) so it checks os.Getenv("NAMESPACE") == "vip" or
uses strings.HasPrefix/HasSuffix or a regex like `(^|-)vip(-|$)` to only opt-in
true VIP namespaces while retaining the same appending of
rmclient.WithWaitRetryInterval(vipWaitRetryInterval) and
rmclient.WithWaitRetryTimes(vipWaitRetryTimes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e424e4fc-124f-4549-a80c-a8482598e532
📒 Files selected for processing (3)
pkg/domain/BUILD.bazelpkg/domain/runaway.gopkg/domain/runaway_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68105 +/- ##
================================================
- Coverage 76.3194% 75.4428% -0.8766%
================================================
Files 2041 2087 +46
Lines 560092 587495 +27403
================================================
+ Hits 427459 443223 +15764
- Misses 131732 141644 +9912
- Partials 901 2628 +1727
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ystaticy
changed the title
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
labels
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/domain/runaway_test.go (1)
82-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated controller-creation/assertion pattern.
The Starter-edition and Community-edition blocks (lines 98-119 and 121-138) duplicate the controller construction, start, and defer-stop pattern almost verbatim aside from the edition value and expected outcome. A small helper (e.g.,
newTestController(t, ctx, provider, id)) or splitting into twot.Runsubtests would reduce duplication and make failures easier to attribute.🤖 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 `@pkg/domain/runaway_test.go` around lines 82 - 138, The test in runaway_test.go repeats the same ResourceGroupController setup, Start, and deferred Stop flow for both edition cases. Refactor the duplicated controller-creation/assertion pattern around NewResourceGroupController, Start, Stop, and GetResourceGroup into a small helper or separate t.Run subtests so the Starter and Community scenarios only differ in TiDBEdition and expected result, while keeping the assertions clear and failure attribution easier.
🤖 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.
Nitpick comments:
In `@pkg/domain/runaway_test.go`:
- Around line 82-138: The test in runaway_test.go repeats the same
ResourceGroupController setup, Start, and deferred Stop flow for both edition
cases. Refactor the duplicated controller-creation/assertion pattern around
NewResourceGroupController, Start, Stop, and GetResourceGroup into a small
helper or separate t.Run subtests so the Starter and Community scenarios only
differ in TiDBEdition and expected result, while keeping the assertions clear
and failure attribution easier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b3cac21d-5089-4985-9d4d-e92abcfd2496
📒 Files selected for processing (3)
pkg/domain/BUILD.bazelpkg/domain/runaway.gopkg/domain/runaway_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/domain/runaway.go
- pkg/domain/BUILD.bazel
ystaticy
changed the title
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 2
- Inline comments: 2
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (2)
- Degraded RU fallback masks lookup failures and accepts missing switch groups (
pkg/domain/runaway.go:62) - Environment substring gate hides VIP token-wait retry policy (
pkg/domain/runaway.go:65)
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/tidb-server/main.go`:
- Around line 483-486: Ensure the external workload manager initialized in the
deploymode.IsStarter() branch is closed before every os.Exit path, including
keyspace-activation and final nonzero exits. Update the shutdown flow around
closeExternalWorkloadManager so it runs explicitly before those exits rather
than relying only on defer.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dd5df819-32ad-4f56-892a-36b567243593
📒 Files selected for processing (3)
cmd/tidb-server/main.gocmd/tidb-server/main_test.gopkg/config/config.go
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Test-structure and coverage review. The inline comments focus on fallback error semantics, exact RU/retry configuration assertions, and clearer subtest organization.
| if deploymode.IsStarter() { | ||
| opts = append(opts, | ||
| // Keep degraded-group synthesis inside the controller so degraded | ||
| // resource groups are not inserted into the controller cache. |
There was a problem hiding this comment.
Preserve degraded controller state across request and response accounting
Keeping a synthetic group out of the normal metadata cache avoids making stale fallback metadata look authoritative, but the pinned PD controller currently returns an entirely ephemeral groupCostController as a result.
The TiKV interceptor calls OnRequestWait and later OnResponseWait. The request path uses that ephemeral controller, while the response path only calls loadGroupController; because the degraded controller was never stored, it returns empty consumption and logs that the resource group does not exist. The next RPC constructs another controller with a fresh FillRate worth of tokens. As a result, the configured 2,000,000 RU/s setting is effectively a per-RPC allowance rather than a shared limit, response-side RU settlement is lost, and the failing RM lookup is repeated.
Please keep marked degraded controller state separately from the normal metadata cache, share it across request/response accounting, retry the real metadata lookup with bounded backoff, and replace the degraded state immediately after recovery. A regression should exercise multiple request/response pairs during provider failure, verify shared limiter/accounting state and bounded provider calls, then verify recovery to the real group.
There was a problem hiding this comment.
Thanks for the detailed analysis.
I agree that preserving a separate degraded controller state would make the degraded mode more complete from an accounting/throttling perspective. However, that is beyond the original intent of this PR.
For Starter, this fallback is only a temporary best-effort path when GetResourceGroup fails, so that the request path does not return an error and hurt the user experience. The fallback RU setting is
meant to be permissive and usable, not to provide an exact shared 2,000,000 RU/s limit across RPCs while RM is unavailable. In this scenario, availability/UX is more important than precise degraded-mode RU
accounting.
Adding a separate degraded controller cache, bounded retry/recovery state, and request/response accounting preservation would make this path much more complex. Unless we want to define a stricter degraded-
mode contract here, I would prefer to keep this PR scoped to the simple Starter fallback behavior.
I can add a code comment to make this intention explicit if that helps.
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
Signed-off-by: ystaticy <y_static_y@sina.com>
[LGTM Timeline notifier]Timeline:
|
| podName string | ||
| podIP string | ||
| podNamespace string | ||
| enableGetResourceGroupDegraded bool |
There was a problem hiding this comment.
Could we shorten this to enableRGFallback (and correspondingly use EnableRGFallback / enable-rg-fallback)? RG is clear as “resource group” in this context, while fallback describes the behavior more directly: when a resource-group lookup fails, the controller synthesizes a degraded group instead of making GetResourceGroup itself “degraded.” I would avoid RUFallback, since this controls resource-group lookup fallback rather than general RU processing.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ChangRui-Ryan, D3Hunter 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 |
|
@ChangRui-Ryan: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: ref #69671
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
--starter-additional-params, available via runtime configuration.