keyspace: unassign tombstone keyspace from meta service group#10959
keyspace: unassign tombstone keyspace from meta service group#10959bufferflies wants to merge 4 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
@coderabbitai full review |
ti-chi-bot
Bot
added
the
size/M
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughKeyspace state transitions now use the active transaction when updating state, and ChangesKeyspace tombstone unassignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/keyspace/keyspace.go (1)
1124-1136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRun TOMBSTONE cleanup before the same-state early return.
Line 1125 returns before the new TOMBSTONE unassignment branch, so an already-
TOMBSTONEkeyspace that still hasMetaServiceGroupIDKeycannot be repaired by reapplying the TOMBSTONE update; its assignment count can remain inflated.Proposed fix
// If already in the target state, do nothing and return. if meta.GetState() == newState { + if newState == keyspacepb.KeyspaceState_TOMBSTONE { + return manager.unassignKeyspaceFromMetaServiceGroup(txn, meta) + } return nil }🤖 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/keyspace/keyspace.go` around lines 1124 - 1136, In the state transition logic in keyspace state update handling, the TOMBSTONE cleanup branch is being skipped when the current state already matches the requested state. Move the TOMBSTONE unassignment path in the keyspace state change flow so it runs before the early same-state return, and make sure the transition logic around meta.GetState(), newState, and manager.unassignKeyspaceFromMetaServiceGroup still preserves the existing legal-transition checks.
🤖 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/keyspace/keyspace.go`:
- Around line 1124-1136: In the state transition logic in keyspace state update
handling, the TOMBSTONE cleanup branch is being skipped when the current state
already matches the requested state. Move the TOMBSTONE unassignment path in the
keyspace state change flow so it runs before the early same-state return, and
make sure the transition logic around meta.GetState(), newState, and
manager.unassignKeyspaceFromMetaServiceGroup still preserves the existing
legal-transition checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f63bc81-1b3d-4afb-b984-5d1078510333
📒 Files selected for processing (2)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.go
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/keyspace/keyspace.go (1)
1124-1136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRun TOMBSTONE cleanup before returning on same-state updates.
Line 1125 returns before the new TOMBSTONE cleanup, so a legacy keyspace already in
TOMBSTONEbut still carryingmeta_service_group_idstays assigned and can continue blocking group deletion.Proposed fix
// If already in the target state, do nothing and return. if meta.GetState() == newState { + if newState == keyspacepb.KeyspaceState_TOMBSTONE { + return manager.unassignKeyspaceFromMetaServiceGroup(txn, meta) + } return nil }🤖 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/keyspace/keyspace.go` around lines 1124 - 1136, The state update logic in the keyspace transition path returns early when meta.GetState() already matches newState, which skips the new TOMBSTONE cleanup. Update the same-state branch in the state transition handling so that when the target state is keyspacepb.KeyspaceState_TOMBSTONE, manager.unassignKeyspaceFromMetaServiceGroup is still invoked before returning. Keep the legality checks and tombstone cleanup consistent in the keyspace state update flow so legacy tombstoned keyspaces are always unassigned.
🤖 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/keyspace/keyspace.go`:
- Around line 1124-1136: The state update logic in the keyspace transition path
returns early when meta.GetState() already matches newState, which skips the new
TOMBSTONE cleanup. Update the same-state branch in the state transition handling
so that when the target state is keyspacepb.KeyspaceState_TOMBSTONE,
manager.unassignKeyspaceFromMetaServiceGroup is still invoked before returning.
Keep the legality checks and tombstone cleanup consistent in the keyspace state
update flow so legacy tombstoned keyspaces are always unassigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f520cbd-6c91-458f-b38b-772e4d8e57bf
📒 Files selected for processing (2)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.go
Address review follow-ups on the tombstone unassignment change: - Tighten the RemoveKeyspace comment now that the accounting logic lives in the shared helper. - Add GoDoc to unassignKeyspaceFromMetaServiceGroup explaining the idempotent no-op after the config key is cleared and the metaLock requirement. - Extend the regression test to remove the tombstoned keyspace and assert the assignment counter is not decremented a second time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ystaticy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
| groupID := "etcd-group-0" | ||
| metaServiceGroupStore, ok := manager.store.(endpoint.MetaServiceGroupStorage) | ||
| re.True(ok) | ||
| manager.mgm = NewMetaServiceGroupManager(metaServiceGroupStore, map[string]string{ |
There was a problem hiding this comment.
This group is disabled by default, so CreateKeyspace leaves meta_service_group_id empty and the test fails in CI.
| if !slice.Contains(stateTransitionTable[meta.GetState()], newState) { | ||
| return errors.Errorf("cannot change keyspace state from %s to %s", meta.GetState().String(), newState.String()) | ||
| } | ||
| if newState == keyspacepb.KeyspaceState_TOMBSTONE { |
There was a problem hiding this comment.
This branch is skipped when the keyspace is already TOMBSTONE, so legacy tombstoned keyspaces with meta_service_group_id remain assigned.
Address review feedback from @rleungx: - transformKeyspaceState returned early on a same-state update, so a keyspace already in TOMBSTONE that still carried meta_service_group_id (e.g. tombstoned before this cleanup existed) could never be unassigned and kept an inflated group assignment count. Run the idempotent unassignment on same-state TOMBSTONE updates too. - Make TestTombstoneKeyspaceUnassignsMetaServiceGroup independent of meta-service-group auto-assignment at creation time (disabled by default): create without a group, then assign explicitly. Add coverage for the legacy re-tombstone repair path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| if manager.mgm == nil { | ||
| return nil | ||
| } | ||
| manager.mgm.RLock() |
There was a problem hiding this comment.
This can deadlock with the existing config update path. UpdateKeyspaceState and RemoveKeyspace hold the keyspace metaLock before reaching this RLock, while updateKeyspaceConfigTxn runs under runTxnWithMetaGroupLock, so it holds mgm.RLock before taking the same metaLock. Once UpdateGroupsSafely is waiting on mgm.Lock, Go RWMutex blocks new readers, leaving the tombstone/remove path waiting here and the config-update path waiting on metaLock.
Reported by @rleungx: unassignKeyspaceFromMetaServiceGroup ran under the keyspace metaLock and then took mgm.RLock, while the config-update path (updateKeyspaceConfigTxn via runTxnWithMetaGroupLock) takes mgm.RLock before metaLock. Once UpdateGroupsSafely waits on mgm.Lock, Go RWMutex blocks new readers, so the tombstone/remove path and the config-update path deadlock on the inverted lock order. Drop the read lock: updateAssignmentTxn only touches the store, the original RemoveKeyspace path never held it, and the group delete guard relies on the authoritative keyspace scan rather than this counter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (68.42%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #10959 +/- ##
==========================================
+ Coverage 79.20% 79.23% +0.02%
==========================================
Files 541 541
Lines 75450 75692 +242
==========================================
+ Hits 59762 59973 +211
- Misses 11456 11481 +25
- Partials 4232 4238 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3 participants
What problem does this PR solve?
Issue Number: Close #10891
This is a cherry-pick of 60279ef from the PD microservices branch.
Original author: @ystaticy
Problem Summary:
When a keyspace is moved to
TOMBSTONE, it could still keepmeta_service_group_idin its config and the meta service group assignment counter could remain inflated. That stale assignment can block deleting an otherwise-empty meta service group.What is changed and how does it work?
TOMBSTONE.meta_service_group_idfrom the keyspace config and decrement the persisted assignment count through the meta service group manager.RemoveKeyspaceso removal and tombstone paths keep assignment accounting consistent.UpdateKeyspaceStateAPI.Check List
Tests
Code changes
Side effects
Related changes
Release note
Validation
GOFLAGS=-buildvcs=false make gotest GOTEST_ARGS='./pkg/keyspace -run TestKeyspaceTestSuite/TestTombstoneKeyspaceUnassignsMetaServiceGroup -count=1 -timeout=5m'GOFLAGS=-buildvcs=false make gotest GOTEST_ARGS='./pkg/keyspace -run TestKeyspaceTestSuite/TestUpdateKeyspaceState -count=1 -timeout=5m'git diff --check && git diff --cached --checkGOFLAGS=-buildvcs=false make checkSummary by CodeRabbit