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

ref #5851, ref pingcap/tidb#45050 Signed-off-by: glorv <glorvs@163.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
tikv · Jul 3, 2023 · 5f7236f · 5f7236f
1 parent 87b5ade
commit 5f7236f
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 22 deletions.
diff --git a/errors.toml b/errors.toml
@@ -606,11 +606,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
@@ -372,8 +372,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/resourcemanager/server/manager.go b/pkg/mcs/resourcemanager/server/manager.go
@@ -154,6 +154,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 {
@@ -163,12 +165,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/resourcemanager/resource_manager_test.go b/tests/integrations/mcs/resourcemanager/resource_manager_test.go
@@ -711,7 +711,7 @@ func (suite *resourceManagerClientTestSuite) TestBasicResourceGroupCURD() {
 	testCasesSet1 := []struct {
 		name           string
 		mode           rmpb.GroupMode
-		addSuccess     bool
+		isNewGroup     bool
 		modifySuccess  bool
 		expectMarshal  string
 		modifySettings func(*rmpb.ResourceGroup)
@@ -789,8 +789,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!")
 		}
@@ -860,11 +860,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