meta-service-group: change add method to patch (tikv#380)

* change to patch

Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>

* fix swagger doc

Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>

---------

Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com>

Author: AmoebaProtozoa

server/apiv2/handlers/meta_service_group.go

@@ -15,104 +15,91 @@
 package handlers
 
 import (
-	"fmt"
+	"maps"
 	"net/http"
 	"sort"
 
 	"github.com/gin-gonic/gin"
 	"github.com/tikv/pd/pkg/errs"
+	"github.com/tikv/pd/pkg/keyspace"
 	"github.com/tikv/pd/server"
 	"github.com/tikv/pd/server/apiv2/middlewares"
 )
 
 const (
 	metaServiceGroupUninitializedErr = "meta-service groups manager is not initialized"
-	metaServiceGroupAlreadyExistsErr = "meta-service group %s already exists"
 )
 
 // RegisterMetaServiceGroup registers meta-service group related handlers to router paths.
 func RegisterMetaServiceGroup(r *gin.RouterGroup) {
 	router := r.Group("meta-service-groups")
 	router.Use(middlewares.BootstrapChecker())
 	router.GET("", GetMetaServiceGroups)
-	router.POST("", AddMetaServiceGroups)
+	router.PATCH("", PatchMetaServiceGroups)
 }
 
 // MetaServiceGroupStatus represents the status of a meta-service group.
 // NOTE: This type is exported by HTTP API. Please pay more attention when modifying it.
 type MetaServiceGroupStatus struct {
-	ID                string `json:"id"`
-	Addresses         string `json:"addresses"`
-	AssignedKeyspaces int    `json:"assigned_keyspaces"`
-}
-
-// AddMetaServiceGroupRequest represents a request to add a meta-service group.
-// NOTE: This type is exported by HTTP API. Please pay more attention when modifying it.
-type AddMetaServiceGroupRequest struct {
-	ID        string `json:"id"`
+	// ID is the unique identifier of the meta-service group.
+	ID string `json:"id"`
+	// Addresses is a comma-separated list of addresses for the meta-service group.
 	Addresses string `json:"addresses"`
+	// AssignedKeyspaces is the number of keyspaces assigned to this meta-service group.
+	AssignedKeyspaces int `json:"assigned_keyspaces"`
 }
 
-// AddMetaServiceGroups adds one or more meta-service groups as specified in the request.
+// PatchMetaServiceGroups applies a JSON Merge Patch to the meta-service groups.
 //
 // @Tags     meta-service-groups
-// @Summary  Adds new meta-service groups.
-// @Param    body  body  []AddMetaServiceGroupRequest  true  "List of meta-service groups to add"
+// @Summary  Patch meta-service groups using JSON Merge Patch.
+// @Param    body  body  object  true  "JSON Merge Patch for meta-service groups (string values for add/update, null for delete)"
 // @Produce  json
-// @Success  200  {object}  []MetaServiceGroupStatus  "List of newly added plus existing groups"
-// @Failure  400  {string}  string                    "Bad request (invalid JSON or duplicate group)"
+// @Success  200  {object}  []MetaServiceGroupStatus  "List of all meta-service groups after patch"
+// @Failure  400  {string}  string                    "Bad request (invalid JSON or invalid operation)"
 // @Failure  500  {string}  string                    "Internal server error"
-// @Router   /meta-service-groups [post]
-func AddMetaServiceGroups(c *gin.Context) {
+// @Router   /meta-service-groups [patch]
+func PatchMetaServiceGroups(c *gin.Context) {
 	svr := c.MustGet(middlewares.ServerContextKey).(*server.Server)
 	manager := svr.GetMetaServiceGroupManager()
 	if manager == nil {
 		c.AbortWithStatusJSON(http.StatusInternalServerError, metaServiceGroupUninitializedErr)
 		return
 	}
 
-	var requests []AddMetaServiceGroupRequest
-	err := c.BindJSON(&requests)
-	if err != nil {
+	var patch map[string]*string
+	if err := c.BindJSON(&patch); err != nil {
 		c.AbortWithStatusJSON(http.StatusBadRequest, errs.ErrBindJSON.Wrap(err).GenWithStackByCause())
 		return
 	}
-	// Constructs new meta-service groups.
+
 	currentGroups := manager.GetGroups()
 	newGroups := make(map[string]string)
-	for _, request := range requests {
-		// Update existing newGroups is not allowed via the post-method.
-		if _, exists := currentGroups[request.ID]; exists {
-			c.AbortWithStatusJSON(http.StatusBadRequest, fmt.Errorf(metaServiceGroupAlreadyExistsErr, request.ID))
-			return
+	maps.Copy(newGroups, currentGroups)
+
+	for id, addresses := range patch {
+		if addresses == nil {
+			// Remove operation
+			delete(newGroups, id)
+		} else {
+			// Add or update operation
+			newGroups[id] = *addresses
 		}
-		newGroups[request.ID] = request.Addresses
 	}
-	for id, addresses := range currentGroups {
-		newGroups[id] = addresses
-	}
-	// Update persisted pd config.
+
 	keyspaceCfg := svr.GetConfig().Keyspace
 	keyspaceCfg.MetaServiceGroups = newGroups
-	if err = svr.SetKeyspaceConfig(keyspaceCfg); err != nil {
+	if err := svr.SetKeyspaceConfig(keyspaceCfg); err != nil {
 		c.AbortWithStatusJSON(http.StatusInternalServerError, err.Error())
 		return
 	}
 
-	assignmentCounts, err := manager.GetAssignmentCounts()
+	status, err := buildMetaServiceGroupStatus(manager)
 	if err != nil {
 		c.AbortWithStatusJSON(http.StatusInternalServerError, err.Error())
 		return
 	}
-	response := make([]MetaServiceGroupStatus, 0, len(newGroups))
-	for id, addresses := range newGroups {
-		response = append(response, MetaServiceGroupStatus{
-			ID:                id,
-			Addresses:         addresses,
-			AssignedKeyspaces: assignmentCounts[id],
-		})
-	}
-	c.IndentedJSON(http.StatusOK, response)
+	c.IndentedJSON(http.StatusOK, status)
 }
 
 // GetMetaServiceGroups returns a list of all meta-service groups and their assignment counts.
@@ -131,24 +118,32 @@ func GetMetaServiceGroups(c *gin.Context) {
 		return
 	}
 
-	groups := manager.GetGroups()
-	assignmentCounts, err := manager.GetAssignmentCounts()
+	status, err := buildMetaServiceGroupStatus(manager)
 	if err != nil {
 		c.AbortWithStatusJSON(http.StatusInternalServerError, err.Error())
 		return
 	}
+	c.IndentedJSON(http.StatusOK, status)
+}
 
-	response := make([]MetaServiceGroupStatus, 0, len(groups))
-	for id, addresses := range groups {
-		response = append(response, MetaServiceGroupStatus{
+func buildMetaServiceGroupStatus(manager *keyspace.MetaServiceGroupManager) ([]MetaServiceGroupStatus, error) {
+	currentGroups := manager.GetGroups()
+	assignmentCounts, err := manager.GetAssignmentCounts()
+	if err != nil {
+		return nil, err
+	}
+
+	status := make([]MetaServiceGroupStatus, 0, len(currentGroups))
+	for id, addresses := range currentGroups {
+		status = append(status, MetaServiceGroupStatus{
 			ID:                id,
 			Addresses:         addresses,
 			AssignedKeyspaces: assignmentCounts[id],
 		})
 	}
 	// sort for deterministic output
-	sort.Slice(response, func(i, j int) bool {
-		return response[i].ID < response[j].ID
+	sort.Slice(status, func(i, j int) bool {
+		return status[i].ID < status[j].ID
 	})
-	c.IndentedJSON(http.StatusOK, response)
+	return status, nil
 }

tests/server/apiv2/handlers/meta_service_group_test.go

@@ -16,7 +16,6 @@ package handlers
 
 import (
 	"context"
-	"net/http"
 	"testing"
 
 	"github.com/pingcap/failpoint"
@@ -114,18 +113,15 @@ func (suite *metaServiceGroupTestSuite) TestMetaServiceGroupOperations() {
 		re.InDelta(collectedStatus.AssignedKeyspaces, len(keyspaces)/len(groups), 1)
 	}
 	// Add two more meta-service groups.
-	newGroups := []*handlers.AddMetaServiceGroupRequest{
-		{
-			ID:        "etcd-group-4",
-			Addresses: "etcd-group-4.tidb-serverless.cluster.svc.local",
-		},
-		{
-			ID:        "etcd-group-5",
-			Addresses: "etcd-group-5.tidb-serverless.cluster.svc.local",
-		},
+	addr4 := "etcd-group-4.tidb-serverless.cluster.svc.local"
+	addr5 := "etcd-group-5.tidb-serverless.cluster.svc.local"
+	patch := map[string]*string{
+		"etcd-group-4": &addr4,
+		"etcd-group-5": &addr5,
 	}
-	groups = mustAddMetaServiceGroups(re, suite.server, newGroups)
-	re.Equal(len(groups), len(mockMetaServiceGroups())+len(newGroups))
+
+	groups = mustAddMetaServiceGroups(re, suite.server, patch)
+	re.Equal(len(groups), len(mockMetaServiceGroups())+len(patch))
 	// Newly assigned meta-service group should have no assigned keyspace.
 	for _, group := range groups {
 		if collectedGroups[group.ID] == nil {
@@ -144,7 +140,27 @@ func (suite *metaServiceGroupTestSuite) TestMetaServiceGroupOperations() {
 		// Make sure keyspaces are relatively evenly distributed among meta-service groups.
 		re.InDelta(collectedStatus.AssignedKeyspaces, len(keyspaces)/len(groups), 1)
 	}
-	// Add the same keyspace group should result in error.
-	code, _ := tryAddMetaServiceGroups(re, suite.server, newGroups)
-	re.Equal(http.StatusBadRequest, code)
+	// Modify address of etcd-group-1
+	newAddr := "etcd-group-1-modified.tidb-serverless.cluster.svc.local"
+	modifyPatch := map[string]*string{
+		"etcd-group-1": &newAddr,
+	}
+	groups = mustAddMetaServiceGroups(re, suite.server, modifyPatch)
+	found := false
+	for _, group := range groups {
+		if group.ID == "etcd-group-1" {
+			found = true
+			re.Equal(newAddr, group.Addresses)
+		}
+	}
+	re.True(found, "etcd-group-1 should exist after modify")
+
+	// Delete etcd-group-2
+	deletePatch := map[string]*string{
+		"etcd-group-2": nil,
+	}
+	groups = mustAddMetaServiceGroups(re, suite.server, deletePatch)
+	for _, group := range groups {
+		re.NotEqual("etcd-group-2", group.ID, "etcd-group-2 should be deleted")
+	}
 }

tests/server/apiv2/handlers/testutil.go

@@ -287,26 +287,18 @@ func mustLoadMetaServiceGroups(re *require.Assertions, server *tests.TestServer)
 	return groups
 }
 
-func mustAddMetaServiceGroups(re *require.Assertions, server *tests.TestServer, request []*handlers.AddMetaServiceGroupRequest) []*handlers.MetaServiceGroupStatus {
-	code, groups := tryAddMetaServiceGroups(re, server, request)
-	re.Equal(http.StatusOK, code)
-	return groups
-}
-
-func tryAddMetaServiceGroups(re *require.Assertions, server *tests.TestServer, request []*handlers.AddMetaServiceGroupRequest) (int, []*handlers.MetaServiceGroupStatus) {
-	data, err := json.Marshal(request)
+func mustAddMetaServiceGroups(re *require.Assertions, server *tests.TestServer, patch map[string]*string) []*handlers.MetaServiceGroupStatus {
+	data, err := json.Marshal(patch)
 	re.NoError(err)
-	httpReq, err := http.NewRequest(http.MethodPost, server.GetAddr()+metaServiceGroupsPrefix, bytes.NewBuffer(data))
+	httpReq, err := http.NewRequest(http.MethodPatch, server.GetAddr()+metaServiceGroupsPrefix, bytes.NewBuffer(data))
 	re.NoError(err)
 	resp, err := tests.TestDialClient.Do(httpReq)
 	re.NoError(err)
 	defer resp.Body.Close()
 	data, err = io.ReadAll(resp.Body)
 	re.NoError(err)
-	if resp.StatusCode != http.StatusOK {
-		return resp.StatusCode, nil
-	}
+	re.Equal(http.StatusOK, resp.StatusCode)
 	var groups []*handlers.MetaServiceGroupStatus
 	re.NoError(json.Unmarshal(data, &groups))
-	return resp.StatusCode, groups
+	return groups
 }

tools/pd-ctl/pdctl/command/meta_service_group_command.go

@@ -21,7 +21,6 @@ import (
 	"strings"
 
 	"github.com/spf13/cobra"
-	"github.com/tikv/pd/server/apiv2/handlers"
 )
 
 const (
@@ -36,7 +35,8 @@ func NewMetaServiceGroupCommand() *cobra.Command {
 		Short: "meta-service group commands",
 	}
 	cmd.AddCommand(newListMetaServiceGroupCommand())
-	cmd.AddCommand(newAddMetaServiceGroupCommand())
+	cmd.AddCommand(newUpdateMetaServiceGroupCommand())
+	cmd.AddCommand(newDeleteMetaServiceGroupCommand())
 	return cmd
 }
 
@@ -62,54 +62,77 @@ func listMetaServiceGroupFunc(cmd *cobra.Command, args []string) {
 	cmd.Println(resp)
 }
 
-func newAddMetaServiceGroupCommand() *cobra.Command {
+func newUpdateMetaServiceGroupCommand() *cobra.Command {
 	r := &cobra.Command{
-		Use:   "add",
-		Short: "add one or more meta-service groups",
-		Run:   newAddMetaServiceGroupFunc,
+		Use:   "update",
+		Short: "add or update one or more meta-service groups",
+		Run:   newUpdateMetaServiceGroupFunc,
 	}
-	r.Flags().StringArrayP(nmGroup, "g", nil, "meta-service group in format id=addr1,addr2,...")
+	r.Flags().StringArrayP(nmGroup, "g", nil, "meta-service group in format id=addr1,addr2,... (for add/update)")
 	_ = r.MarkFlagRequired(nmGroup)
 	return r
 }
 
-func newAddMetaServiceGroupFunc(cmd *cobra.Command, _ []string) {
-	// Parse the new groups.
+func newUpdateMetaServiceGroupFunc(cmd *cobra.Command, _ []string) {
 	metaServiceGroups, err := cmd.Flags().GetStringArray("group")
 	if err != nil {
 		cmd.PrintErrln("Failed to read --group flag:", err)
 		return
 	}
-
 	if len(metaServiceGroups) == 0 {
 		cmd.PrintErrln("At least one --group must be specified")
 		cmd.Usage()
 		return
 	}
-	var params []handlers.AddMetaServiceGroupRequest
+	patch := make(map[string]*string)
 	for _, group := range metaServiceGroups {
 		parts := strings.SplitN(group, "=", 2)
 		if len(parts) != 2 {
-			cmd.PrintErrf("Invalid --group format: %q (expected id=addr1,addr2,...)\n", group)
+			cmd.PrintErrf("Invalid --group format: %q (expected id=addr1,addr2,...)", group)
 			return
 		}
-		params = append(params, handlers.AddMetaServiceGroupRequest{
-			ID:        strings.TrimSpace(parts[0]),
-			Addresses: strings.TrimSpace(parts[1]),
-		})
+		addr := strings.TrimSpace(parts[1])
+		patch[strings.TrimSpace(parts[0])] = &addr
 	}
-
-	body, err := json.Marshal(params)
+	body, err := json.Marshal(patch)
 	if err != nil {
 		cmd.PrintErrln("Failed to marshal request:", err)
 		return
 	}
-	resp, err := doRequest(cmd, metaServiceGroupPrefix, http.MethodPost,
+	resp, err := doRequest(cmd, metaServiceGroupPrefix, http.MethodPatch,
 		http.Header{"Content-Type": {"application/json"}}, WithBody(bytes.NewBuffer(body)))
 	if err != nil {
-		cmd.PrintErrln("Failed to add meta-service group:", err)
+		cmd.PrintErrln("Failed to update meta-service group:", err)
 		return
 	}
+	cmd.Println(resp)
+}
+
+func newDeleteMetaServiceGroupCommand() *cobra.Command {
+	r := &cobra.Command{
+		Use:   "delete <id> [<id> ...]",
+		Short: "delete one or more meta-service groups by id",
+		Args:  cobra.MinimumNArgs(1),
+		Run:   newDeleteMetaServiceGroupFunc,
+	}
+	return r
+}
 
+func newDeleteMetaServiceGroupFunc(cmd *cobra.Command, args []string) {
+	patch := make(map[string]*string)
+	for _, id := range args {
+		patch[strings.TrimSpace(id)] = nil
+	}
+	body, err := json.Marshal(patch)
+	if err != nil {
+		cmd.PrintErrln("Failed to marshal request:", err)
+		return
+	}
+	resp, err := doRequest(cmd, metaServiceGroupPrefix, http.MethodPatch,
+		http.Header{"Content-Type": {"application/json"}}, WithBody(bytes.NewBuffer(body)))
+	if err != nil {
+		cmd.PrintErrln("Failed to delete meta-service group:", err)
+		return
+	}
 	cmd.Println(resp)
 }