diff --git a/pkg/mcs/resourcemanager/metadataapi/config_service.go b/pkg/mcs/resourcemanager/metadataapi/config_service.go index 900d8056a5b..375ea32f58b 100644 --- a/pkg/mcs/resourcemanager/metadataapi/config_service.go +++ b/pkg/mcs/resourcemanager/metadataapi/config_service.go @@ -43,6 +43,7 @@ type ConfigStore interface { GetResourceGroupList(uint32, bool) ([]*rmserver.ResourceGroup, error) DeleteResourceGroup(uint32, string) error GetControllerConfig() *rmserver.ControllerConfig + UpdateControllerConfigItems(map[string]any) error UpdateControllerConfigItem(string, any) error SetKeyspaceServiceLimit(uint32, float64) error LookupKeyspaceID(context.Context, string) (uint32, error) @@ -94,6 +95,11 @@ func (s *ManagerStore) UpdateControllerConfigItem(key string, value any) error { return s.manager.UpdateControllerConfigItem(key, value) } +// UpdateControllerConfigItems updates controller config items atomically. +func (s *ManagerStore) UpdateControllerConfigItems(items map[string]any) error { + return s.manager.UpdateControllerConfigItems(items) +} + // SetKeyspaceServiceLimit sets keyspace service limit. func (s *ManagerStore) SetKeyspaceServiceLimit(keyspaceID uint32, limit float64) error { return s.manager.SetKeyspaceServiceLimit(keyspaceID, limit) @@ -242,15 +248,13 @@ func (s *ConfigService) SetControllerConfig(c *gin.Context) { } resolvedConf[key] = v } - for key, v := range resolvedConf { - if err := s.configStore.UpdateControllerConfigItem(key, v); err != nil { - if rmserver.IsMetadataWriteDisabledError(err) { - c.String(http.StatusForbidden, err.Error()) - return - } - c.String(http.StatusBadRequest, err.Error()) + if err := s.configStore.UpdateControllerConfigItems(resolvedConf); err != nil { + if rmserver.IsMetadataWriteDisabledError(err) { + c.String(http.StatusForbidden, err.Error()) return } + c.String(http.StatusBadRequest, err.Error()) + return } c.String(http.StatusOK, "Success!") } diff --git a/pkg/mcs/resourcemanager/metadataapi/config_service_test.go b/pkg/mcs/resourcemanager/metadataapi/config_service_test.go index 0a44c826ff7..51243b2f93d 100644 --- a/pkg/mcs/resourcemanager/metadataapi/config_service_test.go +++ b/pkg/mcs/resourcemanager/metadataapi/config_service_test.go @@ -287,6 +287,13 @@ func (*testStore) GetControllerConfig() *rmserver.ControllerConfig { return &rmserver.ControllerConfig{} } +func (s *testStore) UpdateControllerConfigItems(items map[string]any) error { + for key := range items { + s.updatedControllerConfigItems = append(s.updatedControllerConfigItems, key) + } + return nil +} + func (s *testStore) UpdateControllerConfigItem(key string, _ any) error { s.updatedControllerConfigItems = append(s.updatedControllerConfigItems, key) return nil diff --git a/pkg/mcs/resourcemanager/server/manager.go b/pkg/mcs/resourcemanager/server/manager.go index 4acc158604e..c103d763daf 100644 --- a/pkg/mcs/resourcemanager/server/manager.go +++ b/pkg/mcs/resourcemanager/server/manager.go @@ -467,49 +467,46 @@ func (m *Manager) initReserved() { // UpdateControllerConfigItem updates the controller config item. func (m *Manager) UpdateControllerConfigItem(key string, value any) error { + return m.UpdateControllerConfigItems(map[string]any{key: value}) +} + +// UpdateControllerConfigItems updates controller config items atomically. +func (m *Manager) UpdateControllerConfigItems(items map[string]any) error { if !m.writeRole.AllowsMetadataWrite() { return errMetadataWriteDisabled } - kp := strings.Split(key, ".") - if len(kp) == 0 { - return errors.Errorf("invalid key %s", key) - } m.Lock() controllerConfig := cloneControllerConfig(m.controllerConfig) - var config any - switch kp[0] { - case "request-unit": - config = &controllerConfig.RequestUnit - default: - config = controllerConfig - } - updated, found, err := jsonutil.AddKeyValue(config, kp[len(kp)-1], value) - if err != nil { - m.Unlock() - return err + 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 !found { + if len(updatedItems) == 0 { m.Unlock() - return errors.Errorf("config item %s not found", key) + return nil } - // Validate RUVersionPolicy after any update, regardless of the key path, - // since the default branch merges into the full ControllerConfig. - if err := controllerConfig.RUVersionPolicy.validate(); err != nil { + if err := m.storage.SaveControllerConfig(controllerConfig); err != nil { m.Unlock() + log.Error("save controller config failed", zap.Error(err)) return err } - if updated { - 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.controllerConfig = controllerConfig m.Unlock() - if updated { - 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 } @@ -521,6 +518,33 @@ func (m *Manager) GetControllerConfig() *ControllerConfig { return cloneControllerConfig(m.controllerConfig) } +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) + } + // Validate RUVersionPolicy after any update, regardless of the key path, + // since the default branch merges into the full ControllerConfig. + if err := config.RUVersionPolicy.validate(); err != nil { + return false, err + } + return updated, nil +} + // AddResourceGroup puts a resource group. // NOTE: AddResourceGroup should also be idempotent because tidb depends // on this retry mechanism. diff --git a/pkg/mcs/resourcemanager/server/manager_test.go b/pkg/mcs/resourcemanager/server/manager_test.go index 3b4bceb03c4..e2886a2ce0a 100644 --- a/pkg/mcs/resourcemanager/server/manager_test.go +++ b/pkg/mcs/resourcemanager/server/manager_test.go @@ -599,6 +599,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() diff --git a/tests/integrations/mcs/resourcemanager/api_test.go b/tests/integrations/mcs/resourcemanager/api_test.go index d3744820dc7..6097a790214 100644 --- a/tests/integrations/mcs/resourcemanager/api_test.go +++ b/tests/integrations/mcs/resourcemanager/api_test.go @@ -336,6 +336,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{} @@ -344,8 +359,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() {