Skip to content

evict: support new_primary in /primary/evict endpoint #11121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bufferflies wants to merge 4 commits into
base: master
Choose a base branch
from
Open

evict: support new_primary in /primary/evict endpoint #11121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
696ec9e
evict: support new_primary in /primary/evict endpoint
bufferflies Aug 6, 2026
48f7146
evict: fix review feedback on new_primary handling
bufferflies Aug 7, 2026
1ac1ec0
evict: reject self-target by service address too
bufferflies Aug 7, 2026
e181dc8
evict: cover the all-or-nothing rejection for an invalid new_primary
bufferflies Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 81 additions & 23 deletions pkg/mcs/tso/server/apis/v1/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (

"github.com/tikv/pd/pkg/errs"
"github.com/tikv/pd/pkg/keyspace/constant"
"github.com/tikv/pd/pkg/mcs/discovery"
tsoserver "github.com/tikv/pd/pkg/mcs/tso/server"
"github.com/tikv/pd/pkg/mcs/utils"
mcs "github.com/tikv/pd/pkg/mcs/utils/constant"
Expand Down Expand Up @@ -389,28 +390,58 @@ func transferPrimary(c *gin.Context) {
}

// evictPrimary transfers away every keyspace group primary currently held by this
// node, moving each to another member of the same group. It is a best-effort,
// node-local operation: groups that this node is not serving as primary are
// skipped, and a failure on one group does not stop the others.
// node, moving each to another member of the same group. Groups that this node is
// not serving as primary are skipped.
//
// The request is rejected as a whole if this node is the primary of any
// splitting keyspace group: a split target must campaign on the same TSO node as
// its split source, but eviction transfers each group to an independently chosen
// member, which could break that invariant and leave the target keyspaces
// without a primary. A single TransferPrimary is not atomic and the groups are
// iterated in no particular order, so the split state is checked for every
// candidate group up front before transferring anything; otherwise a normal
// group could be moved before a later splitting group aborts the loop, leaving
// partial side effects. Since splitting is transient, the caller should retry
// once it finishes.
// The request is rejected as a whole, before any group is touched, if this node
// is the primary of any splitting keyspace group or if new_primary does not
// identify a member of every candidate group: a split target must campaign on
// the same TSO node as its split source, but eviction transfers each group to an
// independently chosen member, which could break that invariant and leave the
// target keyspaces without a primary; an invalid new_primary would otherwise only
// be discovered mid-loop, after other groups had already been moved. A single
// TransferPrimary is not atomic and the groups are iterated in no particular
// order, so both conditions are checked for every candidate group up front;
// otherwise a normal group could be moved before a later group aborts the loop,
// leaving partial side effects. Since splitting is transient, the caller should
// retry once it finishes. Once past this pre-check, a transfer failure for an
// individual group (e.g. a transient etcd error) is best-effort and does not stop
// the others.
// @Tags primary
// @Summary Evict all keyspace group primaries held by this node.
// @Produce json
// @Success 200 {object} map[string]string
// @Failure 500 {object} map[string]string
// @Param new_primary body string false "new primary name"
// @Success 200 {object} map[string]string
// @Failure 400 {string} string "invalid request"
// @Failure 500 {object} map[string]string
// @Router /primary/evict [post]
Comment on lines +413 to 417

@coderabbitai coderabbitai Bot Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C 4 '`@Param`\s+new_primary|`@Failure`\s+400|func evictPrimary' pkg/mcs/tso/server/apis/v1/api.go
fd -t f -a | rg '(^|/)(swagger\.(json|ya?ml)|docs\.go)$' | while IFS= read -r file; do
  rg -n -C 4 '/primary/evict|new_primary' "$file"
done

Repository: tikv/pd

Length of output: 1678

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== api.go evictPrimary section =="
sed -n '400,450p' pkg/mcs/tso/server/apis/v1/api.go | cat -n

echo
echo "== tso server TransferPrimary definitions/usages =="
rg -n -C 6 'func .*TransferPrimary|TransferPrimary\(' pkg/mcs/tso/server pkg || true

echo
echo "== go.mod gin/middleware versions =="
sed -n '/github.com/gin-gonic\/gin\|github.com/swaggo\|swagger\|swagger-ui\|swaggo/d' go.mod || true
rg -n "github.com/gin-gonic/gin|github.com/swaggo|swagger|swaggo" go.mod go.sum | head -80 || true

echo
echo "== request parsing behavior probe =="
python3 - <<'PY'
import json
samples = ['"node-1"', '{"new_primary":"node-1"}']
names = ['string', 'object']
vals = []
for s in samples:
    try:
        vals.append(json.loads(s))
    except Exception as e:
        vals.append(type(e).__name__ + ': ' + str(e))
print(dict(zip(names, vals)))
PY

echo
echo "== Swagger annotated primary APIs nearby =="
sed -n '305,435p' pkg/mcs/tso/server/apis/v1/api.go | cat -n

Repository: tikv/pd

Length of output: 18869

Document the JSON object request body and the 400 response.

@Param new_primary body string describes a JSON string, but evictPrimary unmarshals an object with new_primary. Generated clients can send "node-1" and receive HTTP 400 from json.Unmarshal.

Use a named request model in the Swagger annotation. Add an HTTP 400 response definition that matches the plaintext error body. Regenerate the Swagger specification with make swagger-spec (SWAGGER=1) after the annotation 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 `@pkg/mcs/tso/server/apis/v1/api.go` around lines 409 - 412, Update the
evictPrimary Swagger annotations to reference a named request model representing
the JSON object with the new_primary field instead of a plain string. Add a 400
failure response matching the endpoint’s plaintext error body, retain the
existing 500 response, and regenerate the Swagger specification using make
swagger-spec with SWAGGER=1.

Source: Coding guidelines

@bufferflies bufferflies Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added @Failure 400 {string} string "invalid request" to document the 400 response, matching the convention used elsewhere in this file (e.g. resetTS). Kept the existing @Param new_primary body string annotation instead of introducing a named request model, since transferPrimary right above uses the same convention for the same JSON shape and this file doesn't use named Swagger models elsewhere.

@coderabbitai coderabbitai Bot Aug 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bufferflies, the added @Failure 400 resolves the response-documentation part.

The request schema issue remains. evictPrimary accepts a JSON object such as {"new_primary":"node-1"}, but @Param new_primary body string describes the complete body as a JSON string. Generated clients can therefore send "node-1", which the handler rejects.

The nearby transferPrimary annotation has the same mismatch. It is not a reason to preserve it here. Please use a request model for /primary/evict and regenerate the Swagger specification.

✏️ Learnings added 
Learnt from: bufferflies
Repo: tikv/pd PR: 11121
File: pkg/mcs/tso/server/apis/v1/api.go:410-414
Timestamp: 2026-08-07T07:25:32.839Z
Learning: In `pkg/mcs/tso/server/apis/v1/api.go`, existing Swagger annotations for `transferPrimary` use `Param new_primary body string` for a JSON object request body. This convention is inconsistent with the handler input shape and should not be treated as a correct request-schema pattern.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

func evictPrimary(c *gin.Context) {
svr := c.MustGet(multiservicesapi.ServiceContextKey).(*tsoserver.Service)

body, err := io.ReadAll(c.Request.Body)
if err != nil {
c.String(http.StatusBadRequest, err.Error())
return
}
var input struct {
NewPrimary string `json:"new_primary"`
}
if len(body) > 0 {
if err := json.Unmarshal(body, &input); err != nil {
c.String(http.StatusBadRequest, err.Error())
return
}
}
// The node being evicted cannot also be its own replacement: TransferPrimary
// treats a target equal to the current primary as a self-transfer and silently
// no-ops, which would report "success" while leaving the node undrained. Match
// on both name and service address since IsValidPrimaryCandidate and
// TransferPrimary accept either as an identifier for new_primary.
if input.NewPrimary != "" && (input.NewPrimary == svr.Name() || input.NewPrimary == svr.GetAdvertiseListenAddr()) {
c.String(http.StatusBadRequest, "new_primary must not be the node being evicted")
return
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

kgm := svr.GetKeyspaceGroupManager()

// Collect the keyspace groups this node is currently the primary of. There is
Expand All @@ -424,15 +455,36 @@ func evictPrimary(c *gin.Context) {
primaryGroupIDs = append(primaryGroupIDs, keyspaceGroupID)
}

// Resolve the service registry once and reuse it for every candidate group
// below, instead of re-fetching per group.
var entries []discovery.ServiceRegistryEntry
if input.NewPrimary != "" {
entries, err = discovery.GetMSMembers(mcs.TSOServiceName, svr.GetClient())
if err != nil {
c.AbortWithStatusJSON(http.StatusInternalServerError, err.Error())
return
}
}

// Pre-check every candidate group before transferring anything so the
// operation is all-or-nothing: reject the whole request if any of them is
// splitting, instead of moving some groups first and only then aborting.
// splitting or new_primary is not one of its members, instead of moving some
// groups first and only then aborting.
for _, keyspaceGroupID := range primaryGroupIDs {
if group := kgm.GetKeyspaceGroupByID(keyspaceGroupID); group != nil && group.IsSplitting() {
group := kgm.GetKeyspaceGroupByID(keyspaceGroupID)
if group == nil {
continue
}
if group.IsSplitting() {
c.AbortWithStatusJSON(http.StatusInternalServerError,
errs.ErrKeyspaceGroupInSplit.FastGenByArgs(keyspaceGroupID).Error())
return
}
if !utils.IsValidPrimaryCandidate(entries, input.NewPrimary, keyspaceGroupMemberMap(group)) {
c.String(http.StatusBadRequest,
fmt.Sprintf("new_primary %q is not a member of keyspace group %d", input.NewPrimary, keyspaceGroupID))
return
}
}

// results maps a keyspace group ID to the transfer outcome ("success" or the
Expand All @@ -458,10 +510,7 @@ func evictPrimary(c *gin.Context) {
}

// only members of the specific group are valid primary candidates.
memberMap := make(map[string]bool, len(group.Members))
for _, member := range group.Members {
memberMap[member.Address] = true
}
memberMap := keyspaceGroupMemberMap(group)

participant, ok := allocator.GetMember().(*member.Participant)
if !ok {
Expand All @@ -471,9 +520,8 @@ func evictPrimary(c *gin.Context) {
// has a higher priority for the group, the priority checker will move the
// primary back to it, so the eviction does not durably drain the node.
// Priority handling is being reworked, so revisit this when needed.
// An empty new primary lets TransferPrimary pick a random other member.
if err := utils.TransferPrimary(svr.GetClient(), participant,

@YuhaoZhang00 YuhaoZhang00 Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment @ Line 446 said: "the operation is all-or-nothing". What if newPrimary is not member of the group? Consider pre-validating membership for every candidate group and rejecting with 400, same shape as the splitting pre-check above.

@ystaticy ystaticy Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a P1 operational-correctness issue. evictPrimary checks only the split state before entering the per-group loop, then calls TransferPrimary with each group\x27s own memberMap. If the source node serves both of these groups:\n\n- Group 1: members A, B, C; primary A\n- Group 2: members A, B; primary A\n\nthen new_primary=C can transfer Group 1 successfully, while Group 2 returns no valid secondary to transfer primary. The handler returns HTTP 500 with a mixed result map, but the earlier transfer has already resigned A and changed cluster state. Since the group iteration order is not deterministic, the exact group that fails first is also nondeterministic.\n\nPlease pre-validate, before any TransferPrimary call, that new_primary belongs to every candidate group and is not the current node; return HTTP 400 on validation failure so the request has no side effects.

@bufferflies bufferflies Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a pre-check pass before any transfer: for every candidate group, new_primary must resolve to a member of that group (via the new utils.IsValidPrimaryCandidate, checked against a service-registry snapshot fetched once), otherwise the whole request is rejected with 400 before touching any group — matching the shape of the existing splitting pre-check.

mcs.TSOServiceName, svr.Name(), "", keyspaceGroupID, memberMap); err != nil {
mcs.TSOServiceName, svr.Name(), input.NewPrimary, keyspaceGroupID, memberMap); err != nil {
log.Warn("failed to evict keyspace group primary",
zap.Uint32("keyspace-group-id", keyspaceGroupID), errs.ZapError(err))
results[keyspaceGroupID] = err.Error()
Expand All @@ -490,6 +538,16 @@ func evictPrimary(c *gin.Context) {
c.IndentedJSON(http.StatusOK, results)
}

// keyspaceGroupMemberMap returns the set of service addresses that are members of
// the group, so callers can filter transfer candidates down to that group.
func keyspaceGroupMemberMap(group *endpoint.KeyspaceGroup) map[string]bool {
memberMap := make(map[string]bool, len(group.Members))
for _, member := range group.Members {
memberMap[member.Address] = true
}
return memberMap
}

// forwardToGroupPrimary forwards the transfer primary request to the primary of
// the given keyspace group, replaying the original request body. The request is
// fully handled (forwarded or aborted with an error) when this returns.
Expand Down
23 changes: 23 additions & 0 deletions pkg/mcs/utils/expected_primary.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,26 @@ func TransferPrimary(client *clientv3.Client, p *member.Participant, serviceName
func isSamePrimary(member discovery.ServiceRegistryEntry, primary string) bool {
return primary != "" && (member.Name == primary || member.ServiceAddr == primary)
}

// IsValidPrimaryCandidate reports whether newPrimary identifies a member of the
// group represented by tsoMembersMap, given the already-fetched registry entries
// for the service, so a caller can reject an invalid target up front instead of
// discovering it only when TransferPrimary itself fails. Callers checking multiple
// groups for one request should fetch entries once (e.g. via discovery.GetMSMembers)
// and reuse them here, rather than re-fetching per group. An empty newPrimary
// always matches: it means "let TransferPrimary pick any member", which is valid
// for every group.
func IsValidPrimaryCandidate(entries []discovery.ServiceRegistryEntry, newPrimary string, tsoMembersMap map[string]bool) bool {
if newPrimary == "" {
return true
}
for _, member := range entries {
if tsoMembersMap != nil && !tsoMembersMap[member.ServiceAddr] {
continue
}
if isSamePrimary(member, newPrimary) {
return true
}
}
return false
}
20 changes: 20 additions & 0 deletions pkg/mcs/utils/expected_primary_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,23 @@ func TestIsSamePrimary(t *testing.T) {
re.False(isSamePrimary(entry, "http://127.0.0.1:2380")) // different address
re.False(isSamePrimary(entry, "")) // empty target never matches
}

// TestIsValidPrimaryCandidate covers the pre-check evictPrimary uses to reject an
// out-of-group new_primary before transferring anything, instead of discovering it
// mid-loop after other groups have already been moved.
func TestIsValidPrimaryCandidate(t *testing.T) {
re := require.New(t)
entries := []discovery.ServiceRegistryEntry{
{Name: "tso-1", ServiceAddr: "http://127.0.0.1:1"},
{Name: "tso-2", ServiceAddr: "http://127.0.0.1:2"},
{Name: "tso-3", ServiceAddr: "http://127.0.0.1:3"},
}
// Only tso-1 and tso-2 belong to the group under evaluation.
groupMembers := map[string]bool{"http://127.0.0.1:1": true, "http://127.0.0.1:2": true}

re.True(IsValidPrimaryCandidate(entries, "", groupMembers), "an empty target always matches")
re.True(IsValidPrimaryCandidate(entries, "tso-2", groupMembers), "tso-2 is a group member")
re.True(IsValidPrimaryCandidate(entries, "http://127.0.0.1:2", groupMembers), "matching by service address must also work")
re.False(IsValidPrimaryCandidate(entries, "tso-3", groupMembers), "tso-3 is registered but not a member of this group")
re.False(IsValidPrimaryCandidate(entries, "tso-unknown", groupMembers), "an unregistered name never matches")
}
Loading
Loading