resource_group: implement async loading for resource groups#10873
resource_group: implement async loading for resource groups#10873bufferflies wants to merge 28 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
|
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:
📝 WalkthroughWalkthroughResource-group initialization now supports asynchronous bulk loading, single-group lazy reads, loading-state errors, reserved placeholders, updated RPC ordering, metrics, and synchronization tests for loading, deletion, legacy keyspaces, watchers, and restarts. ChangesAsync resource-group loading
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceManager
participant StorageEndpoint
Client->>ResourceManager: GetMutableResourceGroup(keyspace,name)
ResourceManager->>StorageEndpoint: LoadResourceGroupSetting/State(keyspace,name)
StorageEndpoint-->>ResourceManager: persisted setting/state
ResourceManager-->>Client: mutable resource group
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10873 +/- ##
==========================================
+ Coverage 79.22% 79.25% +0.02%
==========================================
Files 541 540 -1
Lines 75965 76356 +391
==========================================
+ Hits 60187 60513 +326
- Misses 11531 11568 +37
- Partials 4247 4275 +28
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.
There was a problem hiding this comment.
Fixed in a20e8137 and hardened in 306fd57c: loadResourceGroupIfNeeded now tries the storage point load for default first and only synthesizes the reserved group on a confirmed not-found, and a pre-inserted synthetic entry is tracked as an unconfirmed placeholder (reservedGroups) that never satisfies the cache-hit fast path and gets replaced by a successful storage load.
| zap.Uint32("keyspace-id", keyspaceID), | ||
| zap.String("group-name", name), | ||
| zap.Error(err)) | ||
| return krgm.getMutableResourceGroup(name), false, nil |
There was a problem hiding this comment.
LoadResourceGroupState failed, but returning the group with a nil error exposes a fresh token bucket to AcquireTokenBuckets before recovery. Please propagate the state-load error, or keep the group unavailable until its persisted state is loaded.
There was a problem hiding this comment.
Fixed in 2de53f93. loadResourceGroup now propagates the state-read error instead of returning a metadata-only group, so a group is never exposed to Get/Modify/AcquireTokenBuckets with a fresh token bucket — the request fails and a later attempt (or the bulk merge) loads it fully. This also let us drop the whole state-only reservation machinery (the reservedStateOnly kind and the merge state-adoption path), since a partially-loaded group can no longer enter the cache.
| continue | ||
| } | ||
| if _, exists := krgm.groups[name]; !exists { | ||
| krgm.groups[name] = group |
There was a problem hiding this comment.
The group becomes visible here before it is recorded in syncLoadedGroups below. A bulk merge can enter between those steps and replace a concurrently modified group with its older scan. Please publish the cache entry and sync-loaded marker atomically with respect to the bulk merge.
There was a problem hiding this comment.
Fixed in 2de53f93. The lazy load now publishes the cache entry and the sync-loaded marker in one critical section under the manager lock followed by the keyspace lock — the same order the bulk merge uses — so the merge can no longer slip in between the insert and the marker and replace a concurrently modified group with its older scan. Add/Modify/Delete are covered transitively: their preceding lazy load fully loads and atomically marks the group first, so by the time they mutate the cache the merge already skips it. Added a lazyLoadAfterCachePublish failpoint-based regression test for the window.
…rmed data asyncLoadResourceGroups's merge loop installed the fully-loaded (settings and state) confirmed group into krgm.groups but never cleared reservedGroups for it. A group that got marked reserved by an earlier lazy-load state-read failure would stay reserved forever even after the bulk load correctly recovered it, permanently excluding it from the state persist loop and from loadResourceGroupIfNeeded's fast path. Clear the reserved marker in the same merge step that installs the confirmed data. Add a regression test that injects a one-time LoadResourceGroupState failure during a lazy Get, then lets the async bulk load complete, and asserts the reserved marker is cleared once the confirmed data lands; verified the test fails without the fix. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
newAsyncTestGroup's fillRate parameter always received 100, which tripped the unparam linter in the statics check. Drop the constant parameter and hoist the value into a named asyncTestGroupFillRate constant shared by the helper and the fill-rate assertions. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
loadResourceGroupIfNeeded reads a group from storage without holding the keyspace lock, then inserts it under the lock. A concurrent Delete that completed between the read and the insert would leave the stale insert observing an empty cache and re-adding the just-deleted group. Because the async bulk scan no longer contains that entry, the resurrected group stayed visible for the rest of the manager's lifetime. Add a per-keyspace deleteGen counter, bumped under the write lock on every cache removal. The lazy load snapshots it before its lock-free storage read and re-checks it under the insert lock; if it changed, a Delete raced and the stale result is dropped instead of inserted. A monotonic counter is used rather than a per-name tombstone map so there is no unbounded state to clear and no lifecycle window where a late in-flight reader could still slip through. Add a deterministic regression test that pauses a lazy load right after its storage read, deletes the group, then releases the lazy load and asserts it is not resurrected (verified to fail without the fix). Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
asyncLoadResourceGroups only checked its context at the top of the retry loop. A loader blocked in a storage scan across a leadership change would, after Init ran again for a new term, wake up and merge its stale scan into the new term's maps, clear the new term's syncLoadedGroups, and publish LoadingStateCompleted while the new loader was still running. Add a loadEpoch counter to the manager, bumped by initMetadata under the manager lock and captured by each loader at start. Every shared-state mutation the loader performs (loading-state transitions and the merge) now re-verifies the epoch inside the same critical section, so a stale loader exits instead of touching the newer term's state. Also re-check context cancellation right after the scans return, and make initControllerConfig publish the config via clone-and-swap under the lock, since a re-initialization can race with the previous term's background goroutines still reading it. Add a deterministic regression test that blocks a term-1 loader in its states scan, reinitializes the manager for term 2, deletes the group, then releases the stale loader and asserts it does not resurrect the group or disturb the new term (verified to fail without the fix). Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
When a lazy load reads a group's settings but fails to read its state, the entry is cached as reserved and deliberately kept out of syncLoadedGroups so the async bulk merge can still recover the real state. But the merge replaced the cache entry wholesale, so a modification persisted after the bulk scan captured its (older) settings was silently lost from the serving cache for the rest of the manager's lifetime, while storage kept the new values. Split the reserved marker into two kinds: reservedPlaceholder (settings and state both synthetic, e.g. the pre-inserted default group), which the merge may still replace wholesale, and reservedStateOnly (settings confirmed from storage, state missing), for which the merge now adopts only the scanned state into the existing entry and keeps its settings. Add a regression test that captures the bulk scan before a Modify, fails the lazy load's state reads, modifies the group, then releases the merge and asserts the modified settings survive while the scanned state is adopted (verified to fail without the fix). Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
…elete races Two fixes for the async loading path: initDefaultResourceGroup bailed out whenever any cache entry existed for the default group, including the synthetic placeholder pre-inserted by initReservedInCache. On a fresh store, the confirmed-not-found fallback therefore never created or persisted the default group, and the entry stayed an unconfirmed placeholder for the manager lifetime: its settings were never stored and the persist loop permanently skipped its token and consumption state. Treat a reservedPlaceholder entry as absent (synthesize and persist), while still returning early for confirmed entries and reservedStateOnly ones, whose real settings must not be overwritten with synthetic values. The per-keyspace deleteGen is shared by every group, so deleting group B while group A was being lazily loaded discarded A's valid result and made its request spuriously report the group as missing. Retry the storage read (up to 3 attempts) on a generation mismatch: the re-read observes post-delete storage, so an unrelated deletion just reloads successfully, and a deletion of the group itself now correctly reports not-found instead of silently returning nothing. Add regression tests for both (each verified to fail without its fix). Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
SetStatesIntoResourceGroup wrote the token bucket fields (Tokens, LastUpdate, Initialized) via setState with no lock, while RequestRU mutates the same fields under the group lock. Both the async bulk merge's state adoption and the metadata watcher's runtime state sync call SetStatesIntoResourceGroup on groups that are already serving token requests, racing with them. Take the group lock around setState; UpdateRUConsumption already locks internally. The lock order stays keyspace-manager lock -> group lock, consistent with every other path (resource_group.go and token_buckets.go never reference the keyspace manager). Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
Signed-off-by: tongjian <1045931706@qq.com>
2de53f9 to
f903e37
Compare
| } | ||
| delete(krgm.reservedGroups, name) | ||
| krgm.Unlock() | ||
| if m.syncLoadedGroups != nil { |
There was a problem hiding this comment.
This lazy load can belong to a previous term: krgm was captured before the storage read, but Init may replace m.krgms and syncLoadedGroups while that read is blocked. The stale call then inserts into a detached manager and marks the group in the new term, causing the new bulk merge to skip a group that is absent from its cache. Please capture and validate loadEpoch and the current krgm under m.Lock before publishing and marking, and retry when either changed.
There was a problem hiding this comment.
Fixed in 7fe5201f. Each load attempt now captures the load epoch and the current keyspace manager atomically under the manager lock, and re-validates both inside the same critical section that publishes the cache entry and the sync-loaded marker (and before the confirmed-not-found default synthesis). On a mismatch the attempt retries against the freshly captured state, so a request straddling a re-election still succeeds against the new term instead of poisoning its marker map. markResourceGroupSyncLoaded now also takes the mutated keyspace manager and skips marking when it is no longer the live one, closing the same window for the Add/Modify/Delete and watcher paths.
Added TestAsyncLoadResourceGroupsStaleLazyLoadRetriesNewTerm: a term-1 lazy load is paused inside its storage read, the manager is reinitialized for term 2, then the load is released and must retry and publish into the new term (fails without the fix).
The async loader retries indefinitely until the scan succeeds, so a failed attempt is not a terminal condition and warn level fits better. This also satisfies the error-log-review check on new error-level logs. Signed-off-by: tongjian <1045931706@qq.com>
A lazy load captured its keyspace manager before the lock-free storage read. If the read blocked across a leadership change, Init replaced m.krgms and syncLoadedGroups in the meantime, so the resumed call inserted the group into a detached manager while marking it in the new term's map - making the new bulk merge skip a group its cache doesn't contain, leaving it unavailable for the whole term. Capture the load epoch and the current keyspace manager atomically under the manager lock at the start of every load attempt, and re-validate both in the same critical section that publishes the cache entry and the sync-loaded marker (and before the confirmed-not-found default synthesis). On a mismatch the load retries against the freshly captured state, so a request that straddles a re-election still succeeds against the new term. markResourceGroupSyncLoaded now also takes the mutated keyspace manager and skips marking when it is no longer the live one, closing the same window for Add/Modify/Delete and the watcher paths. Add a regression test that pauses a term-1 lazy load inside its storage read, reinitializes the manager for term 2, then releases the load and asserts it retries and publishes into the new term (verified to fail without the fix). Signed-off-by: tongjian <1045931706@qq.com>
| func (m *Manager) markResourceGroupSyncLoaded(keyspaceID uint32, krgm *keyspaceResourceGroupManager, name string) { | ||
| m.Lock() | ||
| defer m.Unlock() | ||
| if m.krgms[keyspaceID] != krgm { |
There was a problem hiding this comment.
This only skips the marker after krgm becomes detached; the mutation has already run against that old manager. For example, a cross-term Delete can remove storage while the new loader merges its pre-delete snapshot, so the API returns success but the group remains in the live cache. Please revalidate and retry the whole mutation against the current manager.
There was a problem hiding this comment.
Fixed in 017899a7. Each mutation is now split into a storage phase (idempotent, done once) and a publish phase: publishResourceGroupMutation re-resolves the current keyspace manager inside one manager-lock critical section and applies the cache effect and the sync-loaded marker there atomically. Since the bulk merge holds the manager lock across its whole merge step, the two can only be fully ordered — publish first and the merge skips the marked group; merge first and the publish overrides its stale snapshot. Because the current manager is resolved inside the critical section, a mutation straddling a leadership change publishes into the live term by construction, so no revalidate-and-retry loop is needed (that was the intent of your suggestion, achieved without the retry). Your Delete example is covered: the removal and the marker now land in the new term, so the new merge skips its pre-deletion snapshot.
Added TestAsyncLoadResourceGroupsCrossTermDeletePublishesToNewTerm, which parks a Delete before its storage phase via a failpoint, re-initializes the manager for term 2 with its loader holding a pre-deletion snapshot, then resumes the Delete and asserts the group is not resurrected (verified to fail with the previous skip-marker approach).
| krgm.Unlock() | ||
| m.Unlock() | ||
| if attempt >= maxLoadAttempts { | ||
| return nil |
There was a problem hiding this comment.
Returning nil after the third generation or epoch conflict reports a successful load without publishing the group. Callers can then return (nil, nil) or silently skip token acquisition for an existing group. Please return a retry/loading error, or keep retrying, instead.
There was a problem hiding this comment.
Fixed in 017899a7. All exhausted-retry paths in loadResourceGroupIfNeeded now return ErrResourceGroupsLoading instead of nil, so a load that keeps losing the generation/epoch race surfaces a retryable error rather than a bogus success that callers would misread as the group not existing (or silently skip token acquisition for).
Added TestAsyncLoadResourceGroupsExhaustedRetriesReturnLoadingError, which deterministically defeats all three attempts with unrelated deletes timed inside each read window and asserts the caller gets ErrResourceGroupsLoading (fails with the previous return nil).
A metadata mutation (Add/Modify/Delete) applied its cache effect to the keyspace manager captured before its storage I/O and recorded the sync-loaded marker in a separate lock section. Skipping the marker when that manager turned out detached (previous fix) was not enough: the mutation had already run against the old manager, so e.g. a Delete straddling a leadership change removed the group from storage while the new term's bulk merge reinstalled its pre-deletion snapshot - the API reported success but the group stayed in the live cache. Split each mutation into a storage phase (idempotent, done once) and a publish phase: publishResourceGroupMutation re-resolves the current keyspace manager inside one manager-lock critical section and applies the cache effect and the sync-loaded marker there atomically. Since the bulk merge holds the manager lock across its whole merge step, the two can only be fully ordered - publish first and the merge skips the marked group, merge first and the publish overrides its stale snapshot. Because the current manager is resolved inside the critical section, a mutation straddling a leadership change publishes into the live term by construction, with no retry loop. Modify keeps its reserved-placeholder marking gate, and republishes its patched settings if the merge installed a pre-modification snapshot in between. Also make the lazy load's exhausted-retry paths return ErrResourceGroupsLoading instead of nil: reporting success without publishing the group made callers surface an existing group as nonexistent or silently skip token acquisition for it. Add deterministic regression tests for both (each verified to fail without its fix), and make the test storage's state-read pause hook re-armable and filtered by group name. Signed-off-by: tongjian <1045931706@qq.com>
| // state. If the state still hasn't been confirmed (reserved), marking | ||
| // it sync-loaded would make the async bulk merge skip it forever, so | ||
| // the persisted running state would never get applied. | ||
| _, reserved := cur.reservedGroups[grouppb.Name] |
There was a problem hiding this comment.
When this is the new term’s reserved default placeholder, keeping it reserved lets the bulk merge replace the just-patched settings with its pre-modification snapshot. Modify then returns success and storage has the new value, but the live cache reverts to the old settings. Please preserve the modified settings when completing the placeholder’s state, or publish a confirmed group and mark it.
There was a problem hiding this comment.
Fixed in 58186a03. Since reservedStateOnly was removed, reservedGroups only ever holds the synthetic default placeholder, so the old !reserved gate is obsolete — a Modify that persisted settings produced confirmed data yet was left reserved and unmarked, letting the merge (or initReserved re-synthesizing) revert it. The Modify publish now unconditionally clears the reserved marker and records the group as sync-loaded (your "publish a confirmed group and mark it" option); nothing pending is skipped because there is no state-only reserved kind anymore.
Added TestAsyncLoadResourceGroupsCrossTermModifyDefaultStaysConfirmed, which stalls a Modify of the default before publishing via a failpoint, reinitializes the manager for a new term (fresh reserved placeholder), then resumes the publish and asserts the default is confirmed (not reserved) with the modified fill rate surviving — verified to fail with the previous !reserved behavior.
…rved After reservedStateOnly was removed, reservedGroups only ever holds the synthetic default placeholder. The Modify publish path still gated its sync-loaded marker on !reserved and never cleared the marker, so a Modify of a reserved default placeholder (e.g. the fresh placeholder a new term installs while the Modify's publish straddled the leadership change) persisted the new settings but left the entry a reserved, unmarked placeholder. The bulk merge could then revert it to a pre-modification snapshot, or initReserved could re-synthesize a fresh default over it, silently dropping the modified settings from the serving cache while storage kept the new value. Since a persisted-settings group is confirmed data, clear the reserved marker and record it as sync-loaded on every Modify publish. There is no state-only reserved kind anymore, so nothing pending is skipped by marking. Add a cross-term regression test that stalls a Modify of the default before publishing, reinitializes the manager for a new term, then resumes the publish and asserts the default is confirmed with the modified settings (verified to fail without the fix). Signed-off-by: tongjian <1045931706@qq.com>
| // pre-modification snapshot, or initReserved could re-synthesize a | ||
| // fresh default over it, silently dropping the just-modified settings | ||
| // from the serving cache while storage keeps the new value. | ||
| delete(cur.reservedGroups, grouppb.Name) |
There was a problem hiding this comment.
Clearing reserved and marking this group sync-loaded confirms only the patched settings; existing is still the synthetic default placeholder, so its token and consumption state is fresh. The bulk merge now skips the persisted state, and the persist loop can write the synthetic state back to storage. Please restore the confirmed running state before marking, or keep the entry state-unconfirmed until the bulk loader fills it.
There was a problem hiding this comment.
Fixed in 2b7cbefd. You are right — clearing reserved + marking confirmed only the settings, freezing the placeholder's synthetic token/consumption state. The publish now installs the patched group wholesale instead of re-applying the settings patch onto the in-place placeholder. modifyResourceGroup patches an object that loadResourceGroupIfNeeded had already loaded/confirmed, so patched carries both the modified settings and the confirmed running state; installing it replaces the placeholder's fresh state with the real one, so the bulk merge no longer skips a persisted state it dropped and the persist loop no longer writes synthetic state back.
Extended TestAsyncLoadResourceGroupsCrossTermModifyDefaultStaysConfirmed to seed a recognizable persisted running state (RRU 777) and assert it survives the cross-term republish — verified to fail when only the settings patch is re-applied.
… group The previous fix cleared the reserved marker and marked a modified group sync-loaded, but when the current term held a different object (a reserved default placeholder or a bulk-merge snapshot) it only re-applied the settings patch onto that object. The placeholder carries synthetic token/consumption state, so confirming it froze the fresh state: the bulk merge then skipped the persisted state and the persist loop could write the synthetic state back over the real value. Install the patched group wholesale instead of patching the in-place object. modifyResourceGroup patches an object that was loaded/confirmed first, so it carries both the modified settings and the confirmed running state; publishing it replaces the placeholder's synthetic state with the real one, and burstability is synced like any freshly installed group. Extend the cross-term modify-default regression test to seed a recognizable persisted running state and assert it survives the republish (verified to fail when only settings are re-applied). Signed-off-by: tongjian <1045931706@qq.com>
| } | ||
|
|
||
| loaded := 0 | ||
| m.Lock() |
There was a problem hiding this comment.
This holds the global manager lock while merging every loaded group and synchronizing burstability. During loading, point and token requests also need this lock, so a million-group merge can pause all requests and create a large latency spike exactly when async loading completes. Please avoid holding m.Lock across this O(total groups) work, for example by merging in bounded batches, and add a large-scale latency benchmark.
There was a problem hiding this comment.
Fixed in 308788ca. The merge now flattens the loaded groups and installs them in bounded batches (1024), releasing the manager lock between batches and re-validating the load epoch each batch (a term change can land between batches); burstability sync moved outside the manager lock too. Correctness is unchanged — a concurrent write's publish is still atomic under the manager lock and each batch still checks syncLoadedGroups, so publish-before-batch skips the group and batch-before-publish is overridden.
Added BenchmarkAsyncLoadMergeReaderStall, which measures the worst-case manager-read-lock stall of a concurrent reader (probing GetControllerConfig, whose only cost is that lock) during the merge. At 500k groups the worst-case stall drops from ~169ms (single lock over all groups) to ~41ms (bounded batches), and stays bounded as the group count grows instead of scaling with total groups.
The async bulk merge held the manager lock across installing every loaded group and synchronizing its burstability - O(total groups) work. Concurrent point and token requests take the same lock to resolve a keyspace manager, so on a cluster with many resource groups the merge stalled all requests until loading completed, a large latency spike exactly when async loading finished. Flatten the loaded groups and merge them in bounded batches, releasing the manager lock between batches and re-validating the load epoch each batch (a term change can land between batches). Burstability sync now runs outside the manager lock. Correctness is unchanged: a concurrent write's publish is still atomic under the manager lock and each batch still checks syncLoadedGroups, so publish-before-batch skips the group and batch-before-publish is overridden. Add BenchmarkAsyncLoadMergeReaderStall, which measures the worst-case manager-read-lock stall of a concurrent reader during the merge. At 500k groups the worst-case stall drops from ~169ms (single lock over all groups) to ~41ms (bounded batches), and stays bounded as the group count grows. Signed-off-by: tongjian <1045931706@qq.com>
The async-load completion tail set syncLoadedGroups=nil, ran initReserved (which re-resolves keyspace managers and persists synthetic defaults), then published LoadingStateCompleted. Every other shared-state mutation in this file re-checks loadEpoch, but initReserved did not, so a re-election landing between clearing the sync map and publishing completion could let a stale loader synthesize and persist a default into the new term, clobbering a customized default the new term had not loaded yet. Publish completion first via the epoch-guarded storeLoadingStateIfCurrent and only backfill reserved defaults when it succeeds, so a stale loader returns without touching the new term. Keyspaces whose default the loader would have backfilled remain covered: once loading is complete, getOrCreateKeyspaceResourceGroupManager synthesizes the default on demand. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
@bufferflies: The following test 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