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.

Sign up for GitHub

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

Jump to bottom

Fix configuration dynamic reload logic #1194

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Fix configuration dynamic reload logic #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b5d2ec
Fix configuration dynamic reload logic
Chengxuan Feb 22, 2023
30354a5
copy right update
Chengxuan Feb 22, 2023
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
13 changes: 8 additions & 5 deletions cmd/firefly.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright © 2022 Kaleido, Inc.
// Copyright © 2023 Kaleido, Inc.
//
// SPDX-License-Identifier: Apache-2.0
//
Expand Down Expand Up @@ -87,6 +87,10 @@ func resetConfig() {
apiserver.InitConfig()
}

func reloadConfig() error {
resetConfig()
return config.ReadConfig(configSuffix, cfgFile)
}
func getRootManager() namespace.Manager {
if _utManager != nil {
return _utManager
Expand All @@ -102,8 +106,7 @@ func Execute() error {
func run() error {

// Read the configuration
resetConfig()
err := config.ReadConfig(configSuffix, cfgFile)
err := reloadConfig()

// Setup logging after reading config (even if failed), to output header correctly
rootCtx, cancelRootCtx := context.WithCancel(context.Background())
Expand Down Expand Up @@ -153,7 +156,7 @@ func run() error {
<-ffDone
// Re-read the configuration
resetConfig()
if err = config.ReadConfig(configSuffix, cfgFile); err != nil {
if err = reloadConfig(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're calling resetConfig twice in a row along this path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

......

Good catch there will submit a follow on PR to address this.

return err
}
case err := <-errChan:
Expand Down Expand Up @@ -190,7 +193,7 @@ func startFirefly(ctx context.Context, cancelCtx context.CancelFunc, mgr namespa
close(ffDone)
}()

if err = mgr.Init(ctx, cancelCtx, resetChan, resetConfig); err != nil {
if err = mgr.Init(ctx, cancelCtx, resetChan, reloadConfig); err != nil {
errChan <- err
return
}
Expand Down
5 changes: 2 additions & 3 deletions internal/namespace/configreload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright © 2022 Kaleido, Inc.
// Copyright © 2023 Kaleido, Inc.
//
// SPDX-License-Identifier: Apache-2.0
//
Expand Down Expand Up @@ -49,8 +49,7 @@ func (nm *namespaceManager) configFileChanged() {
// the config when it changes and re-read it.
// We are passed this by our parent, as the config initialization of defaults and sections
// might include others than under the namespaces tree (API Server etc. etc.)
nm.resetConfig()
err := viper.ReadInConfig()
err := nm.reloadConfig()
Copy link
Contributor

Choose a reason for hiding this comment

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

Spent a bunch of time thinking about the reason for this difference...

If you look at https://github.com/spf13/viper/blob/c898f59d3345a8317651876020fe4eb7322a00aa/viper.go#L1543 it does not replace v in the viper package variables.

There was a time in the writing of this code where I established the safest thing to do was to leave v in place because the config file had already been established, and it would retain it.

That way if any code was accessing config live during operation, there would not be a time where that was invalid.

However, this is something we try very hard to avoid in the FireFly core codebase, and copy the config only in constructors of long-lived manager.

I can see from the comments here that eventually I must have had to abandon that and call resetConfig which (when it gets down to coreconfig.Reset()) will do a reset in viper that will replace its v.

Sorry for the long detail, but I agree the change now makes sense, and the thing I was worried about already had had to happen in the last rev of this code.

if err != nil {
log.L(nm.ctx).Errorf("Failed to re-read configuration after config reload notification: %s", err)
nm.cancelCtx() // stop the world
Expand Down
34 changes: 20 additions & 14 deletions internal/namespace/configreload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,13 +443,13 @@ func mockInitConfig(nmm *nmMocks) {
func TestConfigListenerE2E(t *testing.T) {

testDir := t.TempDir()
configFilename := fmt.Sprintf("%s/config.firefly.yaml", testDir)
configFilename := fmt.Sprintf("%s/firefly.core", testDir)
err := ioutil.WriteFile(configFilename, []byte(exampleConfig1base), 0664)
assert.NoError(t, err)

coreconfig.Reset()
InitConfig()
err = config.ReadConfig("firefly", configFilename)
err = config.ReadConfig("core", configFilename)
assert.NoError(t, err)
config.Set(coreconfig.ConfigAutoReload, true)

Expand All @@ -458,7 +458,10 @@ func TestConfigListenerE2E(t *testing.T) {
mockInitConfig(nmm)

ctx, cancelCtx := context.WithCancel(context.Background())
err = nm.Init(ctx, cancelCtx, make(chan bool), func() { coreconfig.Reset() })
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error {
coreconfig.Reset()
return config.ReadConfig("core", configFilename)
})
assert.NoError(t, err)
defer func() {
cancelCtx()
Expand All @@ -480,7 +483,7 @@ func TestConfigListenerE2E(t *testing.T) {
func TestConfigListenerUnreadableYAML(t *testing.T) {

testDir := t.TempDir()
configFilename := fmt.Sprintf("%s/config.firefly.yaml", testDir)
configFilename := fmt.Sprintf("%s/firefly.core", testDir)
viper.SetConfigFile(configFilename)

coreconfig.Reset()
Expand All @@ -492,7 +495,10 @@ func TestConfigListenerUnreadableYAML(t *testing.T) {
mockInitConfig(nmm)

ctx, cancelCtx := context.WithCancel(context.Background())
err := nm.Init(ctx, cancelCtx, make(chan bool), func() { coreconfig.Reset() })
err := nm.Init(ctx, cancelCtx, make(chan bool), func() error {
coreconfig.Reset()
return config.ReadConfig("core", configFilename)
})
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -522,7 +528,7 @@ func TestConfigReload1to2(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -593,7 +599,7 @@ func TestConfigReload1to3(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -659,7 +665,7 @@ func TestConfigReloadBadNewConfigPlugins(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

originalPlugins := nm.plugins
Expand Down Expand Up @@ -708,7 +714,7 @@ func TestConfigReloadBadNSMissingRequiredPlugins(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

originalPlugins := nm.plugins
Expand Down Expand Up @@ -767,7 +773,7 @@ func TestConfigDownToNothingOk(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -812,7 +818,7 @@ func TestConfigStartPluginsFails(t *testing.T) {

mockInitConfig(nmm)

err = nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err = nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -856,7 +862,7 @@ func TestConfigReloadInitPluginsFailOnReload(t *testing.T) {
ctx, cancelCtx := context.WithCancel(context.Background())
defer cancelCtx()

err := nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err := nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -899,7 +905,7 @@ func TestConfigReloadInitNamespacesFailOnReload(t *testing.T) {
ctx, cancelCtx := context.WithCancel(context.Background())
defer cancelCtx()

err := nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err := nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down Expand Up @@ -949,7 +955,7 @@ func TestConfigReloadInitNamespacesFailOnStart(t *testing.T) {
ctx, cancelCtx := context.WithCancel(context.Background())
defer cancelCtx()

err := nm.Init(ctx, cancelCtx, make(chan bool), func() {})
err := nm.Init(ctx, cancelCtx, make(chan bool), func() error { return nil })
assert.NoError(t, err)

err = nm.Start()
Expand Down
10 changes: 5 additions & 5 deletions internal/namespace/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ import (
)

type Manager interface {
Init(ctx context.Context, cancelCtx context.CancelFunc, reset chan bool, resetConfig func()) error
Init(ctx context.Context, cancelCtx context.CancelFunc, reset chan bool, reloadConfig func() error) error
Start() error
WaitStop()
Reset(ctx context.Context) error
Expand Down Expand Up @@ -83,7 +83,7 @@ type namespace struct {

type namespaceManager struct {
reset chan bool
resetConfig func()
reloadConfig func() error
ctx context.Context
cancelCtx context.CancelFunc
nsMux sync.Mutex
Expand Down Expand Up @@ -172,9 +172,9 @@ func NewNamespaceManager() Manager {
return nm
}

func (nm *namespaceManager) Init(ctx context.Context, cancelCtx context.CancelFunc, reset chan bool, resetConfig func()) (err error) {
nm.reset = reset // channel to ask our parent to reload us
nm.resetConfig = resetConfig // function to cause our parent to call InitConfig on all components, including us
func (nm *namespaceManager) Init(ctx context.Context, cancelCtx context.CancelFunc, reset chan bool, reloadConfig func() error) (err error) {
nm.reset = reset // channel to ask our parent to reload us
nm.reloadConfig = reloadConfig // function to cause our parent to call InitConfig on all components, including us
nm.ctx = ctx
nm.cancelCtx = cancelCtx

Expand Down
23 changes: 14 additions & 9 deletions internal/namespace/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,15 @@ func newTestNamespaceManager(t *testing.T, initConfig bool) (*namespaceManager,
InitConfig()
ctx, cancelCtx := context.WithCancel(context.Background())
nm := &namespaceManager{
ctx: ctx,
cancelCtx: cancelCtx,
reset: make(chan bool, 1),
resetConfig: InitConfig,
ctx: ctx,
cancelCtx: cancelCtx,
reset: make(chan bool, 1),
reloadConfig: func() error {
coreconfig.Reset()
InitConfig()
viper.SetConfigType("yaml")
return viper.ReadConfig(strings.NewReader(testBaseConfig))
},
namespaces: make(map[string]*namespace),
plugins: make(map[string]*plugin),
tokenBroadcastNames: make(map[string]string),
Expand Down Expand Up @@ -275,7 +280,7 @@ func newTestNamespaceManager(t *testing.T, initConfig bool) (*namespaceManager,
nmm.mdi.On("UpsertNamespace", mock.Anything, mock.AnythingOfType("*core.Namespace"), true).Return(nil).Once()
nmm.mai.On("Init", mock.Anything, mock.Anything, mock.Anything).Return(nil).Once()

err = nmm.nm.Init(nmm.nm.ctx, nmm.nm.cancelCtx, nmm.nm.reset, nmm.nm.resetConfig)
err = nmm.nm.Init(nmm.nm.ctx, nmm.nm.cancelCtx, nmm.nm.reset, nmm.nm.reloadConfig)
assert.NoError(t, err)
}

Expand All @@ -302,7 +307,7 @@ func TestInitEmpty(t *testing.T) {
nmm.mei[1].On("Init", mock.Anything, mock.Anything).Return(nil)
nmm.mei[2].On("Init", mock.Anything, mock.Anything).Return(nil)

err := nm.Init(nm.ctx, nm.cancelCtx, nm.reset, nm.resetConfig)
err := nm.Init(nm.ctx, nm.cancelCtx, nm.reset, nm.reloadConfig)
assert.NoError(t, err)

assert.Len(t, nm.plugins, 3) // events
Expand Down Expand Up @@ -396,7 +401,7 @@ func TestInitEventsFail(t *testing.T) {
mei.On("Init", mock.Anything, mock.Anything).Return(fmt.Errorf("pop")).Maybe()
}

err := nm.Init(nm.ctx, nm.cancelCtx, nm.reset, nm.resetConfig)
err := nm.Init(nm.ctx, nm.cancelCtx, nm.reset, nm.reloadConfig)
assert.EqualError(t, err, "pop")
}

Expand Down Expand Up @@ -831,7 +836,7 @@ func TestIdentityPluginNoType(t *testing.T) {
config.Set("plugins.identity", []fftypes.JSONObject{{}})

ctx, cancelCtx := context.WithCancel(context.Background())
err := nm.Init(ctx, cancelCtx, nm.reset, nm.resetConfig)
err := nm.Init(ctx, cancelCtx, nm.reset, nm.reloadConfig)
assert.Regexp(t, "FF10386.*type", err)
}

Expand Down Expand Up @@ -1318,7 +1323,7 @@ func TestInitBadNamespace(t *testing.T) {
assert.NoError(t, err)

ctx, cancelCtx := context.WithCancel(context.Background())
err = nm.Init(ctx, cancelCtx, nm.reset, nm.resetConfig)
err = nm.Init(ctx, cancelCtx, nm.reset, nm.reloadConfig)
assert.Regexp(t, "FF00140", err)
}

Expand Down
2 changes: 1 addition & 1 deletion mocks/apiservermocks/ffi_swagger_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion mocks/apiservermocks/server.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading