Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow embedding dynamic config in static config #6573

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions common/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ type (
// DynamicConfigClient is the config for setting up the file based dynamic config client
// Filepath should be relative to the root directory
DynamicConfigClient *dynamicconfig.FileBasedClientConfig `yaml:"dynamicConfigClient"`
// If DynamicConfig is present, it is used as the dynamic config for the server (and
// reloaded periodically if dynamicConfigClient.pollInterval is set). This conflicts
// with dynamicConfigClient.filepath.
DynamicConfig any `yaml:"dynamicConfig"`
// NamespaceDefaults is the default config for every namespace
NamespaceDefaults NamespaceDefaults `yaml:"namespaceDefaults"`
// ExporterConfig allows the specification of process-wide OTEL exporters
Expand Down
19 changes: 18 additions & 1 deletion common/config/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
package config

import (
"errors"
"fmt"
stdlog "log"
"os"

"go.temporal.io/server/common/dynamicconfig"
"gopkg.in/validator.v2"
"gopkg.in/yaml.v3"
)
Expand Down Expand Up @@ -71,7 +73,7 @@
// base.yaml
// env.yaml -- environment is one of the input params ex-development
// env_az.yaml -- zone is another input param
func Load(env string, configDir string, zone string, config interface{}) error {
func Load(env string, configDir string, zone string, config *Config) error {
if len(env) == 0 {
env = envDevelopment
}
Expand Down Expand Up @@ -103,6 +105,21 @@
}
}

// Inject self-dynamic-config-client if dynamicConfig section present.
if config.DynamicConfig != nil {
if config.DynamicConfigClient == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is it okay for this not to be nil if dynamic config is specified here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it allowed to leave out dynamicConfigClient.. the only thing in there is pollInterval, if that's zero it'll just not do the refresh loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh your question was the opposite. Then the answer is: there should be a way to specify nonzero pollInterval if you want to get updates. It's easiest to just use the existing setting.

config.DynamicConfigClient = &dynamicconfig.FileBasedClientConfig{}
}
if config.DynamicConfigClient.Filepath != "" {
return errors.New("dynamicConfig can't be used with dynamicConfigClient.filepath")

Check failure on line 114 in common/config/loader.go

View workflow job for this annotation

GitHub Actions / lint

do not define dynamic errors, use wrapped static errors instead: "errors.New(\"dynamicConfig can't be used with dynamicConfigClient.filepath\")" (err113)
} else if len(files) > 1 {
return errors.New("dynamicConfig can't be used when merging mulitple static config files")

Check failure on line 116 in common/config/loader.go

View workflow job for this annotation

GitHub Actions / lint

do not define dynamic errors, use wrapped static errors instead: "errors.New(\"dynamicConfig can't be used when merging mulitple static config files\")" (err113)
}

config.DynamicConfigClient.Filepath = files[0]
config.DynamicConfigClient.BelowDynamicConfigKey = true
}

return validator.Validate(config)
}

Expand Down
34 changes: 15 additions & 19 deletions common/config/loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,6 @@ type (
*require.Assertions
suite.Suite
}

itemsConfig struct {
Item1 string `yaml:"item1"`
Item2 string `yaml:"item2"`
}

testConfig struct {
Items itemsConfig `yaml:"items"`
}
)

func TestLoaderSuite(t *testing.T) {
Expand All @@ -71,11 +62,11 @@ func (s *LoaderSuite) TestBaseYaml() {

for _, env := range envs {
for _, zone := range zones {
var cfg testConfig
var cfg Config
err = Load(env, dir, zone, &cfg)
s.Nil(err)
s.Equal("hello__", cfg.Items.Item1)
s.Equal("world__", cfg.Items.Item2)
s.Equal("hello__", cfg.Log.Level)
s.Equal("world__", cfg.Log.Format)
}
}
}
Expand Down Expand Up @@ -106,16 +97,16 @@ func (s *LoaderSuite) TestHierarchy() {
}

for _, tc := range testCases {
var cfg testConfig
var cfg Config
err := Load(tc.env, dir, tc.zone, &cfg)
s.Nil(err)
s.Equal(tc.item1, cfg.Items.Item1)
s.Equal(tc.item2, cfg.Items.Item2)
s.Equal(tc.item1, cfg.Log.Level)
s.Equal(tc.item2, cfg.Log.Format)
}
}

