tso: update keyspace group revision after initial load#10981
Conversation
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:
📝 WalkthroughWalkthroughKeyspace group loading now preserves the highest observed ModRevision, startup failures in ChangesMod revision tracking and recovery readiness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Etcd
participant LoopWatcher
participant KeyspaceGroupManager
LoopWatcher->>Etcd: initial load (putFn per kv)
Etcd-->>LoopWatcher: kv.ModRevision
LoopWatcher->>LoopWatcher: track maxLoadedModRevision
LoopWatcher->>KeyspaceGroupManager: postEventsFn (no events, maxLoadedModRevision>0)
KeyspaceGroupManager->>KeyspaceGroupManager: SetModRevision(maxLoadedModRevision)
sequenceDiagram
participant TestSuite
participant TSONode
participant Etcd
participant KeyspaceGroupManager
TestSuite->>TSONode: restart node
TestSuite->>Etcd: poll keyspace-group KV
Etcd-->>TestSuite: KV with ModRevision
TestSuite->>KeyspaceGroupManager: check loaded group ID and revision
KeyspaceGroupManager-->>TestSuite: group ID, loaded revision
TestSuite->>TestSuite: confirm keyspace serving by group
TestSuite->>TSONode: proceed with recovery assertions
Possibly related PRs
Suggested labels: 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/utils/etcdutil/etcdutil_test.go (1)
470-496: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a tighter assertion for
GetLoadedRevision.
re.GreaterOrEqual(watcher.GetLoadedRevision(), targetRevision)only proves the loaded revision is not stale; it would not catch a bug where the-1offset is dropped or doubled, since either mistake still yields a value>= targetRevision. Since this test guards the exact race-condition fix targeted by this PR, a precise equality check (e.g., putting the key, then comparing against the exact revision returned by a subsequentGet/resp.Header.Revision) would give stronger regression protection.🤖 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/utils/etcdutil/etcdutil_test.go` around lines 470 - 496, The TestGetLoadedRevision assertion is too loose and won’t catch an incorrect revision offset calculation. Update the test in loopWatcherTestSuite to verify the exact loaded revision from NewLoopWatcher/GetLoadedRevision by comparing against the expected revision from the etcd operations used in the setup, rather than only using GreaterOrEqual. Keep the check focused on the loaded revision behavior so it fails if the -1 offset logic is wrong.
🤖 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/utils/etcdutil/etcdutil.go`:
- Around line 497-499: The batched load in the etcd watcher setup is mixing
revisions because each paginated Get in load() can observe a newer
Header.Revision, so the watcher may start from a point that skips writes between
pages. Update load() and the surrounding watcher initialization in etcdutil to
capture a single snapshot revision up front, reuse that revision for every batch
fetch, and have the final loadedRevision/watchStartRevision logic derive from
that fixed revision rather than each page’s response.
---
Nitpick comments:
In `@pkg/utils/etcdutil/etcdutil_test.go`:
- Around line 470-496: The TestGetLoadedRevision assertion is too loose and
won’t catch an incorrect revision offset calculation. Update the test in
loopWatcherTestSuite to verify the exact loaded revision from
NewLoopWatcher/GetLoadedRevision by comparing against the expected revision from
the etcd operations used in the setup, rather than only using GreaterOrEqual.
Keep the check focused on the loaded revision behavior so it fails if the -1
offset logic is wrong.
🪄 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: a1ed1963-6417-49a9-9731-ed548e16c7cc
📒 Files selected for processing (3)
pkg/tso/keyspace_group_manager.gopkg/utils/etcdutil/etcdutil.gopkg/utils/etcdutil/etcdutil_test.go
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (61.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 #10981 +/- ##
==========================================
+ Coverage 79.22% 79.26% +0.03%
==========================================
Files 541 541
Lines 75965 76042 +77
==========================================
+ Hits 60187 60274 +87
- Misses 11531 11532 +1
+ Partials 4247 4236 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-testserver-run-deadlock
branch
from
65b522a to
4777a56
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
1 similar comment
|
/retest |
Cancel failed server starts without calling the blocking Stop path, and return a non-retryable error when a startup goroutine still does not exit. Keep keyspace-group revision updates on the failpoint-aware watcher callback and strengthen the startup, snapshot, and listener cleanup coverage. Signed-off-by: Ryan Leung <rleungx@gmail.com>
Etcd progress revisions include unrelated writes and can differ across TSO nodes. Update the keyspace-group revision only after a snapshot load; group watch events continue to use their own KV revisions. Signed-off-by: Ryan Leung <rleungx@gmail.com>
| postEventsFn, | ||
| true, /* withPrefix */ | ||
| ) | ||
| kgm.groupWatcher.SetLoadRevisionUpdatedCallback(func(revision int64) { |
There was a problem hiding this comment.
Using the global snapshot revision only during load makes revisions permanently incomparable across old and newly started TSO nodes.
A running node advances modRevision only when this keyspace-group watcher receives a group event. A newly started node instead initializes it from the snapshot header, which includes unrelated etcd writes. For example:
mgr1 loads the group at revision 2
an unrelated etcd key is written at revision 3
mgr2 starts and loads the same group metadata from snapshot revision 3
a client learns revision 3 from mgr2
mgr1 rejects that client forever because 3 > 2
Without another keyspace-group event, mgr1 has no remaining path to catch up. Removing progress updates therefore changes transient cross-node skew into permanent stale rejection. The revision source needs to be consistent across initial load and steady-state updates, for example by keeping all nodes advanced to an applied global revision or by introducing a durable keyspace-group revision marker.
Regression test (fails on this PR)
func (suite *keyspaceGroupManagerTestSuite) TestSnapshotRevisionRemainsComparableAcrossManagers() {
re := suite.Require()
keypath.SetClusterID(rand.Uint64())
cfg1 := suite.createConfig()
cfg2 := suite.createConfig()
mgr1 := suite.newKeyspaceGroupManager(1, cfg1)
mgr2 := suite.newKeyspaceGroupManager(1, cfg2)
defer mgr1.Close()
defer mgr2.Close()
const (
groupID = uint32(1)
keyspaceID = uint32(101)
)
re.NoError(addKeyspaceGroupAssignment(
suite.ctx,
suite.etcdClient,
groupID,
[]string{
mgr1.tsoServiceID.ServiceAddr,
mgr2.tsoServiceID.ServiceAddr,
},
[]int{
mcs.DefaultKeyspaceGroupReplicaPriority,
mcs.DefaultKeyspaceGroupReplicaPriority,
},
[]uint32{keyspaceID},
))
re.NoError(mgr1.Initialize())
_, _, _, oldRevision, err :=
mgr1.FindGroupByKeyspaceID(keyspaceID)
re.NoError(err)
// Advance only the global etcd revision. The keyspace-group metadata is
// unchanged, so mgr1 receives no group event.
_, err = suite.etcdClient.Put(
suite.ctx,
"/unrelated/revision-gap",
"1",
)
re.NoError(err)
re.NoError(mgr2.Initialize())
_, _, _, newRevision, err :=
mgr2.FindGroupByKeyspaceID(keyspaceID)
re.NoError(err)
re.Greater(newRevision, oldRevision)
// A client that learned newRevision from mgr2 must not be rejected forever
// by mgr1 when both managers applied identical keyspace-group metadata.
re.GreaterOrEqual(oldRevision, newRevision)
}Observed result on eaf95e96f:
--- FAIL: TestSnapshotRevisionRemainsComparableAcrossManagers
Error: "2" is not greater than or equal to "3"
Update a durable revision marker atomically with keyspace group storage transactions. TSO watchers load and watch the marker alongside group metadata, preserving delete revisions without including unrelated etcd writes. Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What problem does this PR solve?
Issue Number: Close #10551
TestUpdateMemberWhenRecoverycan still fail after a TSO node restart. The restarted TSO server loads keyspace group metadata from etcd, but its local keyspace group mod revision can remain 0 after the initial load. When an existing client asks with a newer service-discovery mod revision, the server rejects the request as stale even though it already has the keyspace group metadata.What is changed and how does it work?
Check List
Tests
Test scripts
Release note
Summary by CodeRabbit