From 696ec9ee81c785cebee4dc250f5b95b652020706 Mon Sep 17 00:00:00 2001 From: tongjian <1045931706@qq.com> Date: Thu, 6 Aug 2026 17:24:37 +0800 Subject: [PATCH 1/5] evict: support new_primary in /primary/evict endpoint Add an optional new_primary body parameter to the /primary/evict handler so the caller can specify which node should receive the evicted primaries instead of the system picking a random member. close #11120 Signed-off-by: bufferflies Signed-off-by: tongjian <1045931706@qq.com> Signed-off-by: bufferflies <1045931706@qq.com> --- pkg/mcs/tso/server/apis/v1/api.go | 26 ++++++-- tests/integrations/mcs/members/member_test.go | 60 +++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index 6062ebd0a7..ac541281a8 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -406,11 +406,30 @@ func transferPrimary(c *gin.Context) { // @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 500 {object} map[string]string // @Router /primary/evict [post] 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"` + } + newPrimary := "" + if len(body) > 0 { + if err := json.Unmarshal(body, &input); err != nil { + c.String(http.StatusBadRequest, err.Error()) + return + } + newPrimary = input.NewPrimary + } + kgm := svr.GetKeyspaceGroupManager() // Collect the keyspace groups this node is currently the primary of. There is @@ -471,9 +490,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, - mcs.TSOServiceName, svr.Name(), "", keyspaceGroupID, memberMap); err != nil { + mcs.TSOServiceName, svr.Name(), 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() diff --git a/tests/integrations/mcs/members/member_test.go b/tests/integrations/mcs/members/member_test.go index 1c2e1df411..2602e960a0 100644 --- a/tests/integrations/mcs/members/member_test.go +++ b/tests/integrations/mcs/members/member_test.go @@ -422,6 +422,66 @@ func (suite *memberTestSuite) TestEvictPrimary() { return len(served) == len(groupIDs) }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) } + + // Verify that eviction with an explicit new_primary transfers all groups to + // the designated node instead of picking a random member. + src := nodeList[0] + dst := nodeList[1] + for _, id := range groupIDs { + transferData, err := json.Marshal(map[string]any{ + "new_primary": src.Name(), + "keyspace_group_id": id, + }) + re.NoError(err) + testutil.Eventually(re, func() bool { + resp, err := tests.TestDialClient.Post(src.GetAddr()+"/tso/api/v1/primary/transfer", + "application/json", bytes.NewBuffer(transferData)) + if err != nil { + return false + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + } + testutil.Eventually(re, func() bool { + serving := mustGetKeyspaceGroupMembers(re, src.(*tso.Server)) + for _, id := range groupIDs { + if serving[id] == nil || !serving[id].IsPrimary { + return false + } + } + return true + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + + evictData, err := json.Marshal(map[string]any{ + "new_primary": dst.Name(), + }) + re.NoError(err) + resp, _ := tests.TestDialClient.Post(src.GetAddr()+"/tso/api/v1/primary/evict", + "application/json", bytes.NewBuffer(evictData)) + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + results := make(map[uint32]string) + re.NoError(json.Unmarshal(body, &results), string(body)) + re.Equal(http.StatusOK, resp.StatusCode) + for _, id := range groupIDs { + re.Equalf("success", results[id], "group %d result: %q", id, results[id]) + } + // All primaries must have moved to the designated node. + testutil.Eventually(re, func() bool { + serving := mustGetKeyspaceGroupMembers(re, dst.(*tso.Server)) + for _, id := range groupIDs { + if serving[id] == nil || !serving[id].IsPrimary { + return false + } + } + return true + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + + serving := mustGetKeyspaceGroupMembers(re, nodeList[2].(*tso.Server)) + for _, m := range serving { + re.False(m.IsPrimary, "node %s should not hold any primary", nodeList[2].GetAddr()) + } } // TestEvictPrimaryRejectedWhileSplitting verifies that /primary/evict refuses to From 48f7146999e1932493758c402707a080e135c035 Mon Sep 17 00:00:00 2001 From: bufferflies <1045931706@qq.com> Date: Fri, 7 Aug 2026 09:19:58 +0200 Subject: [PATCH 2/5] evict: fix review feedback on new_primary handling Reject an out-of-group or self-targeting new_primary up front so /primary/evict stays all-or-nothing, simplify the request parsing, document the 400 response, and fix two flaky/incorrect assertions in TestEvictPrimary caused by the default keyspace group only replicating on 2 of the 3 tso nodes. Co-Authored-By: Claude Sonnet 5 Signed-off-by: bufferflies <1045931706@qq.com> --- pkg/mcs/tso/server/apis/v1/api.go | 53 +++++++++++++++---- pkg/mcs/utils/expected_primary.go | 23 ++++++++ pkg/mcs/utils/expected_primary_test.go | 20 +++++++ tests/integrations/mcs/members/member_test.go | 45 +++++++++++++--- 4 files changed, 124 insertions(+), 17 deletions(-) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index ac541281a8..74bd32c535 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -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" @@ -408,6 +409,7 @@ func transferPrimary(c *gin.Context) { // @Produce json // @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] func evictPrimary(c *gin.Context) { @@ -421,13 +423,18 @@ func evictPrimary(c *gin.Context) { var input struct { NewPrimary string `json:"new_primary"` } - newPrimary := "" if len(body) > 0 { if err := json.Unmarshal(body, &input); err != nil { c.String(http.StatusBadRequest, err.Error()) return } - newPrimary = input.NewPrimary + } + // 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. + if input.NewPrimary != "" && input.NewPrimary == svr.Name() { + c.String(http.StatusBadRequest, "new_primary must not be the node being evicted") + return } kgm := svr.GetKeyspaceGroupManager() @@ -443,15 +450,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 @@ -477,10 +505,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 { @@ -491,7 +516,7 @@ func evictPrimary(c *gin.Context) { // primary back to it, so the eviction does not durably drain the node. // Priority handling is being reworked, so revisit this when needed. if err := utils.TransferPrimary(svr.GetClient(), participant, - mcs.TSOServiceName, svr.Name(), newPrimary, 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() @@ -508,6 +533,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. diff --git a/pkg/mcs/utils/expected_primary.go b/pkg/mcs/utils/expected_primary.go index 36d90dba80..0310d2cda2 100644 --- a/pkg/mcs/utils/expected_primary.go +++ b/pkg/mcs/utils/expected_primary.go @@ -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 +} diff --git a/pkg/mcs/utils/expected_primary_test.go b/pkg/mcs/utils/expected_primary_test.go index e10b84e5a2..e66a179247 100644 --- a/pkg/mcs/utils/expected_primary_test.go +++ b/pkg/mcs/utils/expected_primary_test.go @@ -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") +} diff --git a/tests/integrations/mcs/members/member_test.go b/tests/integrations/mcs/members/member_test.go index 2602e960a0..9a01f653d6 100644 --- a/tests/integrations/mcs/members/member_test.go +++ b/tests/integrations/mcs/members/member_test.go @@ -424,9 +424,22 @@ func (suite *memberTestSuite) TestEvictPrimary() { } // Verify that eviction with an explicit new_primary transfers all groups to - // the designated node instead of picking a random member. - src := nodeList[0] - dst := nodeList[1] + // the designated node instead of picking a random member. src and dst are + // both picked from the default keyspace group's members (tsoAvailMembers) so + // dst is guaranteed a valid transfer target even if src also happens to be + // the default group's primary: unlike the 12 groups created above, the + // default group only replicates on DefaultKeyspaceGroupReplicaCount (2) of + // the 3 tso nodes, so an arbitrary dst could otherwise get rejected as "not a + // member" for it. + var defaultGroupNodes []bs.Server + for _, node := range nodeList { + if suite.tsoAvailMembers[node.GetAddr()] { + defaultGroupNodes = append(defaultGroupNodes, node) + } + } + re.Len(defaultGroupNodes, 2) + src := defaultGroupNodes[0] + dst := defaultGroupNodes[1] for _, id := range groupIDs { transferData, err := json.Marshal(map[string]any{ "new_primary": src.Name(), @@ -457,9 +470,11 @@ func (suite *memberTestSuite) TestEvictPrimary() { "new_primary": dst.Name(), }) re.NoError(err) - resp, _ := tests.TestDialClient.Post(src.GetAddr()+"/tso/api/v1/primary/evict", + resp, err := tests.TestDialClient.Post(src.GetAddr()+"/tso/api/v1/primary/evict", "application/json", bytes.NewBuffer(evictData)) - body, _ := io.ReadAll(resp.Body) + re.NoError(err) + body, err := io.ReadAll(resp.Body) + re.NoError(err) resp.Body.Close() results := make(map[uint32]string) re.NoError(json.Unmarshal(body, &results), string(body)) @@ -478,9 +493,23 @@ func (suite *memberTestSuite) TestEvictPrimary() { return true }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) - serving := mustGetKeyspaceGroupMembers(re, nodeList[2].(*tso.Server)) - for _, m := range serving { - re.False(m.IsPrimary, "node %s should not hold any primary", nodeList[2].GetAddr()) + // The remaining node (neither src nor dst) must not hold the primary of any of + // the groups this eviction covers. Restricted to groupIDs rather than every + // group the node serves: it may independently be the primary of the default + // keyspace group (untouched by this eviction, since src was never its + // primary), and that must not fail this assertion. + var other bs.Server + for _, node := range nodeList { + if node.GetAddr() != src.GetAddr() && node.GetAddr() != dst.GetAddr() { + other = node + break + } + } + re.NotNil(other) + serving := mustGetKeyspaceGroupMembers(re, other.(*tso.Server)) + for _, id := range groupIDs { + m := serving[id] + re.False(m != nil && m.IsPrimary, "node %s should not hold primary of group %d", other.GetAddr(), id) } } From 1ac1ec0fd1f1359324aa39859d851eb85895335b Mon Sep 17 00:00:00 2001 From: bufferflies <1045931706@qq.com> Date: Fri, 7 Aug 2026 09:53:36 +0200 Subject: [PATCH 3/5] evict: reject self-target by service address too new_primary matching via IsValidPrimaryCandidate and TransferPrimary accepts either name or service address, but the self-target guard only compared against the node's name, so a caller passing its own advertise address could still hit TransferPrimary's silent self-transfer no-op and get a misleading success. Also correct the evictPrimary doc comment, which no longer matched the all-or-nothing pre-check behavior. Co-Authored-By: Claude Sonnet 5 Signed-off-by: bufferflies <1045931706@qq.com> --- pkg/mcs/tso/server/apis/v1/api.go | 35 +++++++++++-------- tests/integrations/mcs/members/member_test.go | 14 ++++++++ 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index 74bd32c535..b4fa5fa66a 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -390,20 +390,23 @@ 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 @@ -431,8 +434,10 @@ func evictPrimary(c *gin.Context) { } // 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. - if input.NewPrimary != "" && input.NewPrimary == svr.Name() { + // 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 } diff --git a/tests/integrations/mcs/members/member_test.go b/tests/integrations/mcs/members/member_test.go index 9a01f653d6..0b497deb89 100644 --- a/tests/integrations/mcs/members/member_test.go +++ b/tests/integrations/mcs/members/member_test.go @@ -440,6 +440,20 @@ func (suite *memberTestSuite) TestEvictPrimary() { re.Len(defaultGroupNodes, 2) src := defaultGroupNodes[0] dst := defaultGroupNodes[1] + + // A new_primary that identifies the node being evicted, whether by name or by + // service address, must be rejected outright instead of silently no-oping + // while reporting success. + for _, self := range []string{src.Name(), src.(*tso.Server).GetAdvertiseListenAddr()} { + selfEvictData, err := json.Marshal(map[string]any{"new_primary": self}) + re.NoError(err) + resp, err := tests.TestDialClient.Post(src.GetAddr()+"/tso/api/v1/primary/evict", + "application/json", bytes.NewBuffer(selfEvictData)) + re.NoError(err) + re.Equal(http.StatusBadRequest, resp.StatusCode, "new_primary=%q should be rejected", self) + resp.Body.Close() + } + for _, id := range groupIDs { transferData, err := json.Marshal(map[string]any{ "new_primary": src.Name(), From e181dc8c9673af7070c73c58b09141d7f39ebde2 Mon Sep 17 00:00:00 2001 From: bufferflies <1045931706@qq.com> Date: Thu, 13 Aug 2026 08:46:45 +0200 Subject: [PATCH 4/5] evict: cover the all-or-nothing rejection for an invalid new_primary TestEvictPrimary only exercised /primary/evict with a new_primary valid for every candidate group, since the 12 groups it creates replicate on all 3 tso nodes and the default group's 2 members are always a subset of that. Add TestEvictPrimaryRejectedForInvalidCandidate, which creates a group new_primary is not a member of alongside several it is, and asserts the request is rejected with 400 before any of them is transferred. Signed-off-by: bufferflies <1045931706@qq.com> --- tests/integrations/mcs/members/member_test.go | 144 ++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/tests/integrations/mcs/members/member_test.go b/tests/integrations/mcs/members/member_test.go index 0b497deb89..2c86b93fad 100644 --- a/tests/integrations/mcs/members/member_test.go +++ b/tests/integrations/mcs/members/member_test.go @@ -641,6 +641,150 @@ func (suite *memberTestSuite) TestEvictPrimaryRejectedWhileSplitting() { re.True(g.IsPrimary) } +// TestEvictPrimaryRejectedForInvalidCandidate verifies that /primary/evict +// rejects the whole request, before transferring anything, when new_primary is +// not a member of every candidate group. The candidate groups are iterated in +// indeterminate map order, so a broken implementation that folds the +// membership check into the transfer loop would still pass this test on the +// runs where the restricted group happens to be visited first. Using several +// normal groups alongside the one restricted group raises the odds that at +// least one of them is visited, and thus wrongly transferred, before that +// happens. +func (suite *memberTestSuite) TestEvictPrimaryRejectedForInvalidCandidate() { + re := suite.Require() + re.Len(suite.tsoNodes, 3) + + // target must not be a default keyspace group member: that group's primary + // election is not controlled by this test, and if target ended up serving it, + // evictPrimary's pre-check would run against it too, on top of the groups + // created below. suite.tsoAvailMembers is captured once in SetupTest right + // after the nodes start, before default group allocation is guaranteed to have + // converged to its full replica count, so re-read the current membership here + // instead of trusting that snapshot. + var probe bs.Server + for _, node := range suite.tsoNodes { + probe = node + break + } + defaultGroupMemberAddrs := make(map[string]bool, mcs.DefaultKeyspaceGroupReplicaCount) + testutil.Eventually(re, func() bool { + g, ok := mustGetKeyspaceGroupMembers(re, probe.(*tso.Server))[constant.DefaultKeyspaceGroupID] + if !ok || len(g.Group.Members) != mcs.DefaultKeyspaceGroupReplicaCount { + return false + } + for _, m := range g.Group.Members { + defaultGroupMemberAddrs[m.Address] = true + } + return true + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(100*time.Millisecond)) + + var target bs.Server + defaultGroupNodes := make([]bs.Server, 0, mcs.DefaultKeyspaceGroupReplicaCount) + for _, node := range suite.tsoNodes { + if defaultGroupMemberAddrs[node.GetAddr()] { + defaultGroupNodes = append(defaultGroupNodes, node) + } else { + target = node + } + } + re.NotNil(target) + re.Len(defaultGroupNodes, mcs.DefaultKeyspaceGroupReplicaCount) + other := defaultGroupNodes[0] + outsider := defaultGroupNodes[1] + + nodeList := []bs.Server{target, other, outsider} + allMembers := make([]endpoint.KeyspaceGroupMember, 0, len(nodeList)) + for _, node := range nodeList { + allMembers = append(allMembers, endpoint.KeyspaceGroupMember{ + Address: node.GetAddr(), + Priority: mcs.DefaultKeyspaceGroupReplicaPriority, + }) + } + restrictedMembers := []endpoint.KeyspaceGroupMember{ + {Address: target.GetAddr(), Priority: mcs.DefaultKeyspaceGroupReplicaPriority}, + {Address: other.GetAddr(), Priority: mcs.DefaultKeyspaceGroupReplicaPriority}, + } + + normalIDs := []uint32{1, 2, 3} + const restrictedID = uint32(4) + // outsider is a member of each of these groups, so it passes their pre-check. + for i, id := range normalIDs { + handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{ + KeyspaceGroups: []*endpoint.KeyspaceGroup{ + { + ID: id, + UserKind: endpoint.Standard.String(), + Members: allMembers, + Keyspaces: []uint32{uint32(3000 + i)}, + }, + }, + }) + } + // outsider is not a member of this group, so evicting target with + // new_primary=outsider must be rejected for it. + handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{ + KeyspaceGroups: []*endpoint.KeyspaceGroup{ + { + ID: restrictedID, + UserKind: endpoint.Standard.String(), + Members: restrictedMembers, + Keyspaces: []uint32{3010}, + }, + }, + }) + + // Concentrate every group's primary on target. + allIDs := append(append([]uint32{}, normalIDs...), restrictedID) + for _, id := range allIDs { + transferData, err := json.Marshal(map[string]any{ + "new_primary": target.Name(), + "keyspace_group_id": id, + }) + re.NoError(err) + testutil.Eventually(re, func() bool { + resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/transfer", + "application/json", bytes.NewBuffer(transferData)) + if err != nil { + return false + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + } + testutil.Eventually(re, func() bool { + serving := mustGetKeyspaceGroupMembers(re, target.(*tso.Server)) + for _, id := range allIDs { + if serving[id] == nil || !serving[id].IsPrimary { + return false + } + } + return true + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + + // outsider is a valid candidate for every normal group but not for + // restrictedID. + evictData, err := json.Marshal(map[string]any{"new_primary": outsider.Name()}) + re.NoError(err) + resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/evict", + "application/json", bytes.NewBuffer(evictData)) + re.NoError(err) + body, err := io.ReadAll(resp.Body) + re.NoError(err) + resp.Body.Close() + re.Equal(http.StatusBadRequest, resp.StatusCode, string(body)) + re.Contains(string(body), fmt.Sprintf("keyspace group %d", restrictedID)) + + // The rejection must happen before any transfer, so every normal group's + // primary is still on target even though outsider would have been a valid + // destination for all of them. + serving := mustGetKeyspaceGroupMembers(re, target.(*tso.Server)) + for _, id := range normalIDs { + g, ok := serving[id] + re.True(ok) + re.True(g.IsPrimary) + } +} + func (suite *memberTestSuite) TestCampaignPrimaryAfterTransfer() { re := suite.Require() supportedServices := []string{"tso", "scheduling", "resource_manager"} From 5480cfd5454c201ee75f43b74009f558f58818a9 Mon Sep 17 00:00:00 2001 From: bufferflies <1045931706@qq.com> Date: Fri, 14 Aug 2026 06:48:05 +0200 Subject: [PATCH 5/5] evict: harden the new_primary regression tests Poll all three tso nodes for the default group's live membership instead of trusting SetupTest's early suite.tsoAvailMembers snapshot or an arbitrarily chosen probe node, which can miss group 0 entirely if it happens to pick the one node that is not a member. Repeat the invalid-candidate scenario across 5 trials with fresh group IDs so an undetected regression would have to miss the map-iteration-order coin flip on every trial, and clean up each trial's primaries so they do not leak into the next one's candidate set. Check the response body Close error at each call site touched here. Signed-off-by: bufferflies <1045931706@qq.com> --- tests/integrations/mcs/members/member_test.go | 247 +++++++++++------- 1 file changed, 151 insertions(+), 96 deletions(-) diff --git a/tests/integrations/mcs/members/member_test.go b/tests/integrations/mcs/members/member_test.go index 2c86b93fad..28d75954e0 100644 --- a/tests/integrations/mcs/members/member_test.go +++ b/tests/integrations/mcs/members/member_test.go @@ -425,19 +425,38 @@ func (suite *memberTestSuite) TestEvictPrimary() { // Verify that eviction with an explicit new_primary transfers all groups to // the designated node instead of picking a random member. src and dst are - // both picked from the default keyspace group's members (tsoAvailMembers) so - // dst is guaranteed a valid transfer target even if src also happens to be - // the default group's primary: unlike the 12 groups created above, the - // default group only replicates on DefaultKeyspaceGroupReplicaCount (2) of - // the 3 tso nodes, so an arbitrary dst could otherwise get rejected as "not a - // member" for it. + // both picked from the default keyspace group's members so dst is guaranteed + // a valid transfer target even if src also happens to be the default group's + // primary: unlike the 12 groups created above, the default group only + // replicates on DefaultKeyspaceGroupReplicaCount (2) of the 3 tso nodes, so + // an arbitrary dst could otherwise get rejected as "not a member" for it. + // suite.tsoAvailMembers is captured once in SetupTest right after the nodes + // start, before default group allocation is guaranteed to have converged to + // its full replica count, so poll the current membership here instead of + // trusting that snapshot: check every node and use whichever one first + // reports the default group at its full replica count. + defaultGroupMemberAddrs := make(map[string]bool, mcs.DefaultKeyspaceGroupReplicaCount) + testutil.Eventually(re, func() bool { + for _, node := range nodeList { + g, ok := mustGetKeyspaceGroupMembers(re, node.(*tso.Server))[constant.DefaultKeyspaceGroupID] + if !ok || len(g.Group.Members) != mcs.DefaultKeyspaceGroupReplicaCount { + continue + } + for _, m := range g.Group.Members { + defaultGroupMemberAddrs[m.Address] = true + } + return true + } + return false + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(100*time.Millisecond)) + var defaultGroupNodes []bs.Server for _, node := range nodeList { - if suite.tsoAvailMembers[node.GetAddr()] { + if defaultGroupMemberAddrs[node.GetAddr()] { defaultGroupNodes = append(defaultGroupNodes, node) } } - re.Len(defaultGroupNodes, 2) + re.Len(defaultGroupNodes, mcs.DefaultKeyspaceGroupReplicaCount) src := defaultGroupNodes[0] dst := defaultGroupNodes[1] @@ -451,7 +470,7 @@ func (suite *memberTestSuite) TestEvictPrimary() { "application/json", bytes.NewBuffer(selfEvictData)) re.NoError(err) re.Equal(http.StatusBadRequest, resp.StatusCode, "new_primary=%q should be rejected", self) - resp.Body.Close() + re.NoError(resp.Body.Close()) } for _, id := range groupIDs { @@ -466,8 +485,8 @@ func (suite *memberTestSuite) TestEvictPrimary() { if err != nil { return false } - defer resp.Body.Close() - return resp.StatusCode == http.StatusOK + ok := resp.StatusCode == http.StatusOK + return resp.Body.Close() == nil && ok }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) } testutil.Eventually(re, func() bool { @@ -489,7 +508,7 @@ func (suite *memberTestSuite) TestEvictPrimary() { re.NoError(err) body, err := io.ReadAll(resp.Body) re.NoError(err) - resp.Body.Close() + re.NoError(resp.Body.Close()) results := make(map[uint32]string) re.NoError(json.Unmarshal(body, &results), string(body)) re.Equal(http.StatusOK, resp.StatusCode) @@ -644,12 +663,13 @@ func (suite *memberTestSuite) TestEvictPrimaryRejectedWhileSplitting() { // TestEvictPrimaryRejectedForInvalidCandidate verifies that /primary/evict // rejects the whole request, before transferring anything, when new_primary is // not a member of every candidate group. The candidate groups are iterated in -// indeterminate map order, so a broken implementation that folds the -// membership check into the transfer loop would still pass this test on the -// runs where the restricted group happens to be visited first. Using several -// normal groups alongside the one restricted group raises the odds that at -// least one of them is visited, and thus wrongly transferred, before that -// happens. +// indeterminate map order in production, so a broken implementation that folds +// the membership check into the transfer loop would still pass a single trial +// whenever the restricted group happens to be visited first: with 3 normal +// groups alongside the 1 restricted group, that is a 1-in-4 chance per trial. +// Repeat the scenario with fresh group IDs so an undetected regression would +// have to miss on every trial: trials independently reshuffle the map, so the +// miss probability compounds to 0.25^trials. func (suite *memberTestSuite) TestEvictPrimaryRejectedForInvalidCandidate() { re := suite.Require() re.Len(suite.tsoNodes, 3) @@ -658,24 +678,25 @@ func (suite *memberTestSuite) TestEvictPrimaryRejectedForInvalidCandidate() { // election is not controlled by this test, and if target ended up serving it, // evictPrimary's pre-check would run against it too, on top of the groups // created below. suite.tsoAvailMembers is captured once in SetupTest right - // after the nodes start, before default group allocation is guaranteed to have - // converged to its full replica count, so re-read the current membership here - // instead of trusting that snapshot. - var probe bs.Server - for _, node := range suite.tsoNodes { - probe = node - break - } + // after the nodes start, before default group allocation is guaranteed to + // have converged to its full replica count, so poll the current membership + // here instead of trusting that snapshot: check every node and use whichever + // one first reports the default group at its full replica count. Querying a + // single arbitrarily chosen node would not work here, since a node reports + // group 0 only if it is itself a member of it. defaultGroupMemberAddrs := make(map[string]bool, mcs.DefaultKeyspaceGroupReplicaCount) testutil.Eventually(re, func() bool { - g, ok := mustGetKeyspaceGroupMembers(re, probe.(*tso.Server))[constant.DefaultKeyspaceGroupID] - if !ok || len(g.Group.Members) != mcs.DefaultKeyspaceGroupReplicaCount { - return false - } - for _, m := range g.Group.Members { - defaultGroupMemberAddrs[m.Address] = true + for _, node := range suite.tsoNodes { + g, ok := mustGetKeyspaceGroupMembers(re, node.(*tso.Server))[constant.DefaultKeyspaceGroupID] + if !ok || len(g.Group.Members) != mcs.DefaultKeyspaceGroupReplicaCount { + continue + } + for _, m := range g.Group.Members { + defaultGroupMemberAddrs[m.Address] = true + } + return true } - return true + return false }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(100*time.Millisecond)) var target bs.Server @@ -705,83 +726,117 @@ func (suite *memberTestSuite) TestEvictPrimaryRejectedForInvalidCandidate() { {Address: other.GetAddr(), Priority: mcs.DefaultKeyspaceGroupReplicaPriority}, } - normalIDs := []uint32{1, 2, 3} - const restrictedID = uint32(4) - // outsider is a member of each of these groups, so it passes their pre-check. - for i, id := range normalIDs { + const ( + trials = 5 + normalPerTrial = 3 + ) + for trial := range trials { + base := uint32(trial*10) + 1 + normalIDs := make([]uint32, normalPerTrial) + for i := range normalIDs { + normalIDs[i] = base + uint32(i) + } + restrictedID := base + normalPerTrial + + // outsider is a member of each of these groups, so it passes their + // pre-check. + for i, id := range normalIDs { + handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{ + KeyspaceGroups: []*endpoint.KeyspaceGroup{ + { + ID: id, + UserKind: endpoint.Standard.String(), + Members: allMembers, + Keyspaces: []uint32{3000 + base + uint32(i)}, + }, + }, + }) + } + // outsider is not a member of this group, so evicting target with + // new_primary=outsider must be rejected for it. handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{ KeyspaceGroups: []*endpoint.KeyspaceGroup{ { - ID: id, + ID: restrictedID, UserKind: endpoint.Standard.String(), - Members: allMembers, - Keyspaces: []uint32{uint32(3000 + i)}, + Members: restrictedMembers, + Keyspaces: []uint32{3000 + base + normalPerTrial}, }, }, }) - } - // outsider is not a member of this group, so evicting target with - // new_primary=outsider must be rejected for it. - handlersutil.MustCreateKeyspaceGroup(re, suite.server, &handlers.CreateKeyspaceGroupParams{ - KeyspaceGroups: []*endpoint.KeyspaceGroup{ - { - ID: restrictedID, - UserKind: endpoint.Standard.String(), - Members: restrictedMembers, - Keyspaces: []uint32{3010}, - }, - }, - }) - // Concentrate every group's primary on target. - allIDs := append(append([]uint32{}, normalIDs...), restrictedID) - for _, id := range allIDs { - transferData, err := json.Marshal(map[string]any{ - "new_primary": target.Name(), - "keyspace_group_id": id, - }) - re.NoError(err) + // Concentrate every group's primary on target. + allIDs := append(append([]uint32{}, normalIDs...), restrictedID) + for _, id := range allIDs { + transferData, err := json.Marshal(map[string]any{ + "new_primary": target.Name(), + "keyspace_group_id": id, + }) + re.NoError(err) + testutil.Eventually(re, func() bool { + resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/transfer", + "application/json", bytes.NewBuffer(transferData)) + if err != nil { + return false + } + ok := resp.StatusCode == http.StatusOK + return resp.Body.Close() == nil && ok + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + } testutil.Eventually(re, func() bool { - resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/transfer", - "application/json", bytes.NewBuffer(transferData)) - if err != nil { - return false + serving := mustGetKeyspaceGroupMembers(re, target.(*tso.Server)) + for _, id := range allIDs { + if serving[id] == nil || !serving[id].IsPrimary { + return false + } } - defer resp.Body.Close() - return resp.StatusCode == http.StatusOK + return true }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) - } - testutil.Eventually(re, func() bool { + + // outsider is a valid candidate for every normal group but not for + // restrictedID. + evictData, err := json.Marshal(map[string]any{"new_primary": outsider.Name()}) + re.NoError(err) + resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/evict", + "application/json", bytes.NewBuffer(evictData)) + re.NoError(err) + body, err := io.ReadAll(resp.Body) + re.NoError(err) + re.NoError(resp.Body.Close()) + re.Equal(http.StatusBadRequest, resp.StatusCode, string(body)) + re.Contains(string(body), fmt.Sprintf("keyspace group %d", restrictedID)) + + // The rejection must happen before any transfer, so every normal + // group's primary is still on target even though outsider would have + // been a valid destination for all of them. serving := mustGetKeyspaceGroupMembers(re, target.(*tso.Server)) - for _, id := range allIDs { - if serving[id] == nil || !serving[id].IsPrimary { - return false - } + for _, id := range normalIDs { + g, ok := serving[id] + re.True(ok) + re.True(g.IsPrimary) } - return true - }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) - // outsider is a valid candidate for every normal group but not for - // restrictedID. - evictData, err := json.Marshal(map[string]any{"new_primary": outsider.Name()}) - re.NoError(err) - resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/evict", - "application/json", bytes.NewBuffer(evictData)) - re.NoError(err) - body, err := io.ReadAll(resp.Body) - re.NoError(err) - resp.Body.Close() - re.Equal(http.StatusBadRequest, resp.StatusCode, string(body)) - re.Contains(string(body), fmt.Sprintf("keyspace group %d", restrictedID)) - - // The rejection must happen before any transfer, so every normal group's - // primary is still on target even though outsider would have been a valid - // destination for all of them. - serving := mustGetKeyspaceGroupMembers(re, target.(*tso.Server)) - for _, id := range normalIDs { - g, ok := serving[id] - re.True(ok) - re.True(g.IsPrimary) + // Move every group's primary off target so the next trial's groups are + // the only ones evictPrimary considers: since this trial's eviction was + // rejected, target is still the primary of all of them, and leaving them + // there would make the next trial's error message reference this + // trial's restrictedID instead of its own. + for _, id := range allIDs { + transferData, err := json.Marshal(map[string]any{ + "new_primary": other.Name(), + "keyspace_group_id": id, + }) + re.NoError(err) + testutil.Eventually(re, func() bool { + resp, err := tests.TestDialClient.Post(target.GetAddr()+"/tso/api/v1/primary/transfer", + "application/json", bytes.NewBuffer(transferData)) + if err != nil { + return false + } + ok := resp.StatusCode == http.StatusOK + return resp.Body.Close() == nil && ok + }, testutil.WithWaitFor(10*time.Second), testutil.WithTickInterval(50*time.Millisecond)) + } } }