func (s *LoaderSuite) TestInvalidPath() {
var cfg testConfig
var cfg Config
err := Load("prod", "", "", &cfg)
s.NotNil(err)
}
Expand All @@ -129,7 +120,12 @@ func buildConfig(env, zone string) string {
item1 := concat("hello", concat(env, zone))
item2 := concat("world", concat(env, zone))
return `
items:
item1: ` + item1 + `
item2: ` + item2
log:
level: ` + item1 + `
format: ` + item2 + `
# make validator happy:
persistence:
defaultStore: dummy
numHistoryShards: 10
`
}
58 changes: 38 additions & 20 deletions common/dynamicconfig/file_based_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ type (
FileBasedClientConfig struct {
Filepath string `yaml:"filepath"`
PollInterval time.Duration `yaml:"pollInterval"`
// If this is false (default), then the top level of the file is expected to contain
// dynamic config settings. If it's true, then the top level should have a key
// "dynamicConfig" which should have settings under that.
BelowDynamicConfigKey bool `yaml:"belowDynamicConfigKey"`
}

configValueMap map[string][]ConstrainedValue
Expand Down Expand Up @@ -94,7 +98,7 @@ type (
)

func ValidateFile(contents []byte) *LoadResult {
_, lr := loadFile(contents)
_, lr := loadFile(contents, false)
return lr
}

Expand Down Expand Up @@ -153,21 +157,23 @@ func (fc *fileBasedClient) init() error {
return fmt.Errorf("unable to read dynamic config: %w", err)
}

go func() {
ticker := time.NewTicker(fc.config.PollInterval)
for {
select {
case <-ticker.C:
err := fc.Update()
if err != nil {
fc.logger.Error("Unable to update dynamic config.", tag.Error(err))
if fc.config.PollInterval > 0 {
go func() {
ticker := time.NewTicker(fc.config.PollInterval)
for {
select {
case <-ticker.C:
err := fc.Update()
if err != nil {
fc.logger.Error("Unable to update dynamic config.", tag.Error(err))
}
case <-fc.doneCh:
ticker.Stop()
return
}
case <-fc.doneCh:
ticker.Stop()
return
}
}
}()
}()
}

return nil
}
Expand All @@ -189,7 +195,7 @@ func (fc *fileBasedClient) Update() error {
return fmt.Errorf("dynamic config file: %s: %w", fc.config.Filepath, err)
}

newValues, lr := loadFile(contents)
newValues, lr := loadFile(contents, fc.config.BelowDynamicConfigKey)
for _, e := range lr.Errors {
fc.logger.Warn("dynamic config error", tag.Error(e))
}
Expand Down Expand Up @@ -221,15 +227,27 @@ func (fc *fileBasedClient) Update() error {
return nil
}

func loadFile(contents []byte) (configValueMap, *LoadResult) {
func loadFile(contents []byte, belowDynamicConfigKey bool) (configValueMap, *LoadResult) {
lr := &LoadResult{}

var yamlValues map[string][]struct {
type valueMap map[string][]struct {
Constraints map[string]any
Value any
}
if err := yaml.Unmarshal(contents, &yamlValues); err != nil {
return nil, lr.errorf("decode error: %w", err)
var yamlValues valueMap

if belowDynamicConfigKey {
var topLevel struct {
DynamicConfig valueMap `yaml:"dynamicConfig"`
}
if err := yaml.Unmarshal(contents, &topLevel); err != nil {
return nil, lr.errorf("decode error: %w", err)
}
yamlValues = topLevel.DynamicConfig
} else {
if err := yaml.Unmarshal(contents, &yamlValues); err != nil {
return nil, lr.errorf("decode error: %w", err)
}
}

newValues := make(configValueMap, len(yamlValues))
Expand Down Expand Up @@ -276,7 +294,7 @@ func (fc *fileBasedClient) validateStaticConfig(config *FileBasedClientConfig) e
if _, err := fc.reader.GetModTime(); err != nil {
return fmt.Errorf("dynamic config: %s: %w", config.Filepath, err)
}
if config.PollInterval < minPollInterval {
if config.PollInterval > 0 && config.PollInterval < minPollInterval {
return fmt.Errorf("poll interval should be at least %v", minPollInterval)
}
return nil
Expand Down
27 changes: 27 additions & 0 deletions common/dynamicconfig/file_based_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -665,3 +665,30 @@ testGetBoolPropertyKey:
}
s.Equal(3, found)
}

func (s *fileBasedClientSuite) TestBelowDynamicConfigKey() {
data := []byte(`
dynamicConfig:
setting1:
- value: 1000
`)
ctrl := gomock.NewController(s.T())
doneCh := make(chan interface{})
reader := dynamicconfig.NewMockFileReader(ctrl)
reader.EXPECT().GetModTime().Return(time.Now(), nil).AnyTimes()
reader.EXPECT().ReadFile().Return(data, nil).AnyTimes()

client, err := dynamicconfig.NewFileBasedClientWithReader(
reader,
&dynamicconfig.FileBasedClientConfig{
Filepath: "dummy",
BelowDynamicConfigKey: true,
},
log.NewNoopLogger(),
doneCh)
s.NoError(err)
defer close(doneCh)

s.NotNil(client.GetValue(dynamicconfig.Key("setting1")))
s.Nil(client.GetValue(dynamicconfig.Key("setting2")))
}
3 changes: 2 additions & 1 deletion temporal/server_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ func NewServerFxImpl(

func (s *ServerImpl) Start(ctx context.Context) error {
s.logger.Info("Starting server for services", tag.Value(s.so.serviceNames))
s.logger.Debug(s.so.config.String())
// Using NewAnyTag on config (which is a fmt.Stringer) avoids marshaling unless it's needed.
s.logger.Debug("Using static config", tag.NewAnyTag("static-config", s.so.config))

if err := initSystemNamespaces(
ctx,
Expand Down
Loading