Khalil/1888 network config improvements #4340
Conversation
- add network config to config.yml - update network config cli flags to use defaults from config package - add config parsing and flag binding
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 5ab3243 The command Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## master #4340 +/- ##
==========================================
+ Coverage 54.09% 54.16% +0.06%
==========================================
Files 563 896 +333
Lines 56220 84264 +28044
==========================================
+ Hits 30414 45644 +15230
- Misses 23459 35096 +11637
- Partials 2347 3524 +1177
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
I'm very happy to see movement on standardizing config management - it could certainly use it. Here are some general comments on the approach so far:
Singleton Pattern
I think we should design the config package so that it does not need to be a global singleton. There should a type representing Config, a constructor, and getters should be defined on the type, rather than as public methods on a global variable.
Locality
Most components which can be configured currently define a Config type in their local package (and often a default value for these configs). They then accept an instance of Config in their constructor. Config validation is also generally implemented in the package.
We can think of the global app config as the union of these component configs:
type FlowConfig struct {
NetworkingConfig
ConsensusEngineConfig
...
}I think it is beneficial to maintain this component/config locality, for similar reasons to the above point, so I'm wondering how to incorporate that pattern with the changes you're proposing here.
One extreme is, the entire app config is passed into to each component. The global config package handles dynamic updating, validation etc. And the component just read whichever values they need.
The other extreme is, Viper acts as an initialization process, it produces the initial config as output, from which we can extract component-specific configs, then components are individually responsible for validation, dynamic updating, etc.
I feel like the current approach is leaning toward the first extreme. My gut feeling leans more toward the second extreme, and some of my comments highlight that, but the draft is still early so it's difficult to see. I think applying this approach to one of the components which follow the component-defined config pattern, and teasing out how that will interact with this new package, would be a good next step.
config/network.go
Outdated
| // NetworkConnectionPruning returns the network connection pruning config value. | ||
| func NetworkConnectionPruning() bool { | ||
| return conf.GetBool(NetworkingConnectionPruningKey) | ||
| } |
There was a problem hiding this comment.
By making the config getter functions global, rather than for example attached to a config.Config type, we are committing to a singleton pattern. In my experience this is generally a bad idea.
Even in cases where a singleton is desired, it is generally preferable to create the object in a way which could support multiple instances (even if you only end up using one). That way, for testing purposes we can create individual instances. It also encourages dependency-injection style construction of objects -- it is preferable for a dependency to be passed into an object's constructor, than for the object to be able to access a global singleton representing that dependency.
There was a problem hiding this comment.
I've taken your feedback into consideration and did away with the singleton style and opted for defining the go structs for each module's config. This updating, validation, and CLI flag instantiation will now be contained the in same package corresponding to the relevant component. i.e: All network configuration is moved to the config/network package.
There was a problem hiding this comment.
Thanks for putting this together @kc1116!
I really like the idea of simplifying the config management, and supporting file based configs. I also think moving to a library makes a lot of sense.
config/keys.go
Outdated
| func toMapStrInt(vals map[string]interface{}) map[string]int { | ||
| m := make(map[string]int) | ||
| for key, val := range vals { | ||
| m[key] = val.(int) | ||
| } | ||
| return m | ||
| } |
There was a problem hiding this comment.
this looks like a good usecase for generics
config/keys.go
Outdated
|
|
||
| // toMapStrInt converts map[string]interface{} -> map[string]int | ||
| func toMapStrInt(vals map[string]interface{}) map[string]int { | ||
| m := make(map[string]int) |
There was a problem hiding this comment.
| m := make(map[string]int) | |
| m := make(map[string]int, len(vals)) |
config/keys.go
Outdated
|
|
||
| const ( | ||
| // network configuration keys | ||
| NetworkingConnectionPruningKey = "networking-connection-pruning" |
There was a problem hiding this comment.
Given the rest of our codebase, I seems more natural to define the default and yaml mapping in go. e.g.
type NetworkConfig struct {
// Connection pruning determines whether connections to nodes
// that are not part of protocol state should be trimmed
ConnectionPruningEnabled bool `yaml:"networking-connection-pruning"`
...
}
var defaultNetworkConfig = NetworkConfig{
ConnectionPruningEnabled: true,
...
}then we could generate the default yaml config using yaml.Marshal()
this comes with a few benefits:
- Define config and field names in the same file (fewer files to manage)
- Less likely to have odd whitespace issues due to yaml parsing
- When we add file based configs for operators, there's a consistent code path for the config file generation
- We now have a type to add methods to. e.g.
BindParamsto encapsulate adding pflags
cmd/scaffold.go
Outdated
| fnb.flags.DurationVar(&fnb.BaseConfig.GossipSubConfig.LocalMeshLogInterval, "gossipsub-local-mesh-logging-interval", defaultConfig.GossipSubConfig.LocalMeshLogInterval, "logging interval for local mesh in gossipsub") | ||
| fnb.flags.DurationVar(&fnb.BaseConfig.GossipSubConfig.ScoreTracerInterval, "gossipsub-score-tracer-interval", defaultConfig.GossipSubConfig.ScoreTracerInterval, "logging interval for peer score tracer in gossipsub, set to 0 to disable") | ||
| // network config cli flags | ||
| fnb.flags.Bool(config.NetworkingConnectionPruningKey, config.NetworkConnectionPruning(), "enabling connection trimming") |
There was a problem hiding this comment.
if we're centralizing the config, what if we move the description as well. This could become:
fnb.flags.Bool(config.NetworkConnectionPruningFlag())where NetworkConnectionPruningFlag is something like
// NetworkConnectionPruningFlag returns pflag config for networking-connection-pruning
func NetworkConnectionPruningFlag() (string, bool, string) {
return NetworkingConnectionPruningKey,
conf.GetBool(NetworkingConnectionPruningKey),
"enabling connection trimming"
}This way we don't need to expose the key name const, and can keep it only within the library.
If you implement something like my comment re: NetworkConfig, it could look something like
// NetworkConnectionPruningFlag returns pflag config for networking-connection-pruning
func (c NetworkConfig) NetworkConnectionPruningFlag() (string, bool, string) {
return NetworkingConnectionPruningKey,
c.ConnectionPruningEnabled,
"enabling connection trimming"
}| @@ -39,7 +39,7 @@ func NewRateLimiter(limit rate.Limit, burst int, lockoutDuration time.Duration, | |||
| limiterMap: internal.NewLimiterMap(rateLimiterTTL, cleanUpTickInterval), | |||
| limit: limit, | |||
| burst: burst, | |||
| rateLimitLockoutDuration: lockoutDuration * time.Second, | |||
| rateLimitLockoutDuration: lockoutDuration, | |||
There was a problem hiding this comment.
can you split these fixes out into their own PR. this looks like the rate limits currently have a permanent lockout
…etwork-config-improvements
- define Flow configuration func - move all network configuration structs to a subpackage of the config package - update all builder, test and fixtures
…onflow/flow-go into khalil/1888-network-config-improvements
kc1116
requested review from
jordanschalm,
peterargue and
AlexHentschel
and removed request for
gomisha
There was a problem hiding this comment.
All configuration values should be initialized in one space from a single source of truth config.yml
Have you given any thought to role-specific configs? For example, only the consensus node runs the DKG engine. It defines those configs within its main.go file, separate from the FlowNodeBuilder config fields.
Would we want to include those role-specific configs in the central config package as well, or is the idea to focus on role-generic configs only?
| func (c *CtrlMsgValidationConfig) SetRateLimiter(r p2p.BasicRateLimiter) { | ||
| c.rateLimiter = r | ||
| } | ||
|
|
||
| func (c *CtrlMsgValidationConfig) RateLimiter() p2p.BasicRateLimiter { | ||
| return c.rateLimiter | ||
| } |
There was a problem hiding this comment.
Might make more sense to put this into ControlMsgValidationInspector, since it is essentially a dependency of that component. Feels a bit out of place here, since it is created in the constructor, rather than read in from flags/config file. Not a big deal though.
follower/follower_builder.go
Outdated
| @@ -478,8 +479,9 @@ func (builder *FollowerServiceBuilder) InitIDProviders() { | |||
| return fmt.Errorf("could not initialize ProtocolStateIDCache: %w", err) | |||
| } | |||
| builder.IDTranslator = translator.NewHierarchicalIDTranslator(idCache, translator.NewPublicNetworkIDTranslator()) | |||
| fmt.Println(builder.BaseConfig.FlowConfig) | |||
config/network/errors.go
Outdated
| type ErrInvalidLimitConfig struct { | ||
| // controlMsg the control message type. | ||
| controlMsg p2p.ControlMessageType | ||
| err error |
There was a problem hiding this comment.
if we want to be able to interpret this as part of the error chain, need to implement Unwrap() method
config/network/errors.go
Outdated
| func NewInvalidLimitConfigErr(controlMsg p2p.ControlMessageType, err error) ErrInvalidLimitConfig { | ||
| return ErrInvalidLimitConfig{controlMsg: controlMsg, err: err} |
There was a problem hiding this comment.
| func NewInvalidLimitConfigErr(controlMsg p2p.ControlMessageType, err error) ErrInvalidLimitConfig { | |
| return ErrInvalidLimitConfig{controlMsg: controlMsg, err: err} | |
| func NewInvalidLimitConfigError(controlMsg p2p.ControlMessageType, msg string, args ...any) ErrInvalidLimitConfig { | |
| return InvalidLimitConfigError{controlMsg: controlMsg, err: fmt.Errorf(msg, args...)} |
Optional: I find this is a useful pattern to define error types:
- guarantees
MyError.erris non-nil - invocation is similar to
fmt.Errorf+ don't need to embedfmt.Errorfat the callsite + encourages the caller to add additional context (likeErrorf)
There was a problem hiding this comment.
This pattern is less useful for the usage of NewInvalidLimitConfigError since the error is simple the only context needed is the control msg type and the validation err to wrap.
config/config.yml
Outdated
| @@ -0,0 +1,124 @@ | |||
| # Network Configuration | |||
There was a problem hiding this comment.
Maybe we could call this file default-config.yml?
config/config.go
Outdated
| //go:embed config.yml | ||
| configFile string |
There was a problem hiding this comment.
Right now the yml file essentially acts as an encoded representation of config defaults. Do we want to allow node operators to actually provide a config.yml file to configure their node as well? I think doing so would be beneficial (and mostly leverage functionality already provided by Viper).
But if we support that, we'll need to adjust this package to accept a file input from the node scaffold, and rename configFile etc. to make it clear it is only defaults.
- Fix flag binding not working as expected by ensuring there is a alias set from config flag name -> conf file key due to the fact viper prepends property names to keys for nested config values - reduce the network config to one nested layer under the network-config property to reduce complexity with aliases - fix printing configs when configuration is loaded - use mapstructure .squash tag when using nested config structs
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
I think we should group configs by component or collection of components (used together and required for some specific functionality). There may or may not be components used across multiple node roles. For example, the "Network" is not an explicitly defined component but a set of components that work together to ensure a functioning network. Furthermore "Consensus" is not an explicitly defined component but a collection of components that work together to ensure a functioning consensus layer. So I would want to include all configuration structs for consensus-related components under a single package in the config package. Currently, DKG configs are defined in the main.go for consensus nodes but it probably should be defined in a config/consensus package where we can take a look at a single package and get a full view of all consensus-related configs. |
@AlexHentschel I agree we should not have a single config for each, and thats not an issue. Currently there is a single default-config.yml because only the network-config is defined in the package. In order to improve readability we can define a .yml file per sub config package in the future. The main goal is to move all configuration to a centralized location to improve clarity, and make updating or editing configurations simpler. |
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
…onflow/flow-go into khalil/1888-network-config-improvements
config/config.go
Outdated
| // | ||
| // error: if there is any error encountered while reading new config file, all errors are considered irrecoverable. | ||
| // bool: true if the config was overridden by the new config file, false otherwise or if an error is encountered reading the new config file. | ||
| func overrideConfigFile(flags *pflag.FlagSet) (error, bool) { |
There was a problem hiding this comment.
(1) The function overrideConfigFile returns (error, bool). It is more idiomatic in Go to have the error as the last return value. So it is better to have it return (bool, error).
(2) Also, it is good to see that functions have comments that explain what they do. However, the documentation is not self-sufficient regarding the configFilePath.
| @@ -92,6 +113,71 @@ func Unmarshall(flowConfig *FlowConfig) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Print prints current configuration keys and values. | |||
There was a problem hiding this comment.
Regarding "prints current configuration keys and values", the documentation does not explicitly show any print statements. Instead, it stores key-value pairs into a logger (info). This might be slightly confusing for someone reading the comment and expecting direct print output. To improve the structure of the Print function, I'd recommend renaming it to better reflect its purpose and improving its documentation. I'd also recommend passing the necessary configurations as parameters instead of relying on the global state.
Suggestion:
// LogConfig logs configuration keys and values if they were overridden with a config file.
// It also returns a map of keys for which the values were set by a config file.
//
// Parameters:
// - logger: *zerolog.Event to which the configuration keys and values will be logged.
// - flags: *pflag.FlagSet containing the set flags.
// - conf: configuration store (assumed to be of a type that supports AllKeys and Get methods).
//
// Returns:
// - map[string]struct{}: map of keys for which the values were set by a config file.
func LogConfig(logger *zerolog.Event, flags *pflag.FlagSet, conf interface{ AllKeys() []string; Get(key string) interface{} }) map[string]struct{} {
keysToAvoid := make(map[string]struct{})
if flags.Lookup(configFilePath).Changed {
for _, key := range conf.AllKeys() {
logger.Str(key, fmt.Sprintf("%v", conf.Get(key)))
parts := strings.Split(key, ".")
if len(parts) == 2 {
keysToAvoid[parts[1]] = struct{}{}
} else {
keysToAvoid[key] = struct{}{}
}
}
}
return keysToAvoid
}| m := make(map[string]struct{}) | ||
| if flags.Lookup(configFilePath).Changed { | ||
| for _, key := range conf.AllKeys() { | ||
| info.Str(key, fmt.Sprintf("%v", conf.Get(key))) |
There was a problem hiding this comment.
It can be simplified into info.Str(key, fmt.Sprint(conf.Get(key))).
| } | ||
|
|
||
| // getConfigNameFromPath returns the directory and name of the config file from the provided path string. | ||
| func splitConfigPath(path string) (string, string) { |
There was a problem hiding this comment.
In this function, you are trying to split the path into the directory and the base name of the config file without the extension. This is fine, but it’s good to note that if someone names a config file with multiple dots (e.g., my.config.yaml), this function would consider my as the name which might not be intended. I suggest the following implementation that uses a regular expression pattern and panics if the input path does not match the pattern:
// splitConfigPath returns the directory and base name (without extension) of the config file from the provided path string.
// If the file name does not match the expected pattern, the function panics.
//
// The expected pattern for file names is that they must consist of alphanumeric characters, hyphens, or underscores,
// followed by a single dot and then the extension.
//
// Legitimate Inputs:
// - /path/to/my_config.yaml
// - /path/to/my-config123.yaml
// - my-config.yaml (when in the current directory)
//
// Illegitimate Inputs:
// - /path/to/my.config.yaml (contains multiple dots)
// - /path/to/my config.yaml (contains spaces)
// - /path/to/.config.yaml (does not have a file name before the dot)
//
// Args:
// - path: The file path string to be split into directory and base name.
//
// Returns:
// - The directory and base name without extension.
//
// Panics:
// - If the file name does not match the expected pattern.
func splitConfigPath(path string) (string, string) {
// Regex to match filenames like 'my_config.yaml' or 'my-config.yaml' but not 'my.config.yaml'
validFileNamePattern := regexp.MustCompile(`^[a-zA-Z0-9_-]+\.[a-zA-Z0-9]+$`)
dir, name := filepath.Split(path)
// Panic if the file name does not match the expected pattern
if !validFileNamePattern.MatchString(name) {
panic(fmt.Errorf("Invalid config file name '%s'. Expected pattern: alphanumeric, hyphens, or underscores followed by a single dot and extension", name))
}
// Extracting the base name without extension
baseName := strings.Split(name, ".")[0]
return dir, baseName
}| if !ok { | ||
| return fmt.Errorf("invalid network configuration missing configuration key flag name %s check config file and cli flags", flagName) | ||
| } | ||
| conf.RegisterAlias(fullKey, flagName) |
There was a problem hiding this comment.
shouldn't this be conf.RegisterAlias(flagName, fullKey) according to the documentation of Viper?
There was a problem hiding this comment.
No the usage is correct. When keys bound from the CLI are added to the config store they are added without any of the prefixes. Because we want to access config values without using long keys with prefixes we set an alias from the keys bound from the CLI -> keys read in from config files that may have prefixes.
…etwork-config-improvements
This PR addresses the current issues related to config management in Flow. Here is the current structure when adding a new config value;
This process requires multiple changes to multiple files in multiple packages of the code base and can be error-prone when updating or adding multiple config values. This structure leads to default values existing in multiple different places throughout the code base making it hard for developers to get a full picture of the entire Flow configuration with default values. Furthermore, each config value can easily be erroneously overridden by the application code during node startup because the entire NodeConfig is accessible from the node builder struct.
This PR aims to address these issues by providing the following changes;
config.yml. All configuration values should be accessible from a single package and should not be overridable by the application code.config.yml.This PR establishes a new config package and a standardized default configuration file config.yml. config.yml represents the entire Flow config with default values for each of the relevant subcomponents for Flow. You will also find a config.go file that defines the FlowConfig struct. This struct is used to unmarshal config.yml after it has been read. The FlowConfig struct fields are configuration structs for the various subcomponents of Flow. The FlowConfig struct contains a Validate() func that invokes all the validation funcs defined on each sub-configuration structs, allowing devs to define Validate methods of any of the sub-configuration structs. Finally, defining CLI flags for each sub-component should be done in the corresponding sub-package for that component in the config package. Now configuration structs, validation funcs and pflag instantiation are contained in the config sub-package for the component they are related to rather then all flags being instantiated inside scaffold.go separate from config struct definitions and separate from any default value definitions. All configuration for the flow network has been moved to the sub-packge config/network. In the package a configuration struct for each subcomponent of the network is defined, validate funcs are defined as required on any of the configuration structs and all network CLI flags are defined in this package. This setup now contains and updates the entire network configuration from a single package.
Example steps to add new configuration value for the network config.
config.ymlNow adding a new config value and accessing the new config value is completely contained in the config package.
I suggest starting PR review from the new config package, most of the other changes are usage updates.