Repository navigation
crossks: add runtime handles and support aquire/release - #68890
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
@D3Hunter I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XL
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds KSRuntime and KSRuntimeHandle interfaces; refactors cross-keyspace Manager to runtimeEntry-based Acquire/Release with per-holder bookkeeping and idempotent handles; exposes Domain.AcquireKSRuntime; updates Bazel tests; and adds unit and integration tests validating handle behavior. ChangesCross-keyspace runtime acquisition system
Sequence DiagramsequenceDiagram
participant Domain
participant Manager
participant runtimeEntry
participant SessionManager
participant runtimeHandle
Domain->>Manager: Acquire(targetKS, holderID)
Manager->>Manager: getOrCreateEntryWithoutLock(targetKS)
Manager->>runtimeEntry: add/check activeHolders(holderID)
alt duplicate holder
Manager-->>Domain: error
else success
Manager->>runtimeHandle: create(handle -> runtimeEntry)
runtimeHandle->>SessionManager: delegate Store()/SessPool()
Manager-->>Domain: return runtimeHandle
end
Domain->>runtimeHandle: Store() / SessPool()
runtimeHandle->>SessionManager: return storage / pool
Domain->>runtimeHandle: Release()
runtimeHandle->>Manager: release(holderID)
Manager->>runtimeEntry: Remove holder, set lastReleaseAt if empty
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/domain/crossks/cross_ks_test.go (1)
47-55: 💤 Low valueOptional: surface a previously failed registration to later callers.
erris only assigned inside theOnce, so any caller after the first always observesnil. If the one-timeRegisterever fails, only the first test reports it while every subsequentregisterUnistoresilently passes. Caching the error outside the closure makes the failure deterministic for all callers.♻️ Cache registration result
var registerUnistoreOnce sync.Once +var registerUnistoreErr error func registerUnistore(t *testing.T) { - var err error registerUnistoreOnce.Do(func() { - err = kvstore.Register(config.StoreTypeUniStore, mockstore.EmbedUnistoreDriver{}) + registerUnistoreErr = kvstore.Register(config.StoreTypeUniStore, mockstore.EmbedUnistoreDriver{}) }) - require.NoError(t, err) + require.NoError(t, registerUnistoreErr) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/domain/crossks/cross_ks_test.go` around lines 47 - 55, The one-time registration error is stored in a local variable inside the sync.Once closure so subsequent callers always see nil; declare a package-level error variable (e.g. registerUnistoreErr) and assign it inside registerUnistoreOnce.Do when calling kvstore.Register(config.StoreTypeUniStore, mockstore.EmbedUnistoreDriver{}), then have registerUnistore call require.NoError(t, registerUnistoreErr) so any registration failure is observable by all callers of registerUnistore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/domain/crossks/cross_ks_test.go`:
- Around line 47-55: The one-time registration error is stored in a local
variable inside the sync.Once closure so subsequent callers always see nil;
declare a package-level error variable (e.g. registerUnistoreErr) and assign it
inside registerUnistoreOnce.Do when calling
kvstore.Register(config.StoreTypeUniStore, mockstore.EmbedUnistoreDriver{}),
then have registerUnistore call require.NoError(t, registerUnistoreErr) so any
registration failure is observable by all callers of registerUnistore.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d07ff42-06ff-47fc-899f-a9649cdee94c
📒 Files selected for processing (6)
pkg/domain/crossks/BUILD.bazelpkg/domain/crossks/cross_ks.gopkg/domain/crossks/cross_ks_internal_test.gopkg/domain/crossks/cross_ks_test.gopkg/domain/domain.gopkg/domain/sqlsvrapi/server.go
D3Hunter
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68890 +/- ##
================================================
+ Coverage 76.3138% 76.7276% +0.4137%
================================================
Files 2041 2054 +13
Lines 562941 573044 +10103
================================================
+ Hits 429602 439683 +10081
+ Misses 132425 131534 -891
- Partials 914 1827 +913
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 13
- Inline comments: 12
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (1)
- New AcquireKSRuntime / KSRuntimeHandle surface documents no lifecycle or error contract (pkg/domain/sqlsvrapi/server.go:38, pkg/domain/sqlsvrapi/server.go:30, pkg/domain/sqlsvrapi/server.go:32)
🟡 [Minor] (10)
- runtimeEntry.lastReleaseAt is written but never read outside tests (pkg/domain/crossks/cross_ks.go:56, pkg/domain/crossks/cross_ks.go:336)
bookkeeperparameter name does not convey caller-identity / uniqueness semantics (pkg/domain/crossks/cross_ks.go:125, pkg/domain/sqlsvrapi/server.go:38, pkg/domain/domain.go:304)- crossks now depends on sqlsvrapi only to name Acquire's return type (pkg/domain/crossks/cross_ks.go:127, pkg/domain/crossks/cross_ks.go:33, pkg/domain/crossks/BUILD.bazel:9)
- Acquire creates the runtime under the global write lock, serializing all cross-keyspace handle operations behind blocking store/etcd init (pkg/domain/crossks/cross_ks.go:135, pkg/domain/crossks/cross_ks.go:173, pkg/domain/crossks/cross_ks.go:193, pkg/domain/crossks/cross_ks.go:327)
- New concurrent Acquire/Release reference-counting protocol has no concurrent (race) regression coverage (pkg/domain/crossks/cross_ks.go:142, pkg/domain/crossks/cross_ks.go:192, pkg/domain/crossks/cross_ks.go:326, pkg/domain/crossks/cross_ks_internal_test.go:91)
- Stale-handle identity guard in Manager.release has no rationale comment (pkg/domain/crossks/cross_ks.go:331)
- Acquire/Release handle pair performs no runtime teardown; released cross-keyspace runtimes are never reclaimed (pkg/domain/crossks/cross_ks.go:326, pkg/domain/crossks/cross_ks.go:388, pkg/domain/crossks/cross_ks.go:345, pkg/domain/domain.go:885)
- New runtime reference-counting has no test covering the close-while-acquired and stale-handle release paths (pkg/domain/crossks/cross_ks.go:348, pkg/domain/crossks/cross_ks.go:326, pkg/domain/crossks/cross_ks_internal_test.go:79)
KSRuntimename now denotes two differently-shaped keyspace-runtime types (pkg/domain/sqlsvrapi/server.go:24, tests/realtikvtest/testkit.go:161)- Server now exposes two parallel keyspace-access contracts (tracked Acquire vs untracked GetKS*) over the same runtimeEntry (pkg/domain/sqlsvrapi/server.go:38, pkg/domain/crossks/cross_ks.go:142, pkg/domain/crossks/cross_ks.go:192, pkg/domain/domain.go:885)
🧹 [Nit] (2)
release sync.Oncefield name collides with theRelease()method andManager.release(pkg/domain/crossks/cross_ks.go:377)tracks bookkeeperssubtest bundles several distinct behaviors, reducing failure isolation (pkg/domain/crossks/cross_ks_internal_test.go:91)
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 13
- Inline comments: 12
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (3)
- Acquire/Release bookkeeping does not gate runtime teardown (pkg/domain/crossks/cross_ks.go:345, pkg/domain/crossks/cross_ks.go:355, pkg/domain/crossks/cross_ks.go:123, pkg/domain/crossks/cross_ks.go:326)
- Release() name signals resource teardown but only updates bookkeeping (pkg/domain/sqlsvrapi/server.go:32, pkg/domain/crossks/cross_ks.go:388, pkg/domain/crossks/cross_ks.go:326)
- Public AcquireKSRuntime exposes undocumented, non-domain
bookkeeperparameter (pkg/domain/sqlsvrapi/server.go:38, pkg/domain/crossks/cross_ks.go:123, pkg/domain/domain.go:304)
🟡 [Minor] (8)
- lastReleaseAt is written but never read (pkg/domain/crossks/cross_ks.go:56, pkg/domain/crossks/cross_ks.go:336)
- Bookkeeper accounting is bypassed by the existing GetKSStore/GetKSSessPool paths (pkg/domain/crossks/cross_ks.go:123, pkg/domain/crossks/cross_ks.go:173, pkg/domain/domain.go:867, pkg/domain/domain.go:885)
- Acquire/Release serialize on the global mutex held through a multi-second keyspace cold start (pkg/domain/crossks/cross_ks.go:138, pkg/domain/crossks/cross_ks.go:326, pkg/domain/crossks/cross_ks.go:193)
- Reference-counted Acquire/Release model runs in parallel with unmanaged GetKSStore/GetKSSessPool over the same runtimeEntry (pkg/domain/crossks/cross_ks.go:149, pkg/domain/crossks/cross_ks.go:173, pkg/domain/crossks/cross_ks.go:344, pkg/domain/sqlsvrapi/server.go:38)
- Stale-entry guard in Manager.release has no rationale comment for a non-obvious invariant (pkg/domain/crossks/cross_ks.go:330)
- New runtime-handle lifecycle-race paths are untested (Release after CloseKS, stale-entry guard, Close-while-acquired) (pkg/domain/crossks/cross_ks.go:331, pkg/domain/crossks/cross_ks.go:348, pkg/domain/crossks/cross_ks_internal_test.go:379)
- CloseKS/Close tear down a runtime while bookkeepers are still active (pkg/domain/crossks/cross_ks.go:349, pkg/domain/crossks/cross_ks.go:359, pkg/domain/crossks/cross_ks.go:384)
- Newly exported KSRuntime interface is only ever embedded once, growing public surface without a standalone consumer (pkg/domain/sqlsvrapi/server.go:24)
ℹ️ [Info] (1)
- Release records lastReleaseAt but never reclaims the idle runtime (pkg/domain/crossks/cross_ks.go:336, pkg/domain/crossks/cross_ks.go:56)
🧹 [Nit] (1)
- runtimeHandle.release field name collides with Release() and Manager.release (pkg/domain/crossks/cross_ks.go:377)
D3Hunter
changed the title
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)
pkg/domain/crossks/cross_ks.go (1)
122-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose partially initialized runtime state on failed acquire.
Acquireis now the public entry point intocreateSessionManager, but that failure path still only tears downetcdCliandstore. If cold start fails aftersessPoolis built—e.g.schemaVerSyncer.Init()orisSyncer.Reload()returns an error—the pool (and a successfully initialized syncer) stays open, so repeated failed acquires can leak sessions/background state.Suggested fix in
createSessionManagerfunc (*Manager) createSessionManager( ks string, ksSessFactoryGetter func(string, validatorapi.Validator) pools.Factory, ) (_ *SessionManager, err error) { startTime := time.Now() getStoreFn := getOrCreateStore failpoint.InjectCall("beforeGetStore", &getStoreFn) + var schemaVerSyncer schemaver.Syncer var store kv.Storage store, err = getStoreFn(ks) if err != nil { return nil, err } @@ sessPool := util.NewSessionPool( crossKSSessPoolSize, ksSessFactoryGetter(ks, isValidator), @@ ) + defer func() { + if err != nil { + sessPool.Close() + if schemaVerSyncer != nil { + schemaVerSyncer.Close() + } + } + }() @@ - schemaVerSyncer := schemaver.NewEtcdSyncer(etcdCli, virtualSvrID) + schemaVerSyncer = schemaver.NewEtcdSyncer(etcdCli, virtualSvrID) if err = schemaVerSyncer.Init(ctx); err != nil { return nil, errors.Trace(err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/domain/crossks/cross_ks.go` around lines 122 - 159, The createSessionManager path currently only cleans etcdCli and store on error, leaking sessPool and syncer state; update createSessionManager (used by getOrCreateEntryWithoutLock and Acquire) to perform full cleanup on any error after partial initialization: explicitly Close/Stop the session pool (sessPool), stop/schema/version syncers (e.g., schemaVerSyncer, isSyncer), and release any other resources attached to the entry (etcdCli/store already handled) before returning the error, and ensure entry.activeHolders or other maps are not left in a partially populated state.
🤖 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 `@pkg/domain/crossks/cross_ks.go`:
- Around line 122-159: The createSessionManager path currently only cleans
etcdCli and store on error, leaking sessPool and syncer state; update
createSessionManager (used by getOrCreateEntryWithoutLock and Acquire) to
perform full cleanup on any error after partial initialization: explicitly
Close/Stop the session pool (sessPool), stop/schema/version syncers (e.g.,
schemaVerSyncer, isSyncer), and release any other resources attached to the
entry (etcdCli/store already handled) before returning the error, and ensure
entry.activeHolders or other maps are not left in a partially populated state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e6f6d98f-39c5-4fdc-a146-42017af2009e
📒 Files selected for processing (3)
pkg/domain/crossks/BUILD.bazelpkg/domain/crossks/cross_ks.gopkg/domain/crossks/export_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/domain/crossks/BUILD.bazel
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 8
- Inline comments: 8
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🟡 [Minor] (8)
- runtimeEntry.lastReleaseAt invariant is undocumented and the name reads as a per-release timestamp (pkg/domain/crossks/cross_ks.go:56, pkg/domain/crossks/cross_ks.go:341)
- lastReleaseAt is not cleared when an idle runtime entry is re-acquired (pkg/domain/crossks/cross_ks.go:148, pkg/domain/crossks/cross_ks.go:342)
- Acquire surfaces "session manager" error text despite returning a runtime handle (pkg/domain/crossks/cross_ks.go:167)
- Acquire/release docs promise resource cleanup that is not implemented (pkg/domain/crossks/cross_ks.go:326, pkg/domain/crossks/cross_ks.go:120)
- Global manager lock is held across keyspace creation, stalling the new Release() path and all other keyspace ops (pkg/domain/crossks/cross_ks.go:329, pkg/domain/crossks/cross_ks.go:134, pkg/domain/crossks/cross_ks.go:192)
- New runtime-handle test leaves process-global config.KeyspaceName mutated with no restore (pkg/domain/crossks/cross_ks_test.go:317)
- KSRuntimeHandle.Release contract omits the idempotency that callers and tests rely on (pkg/domain/sqlsvrapi/server.go:32, pkg/domain/crossks/cross_ks.go:374)
- AcquireKSRuntime adds a holder-tracked path parallel to the untracked GetKSStore/GetKSSessPool (pkg/domain/sqlsvrapi/server.go:44, pkg/domain/domain.go:867, pkg/domain/domain.go:885, pkg/domain/crossks/cross_ks.go:328)
ti-chi-bot
Bot
added
size/XXL
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
| func (m *Manager) getOrCreateEntryWithoutLock( | ||
| ks string, | ||
| ksSessFactoryGetter func(string, validatorapi.Validator) pools.Factory, | ||
| ) (_ *runtimeEntry, err error) { |
There was a problem hiding this comment.
| ) (_ *runtimeEntry, err error) { | |
| ) (*runtimeEntry, error) { |
| KSRuntime | ||
| // Release releases the holding of the runtime handle. After calling Release, | ||
| // the handle should not be used anymore. | ||
| // the underlying runtime have different lifecycle, the handle is just a view |
There was a problem hiding this comment.
| // the underlying runtime have different lifecycle, the handle is just a view | |
| // the underlying runtime has different lifecycle, the handle is just a view |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@wuhuizuo: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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: joechenrh, wjhuang2016 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
5 participants
What problem does this PR solve?
Issue Number: ref #68883
Problem Summary:
This PR is a partial step for reclaiming NextGen cross-keyspace runtimes. Current crossKS consumers can obtain raw stores and session pools, but the manager does not know which task-level owner is holding a target-keyspace runtime. That makes duplicate ownership checks and release bookkeeping unavailable.
What changed and how does it work?
sqlsvrapi.KSRuntimeandsqlsvrapi.KSRuntimeHandle, and exposeAcquireKSRuntimethroughsqlsvrapi.Server.Domain.AcquireKSRuntimeby delegating to the crossKS manager.Manager.Acquireto require non-empty bookkeepers, reject duplicate active bookkeepers for the same keyspace, and return an idempotent release handle.GetOrCreate,Get,CloseKS, andClosebehavior working through the new runtime entries.Check List
Tests
Test commands:
make bazel_prepare./tools/check/failpoint-go-test.sh pkg/domain/crossks -run 'TestAcquireRuntimeHandle$' -count=1./tools/check/failpoint-go-test.sh pkg/domain/crossks -run 'Test(AcquireRuntimeHandle|DomainAcquireKSRuntimeHandle)$' -tags=intest,deadlock,nextgen -count=1make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes
Tests