node: Read config files from dir even if config file not set via --config #245

Merged
fyrchik merged 3 commits from acid-ant/frostfs-node:bugfix/238-fix-read-cfg-from-dir into master 2023-04-14 10:19:11 +00:00
7 changed files with 37 additions and 19 deletions

View file

@ -7,6 +7,7 @@ Changelog for FrostFS Node
### Changed
### Fixed
- Take network settings into account during netmap contract update (#100)
- Read config files from dir even if config file not provided via `--config` for node (#238)
### Removed
### Updated

View file

@ -0,0 +1,24 @@
package config
import (
"os"
"path"
"testing"
"github.com/spf13/cast"
"github.com/stretchr/testify/require"
)
func TestConfigDir(t *testing.T) {
dir := t.TempDir()
ale64bit marked this conversation as resolved Outdated
[t.TempDir()](https://pkg.go.dev/testing#T.TempDir)

Perfect, thanks!

Perfect, thanks!
cfgFileName0 := path.Join(dir, "cfg_00.json")
cfgFileName1 := path.Join(dir, "cfg_01.yml")
require.NoError(t, os.WriteFile(cfgFileName0, []byte(`{"storage":{"shard_pool_size":15}}`), 0777))
ale64bit marked this conversation as resolved Outdated

if you use backticks, you don't need to quote the special characters

i.e.
[]byte{`{ "foo" : "bar" }`}

if you use backticks, you don't need to quote the special characters i.e. ```[]byte{`{ "foo" : "bar" }`}```
require.NoError(t, os.WriteFile(cfgFileName1, []byte("logger:\n level: debug"), 0777))
ale64bit marked this conversation as resolved Outdated

Consider simplifying to

require.NoError(t, os.WriteFile(cfgFileName0, `{ "storage": { "shard_pool_size": 15 } }` , 0777))

Same below.

Consider simplifying to ``` require.NoError(t, os.WriteFile(cfgFileName0, `{ "storage": { "shard_pool_size": 15 } }` , 0777)) ``` Same below.

Fixed.

Fixed.
c := New(Prm{}, WithConfigDir(dir))
require.Equal(t, "debug", cast.ToString(c.Sub("logger").Value("level")))
require.EqualValues(t, 15, cast.ToUint32(c.Sub("storage").Value("shard_pool_size")))
}

View file

@ -187,4 +187,4 @@ FROSTFS_STORAGE_SHARD_1_GC_REMOVER_SLEEP_INTERVAL=5m
FROSTFS_TRACING_ENABLED=true
FROSTFS_TRACING_ENDPOINT="localhost"
FROSTFS_TRACING_EXPORTER="otlp_grpc"
FROSTFS_TRACING_EXPORTER="otlp_grpc"
fyrchik marked this conversation as resolved Outdated

What has changed here?

What has changed here?

New line added.

New line added.

View file

@ -219,4 +219,3 @@ tracing:
enabled: true
exporter: "otlp_grpc"
endpoint: "localhost"

View file

@ -490,7 +490,7 @@ const (
NetmapCantInvokeNetmapUpdatePeer = "can't invoke netmap.UpdatePeer" // Error in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapNonAlphabetModeIgnoreRemoveNodeFromSubnetNotification = "non alphabet mode, ignore remove node from subnet notification" // Info in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapCouldNotGetNetworkMapCandidates = "could not get network map candidates" // Warn in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapCouldNotUnmarshalSubnetId = "could not unmarshal subnet id" // Warn in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapCouldNotUnmarshalSubnetID = "could not unmarshal subnet id" // Warn in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapGotZeroSubnetInRemoveNodeNotification = "got zero subnet in remove node notification" // Warn in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapCouldNotIterateOverSubnetworksOfTheNode = "could not iterate over subnetworks of the node" // Warn in ../node/pkg/innerring/processors/netmap/process_peers.go
NetmapCouldNotInvokeNetmapUpdateState = "could not invoke netmap.UpdateState" // Error in ../node/pkg/innerring/processors/netmap/process_peers.go

View file

@ -159,7 +159,7 @@ func (np *Processor) processRemoveSubnetNode(ev subnetEvent.RemoveNode) {
err = subnetToRemoveFrom.Unmarshal(rawSubnet)
if err != nil {
np.log.Warn(logs.NetmapCouldNotUnmarshalSubnetId,
np.log.Warn(logs.NetmapCouldNotUnmarshalSubnetID,
zap.Error(err),
)
return

View file

@ -1,6 +1,7 @@
package config
import (
"fmt"
"os"
"path"
@ -20,7 +21,7 @@ func ReadConfigDir(v *viper.Viper, configDir string) error {
continue
}
ext := path.Ext(entry.Name())
if ext != ".yaml" && ext != ".yml" {

I think we should still have .yaml/.json check -- we don't want to read arbitrary garbage (may be we do, but that is a separate feature).

I think we should still have `.yaml`/`.json` check -- we don't want to read arbitrary garbage (may be we do, but that is a separate feature).
Review

Reverted, added support for json.

Reverted, added support for json.
if ext != ".yaml" && ext != ".yml" && ext != ".json" {
continue
}
@ -33,22 +34,15 @@ func ReadConfigDir(v *viper.Viper, configDir string) error {
}
fyrchik marked this conversation as resolved Outdated

Have you thought about "just setting empty config map" in the beginning and performing Merge everywhere else?

Have you thought about "just setting empty config map" in the beginning and performing `Merge` everywhere else?

The main problem with merge with empty map that viper unable to parse incoming data because we do not have an API to set config type for this data. Only if we set it directly at the beginning viper able to parse data which we provide with Merge. But this will block us from merging data in multiple formats.

The main problem with merge with empty map that viper unable to parse incoming data because we do not have an API to set config type for this data. Only if we set it directly at the beginning viper able to parse data which we provide with Merge. But this will block us from merging data in multiple formats.

They still have an open issue for merge https://github.com/spf13/viper/issues/181

They still have an open issue for merge https://github.com/spf13/viper/issues/181
// mergeConfig reads config file and merge its content with current viper.
func mergeConfig(v *viper.Viper, fileName string) (err error) {
var cfgFile *os.File
cfgFile, err = os.Open(fileName)
func mergeConfig(v *viper.Viper, fileName string) error {
cv := viper.New()
cv.SetConfigFile(fileName)
err := cv.ReadInConfig()
if err != nil {
return err
return fmt.Errorf("failed to read config: %w", err)
}
defer func() {
errClose := cfgFile.Close()
if err == nil {
err = errClose
}
}()
if err = v.MergeConfig(cfgFile); err != nil {
return err
if err = v.MergeConfigMap(cv.AllSettings()); err != nil {
return fmt.Errorf("failed to merge config: %w", err)
}
return nil