server: add microservice metadata cleanup API - #11141
Conversation
Add a synchronous admin endpoint that clears stale TSO keyspace group member assignments in normal PD mode. Fence the cleanup with the exact PD leadership term and keyspace group revisions while preserving durable topology, assignment markers, and timestamps. Add unit, API, and mode-switch coverage. Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
release-note
|
[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 Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a fenced server operation that clears stale microservice metadata, exposes it through a POST admin endpoint, and tests leadership, concurrency, mode-switch, keyspace-group, and timestamp behavior. ChangesMicroservice metadata cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The cleanup API can return 503 for a healthy request when the initial metadata read consumes the shared deadline, causing avoidable retries during mode switches. The change is otherwise mergeable with explicit owner awareness or follow-up to separate or correctly budget the read and transaction deadlines. Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminAPI
participant Server
participant Etcd
Client->>AdminAPI: POST /api/v1/admin/microservice/metadata/cleanup
AdminAPI->>Server: CleanupMicroserviceMetadata
Server->>Etcd: Read leadership and keyspace-group metadata
Server->>Etcd: Commit fenced member removal
Etcd-->>Server: Commit result
Server-->>AdminAPI: Changed status or classified error
AdminAPI-->>Client: JSON response or HTTP error
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.
🧹 Nitpick comments (5)
tests/integrations/mcs/tso/server_test.go (3)
974-974: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPut
ctxfirst inwaitForTSOMonotonic.
waitForTSOMonotonic(re *require.Assertions, ctx context.Context, ...)takes the context as the second parameter.checkTSOMonotonicin the same file takesctxfirst. Movectxto the first position for consistency and to satisfy the context-first convention.As per coding guidelines: "First parameter must be
context.Contextfor external effects; never store contexts in structs".♻️ Proposed change
-func waitForTSOMonotonic(re *require.Assertions, ctx context.Context, client pd.Client, globalLastTS *uint64) { +func waitForTSOMonotonic(ctx context.Context, re *require.Assertions, client pd.Client, globalLastTS *uint64) {Update the four call sites at Lines 834, 835, 855, and 856 accordingly.
🤖 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 `@tests/integrations/mcs/tso/server_test.go` at line 974, Update waitForTSOMonotonic to accept ctx as its first parameter, matching checkTSOMonotonic and the context-first convention, then adjust all four call sites to pass the arguments in the new order.Source: Coding guidelines
890-894: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the exported response type instead of a local struct.
cleanupMicroserviceMetadataViaHTTPdeclares an anonymous struct with achangedJSON tag.server/api.CleanupMicroserviceMetadataResponsealready defines this wire contract, andtests/server/api/admin_test.gouses it. Reuse the exported type so a JSON tag change fails both tests.🤖 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 `@tests/integrations/mcs/tso/server_test.go` around lines 890 - 894, Update cleanupMicroserviceMetadataViaHTTP to unmarshal into the exported server/api.CleanupMicroserviceMetadataResponse type instead of its local anonymous struct, preserving the existing result.Changed return behavior and shared JSON contract.
982-982: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
tsoutil.ComposeTSfor the timestamp composition.The expression
(uint64(physical) << 18) + uint64(logical)repeats the logical-bit shift. The file already importstsoutiland usestsoutil.ComposeTSat Line 819. Calltsoutil.ComposeTS(physical, logical)here so the shift constant stays in one place.🤖 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 `@tests/integrations/mcs/tso/server_test.go` at line 982, Replace the manual timestamp composition in the test with tsoutil.ComposeTS(physical, logical), reusing the existing imported helper and matching the usage already present near line 819.server/api/admin.go (1)
61-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRegenerate the Swagger spec for the new annotations.
This change adds new
@Routerand@Successannotations. Runmake swagger-specwithSWAGGER=1so the committed spec matches the code.As per coding guidelines: "Regenerate Swagger spec with
make swagger-spec(SWAGGER=1) when API annotations change".🤖 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/api/admin.go` around lines 61 - 71, Regenerate the committed Swagger specification for the updated annotations on CleanupMicroserviceMetadata by running make swagger-spec with SWAGGER=1, ensuring the new `@Router` and `@Success` definitions are reflected in the generated spec.Source: Coding guidelines
server/microservice_cleanup.go (1)
140-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the fenced transaction its own request timeout.
The cleanup operation currently reuses the request context created for the initial metadata read. A slow read, or the test failpoint that blocks before commit, can consume the shared deadline and cause the transaction to return a generic unavailable error before the intended compare-and-fence path is exercised. Derive a fresh timeout context for the commit so the transaction receives its own request budget.
🤖 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 140 - 147, Update the transaction commit path around kv.NewSlowLogTxnWithContext to use a separate context with its own etcdutil.DefaultRequestTimeout budget, derived after the existing Get context is used. Keep operationCtx for the read and pass the new transaction-specific context to the fenced transaction. Apply the same fix in `@server/microservice_cleanup_test.go` around lines 176 - 199: The blocked-commit test can otherwise expire the shared operation deadline and mask the fencing behavior.
🤖 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 `@server/api/admin.go`:
- Around line 61-71: Regenerate the committed Swagger specification for the
updated annotations on CleanupMicroserviceMetadata by running make swagger-spec
with SWAGGER=1, ensuring the new `@Router` and `@Success` definitions are reflected
in the generated spec.
In `@server/microservice_cleanup.go`:
- Around line 140-147: Update the transaction commit path around
kv.NewSlowLogTxnWithContext to use a separate context with its own
etcdutil.DefaultRequestTimeout budget, derived after the existing Get context is
used. Keep operationCtx for the read and pass the new transaction-specific
context to the fenced transaction.
Apply the same fix in `@server/microservice_cleanup_test.go` around lines 176 -
199: The blocked-commit test can otherwise expire the shared operation deadline
and mask the fencing behavior.
In `@tests/integrations/mcs/tso/server_test.go`:
- Line 974: Update waitForTSOMonotonic to accept ctx as its first parameter,
matching checkTSOMonotonic and the context-first convention, then adjust all
four call sites to pass the arguments in the new order.
- Around line 890-894: Update cleanupMicroserviceMetadataViaHTTP to unmarshal
into the exported server/api.CleanupMicroserviceMetadataResponse type instead of
its local anonymous struct, preserving the existing result.Changed return
behavior and shared JSON contract.
- Line 982: Replace the manual timestamp composition in the test with
tsoutil.ComposeTS(physical, logical), reusing the existing imported helper and
matching the usage already present near line 819.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 371f67f6-7ebe-4159-b323-bf6604475a2a
📒 Files selected for processing (6)
server/api/admin.goserver/api/router.goserver/microservice_cleanup.goserver/microservice_cleanup_test.gotests/integrations/mcs/tso/server_test.gotests/server/api/admin_test.go
Fail closed when the default TSO keyspace group is missing so the cleanup API cannot certify an ambiguous assignment state. Preserve revision fencing when the group is deleted concurrently. Fix the mode-switch integration test input and static-check issues, and cover the missing-group HTTP behavior. Signed-off-by: Ryan Leung <rleungx@gmail.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11141 +/- ##
========================================
Coverage 79.43% 79.43%
========================================
Files 542 542
Lines 77117 77260 +143
========================================
+ Hits 61259 61375 +116
- Misses 11571 11584 +13
- Partials 4287 4301 +14
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>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Rewrite only the members field in the persisted keyspace group JSON so additive fields written by newer versions survive cleanup. Reject ambiguous or malformed group objects before returning a successful cleanup certificate. Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
Signed-off-by: Ryan Leung <rleungx@gmail.com>
1 participant
What problem does this PR solve?
Issue Number: Close #10990
After PD switches from microservice mode to PD mode, stale TSO member
assignments can remain in the persisted default keyspace group and block a
later switch back to microservice mode.
This is an independent, master-based alternative to #10996. It uses an explicit
synchronous admin operation instead of leader-start cleanup, so cleanup does not
delay leader readiness in PD mode and the mode-switch controller has an explicit
completion barrier.
What is changed and how does it work?
The endpoint is
POST /pd/api/v1/admin/microservice/metadata/cleanup:200returns{"changed": true|false}after the fenced transaction commits.409means the current mode or keyspace-group topology is not safe to clean.503means leadership or metadata changed concurrently; the request is safeto retry on the serving leader in PD mode.
The external mode-switch sequence is part of the contract:
and wait for all split and merge operations to finish.
200before proceeding.TSO.
A
200response is a linearizable point-in-time certificate, not a durablewrite fence. A late old API-mode writer can make the metadata stale again, so
callers must enforce the ordering above. During rolling upgrades, callers must
also ensure the serving leader supports this endpoint; an old leader returns
404.The endpoint is intentionally named around microservice metadata so more
explicitly safe cleanup operations can be added later. Its initial behavior is
deliberately limited to group 0's persisted
Members.Check List
Tests
Validation status:
pending.
Code changes
Release note
Summary by CodeRabbit
New Features
Bug Fixes