resourcemanager: do not check existence when add resource group (#6717)…

… (#6739) ref #5851, ref pingcap/tidb#45050 Signed-off-by: glorv <glorvs@163.com> Co-authored-by: glorv <glorvs@163.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
tikv · Oct 12, 2023 · b0477c9 · b0477c9
1 parent 627ec8a
commit b0477c9
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 22 deletions.
diff --git a/errors.toml b/errors.toml
@@ -596,11 +596,6 @@ error = '''
 invalid group settings, please check the group name, priority and the number of resources
 '''
 
-["PD:resourcemanager:ErrResourceGroupAlreadyExists"]
-error = '''
-the %s resource group already exists
-'''
-
 ["PD:schedule:ErrCreateOperator"]
 error = '''
 unable to create operator, %s

diff --git a/pkg/errs/errno.go b/pkg/errs/errno.go
@@ -368,8 +368,7 @@ var (
 
 // Resource Manager errors
 var (
-	ErrResourceGroupAlreadyExists = errors.Normalize("the %s resource group already exists", errors.RFCCodeText("PD:resourcemanager:ErrResourceGroupAlreadyExists"))
-	ErrResourceGroupNotExists     = errors.Normalize("the %s resource group does not exist", errors.RFCCodeText("PD:resourcemanager:ErrGroupNotExists"))
-	ErrDeleteReservedGroup        = errors.Normalize("cannot delete reserved group", errors.RFCCodeText("PD:resourcemanager:ErrDeleteReservedGroup"))
-	ErrInvalidGroup               = errors.Normalize("invalid group settings, please check the group name, priority and the number of resources", errors.RFCCodeText("PD:resourcemanager:ErrInvalidGroup"))
+	ErrResourceGroupNotExists = errors.Normalize("the %s resource group does not exist", errors.RFCCodeText("PD:resourcemanager:ErrGroupNotExists"))
+	ErrDeleteReservedGroup    = errors.Normalize("cannot delete reserved group", errors.RFCCodeText("PD:resourcemanager:ErrDeleteReservedGroup"))
+	ErrInvalidGroup           = errors.Normalize("invalid group settings, please check the group name, priority and the number of resources", errors.RFCCodeText("PD:resourcemanager:ErrInvalidGroup"))
 )
diff --git a/pkg/mcs/resource_manager/server/manager.go b/pkg/mcs/resource_manager/server/manager.go
@@ -156,6 +156,8 @@ func (m *Manager) Init(ctx context.Context) {
 }
 
 // AddResourceGroup puts a resource group.
+// NOTE: AddResourceGroup should also be idempotent because tidb depends
+// on this retry mechanism.
 func (m *Manager) AddResourceGroup(grouppb *rmpb.ResourceGroup) error {
 	// Check the name.
 	if len(grouppb.Name) == 0 || len(grouppb.Name) > 32 {
@@ -165,12 +167,6 @@ func (m *Manager) AddResourceGroup(grouppb *rmpb.ResourceGroup) error {
 	if grouppb.GetPriority() > 16 {
 		return errs.ErrInvalidGroup
 	}
-	m.RLock()
-	_, ok := m.groups[grouppb.Name]
-	m.RUnlock()
-	if ok {
-		return errs.ErrResourceGroupAlreadyExists.FastGenByArgs(grouppb.Name)
-	}
 	group := FromProtoResourceGroup(grouppb)
 	m.Lock()
 	defer m.Unlock()

diff --git a/tests/integrations/mcs/resource_manager/resource_manager_test.go b/tests/integrations/mcs/resource_manager/resource_manager_test.go
@@ -663,7 +663,7 @@ func (suite *resourceManagerClientTestSuite) TestBasicResourceGroupCURD() {
 	testCasesSet1 := []struct {
 		name           string
 		mode           rmpb.GroupMode
-		addSuccess     bool
+		isNewGroup     bool
 		modifySuccess  bool
 		expectMarshal  string
 		modifySettings func(*rmpb.ResourceGroup)
@@ -738,8 +738,8 @@ func (suite *resourceManagerClientTestSuite) TestBasicResourceGroupCURD() {
 		}
 		// Create Resource Group
 		resp, err := cli.AddResourceGroup(suite.ctx, group)
-		checkErr(err, tcase.addSuccess)
-		if tcase.addSuccess {
+		checkErr(err, true)
+		if tcase.isNewGroup {
 			finalNum++
 			re.Contains(resp, "Success!")
 		}
@@ -809,11 +809,9 @@ func (suite *resourceManagerClientTestSuite) TestBasicResourceGroupCURD() {
 		resp, err := http.Post(getAddr(i)+"/resource-manager/api/v1/config/group", "application/json", strings.NewReader(string(createJSON)))
 		re.NoError(err)
 		defer resp.Body.Close()
-		if tcase.addSuccess {
-			re.Equal(http.StatusOK, resp.StatusCode)
+		re.Equal(http.StatusOK, resp.StatusCode)
+		if tcase.isNewGroup {
 			finalNum++
-		} else {
-			re.Equal(http.StatusInternalServerError, resp.StatusCode)
 		}
 
 		// Modify Resource Group