resource_group: implement async loading for resource groups - #10873
resource_group: implement async loading for resource groups#10873bufferflies wants to merge 52 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.17% 79.41% +0.24%
==========================================
Files 541 542 +1
Lines 76487 77595 +1108
==========================================
+ Hits 60558 61624 +1066
- Misses 11629 11650 +21
- Partials 4300 4321 +21
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.
…ock when no service limit is set syncBurstabilityWithServiceLimitLocked unconditionally took the group's write lock before checking whether the keyspace even has an active service limit configured, regressing from the previous short-circuit that used only read locks in that case. This function runs on every group insert/update across the system, including the async bulk merge's up-to-500k-group batches (see BenchmarkAsyncLoadMergeReaderStall), so escalating to a write lock in the common no-op case - most keyspaces have no service limit set - added needless contention against a concurrent RequestRU call already holding the same, already-live group's lock. Fixed by checking isSet/serviceLimit (a cheap read of krgm's already-locked state, no extra lock) before taking group's lock, returning immediately when there is nothing to apply. Regression test (TestSyncBurstabilityWithServiceLimitLockedSkipsGroupLockWhenNoServiceLimit) holds the group's lock externally and confirms the call returns without blocking when no service limit is set; verified it fails (blocks until timeout) against the pre-fix code and passes against the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
Quality-only cleanup, no behavior change. Ran a 4-angle simplify pass (reuse/simplification/efficiency/altitude) over the full PR diff and applied the findings that were safe, mechanical, and didn't touch behavior: - Moved WrapLoadingError (pkg/mcs/resourcemanager/server/grpc_service.go) into pkg/errs/errno.go as ErrResourceGroupsLoadingGRPC, matching the existing domain-error-to-gRPC-status pattern already used there (ErrGRPCRateLimitExceeded, ErrNotLeader) instead of a new, differently-located wrapper. Updated all 8 call sites across grpc_service.go and server/resource_group_proxy_service.go. - persistResourceGroupRunningState re-derived the same "present and not a reserved placeholder" check hasConfirmedResourceGroup already encodes. Extracted confirmedResourceGroupLocked (returns the group too, so the persist loop doesn't need a second lock round trip) and had both hasConfirmedResourceGroup and the persist loop use it. - Dropped RLock/RUnlock around reads of tempKrgms in the async merge's flatten step: tempKrgms is a map this goroutine alone constructs and holds - not yet reachable from m.krgms or any other goroutine at that point - so the locking was pure overhead on every keyspace loaded per cycle. - The "look up or create in m.krgms" snippet was duplicated at 4 call sites (3 of which already hold m.Lock() and couldn't call the public getOrCreateKeyspaceResourceGroupManager without double-locking). Extracted getOrCreateKeyspaceResourceGroupManagerLocked and had all 4 sites (SetKeyspaceServiceLimit's mirror step, the merge loop, loadResourceGroupIfNeeded's retry loop, publishResourceGroupMutation) use it. - Same shape for the sync-loaded marker write: extracted markResourceGroupSyncLoadedLocked and had markResourceGroupSyncLoaded, loadResourceGroupIfNeeded, and publishResourceGroupMutation share it instead of each inlining the map write. Findings considered and deliberately not applied: - Deleting krgm.addResourceGroup/deleteResourceGroup as "dead code": both still have ~19 test call sites using them as setup helpers; not dead. - Folding the defaultGroupMu guard duplicated in AddResourceGroup/ ModifyResourceGroup into persistResourceGroup/modifyResourceGroup: would self-deadlock when called from initDefaultResourceGroup, which already holds the same (non-reentrant) defaultGroupMu before calling persistResourceGroup - the duplication at the two call sites is what lets each caller decide for itself whether it already holds the lock. - Folding the duplicated "!enableMetadataWatcher && LoadingStateCompleted" check in getOrCreateKeyspaceResourceGroupManager into loadResourceGroupIfNeeded: the existing comment on that check explicitly documents this split exists to avoid a recursive call between the two functions. - Returning the resolved krgm/group from loadResourceGroupIfNeeded so GetResourceGroup/GetMutableResourceGroup don't re-resolve it in metadata-watcher mode: real double-lookup, but changes a heavily-used function's return contract and its callers - left as a follow-up rather than folded into a no-behavior-change cleanup pass. Full pkg/mcs/resourcemanager/... suite green under -race with failpoints enabled, 2 iterations; go build ./... and go vet clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
| serviceLimit, err := m.storage.LoadServiceLimit(keyspaceID) | ||
| if err != nil { | ||
| log.Warn("failed to reload service limit", zap.Uint32("keyspace-id", keyspaceID), zap.Error(err)) | ||
| return |
There was a problem hiding this comment.
When this point read fails, the callback only logs and returns, so LoadServiceLimits still returns nil and Init starts with the persisted limit absent from the cache. The keyspace then runs with a zero service limit and burstable groups can bypass the configured cap indefinitely because this path has no retry.
There was a problem hiding this comment.
Fixed in b35672d3e. The point re-read now retries a few times, and if it still fails, falls back to the bulk-scanned value (previously ignored via _) instead of dropping the update - so a persistent point-read failure no longer leaves the keyspace with no service limit cached at all. This re-admits the point re-read's own narrow staleness window only in that rare fallback case, which is strictly better than losing the limit entirely until the next Init.
…its' point re-read fails loadServiceLimits re-reads each keyspace's service limit from storage under serviceLimitLocks to avoid applying a bulk-scan snapshot that a concurrent SetKeyspaceServiceLimit might have already made stale. But that point re-read can itself fail transiently, and until now a single failure just logged and dropped the update - leaving the keyspace with no service limit cached at all until the next Init, silently letting burstable groups bypass the configured cap indefinitely. Retry the point read a few times, and on exhausting the budget, fall back to the bulk-scanned value instead of dropping the update. This re-admits the point re-read's own narrow staleness window only in this rare failure case, which is strictly better than losing the limit entirely. Added TestLoadServiceLimitsRetriesPointReadBeforeFallingBack and TestLoadServiceLimitsFallsBackToBulkValueOnPersistentFailure, both verified to fail against the pre-fix code. Full pkg/mcs/resourcemanager/... suite green under -race with failpoints enabled; go build/vet and gofmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
| // groups to bypass it) until the next Init. | ||
| log.Error("giving up reloading service limit, falling back to the bulk-scanned value", | ||
| zap.Uint32("keyspace-id", keyspaceID), zap.Error(err)) | ||
| serviceLimit = bulkScannedLimit |
There was a problem hiding this comment.
If a concurrent SetKeyspaceServiceLimit publishes a newer value after the bulk scan but before this callback acquires serviceLimitLocks, and the subsequent point reads all fail, this fallback reapplies the stale bulk value over the live cache while storage retains the newer limit. The cache then remains stale until another Init or setter, so a lowered limit can be exceeded indefinitely.
There was a problem hiding this comment.
Fixed in 422a3cf8a. The callback now checks whether the keyspace's cache already has a service limit before applying the fallback: SetKeyspaceServiceLimit holds this same serviceLimitLocks entry across both its storage write and its cache mirror, so there's no partially-applied state visible to this callback - by the time it holds the lock, a concurrent call has either fully landed (cache already reflects a strictly newer value) or hasn't started yet. If the cache already carries such a value, the fallback is skipped and it's left untouched.
One narrower gap is left as a documented TODO rather than fixed: a service limit has no flag separate from its value, so a concurrent SetKeyspaceServiceLimit(id, 0) landing in the same window is indistinguishable here from "never configured," and would still get overwritten by the stale fallback. That requires the point read to keep failing for the whole retry budget on top of that exact interleaving, so it's left documented in the code rather than closed here.
…rrently-set newer value loadServiceLimits' fallback to the bulk-scanned value on a persistent point-read failure (b35672d) could overwrite a value a concurrent SetKeyspaceServiceLimit had already fully committed - both storage and cache - for the same keyspace before the callback ever acquired serviceLimitLocks. SetKeyspaceServiceLimit holds that same per-keyspace lock across its entire storage-write-then-cache-mirror sequence, so by the time the callback holds the lock, a concurrent call has either fully landed or hasn't started - there's no partial state to race against. Check the cache for an already-newer value before applying the fallback, and leave it untouched instead of overwriting it. Known, narrower gap left undocumented as a TODO only, not fixed: since a service limit has no flag separate from its value, a concurrent SetKeyspaceServiceLimit(id, 0) landing in the same window is indistinguishable here from "never configured" and would still get overwritten by the stale fallback. Left as a documented residual gap per user decision - it additionally requires the point read to keep failing for the whole retry budget on top of that exact interleaving. Added TestLoadServiceLimitsDoesNotClobberConcurrentSetOnPersistentPointReadFailure, verified to fail against the pre-fix code. Full pkg/mcs/resourcemanager/... suite green under -race with failpoints enabled; go build/vet and gofmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
…e cross-term collision publishResourceGroupMutation's confirmed-write guard silently drops a mutation's cache effect when a newer term already has confirmed data for the same group. If the mutation's storage write still lands after that guard was checked, storage ends up mutated but the cache is stuck on stale data indefinitely - the tikv#11105 interleaving. For the specific case where the mutation was parked *before* its storage-write phase even started (not while a write was already in flight - that still needs tikv#11105's CAS work), this is preventable: re-check whether the current term already has confirmed data for the target group right before the write, and abort with a retryable error instead of writing on behalf of a result that's going to be dropped anyway. Deliberately narrower than a plain "did the term change" check: a term change alone with nothing yet confirmed for this group is harmless and already handled gracefully by publishResourceGroupMutation re-resolving the current manager - only abort when there's an actual confirmed collision to race against (hasNewerConfirmedWrite). Verified this distinction against TestAsyncLoadResourceGroupsCrossTermDeletePublishesToNewTerm and TestAsyncLoadResourceGroupsCrossTermModifyDefaultStaysConfirmed, whose "harmless term change, mutation should still succeed" scenarios must keep passing unchanged. Added new addResourceGroupBeforeStorage/modifyResourceGroupBeforeStorage failpoints (Delete reuses the existing deleteResourceGroupBeforeStorage) and three regression tests, one per mutation, each verified to fail against the pre-fix code by reproducing the tikv#11105 shape directly (mutation returns success while its cache effect is silently dropped). Also updated tikv#11105 with the initDefaultResourceGroup in-flight window as an explicit tracked item, per review discussion. Full pkg/mcs/resourcemanager/... suite green under -race with failpoints enabled, 2 iterations; go build/vet and gofmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: bufferflies <1045931706@qq.com>
YuhaoZhang00
left a comment
YuhaoZhang00
left a comment
There was a problem hiding this comment.
doc nits: the "Known gap" comment on publishResourceGroupMutation still calls the parked-Delete case open and untested - now outdated.
|
@YuhaoZhang00: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: disksing, YuhaoZhang00 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 |
| return false, errs.ErrResourceGroupsLoading | ||
| } | ||
| defaultGroup := newDefaultResourceGroup() | ||
| group, err := krgm.persistResourceGroup(defaultGroup.IntoProtoResourceGroup(krgm.keyspaceID)) |
There was a problem hiding this comment.
In watcher mode, the confirmed not-found read above is not fenced against a concurrent PD metadata write. If PD persists a customized default before this synthetic write, defaultGroupMu cannot coordinate with that writer, so this call can overwrite the successful metadata update in storage and then publish the synthetic group to the cache.
There was a problem hiding this comment.
Good catch, and you're right this isn't fenced. Confirmed: the independent RM service runs with writeRole=LegacyAll and enableMetadataWatcher=true at the same time (server.go), and ShouldRejectMetadataWritesViaGRPC only gates the RM's own gRPC surface - it doesn't cover this internal persistResourceGroup call, which still does a real, unconditional Put. So this can genuinely overwrite PD's write in storage itself, not just in cache, with no later resync to recover it.
Tracked as a new scope note in #11105 (#11105 (comment)), alongside the other cross-term interleavings there - all of them ultimately need the same storage-side conditional (CAS) write to close for good. Deferring the actual fix to that follow-up rather than bolting a narrower mitigation onto this PR, since a real fix here means a new storage primitive (CreateResourceGroupSettingIfAbsent or similar), not just an in-memory check like the term-change guards added earlier in this PR.
| // can end up saving a different config object than the one just mutated | ||
| // above. Matches the pattern initControllerConfig itself already uses - | ||
| // save a locally captured reference, never the field. | ||
| controllerConfig := m.controllerConfig |
There was a problem hiding this comment.
This only captures the same mutable config pointer, not a snapshot. Another SetKeyspaceRUVersion can mutate RUVersionPolicy.Overrides while the save marshals it, and UpdateControllerConfigItem can persist a newer clone that this delayed save later overwrites, causing a map race or lost persisted configuration.
There was a problem hiding this comment.
Confirmed and fixed in e26e74150. RUVersionPolicy.Overrides was a live shared map, not a snapshot, so the unlocked SaveControllerConfig marshal here could run concurrently with another SetKeyspaceRUVersion's locked mutation of the same map - a real concurrent map read/write. And because this call unlocked before saving while UpdateControllerConfigItem saves fully inside its lock, the two weren't ordered against each other, so a delayed save here could persist a stale captured object over a newer write UpdateControllerConfigItem had already committed.
Fix makes this function clone (cloneControllerConfig, reusing what UpdateControllerConfigItem already uses) and hold m.Lock() across the whole mutate-save-publish sequence, matching UpdateControllerConfigItem's existing pattern exactly. Added a regression test (TestSetKeyspaceRUVersionSerializesAgainstUpdateControllerConfigItem) that parks this call right before its save via a new failpoint and asserts a concurrent UpdateControllerConfigItem blocks rather than interleaving, and that neither call's change is lost once both complete - verified it fails against the pre-fix code before landing it.
SetKeyspaceRUVersion mutated the live m.controllerConfig.RUVersionPolicy object in place and only called storage.SaveControllerConfig after unlocking. That left RUVersionPolicy.Overrides - a plain map - both mutable by a concurrent SetKeyspaceRUVersion call and readable by this call's own unlocked marshal (an unsynchronized concurrent map read/write), and let this call's save land out of order relative to UpdateControllerConfigItem's, which clones, saves, and publishes fully inside its own lock - a save delayed past unlock here could silently overwrite a newer config UpdateControllerConfigItem had already committed. Fix it to match UpdateControllerConfigItem's existing pattern: clone before mutating, and hold the lock across save + publish so the two config mutators are fully serialized against each other, not just consistent with the in-memory state at capture time. Adds a setKeyspaceRUVersionBeforeSave failpoint and a regression test proving UpdateControllerConfigItem blocks while SetKeyspaceRUVersion is parked before its save, and that neither call's change is lost once both complete. Verified against pre-fix code (temporarily reverted the production change, kept the test) to confirm it actually catches the bug. Reported by rleungx: tikv#10873 (comment)
initControllerConfig read the current config under RLock, unmarshaled storage's value into a clone, saved that clone back to storage while unlocked, and only then published it under Lock. Save-before-publish ordering avoided one specific race (a concurrent mutator marshaling the not-yet-cloned live object), but the unlocked save itself was never mutually exclusive with SetKeyspaceRUVersion or UpdateControllerConfigItem, which now both save+publish fully inside their own lock: either could commit a newer config to storage and m.controllerConfig in the window between this call's read and its own save, and this call's now-stale snapshot would silently overwrite it - the same class of lost update just fixed for SetKeyspaceRUVersion. Fold the whole clone-merge-save-publish sequence into one m.Lock() critical section, same as the other two config mutators. Requests are gated behind IsServing(), which only flips true after Init (and thus this call) returns, so this does not add contention with live request traffic in the common case.
…write tests TestDeleteResourceGroupAbortsOnNewerConfirmedWriteAcrossTermChange, TestAddResourceGroupAbortsOnNewerConfirmedWriteAcrossTermChange, and TestModifyResourceGroupAbortsOnNewerConfirmedWriteAcrossTermChange were near-identical: same setup, same park-before-storage/term-change/resume choreography, same final assertions - differing only in which API is called, which failpoint parks it, and the group name. Consolidate into TestResourceGroupMutationAbortsOnNewerConfirmedWriteAcrossTermChange, a single table-driven test with one shared harness and a small per-mutation-kind table entry. Verified each subtest still independently catches its corresponding regression: temporarily disabling DeleteResourceGroup's hasNewerConfirmedWrite guard fails only the "delete" subtest, leaving "add" and "modify" green. No production code change.
|
@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. |
| // GetResourceGroupList returns copies of resource group list. | ||
| // Returns error if resource groups are still being loaded asynchronously. | ||
| func (m *Manager) GetResourceGroupList(keyspaceID uint32, withStats bool) ([]*ResourceGroup, error) { | ||
| if !m.isResourceGroupLoadingComplete() { |
There was a problem hiding this comment.
The loading-state check is not atomic with the krgms snapshot. If a request observes the previous term's LoadingStateCompleted and Init resets krgms before this lookup, the list can return a partially rebuilt map instead of ErrResourceGroupsLoading; the same check/use split in loadResourceGroupIfNeeded can surface false not-found results during leadership churn.
| // only flips true after Init (and thus this call) returns, so holding | ||
| // the lock across the save here does not add contention with live | ||
| // request traffic in the common case. | ||
| m.Lock() |
There was a problem hiding this comment.
LoadControllerConfig still happens before this lock. If a concurrent SetKeyspaceRUVersion or UpdateControllerConfigItem commits after that read but before this line, unmarshalling the stale snapshot can restore old values and SaveControllerConfig then overwrites the newer persisted config, so the new serialization still permits a lost update during leadership churn.
| cur := m.getOrCreateKeyspaceResourceGroupManagerLocked(keyspaceID) | ||
| m.Unlock() | ||
| if cur != krgm { | ||
| cur.setServiceLimitFromStorage(serviceLimit) |
There was a problem hiding this comment.
setServiceLimit only logs SaveServiceLimit failures and returns no error, so this branch can mirror a value that never reached storage into the new term's live cache and still return success. A retry with the same value can then short-circuit on the cached value, leaving enforcement inconsistent with storage until another reload.
4 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