tso, pd-ctl: support evicting all keyspace group primaries on a node - #10968
Conversation
ti-chi-bot
Bot
added
release-note
|
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:
📝 WalkthroughWalkthroughAdds a node-local TSO primary eviction endpoint, exposes it through pd-ctl, and validates the eviction flow in integration tests. ChangesEvict Primary Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant pdctl as pd-ctl evict-primary
participant api as TSO /primary/evict
participant utils as TransferPrimary
pdctl->>api: POST /tso/api/v1/primary/evict
loop each primary keyspace group on the node
api->>utils: transfer away from current primary
utils-->>api: success or error
end
api-->>pdctl: per-group results map
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 (2)
tests/integrations/mcs/members/member_test.go (1)
299-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant polling and untested response body.
Lines 312-321 poll twice for the same condition: a manual
Eventuallyloop checking any non-primary node is serving, immediately followed bytests.WaitForPrimaryServing, whose return value is also discarded. These can be consolidated into a singleWaitForPrimaryServing(re, nodes)call. Additionally, the test never inspects the eviction response body (the per-groupresultsmap), so a regression that returns malformed/empty results with status 200 would go undetected.♻️ Simplify polling
- testutil.Eventually(re, func() bool { - for _, member := range nodes { - if member.GetAddr() != primary && member.IsServing() { - return true - } - } - return false - }, testutil.WithWaitFor(5*time.Second), testutil.WithTickInterval(50*time.Millisecond)) - - tests.WaitForPrimaryServing(re, nodes) + tests.WaitForPrimaryServing(re, nodes)🤖 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/members/member_test.go` around lines 299 - 325, The TestEvictPrimary flow is doing redundant polling and not validating the eviction response payload. In memberTestSuite.TestEvictPrimary, remove the manual testutil.Eventually check and rely on tests.WaitForPrimaryServing(re, nodes) once to wait for the new primary. Also inspect the response body from the POST to /tso/api/v1/primary/evict and assert the per-group results map is present and well-formed, so a 200 response with an empty or malformed body is caught.pkg/mcs/tso/server/apis/v1/api.go (1)
397-433: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSequential per-group transfers with no overall timeout/cancellation.
evictPrimaryiterates every serving keyspace group and performs blocking etcd operations (client.Grant,lease.Close,markExpectedPrimaryFlagviautils.TransferPrimary) sequentially, on the request goroutine, with no context deadline or early-exit on client disconnect. On a node hosting many keyspace groups, this endpoint can block for a long time, tying up the HTTP handler with no way to bound total latency.Consider checking
c.Request.Context()(or a derived timeout context) between iterations and aborting the remaining work if it's done/cancelled, so a single slow etcd interaction or large group count cannot indefinitely stall the request.🔧 Example mitigation
results := make(map[uint32]string) failed := false for keyspaceGroupID, group := range kgm.GetServingKeyspaceGroups() { + if c.Request.Context().Err() != nil { + break + } allocator, err := svr.GetTSOAllocator(keyspaceGroupID)🤖 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/mcs/tso/server/apis/v1/api.go` around lines 397 - 433, The evictPrimary handler currently processes all serving keyspace groups sequentially with no cancellation or overall deadline, so it can block the request goroutine for too long. Update evictPrimary in api.go to check c.Request.Context() (or a derived timeout context) before and/or between iterations over kgm.GetServingKeyspaceGroups(), and stop the loop early when the context is done or the client disconnects. Keep the existing per-group transfer logic using utils.TransferPrimary, but abort remaining work and return the partial results when cancellation is detected.
🤖 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 `@pkg/mcs/tso/server/apis/v1/api.go`:
- Around line 397-433: The evictPrimary handler currently processes all serving
keyspace groups sequentially with no cancellation or overall deadline, so it can
block the request goroutine for too long. Update evictPrimary in api.go to check
c.Request.Context() (or a derived timeout context) before and/or between
iterations over kgm.GetServingKeyspaceGroups(), and stop the loop early when the
context is done or the client disconnects. Keep the existing per-group transfer
logic using utils.TransferPrimary, but abort remaining work and return the
partial results when cancellation is detected.
In `@tests/integrations/mcs/members/member_test.go`:
- Around line 299-325: The TestEvictPrimary flow is doing redundant polling and
not validating the eviction response payload. In
memberTestSuite.TestEvictPrimary, remove the manual testutil.Eventually check
and rely on tests.WaitForPrimaryServing(re, nodes) once to wait for the new
primary. Also inspect the response body from the POST to
/tso/api/v1/primary/evict and assert the per-group results map is present and
well-formed, so a 200 response with an empty or malformed body is caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b52a9eea-8095-4bda-9aa7-36b92e0fc605
📒 Files selected for processing (3)
pkg/mcs/tso/server/apis/v1/api.gotests/integrations/mcs/members/member_test.gotools/pd-ctl/pdctl/command/microservice_command.go
7fb8d3f to
16157c3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integrations/mcs/members/member_test.go (1)
319-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider cleaning up the created keyspace groups after the test.
Groups with hardcoded IDs 1 and 2 are created but never deleted. If other tests in
memberTestSuite(sharing the same cluster) later use the same IDs, or run after this test without an isolated cluster, this could leak state and cause flaky collisions.♻️ Suggested cleanup
+ defer func() { + for _, id := range groupIDs { + handlersutil.MustDeleteKeyspaceGroup(re, suite.server, id) + } + }() + for i, id := range groupIDs { handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{🤖 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/members/member_test.go` around lines 319 - 335, This test creates keyspace groups with hardcoded IDs via handlersutil.MustCreateKeyspaceGroup and never removes them, which can leak state into later memberTestSuite cases. Add cleanup in the test itself (for example with defer or suite teardown) to delete the created groups after the assertions complete, using the same IDs and the keyspace group management helpers so the test leaves the cluster in a clean state.
🤖 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 `@tests/integrations/mcs/members/member_test.go`:
- Around line 319-335: This test creates keyspace groups with hardcoded IDs via
handlersutil.MustCreateKeyspaceGroup and never removes them, which can leak
state into later memberTestSuite cases. Add cleanup in the test itself (for
example with defer or suite teardown) to delete the created groups after the
assertions complete, using the same IDs and the keyspace group management
helpers so the test leaves the cluster in a clean state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de2deec2-1688-4fef-aa18-9545e92c815c
📒 Files selected for processing (3)
pkg/mcs/tso/server/apis/v1/api.gotests/integrations/mcs/members/member_test.gotools/pd-ctl/pdctl/command/microservice_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcs/tso/server/apis/v1/api.go
16157c3 to
d57a3d8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10968 +/- ##
==========================================
+ Coverage 79.21% 79.27% +0.06%
==========================================
Files 541 541
Lines 75677 75965 +288
==========================================
+ Hits 59949 60224 +275
- Misses 11491 11509 +18
+ Partials 4237 4232 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/evict", | ||
| "application/json", nil) | ||
| re.NoError(err) | ||
| re.Equal(http.StatusOK, resp.StatusCode) |
There was a problem hiding this comment.
Please also decode and assert the response body, so this test verifies the per-group result map instead of only the status code.
There was a problem hiding this comment.
Done. The test now reads the evict response body, unmarshals it into the per-group result map, and asserts every group the target held (all 12 created groups) reports success.
| Short: "show the tso members status", | ||
| Run: getMembersCommandFunc, | ||
| }) | ||
| d.AddCommand(&cobra.Command{ |
There was a problem hiding this comment.
Please add a pd-ctl test for this new subcommand, so the direct-node endpoint path and argument handling are covered.
There was a problem hiding this comment.
Added microservice_command_test.go with unit tests covering the success path (asserting the request is a POST to <node>/tso/api/v1/primary/evict), the transport-error path, and the missing-argument path.
d57a3d8 to
39e0522
Compare
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pd-ctl/pdctl/command/microservice_command_test.go (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePossible duplication between
recordingRoundTripperandmockRoundTripper.If
mockRoundTripperalready exists in the package, consider consolidating withrecordingRoundTripper(e.g., add anerrfield to one struct) rather than maintaining two similarhttp.RoundTrippertest doubles.Also applies to: 67-71
🤖 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 `@tools/pd-ctl/pdctl/command/microservice_command_test.go` around lines 28 - 39, The test file has two very similar http.RoundTripper doubles, recordingRoundTripper and mockRoundTripper, which should be consolidated. Update the existing test double used by the microservice command tests so it can both capture the request and return a canned response/error, then replace the duplicate helper with that single implementation. Keep the behavior centered around RoundTrip and the request-recording fields so the tests still assert the destination while avoiding duplicated structs.
🤖 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 `@tools/pd-ctl/pdctl/command/microservice_command_test.go`:
- Around line 28-39: The test file has two very similar http.RoundTripper
doubles, recordingRoundTripper and mockRoundTripper, which should be
consolidated. Update the existing test double used by the microservice command
tests so it can both capture the request and return a canned response/error,
then replace the duplicate helper with that single implementation. Keep the
behavior centered around RoundTrip and the request-recording fields so the tests
still assert the destination while avoiding duplicated structs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 821efd1c-0e8d-4277-94ee-c6748028312b
📒 Files selected for processing (4)
pkg/mcs/tso/server/apis/v1/api.gotests/integrations/mcs/members/member_test.gotools/pd-ctl/pdctl/command/microservice_command.gotools/pd-ctl/pdctl/command/microservice_command_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/pd-ctl/pdctl/command/microservice_command.go
- pkg/mcs/tso/server/apis/v1/api.go
39e0522 to
e5befe8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/pd-ctl/pdctl/command/microservice_command_test.go (1)
28-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePossible duplication with an existing mock round tripper.
recordingRoundTripperoverlaps in purpose withmockRoundTripperreferenced at line 70 (both implementRoundTripto return a canned response/error, and this one additionally records the request). IfmockRoundTrippercan be extended to optionally capture the request, the two types could be consolidated to avoid maintaining two near-identical mocks in the same package.🤖 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 `@tools/pd-ctl/pdctl/command/microservice_command_test.go` around lines 28 - 39, The test helper `recordingRoundTripper` duplicates most of the behavior of `mockRoundTripper` in `microservice_command_test.go`; consolidate them by extending `mockRoundTripper` to optionally store the incoming request while still returning the canned response/error. Update the affected tests to use the shared mock and keep any request assertions working through the captured request field so there is only one `RoundTrip` mock implementation in the package.tests/integrations/mcs/members/member_test.go (1)
373-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicate assertion loops.
Lines 388-390 and 391-393 both assert
results[id] == "success"for effectively the same data — the second loop iterating the whole map adds no coverage beyond a length check, since every entry it visits was already validated in the first loop (assumingresultsdoesn't contain unexpected group IDs). Consider dropping one loop, or replacing the second with an explicitre.Len(results, len(groupIDs))to make the extra-entries check explicit.♻️ Simplify redundant checks
results := make(map[uint32]string) re.NoError(json.Unmarshal(body, &results), string(body)) + re.Len(results, len(groupIDs)) for _, id := range groupIDs { re.Equalf("success", results[id], "group %d result: %q", id, results[id]) } - for id, status := range results { - re.Equalf("success", status, "group %d result: %q", id, status) - }🤖 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/members/member_test.go` around lines 373 - 393, The eviction response validation has redundant duplicate assertions: the loop over groupIDs already checks each expected group’s status in results, so the second loop over results adds no new coverage. In the member_test.go eviction test block, either remove the second loop entirely or replace it with an explicit length check on results against groupIDs to make unexpected extra entries a deliberate assertion. Keep the existing json.Unmarshal and per-group success checks, but avoid asserting the same success condition twice.
🤖 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 `@tests/integrations/mcs/members/member_test.go`:
- Around line 373-393: The eviction response validation has redundant duplicate
assertions: the loop over groupIDs already checks each expected group’s status
in results, so the second loop over results adds no new coverage. In the
member_test.go eviction test block, either remove the second loop entirely or
replace it with an explicit length check on results against groupIDs to make
unexpected extra entries a deliberate assertion. Keep the existing
json.Unmarshal and per-group success checks, but avoid asserting the same
success condition twice.
In `@tools/pd-ctl/pdctl/command/microservice_command_test.go`:
- Around line 28-39: The test helper `recordingRoundTripper` duplicates most of
the behavior of `mockRoundTripper` in `microservice_command_test.go`;
consolidate them by extending `mockRoundTripper` to optionally store the
incoming request while still returning the canned response/error. Update the
affected tests to use the shared mock and keep any request assertions working
through the captured request field so there is only one `RoundTrip` mock
implementation in the package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bba15b4-7c08-4155-9df3-f270a9e8abea
📒 Files selected for processing (4)
pkg/mcs/tso/server/apis/v1/api.gotests/integrations/mcs/members/member_test.gotools/pd-ctl/pdctl/command/microservice_command.gotools/pd-ctl/pdctl/command/microservice_command_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/pd-ctl/pdctl/command/microservice_command.go
- pkg/mcs/tso/server/apis/v1/api.go
|
|
||
| lease := allocator.GetExpectedPrimaryLease() | ||
| // An empty new primary lets TransferPrimary pick a random other member. | ||
| if err := utils.TransferPrimary(svr.GetClient(), lease, |
There was a problem hiding this comment.
Bulk eviction can temporarily leave a split target without a primary. During an in-progress split, the target group must campaign on the same TSO node as its split source, but this loop transfers each local group independently and TransferPrimary(..., "", ...) chooses a random next primary per group. If the drained node owns both the source and target, they can get different expected-primary flags, so /evict can report success while the target keyspaces have no TSO primary until that flag expires.
There was a problem hiding this comment.
Good catch. Fixed by rejecting the whole request up front (before transferring anything) when this node is the primary of a splitting keyspace group, returning 500 with ErrKeyspaceGroupInSplit. Since a split target must campaign on the same node as its source, evicting mid-split could otherwise leave the target keyspaces without a primary; splitting is transient, so the caller can retry after it finishes. Added TestEvictPrimaryRejectedWhileSplitting (holds the split via the pauseFinishSplitBeforeTxn failpoint, asserts the 500, then confirms eviction succeeds once the split completes).
e5befe8 to
b7c5bf9
Compare
| Short: "show the tso members status", | ||
| Run: getMembersCommandFunc, | ||
| }) | ||
| d.AddCommand(newEvictTSOPrimaryCommand()) |
There was a problem hiding this comment.
Nit: since this adds another TSO subcommand, please also update the parent usage string to include evict-primary; otherwise pd-ctl microservice tso --help still advertises only <primary|members>.
There was a problem hiding this comment.
Done — updated the parent Use string to tso <primary|members|evict-primary>.
| // error message), so the caller can see the per-group result. | ||
| results := make(map[uint32]string) | ||
| failed := false | ||
| for keyspaceGroupID, group := range servingGroups { |
There was a problem hiding this comment.
The split guard can be stale by the time this loop transfers a group. If a split starts after the pre-check and before this group is processed, /evict can still move the split source away, leaving the split target on a node whose local source allocator is no longer serving; checkTSOSplit then fails to generate TSO for the target keyspaces.
There was a problem hiding this comment.
Addressed by re-checking the split state right before each transfer: the loop now re-fetches the latest group meta via GetKeyspaceGroupByID and aborts with 500 (ErrKeyspaceGroupInSplit) if the group is splitting, in addition to the up-front pre-check. This narrows the window to the gap between that per-group check and the TransferPrimary etcd op; a split starting inside that tiny gap is still not covered. Fully closing it would require serializing eviction with the split state machine (a larger change in kgm/allocator) — happy to do that if you prefer, but note the residual case self-heals: TransferPrimary points the source's expected-primary at the new node, and the split target's campaign checker re-elects it onto that same node once the source serves there, so the target keyspaces only see a brief TSO gap rather than a permanent stall.
de84a65 to
afd608b
Compare
d3c30d3 to
f7a9075
Compare
ti-chi-bot
Bot
added
needs-1-more-lgtm
| re.Contains(result, "mock error") | ||
| } | ||
|
|
||
| func TestEvictTSOPrimaryCommand_MissingArg(t *testing.T) { |
| // error message), so the caller can see the per-group result. | ||
| results := make(map[uint32]string) | ||
| failed := false | ||
| for keyspaceGroupID := range servingGroups { |
There was a problem hiding this comment.
I think the split guard still allows partial side effects before the request is rejected. This loop both checks group.IsSplitting() and calls TransferPrimary, and servingGroups is a Go map with nondeterministic iteration order. If this node is primary for both a normal group and a splitting group, /primary/evict may transfer the normal group first, then hit the splitting group and return 500. That contradicts the comment/previous discussion that the request is rejected for splitting groups, and it also skips the per-group result map promised by the issue/API contract.
Can we pre-collect the candidate primary groups and check all of them for split state before calling any TransferPrimary? Alternatively, if this is intended to be best-effort even in split state, the split case should be recorded in results and the handler should return the result map consistently instead of aborting with a bare error string.
There was a problem hiding this comment.
Restored the all-or-nothing pre-check: evictPrimary now pre-collects the groups this node is primary of and checks every one of them for split state before calling any TransferPrimary. If any is splitting, the whole request is rejected with 500/ErrKeyspaceGroupInSplit and nothing is transferred, so a normal group can no longer be moved before a later splitting group is hit. The per-transfer re-check is kept as a narrow TOCTOU guard for a split that starts mid-eviction.
| }, testutil.WithWaitFor(30*time.Second), testutil.WithTickInterval(200*time.Millisecond)) | ||
| re.NotEmpty(primaryAddr) | ||
|
|
||
| resp, err := tests.TestDialClient.Post(primaryAddr+"/tso/api/v1/primary/evict", |
There was a problem hiding this comment.
Can we extend this regression test to cover the partial-eviction case? Right now the test only creates the splitting source/target, so it cannot catch the case where the handler transfers a non-splitting group first and only later encounters the splitting group.
A useful test shape would be: create one additional normal keyspace group, transfer both the normal group and the splitting source to the same target node, pause the split, call /primary/evict, then assert the normal group is still primary on the target node if the intended behavior is to reject the whole request while any local primary group is splitting. This would verify that the split rejection happens before any transfer side effect.
There was a problem hiding this comment.
Done. TestEvictPrimaryRejectedWhileSplitting now also creates a normal keyspace group, transfers its primary onto the same node as the split source, calls /primary/evict, asserts the 500 rejection, and then asserts the normal group is still primary on that node — verifying the rejection happens before any transfer side effect.
81ce0ca to
5dd4b71
Compare
| } | ||
| isMember := false | ||
| for _, entry := range entries { | ||
| if entry.ServiceAddr == node { |
There was a problem hiding this comment.
Can we normalize the node address before this membership check and before building the proxy target? Existing TSO config/tests allow advertise-listen-addr without a scheme, and nearby keyspace-group code uses typeutil.EqualBaseURLs for node-address matching. With the current exact string comparison, pd-ctl ... evict-primary 127.0.0.1:3379 is rejected when the registry contains http://127.0.0.1:3379; conversely, if the registry itself stores 127.0.0.1:3379, this check can pass but url.Parse(node) below does not produce a usable host/scheme for NewCustomReverseProxies, so the proxy request fails.
Please match against registered TSO members using the existing scheme-insensitive comparison, then construct a normalized URL for the matched registry address before forwarding (for example by defaulting a missing scheme from the server/client TLS scheme). This keeps the open-proxy guard while accepting the address formats the rest of the code already treats as equivalent.
There was a problem hiding this comment.
Done. The membership check now uses typeutil.EqualBaseURLs (same helper as KeyspaceGroupMember.IsAddressEquivalent) instead of an exact string compare, so scheme/advertise-listen-addr differences no longer cause a false negative, and PD proxies to the matched member's canonical ServiceAddr rather than the raw client input.
5dd4b71 to
de41597
Compare
| continue | ||
| } | ||
| // An empty new primary lets TransferPrimary pick a random other member. | ||
| if err := utils.TransferPrimary(svr.GetClient(), participant, |
There was a problem hiding this comment.
This eviction is still only a one-shot TransferPrimary; it does not make the priority checker avoid the evicted node. If this node has a higher priority in a keyspace group, the primary can be moved back by primaryPriorityCheckLoop after /evict succeeds.
I verified this locally with a small test: make the higher-priority node the primary, use the same TransferPrimary(..., newPrimary="", ...) shape as /primary/evict to move it away, and then the priority checker moves the primary back to the higher-priority node. So the current behavior is closer to a one-shot transfer than to the drain workflow described in the issue/PR for restart/upgrade/decommission.
If this API is expected to support draining, could we make the priority checker aware of the draining/evicted node, or temporarily exclude/lower this node as a primary candidate, and add a regression test where evicting a higher-priority node does not move the primary back? If the intended semantics are only best-effort one-shot transfer, please narrow the docs/PR description so users do not assume it durably drains the node.
The full test I used:
// Additional imports needed by this test:
//
// "github.com/tikv/pd/pkg/mcs/utils"
// "github.com/tikv/pd/pkg/member"
func (suite *keyspaceGroupManagerTestSuite) TestEvictStyleTransferToLowerPriorityIsRevertedByPriorityChecker() {
re := suite.Require()
defaultPriority := mcs.DefaultKeyspaceGroupReplicaPriority
cfg1 := suite.createConfig()
cfg2 := suite.createConfig()
cfg1.Name = "tso-evict-low-priority"
cfg2.Name = "tso-evict-high-priority"
svcAddr1 := cfg1.GetAdvertiseListenAddr()
svcAddr2 := cfg2.GetAdvertiseListenAddr()
re.NoError(suite.registerTSOServer(re, svcAddr1, cfg1))
defer func() {
re.NoError(suite.deregisterTSOServer(svcAddr1))
}()
re.NoError(suite.registerTSOServer(re, svcAddr2, cfg2))
defer func() {
re.NoError(suite.deregisterTSOServer(svcAddr2))
}()
const groupID = uint32(1234)
re.NoError(addKeyspaceGroupAssignment(
suite.ctx, suite.etcdClient, groupID,
[]string{svcAddr1, svcAddr2}, []int{defaultPriority, defaultPriority + 1}, []uint32{groupID}))
defer func() {
re.NoError(deleteKeyspaceGroupInEtcd(suite.ctx, suite.etcdClient, groupID))
}()
mgr1 := suite.newKeyspaceGroupManager(1, cfg1)
re.NotNil(mgr1)
mgr1.primaryPriorityCheckInterval = 200 * time.Millisecond
defer mgr1.Close()
re.NoError(mgr1.Initialize())
mgr2 := suite.newKeyspaceGroupManager(1, cfg2)
re.NotNil(mgr2)
mgr2.primaryPriorityCheckInterval = 200 * time.Millisecond
defer mgr2.Close()
re.NoError(mgr2.Initialize())
waitForPrimariesServing(re, []*KeyspaceGroupManager{mgr2}, []uint32{groupID})
allocator, err := mgr2.GetAllocator(groupID)
re.NoError(err)
participant, ok := allocator.GetMember().(*member.Participant)
re.True(ok)
memberMap := map[string]bool{
svcAddr1: true,
svcAddr2: true,
}
// This is the same one-shot transfer shape used by /primary/evict: the
// current primary is moved to any other member, without marking this node as
// drained or changing replica priority.
re.NoError(utils.TransferPrimary(suite.etcdClient, participant,
mcs.TSOServiceName, cfg2.Name, "", groupID, memberMap))
waitForPrimariesServing(re, []*KeyspaceGroupManager{mgr1}, []uint32{groupID})
// Since mgr2 still has the higher priority, the priority checker moves the
// primary back. This is the behavior that makes the current evict API a
// one-shot transfer rather than a durable drain.
waitForPrimariesServing(re, []*KeyspaceGroupManager{mgr2}, []uint32{groupID})
}There was a problem hiding this comment.
Good point — with equal priorities (the default) the transfer sticks since the checker only moves to a strictly higher-priority member, but if the evicted node has a higher priority the checker will move the primary back, so this doesn't durably drain such a node. We'd rather not build priority-aware eviction into this PR: priority has had a number of production issues and is largely deprecated right now, so we'll revisit a durable-drain mechanism if/when it's needed. I've left a TODO at the transfer site to reconsider priority here.
de41597 to
3445382
Compare
| c.AbortWithStatusJSON(http.StatusBadRequest, fmt.Sprintf("%s is not a tso node", node)) | ||
| return | ||
| } | ||
| u, err := url.Parse(target) |
There was a problem hiding this comment.
We still need to normalize the matched registry address into a URL that is valid for the reverse proxy. The current update fixes the membership match with EqualBaseURLs, but after target = entry.ServiceAddr it still calls url.Parse(target) directly. If the registry stores a valid bare advertise-listen-addr, such as 127.0.0.1:3379 or localhost:3379, url.Parse will not produce a usable Host/Scheme, and NewCustomReverseProxies will still forward with a broken request URL.
This is a supported configuration today: pkg/mcs/tso/server/config_test.go already covers advertise addresses without a scheme. Could we normalize entry.ServiceAddr after it is matched, defaulting a missing scheme to http:// or https:// according to the current server/client TLS setup, before passing it to the reverse proxy? It would also be useful to add a test where the registered TSO member address has no scheme and the pd-ctl input, with or without a scheme, still forwards successfully.
There was a problem hiding this comment.
Done. After matching, PD now normalizes the target via a small resolveProxyURL helper: if the registered ServiceAddr has no scheme (a bare advertise-listen-addr like 127.0.0.1:3379 or localhost:3379, where url.Parse yields an empty Host), it prepends the scheme derived from the server's TLS config (https when a cert is configured, http otherwise) before building the reverse-proxy target; an address that already carries a scheme is kept as-is. Added a unit test (TestResolveProxyURL / TestHTTPSchemeFromTLS) covering scheme-less and scheme'd addresses under both schemes, and the pd-ctl integration test now passes the node address without a scheme so the end-to-end forward still succeeds.
3445382 to
d827623
Compare
ti-chi-bot
Bot
added
size/XXL
| "github.com/tikv/pd/pkg/utils/grpcutil" | ||
| ) | ||
|
|
||
| func TestHTTPSchemeFromTLS(t *testing.T) { |
There was a problem hiding this comment.
Fixed. The lint failure was leakcheck: this is the first test file in the handlers package, so the package needs a TestMain with goleak.VerifyTestMain(m, testutil.LeakOptions...). Added it; leakcheck now passes locally.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, rleungx 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 |
Add POST /tso/api/v1/primary/evict, which transfers away every keyspace group primary currently held by the target TSO node. It is a best-effort, node-local operation: groups the node does not serve as primary are skipped, a failure on one group does not abort the others, and a per-group result map is returned. Expose it through pd-ctl as `microservice tso evict-primary <address>`, sending the request directly to the target node. Issue Number: close tikv#10967 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
d827623 to
4fb19f6
Compare
|
/retest |
3 participants
What problem does this PR solve?
Issue Number: Close #10967
A TSO node exposes
POST /tso/api/v1/primary/transferto move a singlekeyspace group primary away. Draining a node (restart/upgrade/decommission)
requires moving all keyspace group primaries off it, which today means
calling
transferonce per group — tedious and error-prone.What is changed and how does it work?
Add a node-local endpoint and a pd-ctl command to evict every keyspace group
primary held by a node in one step.
Check List
Tests
Code changes
Release note
Summary by CodeRabbit
POST /tso/api/v1/primary/evict) to trigger best-effort eviction of eligible keyspace-group primaries served by the target node.pd-ctl microservice tso evict-primary <tso_node_address>command to invoke the endpoint and display per-group outcomes.pd-ctlcommand.