resource_group: implement async loading for resource groups#10873
resource_group: implement async loading for resource groups#10873bufferflies wants to merge 20 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.23%
========================================
Files 541 541
Lines 75965 76197 +232
========================================
+ Hits 60187 60374 +187
- Misses 11531 11567 +36
- Partials 4247 4256 +9
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.
…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>
|
Skipping reserved placeholders in the persistence loop fixes the overwrite path, but the other two correctness issues remain. When The point setting/state loaders also still lack the legacy null-keyspace fallback used by the bulk loaders, so lazy access during startup can incorrectly treat a legacy group as missing. Please make the point and bulk loading paths compatible and add legacy-data upgrade coverage. The latest commit does not add tests for these paths. |
getOrCreateKeyspaceResourceGroupManager(id, true) is used by both AddResourceGroup and SetKeyspaceServiceLimit to make sure a keyspace's default resource group exists. It called initDefaultResourceGroup directly, which persists a synthetic default whenever one isn't cached yet, with no attempt to check storage first. For a keyspace touched for the first time while async loading is still in progress, this could overwrite a customized default group's stored settings before the real data had a chance to load - the same class of bug fixed earlier in loadResourceGroupIfNeeded, reachable via two more entry points. Make initDefault=true go through loadResourceGroupIfNeeded (storage point load first, synthesize only on confirmed not-found) while async loading is in progress. Once loading has completed, any default still missing from the cache is confirmed absent from storage, so it's synthesized directly as before. loadResourceGroupIfNeeded's own not-found fallback now calls initDefaultResourceGroup directly instead of going through getOrCreateKeyspaceResourceGroupManager(id, true), to avoid recursing back into itself. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
krgm.modifyResourceGroup unconditionally cleared reservedGroups after patching settings, and Manager.ModifyResourceGroup unconditionally called markResourceGroupSyncLoaded afterwards. Modify only patches settings, it never loads or establishes a group's state, so if the preceding lazy load had failed to read the persisted state (stateLoaded=false), a Modify call would incorrectly promote the entry to fully confirmed at both the keyspace-manager level (reservedGroups) and the manager level (syncLoadedGroups). The async bulk merge would then skip it forever, so the real persisted running state would never get applied and the cached token bucket could remain initialized with empty state indefinitely. Leave reservedGroups untouched in modifyResourceGroup, and only mark the group sync-loaded in ModifyResourceGroup when it isn't still reserved. This lets the existing retry-on-next-access path (and the async bulk merge) keep trying to fill in the real state instead of the settings-only patch being treated as a full confirmation. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
Review feedback questioned whether the point loaders (LoadResourceGroupSetting/LoadResourceGroupState) miss the legacy, pre-keyspace resource_group/* path that the bulk loaders fall back to for constant.NullKeyspaceID. They don't: KeyspaceResourceGroupSettingPath and KeyspaceResourceGroupStatePath already resolve to the legacy path for NullKeyspaceID, the same helper both point and bulk loaders use. Add a regression test exercising the actual lazy-load path (GetResourceGroup during blocked async loading) against a legacy group saved under NullKeyspaceID, to make this guarantee explicit and catch any future divergence between the point and bulk loaders. Signed-off-by: tongjian <doufuxiaowangzi@gmail.com> Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/mcs/resourcemanager/server/manager_test.go (1)
497-501: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent potential test deadlocks by using a read timeout.
Reading directly from a channel can hang the test runner indefinitely if
dispatchConsumptionever fails to send an item due to a bug or future regression. Wrapping the read in aselectblock with a timeout ensures the test fails gracefully and quickly.💡 Proposed fix to add a read timeout
- item := <-m.consumptionDispatcher - re.Equal(uint32(42), item.keyspaceID) - re.Equal(req.GetResourceGroupName(), item.resourceGroupName) - re.Equal(req.GetConsumptionSinceLastRequest(), item.Consumption) + select { + case item := <-m.consumptionDispatcher: + re.Equal(uint32(42), item.keyspaceID) + re.Equal(req.GetResourceGroupName(), item.resourceGroupName) + re.Equal(req.GetConsumptionSinceLastRequest(), item.Consumption) + case <-time.After(5 * time.Second): + re.FailNow("timeout waiting for consumption dispatcher item") + }🤖 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/manager_test.go` around lines 497 - 501, Update the channel read in the consumption dispatch test around consumptionDispatcher to use a select with a bounded timeout, failing the test clearly if no item arrives; preserve the existing assertions for keyspaceID, resourceGroupName, and Consumption after receiving the item.
🤖 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/manager_test.go`:
- Around line 497-501: Update the channel read in the consumption dispatch test
around consumptionDispatcher to use a select with a bounded timeout, failing the
test clearly if no item arrives; preserve the existing assertions for
keyspaceID, resourceGroupName, and Consumption after receiving the item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 396cd0f7-46db-4e91-9be2-bc164f1576a3
📒 Files selected for processing (7)
pkg/mcs/resourcemanager/server/grpc_service.gopkg/mcs/resourcemanager/server/keyspace_manager.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.gopkg/mcs/resourcemanager/server/manager_test.gopkg/mcs/resourcemanager/server/metrics.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/mcs/resourcemanager/server/metrics.go
- tests/integrations/mcs/resourcemanager/resource_manager_test.go
- pkg/mcs/resourcemanager/server/manager.go
|
Re: the point-loader legacy-path concern — I dug into this and don't think it's a bug.
Added |
|
You're right about the legacy path: both point loaders already use the same The state-read issue still blocks approval, though. There is also a follow-on issue in the recovery path: when the async bulk merge later installs the fully loaded group, it replaces |
…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>
|
Thanks, The state-read blocker remains, though. Please also extend the regression to use a distinctive persisted state, assert Get/Acquire/Modify failure behavior, verify the recovered in-memory state, and then prove the persist loop writes state again after recovery. Clearing the marker alone does not cover the full failure → recovery → persistence contract. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: disksing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
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>
| // This calls initDefaultResourceGroup directly instead of going | ||
| // through getOrCreateKeyspaceResourceGroupManager(id, true), which | ||
| // now routes back into this same function and would recurse. | ||
| krgm.initDefaultResourceGroup() |
There was a problem hiding this comment.
On a fresh store, initReservedInCache has already inserted default, so this fallback calls initDefaultResourceGroup and immediately returns on its cache-exists check. With no stored default for the bulk scan to replace, the entry stays reserved for the manager lifetime: its settings are never created and the persist loop never saves its token or consumption state.
There was a problem hiding this comment.
Fixed in f3a60698. initDefaultResourceGroup now treats a reservedPlaceholder entry as absent: on the confirmed-not-found fallback (and in the post-merge initReserved) it synthesizes and persists the default group via addResourceGroup, which also clears the reserved marker, so the persist loop covers its state from then on. A reservedStateOnly entry (real settings, missing state) still returns early so confirmed settings are never overwritten with synthetic ones.
Added TestAsyncLoadResourceGroupsFreshStoreDefaultPersisted: on a completely fresh store, a lazy Get of default during loading must leave it confirmed (not reserved) with its settings persisted to storage (fails without the fix).
|
|
||
| atomic.StoreInt32(&m.loadingState, LoadingStateInProgress) | ||
| startTime := time.Now() | ||
| tempKrgms, err := m.loadKeyspaceResourceGroupsFromStorage() |
There was a problem hiding this comment.
If this context is canceled while either range scan is blocked, the goroutine does not observe cancellation again before merging. Init can be invoked again on the same manager after another leadership win, so an old loader can merge into the new term's maps, clear its syncLoadedGroups, and publish LoadingStateCompleted while the new loader is still running.
There was a problem hiding this comment.
Fixed in a4e1729b. Added a loadEpoch counter bumped by initMetadata under the manager lock; the loader captures it at start and re-verifies it inside the same critical section as every shared-state mutation (the loading-state transitions and the merge), so a loader from a previous term exits instead of merging stale data or publishing completion for a newer term. Also re-check ctx right after the scans return, and made initControllerConfig publish the config via clone-and-swap under the lock since re-initialization can race with the previous term's background goroutines still reading it.
Added TestAsyncLoadResourceGroupsStaleLoaderDoesNotPolluteNewTerm, which blocks a term-1 loader in its states scan, reinitializes for term 2, deletes the group, then releases the stale loader and asserts nothing is resurrected (fails without the fix).
| for name, group := range tempKrgm.groups { | ||
| key := trackerKey{keyspaceID: keyspaceID, groupName: name} | ||
| if !m.syncLoadedGroups[key] { | ||
| krgm.groups[name] = group |
There was a problem hiding this comment.
When a point load has read the settings but failed to read the state, ModifyResourceGroup intentionally leaves the group out of syncLoadedGroups. If the bulk loader captured the old settings before that Modify, this assignment replaces the cache entry with the stale bulk copy while recovering the state, so the successful settings update is lost from the serving cache for the rest of this manager lifetime.
There was a problem hiding this comment.
Fixed in 1bd5ac8f. The reserved marker is now split into two kinds: reservedPlaceholder (settings and state both synthetic, e.g. the pre-inserted default) which the merge may still replace wholesale, and reservedStateOnly (settings confirmed from storage, state missing). For a reservedStateOnly entry the bulk merge now adopts only the scanned state into the existing cached group (SetStatesIntoResourceGroup) and keeps its settings, so a modification persisted after the scan captured its older copy is no longer clobbered, while the state recovery still happens and the marker is cleared.
Added TestAsyncLoadResourceGroupsMergeKeepsModifiedSettings: it holds the bulk loader between its settings and states scans, lazily loads the group with failing state reads, modifies it (fill rate 100 → 200), then releases the merge and asserts the modified settings survive while the scanned state (a recognizable consumption value) is adopted. Verified the test fails without the fix.
| return m.loadServiceLimits() | ||
| inserted := false | ||
| krgm.Lock() | ||
| if krgm.deleteGen != deleteGen { |
There was a problem hiding this comment.
deleteGen is shared by every group in the keyspace, so deleting group B while group A is being lazily loaded discards A's valid result here. A Get, Modify, or Delete of A can then report it as nonexistent even though only B changed, making unrelated startup-time deletions cause spurious request failures.
There was a problem hiding this comment.
Fixed in f3a60698. On a generation mismatch the lazy load now retries its storage read (up to 3 attempts) instead of dropping the result: the re-read observes post-delete storage, so an unrelated deletion just reloads the group successfully, and a deletion of the group itself now correctly reports not-found. Only a continuous stream of deletes racing every attempt would give up without inserting, which stays safe (a later request or the bulk merge reloads).
Added TestAsyncLoadResourceGroupsUnrelatedDeleteDoesNotFailLazyLoad: group A's lazy load is paused mid-read, group B is deleted, and A's request must still succeed (fails without the retry).
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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/mcs/resourcemanager/server/manager_async_test.go (1)
364-423: 📐 Maintainability & Code Quality | 🔵 TrivialTest coverage gap vs. its own docstring; sleep-based synchronization.
The docstring says the stale loader "must not merge its stale scan into the new term's maps, clear the new term's syncLoadedGroups, or publish completion", but the assertions (Lines 415-422) only check for merge/resurrection. The "clear syncLoadedGroups" and "publish completion" claims aren't verified — consider asserting on
m.syncLoadedGroupsstate (or loadingState) after the stale loader is released to actually cover what the comment promises.Separately, Line 413's
time.Sleep(200 * time.Millisecond)is used to let the released stale loader finish before the negative assertions run; this is inherently racy under CI load (a slow scheduler could still be mid-merge when assertions fire, causing intermittent false negatives that mask a real bug rather than a flaky pass).🤖 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/manager_async_test.go` around lines 364 - 423, The stale-loader test only verifies that the deleted group is not resurrected and relies on a racy sleep. Update TestAsyncLoadResourceGroupsStaleLoaderDoesNotPolluteNewTerm to wait deterministically for the released stale loader’s completion, then assert the new term’s syncLoadedGroups/loadingState remains correct and no stale completion is published, while preserving the existing non-resurrection assertions.
🤖 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/manager_async_test.go`:
- Around line 364-423: The stale-loader test only verifies that the deleted
group is not resurrected and relies on a racy sleep. Update
TestAsyncLoadResourceGroupsStaleLoaderDoesNotPolluteNewTerm to wait
deterministically for the released stale loader’s completion, then assert the
new term’s syncLoadedGroups/loadingState remains correct and no stale completion
is published, while preserving the existing non-resurrection assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7371d44-8f9c-4fa5-988d-cfcb3a68d695
📒 Files selected for processing (2)
pkg/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
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>
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/manager.go`:
- Line 724: Update loadResourceGroupIfNeeded and the reservedGroups handling so
a persisted state-read failure is propagated or the group remains unavailable
until recovered state is installed; do not return success or expose
reservedStateOnly as usable token-bucket state. Preserve eventual bulk recovery,
return the original error up-stack, and use errors.Is/As for sentinel checks
with deferred cleanup on all exits.
🪄 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: 19489089-01e9-450c-b731-c452a8f82b77
📒 Files selected for processing (3)
pkg/mcs/resourcemanager/server/keyspace_manager.gopkg/mcs/resourcemanager/server/manager.gopkg/mcs/resourcemanager/server/manager_async_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/mcs/resourcemanager/server/manager_async_test.go
- pkg/mcs/resourcemanager/server/keyspace_manager.go
…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>
|
@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