resourcemanager: make controller config updates atomic

Signed-off-by: okjiang <819421878@qq.com>

Author: okJiang

pkg/mcs/resourcemanager/server/apis/v1/api.go

@@ -330,16 +330,18 @@ func (s *Service) setControllerConfig(c *gin.Context) {
 		c.String(http.StatusBadRequest, err.Error())
 		return
 	}
+	resolvedConf := make(map[string]any, len(conf))
 	for k, v := range conf {
 		key := reflectutil.FindJSONFullTagByChildTag(reflect.TypeOf(rmserver.ControllerConfig{}), k)
 		if key == "" {
 			c.String(http.StatusBadRequest, fmt.Sprintf("config item %s not found", k))
 			return
 		}
-		if err := s.manager.UpdateControllerConfigItem(key, v); err != nil {
-			c.String(http.StatusBadRequest, err.Error())
-			return
-		}
+		resolvedConf[key] = v
+	}
+	if err := s.manager.UpdateControllerConfigItems(resolvedConf); err != nil {
+		c.String(http.StatusBadRequest, err.Error())
+		return
 	}
 	c.String(http.StatusOK, "Success!")
 }

pkg/mcs/resourcemanager/server/manager.go

@@ -270,34 +270,47 @@ func (m *Manager) initReserved() {
 
 // UpdateControllerConfigItem updates the controller config item.
 func (m *Manager) UpdateControllerConfigItem(key string, value any) error {
-	kp := strings.Split(key, ".")
-	if len(kp) == 0 {
-		return errors.Errorf("invalid key %s", key)
-	}
+	return m.UpdateControllerConfigItems(map[string]any{key: value})
+}
+
+// UpdateControllerConfigItems updates controller config items atomically.
+func (m *Manager) UpdateControllerConfigItems(items map[string]any) error {
 	m.Lock()
-	var config any
-	switch kp[0] {
-	case "request-unit":
-		config = &m.controllerConfig.RequestUnit
-	default:
-		config = m.controllerConfig
-	}
-	updated, found, err := jsonutil.AddKeyValue(config, kp[len(kp)-1], value)
+	controllerConfig, err := cloneControllerConfig(m.controllerConfig)
 	if err != nil {
 		m.Unlock()
 		return err
 	}
-
-	if !found {
+	updatedItems := make([]struct {
+		key   string
+		value any
+	}, 0, len(items))
+	for key, value := range items {
+		updated, err := applyControllerConfigItem(controllerConfig, key, value)
+		if err != nil {
+			m.Unlock()
+			return err
+		}
+		if updated {
+			updatedItems = append(updatedItems, struct {
+				key   string
+				value any
+			}{key: key, value: value})
+		}
+	}
+	if len(updatedItems) == 0 {
 		m.Unlock()
-		return errors.Errorf("config item %s not found", key)
+		return nil
 	}
+	if err := m.storage.SaveControllerConfig(controllerConfig); err != nil {
+		m.Unlock()
+		log.Error("save controller config failed", zap.Error(err))
+		return err
+	}
+	m.controllerConfig = controllerConfig
 	m.Unlock()
-	if updated {
-		if err := m.storage.SaveControllerConfig(m.controllerConfig); err != nil {
-			log.Error("save controller config failed", zap.Error(err))
-		}
-		log.Info("updated controller config item", zap.String("key", key), zap.Any("value", value))
+	for _, item := range updatedItems {
+		log.Info("updated controller config item", zap.String("key", item.key), zap.Any("value", item.value))
 	}
 	return nil
 }
@@ -309,6 +322,40 @@ func (m *Manager) GetControllerConfig() *ControllerConfig {
 	return m.controllerConfig
 }
 
+func cloneControllerConfig(config *ControllerConfig) (*ControllerConfig, error) {
+	data, err := json.Marshal(config)
+	if err != nil {
+		return nil, err
+	}
+	cloned := &ControllerConfig{}
+	if err := json.Unmarshal(data, cloned); err != nil {
+		return nil, err
+	}
+	return cloned, nil
+}
+
+func applyControllerConfigItem(config *ControllerConfig, key string, value any) (bool, error) {
+	kp := strings.Split(key, ".")
+	if len(kp) == 0 {
+		return false, errors.Errorf("invalid key %s", key)
+	}
+	var target any
+	switch kp[0] {
+	case "request-unit":
+		target = &config.RequestUnit
+	default:
+		target = config
+	}
+	updated, found, err := jsonutil.AddKeyValue(target, kp[len(kp)-1], value)
+	if err != nil {
+		return false, err
+	}
+	if !found {
+		return false, errors.Errorf("config item %s not found", key)
+	}
+	return updated, nil
+}
+
 // AddResourceGroup puts a resource group.
 // NOTE: AddResourceGroup should also be idempotent because tidb depends
 // on this retry mechanism.

pkg/mcs/resourcemanager/server/manager_test.go

@@ -334,6 +334,27 @@ func TestKeyspaceServiceLimit(t *testing.T) {
 	re.Equal(DefaultResourceGroupName, krgm.getMutableResourceGroup(DefaultResourceGroupName).Name)
 }
 
+func TestUpdateControllerConfigItemsAtomic(t *testing.T) {
+	re := require.New(t)
+	m := prepareManager()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	re.NoError(m.Init(ctx))
+
+	before := *m.GetControllerConfig()
+	err := m.UpdateControllerConfigItems(map[string]any{
+		"request-unit.write-base-cost": 2.0,
+		"ltb-max-wait-duration":        "not-a-duration",
+	})
+	re.Error(err)
+
+	after := m.GetControllerConfig()
+	re.Equal(before.RequestUnit.WriteBaseCost, after.RequestUnit.WriteBaseCost)
+	re.Equal(before.LTBMaxWaitDuration, after.LTBMaxWaitDuration)
+	re.Equal(before.EnableControllerTraceLog, after.EnableControllerTraceLog)
+}
+
 func TestKeyspaceNameLookup(t *testing.T) {
 	re := require.New(t)
 	m := prepareManager()

tests/integrations/mcs/resourcemanager/api_test.go

@@ -311,6 +311,21 @@ func (suite *resourceManagerAPITestSuite) TestControllerConfigAPI() {
 	re.Equal(2.0, config.RequestUnit.WriteBaseCost)
 }
 
+func (suite *resourceManagerAPITestSuite) TestControllerConfigAPIAllOrNothing() {
+	re := suite.Require()
+
+	before := suite.mustGetControllerConfig(re)
+	resp, statusCode := tryToSetControllerConfig(re, suite.cluster.GetLeaderServer().GetAddr(), map[string]any{
+		"enable-controller-trace-log": "true",
+		"ltb-max-wait-duration":       "not-a-duration",
+	})
+	re.Equal(http.StatusBadRequest, statusCode)
+	re.Contains(resp, "time:")
+
+	after := suite.mustGetControllerConfig(re)
+	re.Equal(before, after)
+}
+
 func (suite *resourceManagerAPITestSuite) mustGetControllerConfig(re *require.Assertions) *server.ControllerConfig {
 	bodyBytes := suite.mustSendRequest(re, http.MethodGet, "/config/controller", nil)
 	config := &server.ControllerConfig{}
@@ -319,8 +334,14 @@ func (suite *resourceManagerAPITestSuite) mustGetControllerConfig(re *require.As
 }
 
 func (suite *resourceManagerAPITestSuite) mustSetControllerConfig(re *require.Assertions, config map[string]any) {
-	bodyBytes := suite.mustSendRequest(re, http.MethodPost, "/config/controller", config)
-	re.Equal("Success!", string(bodyBytes))
+	body, statusCode := tryToSetControllerConfig(re, suite.cluster.GetLeaderServer().GetAddr(), config)
+	re.Equal(http.StatusOK, statusCode, body)
+	re.Equal("Success!", body)
+}
+
+func tryToSetControllerConfig(re *require.Assertions, leaderAddr string, config map[string]any) (string, int) {
+	bodyBytes, statusCode := sendRequest(re, leaderAddr, http.MethodPost, "/config/controller", nil, config)
+	return string(bodyBytes), statusCode
 }
 
 func (suite *resourceManagerAPITestSuite) TestKeyspaceServiceLimitAPI() {