resource_group: implement async loading for resource groups#10873
resource_group: implement async loading for resource groups#10873bufferflies wants to merge 10 commits into
Conversation
…e startup performance (tikv#411) * feat: implement async loading for resource groups - Add async loading mechanism to reduce startup time - Use atomic operations for loading state management - Implement lazy loading for individual resource groups - Add retry mechanism with infinite retries for reliability - Return error for list requests during loading - Extend storage interface for single group loading - Optimize loading logic to avoid partial loading issues This change significantly improves startup performance by loading resource groups asynchronously while maintaining data integrity. Signed-off-by: disksing <i@disksing.com> * tiny fix Signed-off-by: disksing <i@disksing.com> * test: add comprehensive async loading test with simplified blocking control - Simplify control mechanism from 4 to 2 control points: * blockBeforeLoad: blocks before starting load operation * blockAfterLoad: blocks after loading is completed - Add more test data (test-group-3, test-group-4) for better coverage - Test operations during async loading (read, update, delete, list) - Test operations after async loading completes - Verify syncLoadedGroups mechanism prevents group resurrection - Ensure proper error handling during loading state This test validates the complete async loading workflow with simplified control and comprehensive scenario coverage. Signed-off-by: disksing <i@disksing.com> * fix: resolve testifylint issues in test files - Remove unnecessary fmt.Sprintf calls in assert messages - Use require instead of assert for error assertions - Remove unused fmt imports This fixes all testifylint warnings in the test files. Signed-off-by: disksing <i@disksing.com> * fix: replace assert.NoError with require.NoError in manager_async_test.go - Fix testifylint require-error violations on lines 342, 347, and 353 - Use require.NoError for error assertions to ensure test stops on failure Signed-off-by: disksing <i@disksing.com> * feat: add metrics for resource group loading operations - Add asyncLoadGroupDuration histogram to track async loading performance - Add syncLoadGroupCounter to count synchronous loading operations - Include duration in async loading completion logs for better observability This helps monitor the performance of resource group loading and understand the loading patterns in the system. Signed-off-by: disksing <i@disksing.com> * update error code Signed-off-by: disksing <i@disksing.com> * minor fix Signed-off-by: disksing <i@disksing.com> * fix default group Signed-off-by: disksing <i@disksing.com> * extract addDefaultGroup Signed-off-by: disksing <i@disksing.com> * minor fix Signed-off-by: disksing <i@disksing.com> * fix static check Signed-off-by: disksing <i@disksing.com> * fix lint Signed-off-by: disksing <i@disksing.com> * fix manager reload Signed-off-by: disksing <i@disksing.com> * fix update default group Signed-off-by: disksing <i@disksing.com> * fix test Signed-off-by: disksing <i@disksing.com> * fix when load failed Signed-off-by: disksing <i@disksing.com> --------- Signed-off-by: disksing <i@disksing.com> (cherry picked from commit da1b8ba)
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
the
size/XL
|
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:
📝 WalkthroughWalkthroughAdds async background loading of persisted resource-group settings/states, on-demand single-group lazy loads with sync coordination, a new ErrResourceGroupsLoading error, Prometheus metrics for load observability, and tests updated to wait for async population. ChangesAsync resource-group loading with lazy checkout and state coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/mcs/resourcemanager/server/metrics.go (2)
232-238: ⚡ Quick winImprove help text clarity for histogram metric.
The help text "The duration of the async load group." is grammatically awkward and could be clearer.
📝 Proposed help text improvement
asyncLoadGroupDuration = prometheus.NewHistogram( prometheus.HistogramOpts{ Namespace: namespace, Subsystem: serverSubsystem, Name: "async_load_group_duration_seconds", - Help: "The duration of the async load group.", + Help: "Duration of asynchronous resource group loading in seconds.", })🤖 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/mcs/resourcemanager/server/metrics.go` around lines 232 - 238, The histogram metric asyncLoadGroupDuration created via prometheus.NewHistogram (prometheus.HistogramOpts with Name "async_load_group_duration_seconds") has an awkward Help string; update the Help field to a clearer, grammatically correct description such as "Duration in seconds of async load group operations." so the metric help explicitly states units and purpose.
224-230: ⚡ Quick winFollow Prometheus naming conventions for counter metrics.
The metric name
sync_load_group_counterviolates Prometheus naming conventions:
- Counter metric names should end with
_total(not_counter), per Prometheus best practices.- The help text "The number of the sync load group." is grammatically awkward.
📊 Proposed fix for metric naming and help text
syncLoadGroupCounter = prometheus.NewCounter( prometheus.CounterOpts{ Namespace: namespace, Subsystem: serverSubsystem, - Name: "sync_load_group_counter", - Help: "The number of the sync load group.", + Name: "sync_loaded_groups_total", + Help: "Total number of on-demand resource group loads.", })Note: This change will require updating the call site in
loadResourceGroupIfNeeded(manager.go) if the variable name is also changed.🤖 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/mcs/resourcemanager/server/metrics.go` around lines 224 - 230, Rename the Prometheus counter metric and its help text: change the metric Name from "sync_load_group_counter" to "sync_load_group_total" and update Help to a clear phrase like "Total number of sync load group operations." Update the variable syncLoadGroupCounter (and any references to it) so the code and call sites remain consistent—specifically adjust usages in loadResourceGroupIfNeeded (manager.go) to reference the renamed metric/variable. Ensure only the metric Name and Help text semantics change and that all references compile after the renaming.
🤖 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/mcs/resourcemanager/server/metrics.go`:
- Around line 232-238: The histogram metric asyncLoadGroupDuration created via
prometheus.NewHistogram (prometheus.HistogramOpts with Name
"async_load_group_duration_seconds") has an awkward Help string; update the Help
field to a clearer, grammatically correct description such as "Duration in
seconds of async load group operations." so the metric help explicitly states
units and purpose.
- Around line 224-230: Rename the Prometheus counter metric and its help text:
change the metric Name from "sync_load_group_counter" to "sync_load_group_total"
and update Help to a clear phrase like "Total number of sync load group
operations." Update the variable syncLoadGroupCounter (and any references to it)
so the code and call sites remain consistent—specifically adjust usages in
loadResourceGroupIfNeeded (manager.go) to reference the renamed metric/variable.
Ensure only the metric Name and Help text semantics change and that all
references compile after the renaming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6d75193-729b-4e08-852a-d96780552f15
📒 Files selected for processing (8)
errors.tomlpkg/errs/errno.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metrics.gopkg/storage/endpoint/resource_group.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
👮 Files not reviewed due to content moderation or server errors (7)
- errors.toml
- pkg/errs/errno.go
- pkg/storage/endpoint/resource_group.go
- pkg/mcs/resourcemanager/server/manager.go
- pkg/mcs/resourcemanager/server/manager_async_test.go
- pkg/mcs/resourcemanager/server/manager_test.go
- tests/integrations/mcs/resourcemanager/resource_manager_test.go
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
errors.toml (1)
929-932: 💤 Low valueError definition placement is inconsistent.
The new
ErrResourceGroupsLoadingerror is placed betweenscatterandscheduleerror sections, while otherresourcemanagererrors are grouped together earlier (lines 899-922). This may indicate the file wasn't regenerated withmake generate-errdocafter adding the error code.As per coding guidelines: "Update
errors.tomlviamake generate-errdocwhen adding error codes".🤖 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 `@errors.toml` around lines 929 - 932, The new error definition "PD:resourcemanager:ErrResourceGroupsLoading" is placed out of the existing resourcemanager block; regenerate and reinsert it properly by running the repository's error doc generator and updating errors.toml via "make generate-errdoc" so the ErrResourceGroupsLoading entry is grouped with the other resourcemanager errors (the same section that contains codes from lines ~899-922) and remove the stray entry between scatter and schedule; ensure the generated output contains the exact error key ErrResourceGroupsLoading and the original message text.Source: Coding guidelines
🤖 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 `@pkg/errs/errno.go`:
- Line 544: The ErrResourceGroupsLoading error declaration has misaligned
spacing compared to surrounding error constants; run the project's formatter
(e.g., make fmt or gofmt) and reformat the declaration for
ErrResourceGroupsLoading so its whitespace/indentation matches the other error
declarations (ensure the line defining ErrResourceGroupsLoading uses the same
leading spaces and alignment as the surrounding Err... = errors.Normalize(...)
entries).
In `@pkg/mcs/resourcemanager/server/manager.go`:
- Around line 557-562: When LoadResourceGroupState(keyspaceID, name) returns an
error its value is currently ignored; update the block around
LoadResourceGroupState and setRawStatesIntoResourceGroup so that if err != nil
you log the error with context (keyspaceID and name) using the manager's logger
(e.g., m.logger or existing log facility) before continuing, but still only call
krgm.setRawStatesIntoResourceGroup when err == nil and state != ""; ensure the
log message clearly identifies the failure of m.storage.LoadResourceGroupState
for that resource group.
In `@pkg/mcs/resourcemanager/server/metrics.go`:
- Around line 232-238: The metric asyncLoadGroupDuration is created with
prometheus.NewHistogram and its Help string is vague; update the HistogramOpts
Help to a clear, grammatically complete description such as "Duration of
asynchronous resource group load operations in seconds" or "Duration in seconds
of background async resource group loading" to indicate the measured operation
and units (reference asyncLoadGroupDuration, prometheus.NewHistogram,
HistogramOpts, namespace, serverSubsystem).
- Around line 224-230: The metric defined as syncLoadGroupCounter uses an
unclear Help string and a redundant name; update the prometheus.CounterOpts for
the prometheus.NewCounter call that constructs syncLoadGroupCounter to use a
clearer metric name (e.g., change Name from "sync_load_group_counter" to
"sync_load_groups_total") and improve Help to an explicit sentence such as
"Total number of resource groups loaded synchronously." Ensure you update any
places that reference syncLoadGroupCounter (or the old metric name) so
registration and use remain consistent.
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 275-280: The TestWatchResourceGroup test does a direct
controller.GetResourceGroup(...) followed by re.NotNil(meta) which is flaky due
to async watcher propagation; change that assertion to use testutil.Eventually
like earlier in the test: repeatedly call
controller.GetResourceGroup(group.Name) inside the Eventually predicate and
assert the returned meta is non-nil (and any other expectations) once the
predicate succeeds, mirroring the existing
waitAsyncLoadResourceGroups/testutil.Eventually usage to make the check
resilient to propagation delays.
---
Nitpick comments:
In `@errors.toml`:
- Around line 929-932: The new error definition
"PD:resourcemanager:ErrResourceGroupsLoading" is placed out of the existing
resourcemanager block; regenerate and reinsert it properly by running the
repository's error doc generator and updating errors.toml via "make
generate-errdoc" so the ErrResourceGroupsLoading entry is grouped with the other
resourcemanager errors (the same section that contains codes from lines
~899-922) and remove the stray entry between scatter and schedule; ensure the
generated output contains the exact error key ErrResourceGroupsLoading and the
original message text.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc2afdab-ade5-4c10-9f3d-4835e8c36d16
📒 Files selected for processing (8)
errors.tomlpkg/errs/errno.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metrics.gopkg/storage/endpoint/resource_group.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
Signed-off-by: bufferflies <1045931706@qq.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrations/mcs/resourcemanager/resource_manager_test.go (1)
276-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a cancelable context with per-attempt timeout in async-load polling.
cli.ListResourceGroups(context.TODO())in the retry loop can block indefinitely and stallEventuallyif the RPC hangs. Pass a parent context into this helper and wrap each poll withcontext.WithTimeout(...).As per coding guidelines, "Use context-aware timeouts and backoff for retries".
Suggested patch
-func waitAsyncLoadResourceGroups(re *require.Assertions, cli pd.Client) { +func waitAsyncLoadResourceGroups(ctx context.Context, re *require.Assertions, cli pd.Client) { testutil.Eventually(re, func() bool { - _, err := cli.ListResourceGroups(context.TODO()) + reqCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + defer cancel() + _, err := cli.ListResourceGroups(reqCtx) return err == nil }, testutil.WithTickInterval(100*time.Millisecond)) }-waitAsyncLoadResourceGroups(re, suite.client) +waitAsyncLoadResourceGroups(suite.ctx, re, suite.client)-waitAsyncLoadResourceGroups(re, suite.client) +waitAsyncLoadResourceGroups(suite.ctx, re, suite.client)🤖 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 276 - 280, The helper waitAsyncLoadResourceGroups currently calls cli.ListResourceGroups(context.TODO()) which can block; change the signature to accept a parent context (e.g., ctx context.Context) and inside the testutil.Eventually loop wrap each call with a per-attempt timeout using ctx, e.g., ctxAttempt, cancel := context.WithTimeout(ctx, <short-duration>) and defer cancel() before calling cli.ListResourceGroups(ctxAttempt) so each RPC is bounded and won't stall the Eventually loop. Ensure you propagate the parent context from the test caller into waitAsyncLoadResourceGroups and cancel per-attempt contexts promptly.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 276-280: The helper waitAsyncLoadResourceGroups currently calls
cli.ListResourceGroups(context.TODO()) which can block; change the signature to
accept a parent context (e.g., ctx context.Context) and inside the
testutil.Eventually loop wrap each call with a per-attempt timeout using ctx,
e.g., ctxAttempt, cancel := context.WithTimeout(ctx, <short-duration>) and defer
cancel() before calling cli.ListResourceGroups(ctxAttempt) so each RPC is
bounded and won't stall the Eventually loop. Ensure you propagate the parent
context from the test caller into waitAsyncLoadResourceGroups and cancel
per-attempt contexts promptly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e7ed95a-0515-4118-98e1-ce1afc606568
📒 Files selected for processing (5)
errors.tomlpkg/errs/errno.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/metrics.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- errors.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/mcs/resourcemanager/server/manager_async_test.go`:
- Around line 49-53: The async loader's teardown can hang because s.once.Do
blocks on `<-s.release` if tests abort without calling store.unblock(); after
creating the test store (the variable named "store"), immediately add `defer
store.unblock()` in each test that uses the async manager so the unblock always
runs even on early returns—apply this fix to the other test locations that
create `store` (the other occurrences referenced around the s.once.Do block and
lines noted) to prevent stopAsyncTestManager (which waits on m.wg.Wait) from
deadlocking.
In `@pkg/mcs/resourcemanager/server/manager.go`:
- Around line 776-778: In AddResourceGroup, don’t unconditionally swallow errors
from m.loadResourceGroupIfNeeded; change the error handling so that after
calling m.loadResourceGroupIfNeeded(keyspaceID, grouppb.Name) you only ignore
the error when it is an explicit “not found” case and return any other error to
the caller. Concretely, inside AddResourceGroup check err from
m.loadResourceGroupIfNeeded and if it is not nil and not a NotFound/IsNotExist
style error (use the existing project helper or error type used elsewhere for
not-found checks), return that err; otherwise proceed and keep the existing
debug log for the not-found path. Ensure you reference the same function names
(m.loadResourceGroupIfNeeded and AddResourceGroup) so the change is applied in
the right place.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3834e145-9204-4a8d-b7bf-66b7bb2e66c6
📒 Files selected for processing (8)
errors.tomlpkg/errs/errno.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metrics.gopkg/storage/endpoint/resource_group.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
Signed-off-by: bufferflies <1045931706@qq.com>
|
@coderabbitai full review |
ti-chi-bot
Bot
added
size/XXL
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integrations/mcs/resourcemanager/resource_manager_test.go (1)
278-283:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPass context as a parameter instead of using
context.TODO().Line 280 uses
context.TODO()for theListResourceGroupsRPC call. The coding guideline requires that the first parameter must becontext.Contextfor external effects. The helper should accept a context parameter to respect test timeouts and allow proper cancellation.🛠️ Suggested fix
-func waitAsyncLoadResourceGroups(re *require.Assertions, cli pd.Client) { +func waitAsyncLoadResourceGroups(re *require.Assertions, cli pd.Client, ctx context.Context) { testutil.Eventually(re, func() bool { - _, err := cli.ListResourceGroups(context.TODO()) + _, err := cli.ListResourceGroups(ctx) return err == nil }, testutil.WithTickInterval(100*time.Millisecond)) }Then update the call sites at lines 199 and 559 to pass
suite.ctxor the appropriate context.🤖 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 278 - 283, Update the helper waitAsyncLoadResourceGroups to accept a context.Context argument and use that context when calling pd.Client.ListResourceGroups instead of context.TODO(); change the function signature (waitAsyncLoadResourceGroups) and its internal call to use the passed ctx, then update its call sites (the places that currently call waitAsyncLoadResourceGroups at the two test locations) to pass suite.ctx (or the appropriate test context) so the RPC honors test timeouts and cancellations.Source: Coding guidelines
🧹 Nitpick comments (1)
tests/integrations/mcs/resourcemanager/resource_manager_test.go (1)
1507-1515: 💤 Low valueConsider caching the normalized result to avoid redundant computation.
Lines 1512 and 1514 both call
normalizeResourceGroupsForSettingsCompare(newGroups). While functionally correct, this duplicates work. You could compute and store the normalizednewGroupsonce inside the Eventually callback and reuse it in the final assertion for slightly better efficiency.♻️ Optional refactor
expectedGroups := normalizeResourceGroupsForSettingsCompare(groups) - var newGroups []*rmpb.ResourceGroup + var newGroups, normalizedNewGroups []*rmpb.ResourceGroup testutil.Eventually(re, func() bool { var err error newGroups, err = cli.ListResourceGroups(suite.ctx) - return err == nil && reflect.DeepEqual(expectedGroups, normalizeResourceGroupsForSettingsCompare(newGroups)) + if err != nil { + return false + } + normalizedNewGroups = normalizeResourceGroupsForSettingsCompare(newGroups) + return reflect.DeepEqual(expectedGroups, normalizedNewGroups) }) - re.Equal(expectedGroups, normalizeResourceGroupsForSettingsCompare(newGroups)) + re.Equal(expectedGroups, normalizedNewGroups) }🤖 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 1507 - 1515, The test redundantly calls normalizeResourceGroupsForSettingsCompare(newGroups) twice; modify the Eventually callback to compute normalizedNew := normalizeResourceGroupsForSettingsCompare(newGroups) once (after cli.ListResourceGroups returns) and return err==nil && reflect.DeepEqual(expectedGroups, normalizedNew), then use the same normalizedNew for the final assertion (re.Equal(expectedGroups, normalizedNew)); this uses the existing variables newGroups and expectedGroups and keeps the same test semantics while avoiding duplicate normalization.
🤖 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.
Outside diff comments:
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 278-283: Update the helper waitAsyncLoadResourceGroups to accept a
context.Context argument and use that context when calling
pd.Client.ListResourceGroups instead of context.TODO(); change the function
signature (waitAsyncLoadResourceGroups) and its internal call to use the passed
ctx, then update its call sites (the places that currently call
waitAsyncLoadResourceGroups at the two test locations) to pass suite.ctx (or the
appropriate test context) so the RPC honors test timeouts and cancellations.
---
Nitpick comments:
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 1507-1515: The test redundantly calls
normalizeResourceGroupsForSettingsCompare(newGroups) twice; modify the
Eventually callback to compute normalizedNew :=
normalizeResourceGroupsForSettingsCompare(newGroups) once (after
cli.ListResourceGroups returns) and return err==nil &&
reflect.DeepEqual(expectedGroups, normalizedNew), then use the same
normalizedNew for the final assertion (re.Equal(expectedGroups, normalizedNew));
this uses the existing variables newGroups and expectedGroups and keeps the same
test semantics while avoiding duplicate normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 866d2ec9-b8ef-4d4e-b3c6-2c05d185d068
📒 Files selected for processing (2)
pkg/mcs/resourcemanager/server/manager.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/resourcemanager/server/manager.go
Signed-off-by: bufferflies <1045931706@qq.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/mcs/resourcemanager/server/manager_async_test.go (1)
35-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake async-block teardown failure-safe to prevent test hangs.
If a test exits before reaching Line 113 / Line 137, the loader can stay blocked on
<-s.releaseandstopAsyncTestManager(Line 85-90) can block onm.wg.Wait().Proposed minimal fix
type blockingResourceGroupStorage struct { storage.Storage once sync.Once entered chan struct{} release chan struct{} + releaseOnce sync.Once } @@ func (s *blockingResourceGroupStorage) unblock() { - close(s.release) + s.releaseOnce.Do(func() { close(s.release) }) } @@ func TestAsyncLoadResourceGroupsLazyGet(t *testing.T) { re := require.New(t) store := newBlockingResourceGroupStorage() + defer store.unblock() re.NoError(store.SaveResourceGroupSetting(1, "lazy-group", newAsyncTestGroup("lazy-group", 100))) @@ func TestAsyncLoadResourceGroupsDoesNotRestoreDeletedLazyGroup(t *testing.T) { re := require.New(t) store := newBlockingResourceGroupStorage() + defer store.unblock() re.NoError(store.SaveResourceGroupSetting(1, "deleted-group", newAsyncTestGroup("deleted-group", 100)))As per coding guidelines, “Prevent goroutine leaks: pair with cancellation; consider errgroup” and “Cancel timers/tickers; close resources with defer and error checks.”
Also applies to: 65-67, 94-100, 122-129
Source: Coding guidelines
🧹 Nitpick comments (1)
pkg/errs/errno.go (1)
544-544: ⚡ Quick winAdd GoDoc for
ErrResourceGroupsLoading.This is a new exported error, so it should carry a comment starting with
ErrResourceGroupsLoadinglike the other documented exported identifiers in this file.As per coding guidelines, "Exported identifiers need GoDoc starting with the name."
🤖 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/errs/errno.go` at line 544, Add a GoDoc comment for the exported variable ErrResourceGroupsLoading that begins with the identifier name (e.g., "ErrResourceGroupsLoading ...") and briefly describes the error meaning ("resource groups are still being loaded, please try again later") and context (used when the resource manager hasn't finished loading groups). Follow the style and placement of other documented exported errors in pkg/errs/errno.go so the comment sits immediately above the ErrResourceGroupsLoading declaration.Source: Coding guidelines
🤖 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 `@pkg/mcs/resourcemanager/server/manager.go`:
- Around line 581-583: The fast-path currently returns a synthetic in-memory
default by calling m.getOrCreateKeyspaceResourceGroupManager(keyspaceID, true)
when name == DefaultResourceGroupName; change this to first attempt loading the
single persisted group for the default from storage (the same path used by async
merge/load), and only if that storage call returns a not-found should you create
the synthetic reserved group; ensure Get/Modify/ModifyResourceGroup then operate
against the real persisted metadata when present and mark sync-loaded only when
a persisted record actually exists rather than always synthesizing it.
In `@pkg/storage/endpoint/resource_group.go`:
- Around line 78-80: LoadResourceGroupSetting (and the analogous single-item
loader for state) currently only reads the keyspace-scoped path and can miss
legacy entries under the null/legacy keyspace; update
StorageEndpoint.LoadResourceGroupSetting and the corresponding
LoadResourceGroupState function to attempt the keyspace-specific Load first and
if that returns not-found (or empty), fall back to the legacy null keyspace path
(use constant.NullKeyspaceID with keypath.KeyspaceResourceGroupSettingPath /
KeyspaceResourceGroupStatePath and the same name) and return that value so
single-item lookups mirror the bulk loaders' legacy fallback behavior.
---
Nitpick comments:
In `@pkg/errs/errno.go`:
- Line 544: Add a GoDoc comment for the exported variable
ErrResourceGroupsLoading that begins with the identifier name (e.g.,
"ErrResourceGroupsLoading ...") and briefly describes the error meaning
("resource groups are still being loaded, please try again later") and context
(used when the resource manager hasn't finished loading groups). Follow the
style and placement of other documented exported errors in pkg/errs/errno.go so
the comment sits immediately above the ErrResourceGroupsLoading declaration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f085aac-838e-4a9a-a173-92aaa52b6a05
📒 Files selected for processing (8)
errors.tomlpkg/errs/errno.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metrics.gopkg/storage/endpoint/resource_group.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
| // LoadResourceGroupSetting loads a specific resource group from storage. | ||
| func (se *StorageEndpoint) LoadResourceGroupSetting(keyspaceID uint32, name string) (string, error) { | ||
| return se.Load(keypath.KeyspaceResourceGroupSettingPath(keyspaceID, name)) |
There was a problem hiding this comment.
Mirror the legacy-path fallback in the new single-item loaders.
LoadResourceGroupSettings and LoadResourceGroupStates still hydrate legacy resource_group/* entries as constant.NullKeyspaceID, but these new point-lookups only hit the keyspace-scoped paths. During async bootstrap, lazy Get/Modify/Delete on legacy groups will therefore miss persisted settings or state until the full scan completes.
Please make the single-item helpers follow the same compatibility rule as the bulk loaders, at least for the null/legacy keyspace path.
Also applies to: 112-114
🤖 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/storage/endpoint/resource_group.go` around lines 78 - 80,
LoadResourceGroupSetting (and the analogous single-item loader for state)
currently only reads the keyspace-scoped path and can miss legacy entries under
the null/legacy keyspace; update StorageEndpoint.LoadResourceGroupSetting and
the corresponding LoadResourceGroupState function to attempt the
keyspace-specific Load first and if that returns not-found (or empty), fall back
to the legacy null keyspace path (use constant.NullKeyspaceID with
keypath.KeyspaceResourceGroupSettingPath / KeyspaceResourceGroupStatePath and
the same name) and return that value so single-item lookups mirror the bulk
loaders' legacy fallback behavior.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (70.58%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #10873 +/- ##
==========================================
- Coverage 79.22% 79.19% -0.04%
==========================================
Files 541 541
Lines 75965 76198 +233
==========================================
+ Hits 60187 60342 +155
- Misses 11531 11582 +51
- Partials 4247 4274 +27
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test pull-unit-test-next-gen-2 |
|
/test pull-unit-test-next-gen-3 |
| } | ||
| } | ||
| if name == DefaultResourceGroupName { | ||
| m.getOrCreateKeyspaceResourceGroupManager(keyspaceID, true) |
There was a problem hiding this comment.
This path can create and persist a synthetic default group while async loading is still in progress. If storage already contains a customized default group, an early read or modify can observe or write the built-in defaults instead, and the async merge may not restore the original settings depending on timing.
|
AcquireTokenBuckets still bypasses the new lazy-load path. It calls accessKeyspaceResourceGroupManager before GetMutableResourceGroup, so a persisted non-default group whose keyspace manager is not in memory yet is skipped until the full async load completes. That means token requests can fail during startup even though single-group lazy loading was added for this period. |
Address PR tikv#10873 review comments: - grpc_service: AcquireTokenBuckets checked accessKeyspaceResourceGroupManager before GetMutableResourceGroup, bypassing the lazy-load path for non-default groups whose keyspace manager wasn't in memory yet. Reorder so the lazy load runs first. - manager: loadResourceGroupIfNeeded synthesized the reserved default group unconditionally, which could persist synthetic defaults over a customized one still on disk. Try the storage load first and only fall back to the synthetic default on an explicit not-found error. - manager: AddResourceGroup silently swallowed all loadResourceGroupIfNeeded errors, including transient storage failures. Only ignore the expected not-found case and propagate others. - manager_async_test: blockingResourceGroupStorage.unblock could hang stopAsyncTestManager's wg.Wait() if a test aborted before calling it. Make unblock idempotent and defer it right after Init so teardown can't deadlock. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.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 `@pkg/mcs/resourcemanager/server/grpc_service.go`:
- Around line 237-240: The `GetMutableResourceGroup` handling in
`grpc_service.go` treats every `nil, err` as a missing group and continues,
which hides real load failures. Update the stream handling around
`s.manager.GetMutableResourceGroup` to inspect the returned error: only
`continue` when it is `errs.ErrResourceGroupNotExists`, and for any other error
return it so the caller can fail or retry. Keep the existing warning path for
true not-found cases, but do not swallow non-not-found errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e08ba469-8b6e-4348-ade4-4d4c9e063644
📒 Files selected for processing (3)
pkg/mcs/resourcemanager/server/grpc_service.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/resourcemanager/server/manager.go
GetMutableResourceGroup can return (nil, err) for real failures (e.g. ErrKeyspaceNotExists, storage errors during lazy load), not just a missing group. Treating every rg == nil as "not found" silently dropped token requests on real errors instead of failing the stream. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
…mments # Conflicts: # tests/integrations/mcs/resourcemanager/resource_manager_test.go
| } | ||
| state, err := m.storage.LoadResourceGroupState(keyspaceID, name) | ||
| if err != nil { | ||
| log.Warn("failed to load resource group state, continuing without state", |
There was a problem hiding this comment.
When this state read fails, the group is still returned and subsequently recorded in syncLoadedGroups. The async merge then skips the group even if its bulk state read succeeds, so the cached token bucket can remain initialized without its persisted state for the rest of this manager lifetime and may grant fresh initial tokens.
…ailure When loadResourceGroup's LoadResourceGroupState read fails, it logs a warning and still returns the group using default state. But loadResourceGroupIfNeeded then unconditionally marked the group as sync-loaded, which made asyncLoadResourceGroups's merge permanently skip it, even if the later bulk load successfully read the real persisted state. The group would keep its default/fresh state (e.g. a re-initialized token bucket) for the rest of the manager's lifetime. Only mark the group sync-loaded when its state was actually read, so the async bulk merge remains free to fill in the correct state afterward if nothing else has modified the group in the meantime. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
|
|
…ed data initReservedInCache installs a synthetic default resource group into the cache before async loading starts. loadResourceGroupIfNeeded's early cache-hit check treated any cached entry as already-loaded, so it never attempted the persisted point load for a keyspace's default group as long as the synthetic placeholder occupied the slot. A customized default group would be served with built-in settings for the whole startup window, and the async bulk merge could be blocked from correcting it if a concurrent write marked it sync-loaded first. Track which cache entries are still just placeholders (keyspaceResourceGroupManager.reservedGroups, guarded by the same lock as groups) and clear the mark on any real write. loadResourceGroupIfNeeded now only treats a cached entry as satisfying the call when it isn't a placeholder, and is allowed to replace a placeholder with the result of a real storage load. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
| return errs.ErrKeyspaceNotExists.FastGenByArgs(keyspaceID) | ||
| } | ||
| return krgm.addResourceGroup(grouppb) | ||
| if err := m.loadResourceGroupIfNeeded(keyspaceID, grouppb.Name); err != nil && |
There was a problem hiding this comment.
During async loading, adding a resource group can persist a synthetic default group before the customized default is loaded, overwriting its stored settings.
| } | ||
|
|
||
| m.wg.Add(1) | ||
| go m.asyncLoadResourceGroups(ctx) |
There was a problem hiding this comment.
The persistence loop can run before async loading completes and overwrite the stored default-group state with the placeholder's fresh state.
| krgm = m.getOrCreateKeyspaceResourceGroupManager(keyspaceID, false) | ||
| inserted := false | ||
| krgm.Lock() | ||
| if _, exists := krgm.groups[name]; !exists { |
There was a problem hiding this comment.
A lazy load that starts before a concurrent Delete can reinsert the deleted group here. The point read happens outside the group lock, so if Delete finishes after the read but before this insertion, the stale result observes an empty cache and inserts the group again. Because the bulk scan no longer contains the deleted entry, it remains visible after loading completes.
| if err := krgm.modifyResourceGroup(grouppb); err != nil { | ||
| return err | ||
| } | ||
| m.markResourceGroupSyncLoaded(keyspaceID, grouppb.Name) |
There was a problem hiding this comment.
If the preceding lazy load continues after LoadResourceGroupState fails, this call marks the group as fully sync-loaded. The bulk merge then skips it, so the persisted running state is never applied and the cached token bucket can remain initialized with empty state.
|
Thanks,
The new placeholder tracking itself also needs deterministic coverage for customized persisted defaults and the persistence-before-load case. |
…served marker persistResourceGroupRunningState persisted every entry in krgm.groups regardless of reservedGroups, so the persist loop (running concurrently with async loading) could write a synthetic default's fresh token state back to storage before the real persisted state was ever loaded, permanently overwriting it. Skip reserved entries there. Also fix an oversight in the previous placeholder-tracking commit: loadResourceGroupIfNeeded cleared a group's reserved mark unconditionally after a lazy load, even when LoadResourceGroupState had failed and the group only carries default state. Only clear the mark when the state was actually read, otherwise keep it reserved so a later call or the async bulk merge can still fill in the real state instead of the partial result being treated as final. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
|
@bufferflies: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: Close #10872, ref #10516
This CP ports the upstream Resource Manager change from source commit
da1b8ba1e3873401aef0fcbd99c0898a654952e5to improve Resource Manager startup latency when many resource groups exist.What is changed and how does it work?
ErrResourceGroupsLoadingfor list requests until async loading completes.Source commit:
da1b8ba1e3873401aef0fcbd99c0898a654952e5Original author: disksing i@disksing.com
Check List
Tests
Validation:
GOFLAGS=-buildvcs=false go test ./pkg/mcs/resourcemanager/server -run 'TestAsyncLoadResourceGroups|TestManagerMetadataWatcherLifecycle|TestInitManager|TestLoadKeyspaceResourceGroupsRejectsMismatchedPayloadName' -count=1 -timeout=90sGOFLAGS=-buildvcs=false go test ./pkg/storage ./pkg/mcs/resourcemanager/server -run '^$' -count=1 -timeout=5mGOFLAGS=-buildvcs=false go test ./tests/integrations/mcs/resourcemanager -run '^$' -count=1 -timeout=10mcould not run from the root module because integration tests are a separate Go module.tests/integrations:GOFLAGS=-buildvcs=false go test ./mcs/resourcemanager -run '^$' -count=1 -timeout=10mwas blocked by pre-existing dashboard embedded asset generation errors (undefined: assets,undefined: vfsgen۰FS).Code changes
Side effects
Related changes
Release note
Summary by CodeRabbit