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
|
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 ❌ Your patch check has failed because the patch coverage (72.02%) 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 #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 |
1 similar comment
|
/retest |
JmPotato
left a comment
JmPotato
left a comment
There was a problem hiding this comment.
LGTM. Left two minor test-coverage nits inline, neither is blocking.
| if group.ID != constant.DefaultKeyspaceGroupID { | ||
| return false, rejectMicroserviceMetadataCleanup("found non-default TSO keyspace group %d when cleaning up PD mode microservice metadata", group.ID) | ||
| } | ||
| if group.IsSplitting() { |
There was a problem hiding this comment.
nit: the splitting/merging rejection paths (both here and in deleteDefaultTSOKeyspaceGroup) are not covered by unit tests. A case that sets SplitState on the default keyspace group would cover them cheaply.
| re.NoError(failpoint.Disable("github.com/tikv/pd/client/servicediscovery/fastUpdateServiceMode")) | ||
| } | ||
|
|
||
| func TestPDModeSwitchBetweenMicroserviceAndMonolithMultipleTimes(t *testing.T) { |
There was a problem hiding this comment.
optional: this round-trip only involves the DEFAULT keyspace. Adding a user keyspace assigned to the default group would also exercise the reassignment (fallback/patrol) path after switching back to microservice mode.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JmPotato 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:
|
Potential O(N²) etcd cost in
|
| zap.String("leader-name", s.Name()), | ||
| zap.Duration("total-cost", totalDuration), | ||
| zap.Duration("cost", enableLeaderDuration)) | ||
| s.scheduleMicroserviceMetadataCleanup(ctx) |
There was a problem hiding this comment.
[blocking] Make cleanup completion part of the mode-transition contract.
The service mode is fixed when CreateServer receives the startup-only services slice. This cleanup is launched only after the PD leader reports ready, and its context is canceled when that leader exits. I cannot find any production path that waits for cleanup before starting API mode again.
The integration test currently supplies that missing coordination itself by calling waitMicroserviceMetadataCleaned before the next restart. That means it verifies ms -> normal -> private etcd polling -> ms, rather than the user-visible ms -> normal -> ms workflow. If normal PD is stopped while this goroutine is waiting to retry, the original stale metadata can remain and the next TSO startup can still be blocked.
Please add a durable cleanup-complete state/readiness gate (or make API-mode startup perform/wait for the cleanup) so callers do not need to understand internal etcd paths.
A regression test should remove the private polling dependency and force the incomplete-cleanup window deterministically:
func TestPDModeSwitchWaitsForMetadataCleanup(t *testing.T) {
// Start API-mode PD and TSO, then stop TSO.
// Block cleanup with a callback/failpoint before it reports completion.
normalPD := restartPDForServiceMode(ctx, re, tc, apiPD, nil)
<-cleanupStarted
// Request the next API-mode start without calling
// waitMicroserviceMetadataCleaned.
nextAPIPDCh := restartForServiceModeAsync(ctx, tc, normalPD, []string{mcs.PDServiceName})
// The transition must not become ready while cleanup is incomplete.
re.Never(func() bool { return apiModeReady(nextAPIPDCh) }, time.Second, 20*time.Millisecond)
releaseCleanup()
nextAPIPD := <-nextAPIPDCh
waitDefaultKeyspaceGroupReady(re, nextAPIPD)
tsoCluster := startTSOCluster(re, nextAPIPD)
waitTSOServiceReady(re, tsoCluster)
}The exact gate can differ, but the test should not call waitMicroserviceMetadataCleaned or poll internal metadata from the transition caller.
| if _, err := s.checkMicroserviceEtcdKeysNotLeased(ctx); err != nil { | ||
| return err | ||
| } | ||
| deletedMicroserviceKeys, err := s.deleteMicroserviceEtcdKeys(ctx) |
There was a problem hiding this comment.
[blocking] The lease recheck and prefix deletion still have a TOCTOU window.
checkMicroserviceEtcdKeysNotLeased and deleteMicroserviceEtcdKeys are separate etcd requests. A TSO can register a leased key after this check returns and before the unconditional WithPrefix delete executes. The delete removes that new registry key without revoking its lease; ServiceRegister keeps receiving KeepAlive responses and therefore does not recreate the key, leaving a live service undiscoverable until restart.
Please avoid deleting the whole prefix based on an earlier observation. One safe shape is to list the unleased keys, then delete only those exact keys in bounded transactions with revision/lease comparisons. A key created after the list must not be included.
The existing beforeDeleteMicroserviceEtcdKeys test injects registration before the second check, so it does not cover this remaining window. Add a hook immediately after the check and verify that a newly registered leased key survives:
func TestCleanupDoesNotDeleteKeyRegisteredAfterLeaseCheck(t *testing.T) {
// Arrange the default group/keyspace metadata as in
// TestCleanupMicroserviceMetadataRechecksLeasesBeforeDelete.
registryPath := keypath.RegistryPath(mcs.TSOServiceName, "127.0.0.1:3379")
var leaseID clientv3.LeaseID
require.NoError(t, failpoint.EnableCall(
"github.com/tikv/pd/server/afterCheckMicroserviceEtcdKeysNotLeased",
func() {
var err error
leaseID, err = etcdutil.EtcdKVPutWithTTL(ctx, client, registryPath, "tso", 60)
require.NoError(t, err)
},
))
t.Cleanup(func() {
_ = failpoint.Disable(
"github.com/tikv/pd/server/afterCheckMicroserviceEtcdKeysNotLeased",
)
if leaseID != clientv3.NoLease {
_, _ = client.Revoke(ctx, leaseID)
}
})
_ = svr.cleanupMicroserviceMetadataInPDMode(ctx)
resp, err := etcdutil.EtcdKVGet(client, registryPath)
require.NoError(t, err)
require.Len(t, resp.Kvs, 1)
require.Equal(t, leaseID, clientv3.LeaseID(resp.Kvs[0].Lease))
}This test fails with the current prefix delete even though the two existing lease tests pass.
4 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