Skip to content

Commit f7de7cc

Browse files
okJiangokJiang
committed
resourcemanager: make controller config updates atomic
Signed-off-by: okjiang <819421878@qq.com>
1 parent 99eb5b5 commit f7de7cc

File tree

5 files changed

+117
-40
lines changed

5 files changed

+117
-40
lines changed

‎pkg/mcs/resourcemanager/metadataapi/config_service.go‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ type ConfigStore interface {
4343
GetResourceGroupList(uint32, bool) ([]*rmserver.ResourceGroup, error)
4444
DeleteResourceGroup(uint32, string) error
4545
GetControllerConfig() *rmserver.ControllerConfig
46+
UpdateControllerConfigItems(map[string]any) error
4647
UpdateControllerConfigItem(string, any) error
4748
SetKeyspaceServiceLimit(uint32, float64) error
4849
LookupKeyspaceID(context.Context, string) (uint32, error)
@@ -94,6 +95,11 @@ func (s *ManagerStore) UpdateControllerConfigItem(key string, value any) error {
9495
return s.manager.UpdateControllerConfigItem(key, value)
9596
}
9697

98+
// UpdateControllerConfigItems updates controller config items atomically.
99+
func (s *ManagerStore) UpdateControllerConfigItems(items map[string]any) error {
100+
return s.manager.UpdateControllerConfigItems(items)
101+
}
102+
97103
// SetKeyspaceServiceLimit sets keyspace service limit.
98104
func (s *ManagerStore) SetKeyspaceServiceLimit(keyspaceID uint32, limit float64) error {
99105
return s.manager.SetKeyspaceServiceLimit(keyspaceID, limit)
@@ -242,15 +248,13 @@ func (s *ConfigService) SetControllerConfig(c *gin.Context) {
242248
}
243249
resolvedConf[key] = v
244250
}
245-
for key, v := range resolvedConf {
246-
if err := s.configStore.UpdateControllerConfigItem(key, v); err != nil {
247-
if rmserver.IsMetadataWriteDisabledError(err) {
248-
c.String(http.StatusForbidden, err.Error())
249-
return
250-
}
251-
c.String(http.StatusBadRequest, err.Error())
251+
if err := s.configStore.UpdateControllerConfigItems(resolvedConf); err != nil {
252+
if rmserver.IsMetadataWriteDisabledError(err) {
253+
c.String(http.StatusForbidden, err.Error())
252254
return
253255
}
256+
c.String(http.StatusBadRequest, err.Error())
257+
return
254258
}
255259
c.String(http.StatusOK, "Success!")
256260
}

‎pkg/mcs/resourcemanager/metadataapi/config_service_test.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,13 @@ func (*testStore) GetControllerConfig() *rmserver.ControllerConfig {
287287
return &rmserver.ControllerConfig{}
288288
}
289289

290+
func (s *testStore) UpdateControllerConfigItems(items map[string]any) error {
291+
for key := range items {
292+
s.updatedControllerConfigItems = append(s.updatedControllerConfigItems, key)
293+
}
294+
return nil
295+
}
296+
290297
func (s *testStore) UpdateControllerConfigItem(key string, _ any) error {
291298
s.updatedControllerConfigItems = append(s.updatedControllerConfigItems, key)
292299
return nil

‎pkg/mcs/resourcemanager/server/manager.go‎

Lines changed: 55 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -467,49 +467,46 @@ func (m *Manager) initReserved() {
467467

468468
// UpdateControllerConfigItem updates the controller config item.
469469
func (m *Manager) UpdateControllerConfigItem(key string, value any) error {
470+
return m.UpdateControllerConfigItems(map[string]any{key: value})
471+
}
472+
473+
// UpdateControllerConfigItems updates controller config items atomically.
474+
func (m *Manager) UpdateControllerConfigItems(items map[string]any) error {
470475
if !m.writeRole.AllowsMetadataWrite() {
471476
return errMetadataWriteDisabled
472477
}
473-
kp := strings.Split(key, ".")
474-
if len(kp) == 0 {
475-
return errors.Errorf("invalid key %s", key)
476-
}
477478
m.Lock()
478479
controllerConfig := cloneControllerConfig(m.controllerConfig)
479-
var config any
480-
switch kp[0] {
481-
case "request-unit":
482-
config = &controllerConfig.RequestUnit
483-
default:
484-
config = controllerConfig
485-
}
486-
updated, found, err := jsonutil.AddKeyValue(config, kp[len(kp)-1], value)
487-
if err != nil {
488-
m.Unlock()
489-
return err
480+
updatedItems := make([]struct {
481+
key string
482+
value any
483+
}, 0, len(items))
484+
for key, value := range items {
485+
updated, err := applyControllerConfigItem(controllerConfig, key, value)
486+
if err != nil {
487+
m.Unlock()
488+
return err
489+
}
490+
if updated {
491+
updatedItems = append(updatedItems, struct {
492+
key string
493+
value any
494+
}{key: key, value: value})
495+
}
490496
}
491-
492-
if !found {
497+
if len(updatedItems) == 0 {
493498
m.Unlock()
494-
return errors.Errorf("config item %s not found", key)
499+
return nil
495500
}
496-
// Validate RUVersionPolicy after any update, regardless of the key path,
497-
// since the default branch merges into the full ControllerConfig.
498-
if err := controllerConfig.RUVersionPolicy.validate(); err != nil {
501+
if err := m.storage.SaveControllerConfig(controllerConfig); err != nil {
499502
m.Unlock()
503+
log.Error("save controller config failed", zap.Error(err))
500504
return err
501505
}
502-
if updated {
503-
if err := m.storage.SaveControllerConfig(controllerConfig); err != nil {
504-
m.Unlock()
505-
log.Error("save controller config failed", zap.Error(err))
506-
return err
507-
}
508-
m.controllerConfig = controllerConfig
509-
}
506+
m.controllerConfig = controllerConfig
510507
m.Unlock()
511-
if updated {
512-
log.Info("updated controller config item", zap.String("key", key), zap.Any("value", value))
508+
for _, item := range updatedItems {
509+
log.Info("updated controller config item", zap.String("key", item.key), zap.Any("value", item.value))
513510
}
514511
return nil
515512
}
@@ -521,6 +518,33 @@ func (m *Manager) GetControllerConfig() *ControllerConfig {
521518
return cloneControllerConfig(m.controllerConfig)
522519
}
523520

521+
func applyControllerConfigItem(config *ControllerConfig, key string, value any) (bool, error) {
522+
kp := strings.Split(key, ".")
523+
if len(kp) == 0 {
524+
return false, errors.Errorf("invalid key %s", key)
525+
}
526+
var target any
527+
switch kp[0] {
528+
case "request-unit":
529+
target = &config.RequestUnit
530+
default:
531+
target = config
532+
}
533+
updated, found, err := jsonutil.AddKeyValue(target, kp[len(kp)-1], value)
534+
if err != nil {
535+
return false, err
536+
}
537+
if !found {
538+
return false, errors.Errorf("config item %s not found", key)
539+
}
540+
// Validate RUVersionPolicy after any update, regardless of the key path,
541+
// since the default branch merges into the full ControllerConfig.
542+
if err := config.RUVersionPolicy.validate(); err != nil {
543+
return false, err
544+
}
545+
return updated, nil
546+
}
547+
524548
// AddResourceGroup puts a resource group.
525549
// NOTE: AddResourceGroup should also be idempotent because tidb depends
526550
// on this retry mechanism.

‎pkg/mcs/resourcemanager/server/manager_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,27 @@ func TestKeyspaceServiceLimit(t *testing.T) {
599599
re.Equal(DefaultResourceGroupName, krgm.getMutableResourceGroup(DefaultResourceGroupName).Name)
600600
}
601601

602+
func TestUpdateControllerConfigItemsAtomic(t *testing.T) {
603+
re := require.New(t)
604+
m := prepareManager()
605+
606+
ctx, cancel := context.WithCancel(context.Background())
607+
defer cancel()
608+
re.NoError(m.Init(ctx))
609+
610+
before := *m.GetControllerConfig()
611+
err := m.UpdateControllerConfigItems(map[string]any{
612+
"request-unit.write-base-cost": 2.0,
613+
"ltb-max-wait-duration": "not-a-duration",
614+
})
615+
re.Error(err)
616+
617+
after := m.GetControllerConfig()
618+
re.Equal(before.RequestUnit.WriteBaseCost, after.RequestUnit.WriteBaseCost)
619+
re.Equal(before.LTBMaxWaitDuration, after.LTBMaxWaitDuration)
620+
re.Equal(before.EnableControllerTraceLog, after.EnableControllerTraceLog)
621+
}
622+
602623
func TestKeyspaceNameLookup(t *testing.T) {
603624
re := require.New(t)
604625
m := prepareManager()

‎tests/integrations/mcs/resourcemanager/api_test.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,21 @@ func (suite *resourceManagerAPITestSuite) TestControllerConfigAPI() {
336336
re.Equal(2.0, config.RequestUnit.WriteBaseCost)
337337
}
338338

339+
func (suite *resourceManagerAPITestSuite) TestControllerConfigAPIAllOrNothing() {
340+
re := suite.Require()
341+
342+
before := suite.mustGetControllerConfig(re)
343+
resp, statusCode := tryToSetControllerConfig(re, suite.cluster.GetLeaderServer().GetAddr(), map[string]any{
344+
"enable-controller-trace-log": "true",
345+
"ltb-max-wait-duration": "not-a-duration",
346+
})
347+
re.Equal(http.StatusBadRequest, statusCode)
348+
re.Contains(resp, "time:")
349+
350+
after := suite.mustGetControllerConfig(re)
351+
re.Equal(before, after)
352+
}
353+
339354
func (suite *resourceManagerAPITestSuite) mustGetControllerConfig(re *require.Assertions) *server.ControllerConfig {
340355
bodyBytes := suite.mustSendRequest(re, http.MethodGet, "/config/controller", nil)
341356
config := &server.ControllerConfig{}
@@ -344,8 +359,14 @@ func (suite *resourceManagerAPITestSuite) mustGetControllerConfig(re *require.As
344359
}
345360

346361
func (suite *resourceManagerAPITestSuite) mustSetControllerConfig(re *require.Assertions, config map[string]any) {
347-
bodyBytes := suite.mustSendRequest(re, http.MethodPost, "/config/controller", config)
348-
re.Equal("Success!", string(bodyBytes))
362+
body, statusCode := tryToSetControllerConfig(re, suite.cluster.GetLeaderServer().GetAddr(), config)
363+
re.Equal(http.StatusOK, statusCode, body)
364+
re.Equal("Success!", body)
365+
}
366+
367+
func tryToSetControllerConfig(re *require.Assertions, leaderAddr string, config map[string]any) (string, int) {
368+
bodyBytes, statusCode := sendRequest(re, leaderAddr, http.MethodPost, "/config/controller", nil, config)
369+
return string(bodyBytes), statusCode
349370
}
350371

351372
func (suite *resourceManagerAPITestSuite) TestKeyspaceServiceLimitAPI() {

0 commit comments

Comments
 (0)
⚡