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:
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)
📝 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
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10981 +/- ##
==========================================
- Coverage 79.20% 79.20% -0.01%
==========================================
Files 541 541
Lines 75450 75696 +246
==========================================
+ Hits 59762 59952 +190
- Misses 11456 11510 +54
- Partials 4232 4234 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: The following test 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. |
| ) | ||
| } | ||
| } else if maxLoadedModRevision > 0 { | ||
| kgm.SetModRevision(maxLoadedModRevision) |
There was a problem hiding this comment.
This can still leave a restarted TSO behind clients that observed a delete. During the initial load, deleted keyspace-group records have no KeyValue, so maxLoadedModRevision only reflects surviving groups; if the last metadata change is a group deletion, FindGroupByKeyspaceID will keep returning ErrKeyspaceGroupModRevisionStale even though the loaded snapshot is current.
|
[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:
|
| ) | ||
| } | ||
| } else if maxLoadedModRevision > 0 { | ||
| kgm.SetModRevision(maxLoadedModRevision) |
There was a problem hiding this comment.
What should we do if SetModRevision returns false?
2 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