server: clean up microservice metadata in PD mode#10996
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
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/XXL
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds background cleanup for stale PD microservice metadata, wires it into leader readiness, and tests cleanup validation plus repeated microservice-to-monolith-to-microservice switching. ChangesPD microservice metadata cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PDServer
participant KeyspaceStorage
participant Etcd
participant TSOCluster
PDServer->>PDServer: become leader and schedule cleanup
PDServer->>KeyspaceStorage: validate and clean keyspace metadata
PDServer->>Etcd: delete microservice metadata
TSOCluster->>PDServer: switch service mode and request TSO
PDServer->>Etcd: verify cleanup state
🚥 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 |
![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)
server/microservice_cleanup.go (1)
47-129: 🚀 Performance & Scalability | 🔵 TrivialConsider adding a metric for cleanup outcome/retries.
This background cleanup is on a critical path for the mode-switch fix; a Prometheus counter/gauge for success, rejection, and retry counts would help operators detect a stuck cleanup loop (e.g., the txn-overflow scenario above) without grepping logs.
🤖 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 `@server/microservice_cleanup.go` around lines 47 - 129, The microservice metadata cleanup loop lacks metrics for its outcomes and retries. Add Prometheus counters or gauges for successful cleanup, rejected/skipped cleanup, and retry attempts, and update them in scheduleMicroserviceMetadataCleanup and cleanupMicroserviceMetadataInPDMode at the corresponding return and retry paths so operators can detect stuck cleanup loops.
🤖 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 `@server/microservice_cleanup.go`:
- Around line 155-217: Reduce the batch size used by
cleanupDefaultTSOKeyspaceGroupConfig when calling LoadRangeKeyspace, leaving
enough transaction capacity for the SaveKeyspaceMeta operations performed for
loaded entries. Use an appropriate smaller scan limit so a full cleanup
transaction remains within etcd’s operation limit while preserving the existing
pagination and cleanup behavior.
---
Nitpick comments:
In `@server/microservice_cleanup.go`:
- Around line 47-129: The microservice metadata cleanup loop lacks metrics for
its outcomes and retries. Add Prometheus counters or gauges for successful
cleanup, rejected/skipped cleanup, and retry attempts, and update them in
scheduleMicroserviceMetadataCleanup and cleanupMicroserviceMetadataInPDMode at
the corresponding return and retry paths so operators can detect stuck cleanup
loops.
🪄 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: ba4c4a70-d3b5-45e5-8163-4d6454260ade
📒 Files selected for processing (4)
server/microservice_cleanup.goserver/microservice_cleanup_test.goserver/server.gotests/integrations/mcs/tso/server_test.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
1 similar comment
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10996 +/- ##
==========================================
- Coverage 79.22% 79.19% -0.04%
==========================================
Files 541 542 +1
Lines 75965 76152 +187
==========================================
+ Hits 60187 60307 +120
- Misses 11531 11574 +43
- Partials 4247 4271 +24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| } | ||
| if !needsCleanup && s.client != nil { | ||
| getCtx, cancel := context.WithTimeout(ctx, etcdutil.DefaultRequestTimeout) | ||
| resp, err := s.client.Get(getCtx, microserviceEtcdPrefix(), clientv3.WithPrefix(), clientv3.WithLimit(1)) |
There was a problem hiding this comment.
[P2] Guard cleanup while microservice keys are still leased
The later full-prefix delete can remove registry keys owned by a still-running TSO. Deleting a leased key does not close that process’s KeepAlive stream, so ServiceRegister never enters renewKeepalive and does not recreate the key. This is a low-probability lifecycle overlap rather than a general TSO outage (default-group clients can fall back to the legacy primary), but registry-dependent paths remain empty until the TSO restarts.
The fix can stay small here: always scan this prefix with WithKeysOnly(), and if any returned KV has Lease != 0, return an error before changing keyspace-group metadata. The existing 5-second cleanup retry will proceed automatically after the microservices stop and their leases disappear. No watcher or registration-side recovery is needed.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
|
@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. |
2 participants
What problem does this PR solve?
Issue Number: Close #10990
PD cannot switch from microservice mode to normal mode and then back to
microservice mode because stale keyspace group and microservice metadata
prevents new TSO instances from becoming healthy.
What is changed and how does it work?
Check List
Tests
Code changes
Release note
Summary by CodeRabbit