keyspace: guard meta service group updates#10907
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
the
size/L
|
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)
📝 WalkthroughWalkthroughAdds a test-only ChangesUpdateKeyspaceConfig lock-ordering regression test
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant UpdateKeyspaceConfig
participant assignmentHookStorage
participant MetaServiceGroupManager
Test->>assignmentHookStorage: set beforeIncrementAssignment (pause)
Test->>UpdateKeyspaceConfig: call (goroutine)
UpdateKeyspaceConfig->>assignmentHookStorage: IncrementAssignmentCount
assignmentHookStorage->>Test: block on callback
Test->>MetaServiceGroupManager: attempt Lock()
MetaServiceGroupManager-->>Test: lock not acquired (timeout)
Test->>assignmentHookStorage: release pause
assignmentHookStorage-->>UpdateKeyspaceConfig: IncrementAssignmentCount returns
UpdateKeyspaceConfig-->>Test: success
MetaServiceGroupManager-->>Test: lock acquired
Related issues: Suggested labels: test, keyspace Suggested reviewers: rleungx, binshi-bing 🐰 A hook pauses the count mid-flight, 🚥 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 |
50169ae to
243d766
Compare
![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/keyspace/keyspace.go (1)
813-885: 💤 Low valueConsider using defer for
RUnlockto guard against panics.While storage operations typically return errors rather than panic, the current pattern leaves
RUnlockunguarded if an unexpected panic occurs withinRunInTxn. A defer-based approach would be more resilient.♻️ Suggested refactor using an anonymous function
- if manager.mgm != nil { - manager.mgm.RLock() - } - err := manager.store.RunInTxn(manager.ctx, func(txn kv.Txn) error { + err := func() error { + if manager.mgm != nil { + manager.mgm.RLock() + defer manager.mgm.RUnlock() + } + return manager.store.RunInTxn(manager.ctx, func(txn kv.Txn) error { // ... transaction body unchanged ... - }) - if manager.mgm != nil { - manager.mgm.RUnlock() - } + }) + }()🤖 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 813 - 885, The current code calls manager.mgm.RUnlock() directly after manager.store.RunInTxn() completes, which leaves it unguarded if a panic occurs during the transaction. Wrap the entire transaction block (starting with the manager.mgm.RLock() call) in an anonymous function and defer the manager.mgm.RUnlock() call at the beginning of that function. This ensures that the unlock happens even if a panic occurs within RunInTxn, making the code more resilient.
🤖 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/keyspace/keyspace.go`:
- Around line 813-885: The current code calls manager.mgm.RUnlock() directly
after manager.store.RunInTxn() completes, which leaves it unguarded if a panic
occurs during the transaction. Wrap the entire transaction block (starting with
the manager.mgm.RLock() call) in an anonymous function and defer the
manager.mgm.RUnlock() call at the beginning of that function. This ensures that
the unlock happens even if a panic occurs within RunInTxn, making the code more
resilient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa90f242-0e32-4ab5-8860-cff07f0a20b2
📒 Files selected for processing (5)
pkg/keyspace/keyspace.gopkg/keyspace/meta_service_group.gopkg/keyspace/meta_service_group_test.goserver/server.gotests/server/apiv2/handlers/meta_service_group_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/server/apiv2/handlers/meta_service_group_test.go
- server/server.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
804-806: 💤 Low valueConsider using defer for panic safety.
The current lock/unlock pattern correctly releases the lock after
RunInTxnreturns (with or without error). However, ifRunInTxnpanics, theRUnlockwould not be called. While panics are rare, adeferpattern would provide additional safety:if manager.mgm != nil { manager.mgm.RLock() defer manager.mgm.RUnlock() }The trade-off is that
deferwould hold the lock slightly longer (through theAttachEndpointscall and logging). Given thatAttachEndpointsis a quick in-memory operation and the current pattern works correctly for normal execution, the current approach is acceptable if the team prefers minimizing lock hold time.Also applies to: 874-876
🤖 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 804 - 806, The lock/unlock pattern for manager.mgm requires panic safety to ensure the RUnlock is always called. Modify the code where manager.mgm is locked with RLock to immediately follow with a defer statement that calls RUnlock on the same manager.mgm object, ensuring the lock is released even if a panic occurs during the subsequent operations like RunInTxn or AttachEndpoints. Apply this defer pattern consistently wherever the manager.mgm RLock pattern is used.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.
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 804-806: The lock/unlock pattern for manager.mgm requires panic
safety to ensure the RUnlock is always called. Modify the code where manager.mgm
is locked with RLock to immediately follow with a defer statement that calls
RUnlock on the same manager.mgm object, ensuring the lock is released even if a
panic occurs during the subsequent operations like RunInTxn or AttachEndpoints.
Apply this defer pattern consistently wherever the manager.mgm RLock pattern is
used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a874ca33-898f-4b6f-aa32-3306ca7820d3
📒 Files selected for processing (3)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/meta_service_group.go
✅ Files skipped from review due to trivial changes (1)
- pkg/keyspace/meta_service_group.go
| var meta *keyspacepb.KeyspaceMeta | ||
| oldConfig := make(map[string]string) | ||
| if manager.mgm != nil { | ||
| manager.mgm.RLock() |
There was a problem hiding this comment.
This lock does not cover dynamic meta-service group config updates, so assigned groups can still be deleted.
There was a problem hiding this comment.
Resolved after rebasing the PR on the latest master.
The current implementation no longer uses the old narrow lock scope from this diff. updateKeyspaceConfigTxn now calls runTxnWithMetaGroupLock, which holds mgm.RLock() around the whole store.RunInTxn(...), and reassignKeyspaceLocked performs both the group existence validation and the assignment counter update inside that transaction. Dynamic meta-service group updates go through MetaServiceGroupManager.UpdateGroupsSafely, which takes the write lock before deleting groups, so it cannot delete a group while this reassignment transaction is in progress.
I also kept this PR as a regression test only: TestUpdateKeyspaceConfigHoldsMetaServiceGroupReadLockUntilAssignmentTxnDone pauses during IncrementAssignmentCount and verifies a meta-service group writer cannot acquire the write lock until the assignment transaction is released.
Signed-off-by: lhy1024 <19542290+lhy1024@users.noreply.github.com>
6530f0b to
ca23e87
Compare
|
@lhy1024: 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. |
2 participants
What problem does this PR solve?
Issue Number: close #10906
What is changed and how does it work?
meta_service_group_idassignment updates with meta-service group set updates.Check List
Tests
Summary by CodeRabbit
Bug Fixes
Tests