0ct0pu5/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Merge pull request #43536 from thaJeztah/daemon_fix_hosts_validation_step1g

Browse source 
daemon: improvements to config (re)loading
This commit is contained in:
Sebastiaan van Stijn 2022-04-29 09:39:11 +02:00 • committed by GitHub
commit 4d22584432
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 42 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

51
daemon/config/config.go
View file

@ -343,10 +343,6 @@ func Reload(configFile string, flags *pflag.FlagSet, reload func(*Config)) error
newConfig = New()
}
if err := Validate(newConfig); err != nil {
return errors.Wrap(err, "file configuration validation failed")
}
// Check if duplicate label-keys with different values are found
newLabels, err := GetConflictFreeLabels(newConfig.Labels)
if err != nil {
@ -354,6 +350,30 @@ func Reload(configFile string, flags *pflag.FlagSet, reload func(*Config)) error
}
newConfig.Labels = newLabels
// TODO(thaJeztah) This logic is problematic and needs a rewrite;
// This is validating newConfig before the "reload()" callback is executed.
// At this point, newConfig may be a partial configuration, to be merged
// with the existing configuration in the "reload()" callback. Validating
// this config before it's merged can result in incorrect validation errors.
//
// However, the current "reload()" callback we use is DaemonCli.reloadConfig(),
// which includes a call to Daemon.Reload(), which both performs "merging"
// and validation, as well as actually updating the daemon configuration.
// Calling DaemonCli.reloadConfig() *before* validation, could thus lead to
// a failure in that function (making the reload non-atomic).
//
// While *some* errors could always occur when applying/updating the config,
// we should make it more atomic, and;
//
// 1. get (a copy of) the active configuration
// 2. get the new configuration
// 3. apply the (reloadable) options from the new configuration
// 4. validate the merged results
// 5. apply the new configuration.
if err := Validate(newConfig); err != nil {
return errors.Wrap(err, "file configuration validation failed")
}
reload(newConfig)
return nil
}
@ -374,17 +394,12 @@ func MergeDaemonConfigurations(flagsConfig *Config, flags *pflag.FlagSet, config
return nil, err
}
if err := Validate(fileConfig); err != nil {
return nil, errors.Wrap(err, "configuration validation from file failed")
}
// merge flags configuration on top of the file configuration
if err := mergo.Merge(fileConfig, flagsConfig); err != nil {
return nil, err
}
// We need to validate again once both fileConfig and flagsConfig
// have been merged
// validate the merged fileConfig and flagsConfig
if err := Validate(fileConfig); err != nil {
return nil, errors.Wrap(err, "merged configuration validation from file and command line flags failed")
}
@ -585,16 +600,16 @@ func Validate(config *Config) error {
return err
}
}
// validate MaxConcurrentDownloads
// TODO(thaJeztah) Validations below should not accept "0" to be valid; see Validate() for a more in-depth description of this problem
if config.MaxConcurrentDownloads != nil && *config.MaxConcurrentDownloads < 0 {
return fmt.Errorf("invalid max concurrent downloads: %d", *config.MaxConcurrentDownloads)
}
// validate MaxConcurrentUploads
if config.MaxConcurrentUploads != nil && *config.MaxConcurrentUploads < 0 {
return fmt.Errorf("invalid max concurrent uploads: %d", *config.MaxConcurrentUploads)
}
if err := ValidateMaxDownloadAttempts(config); err != nil {
return err
if config.MaxDownloadAttempts != nil && *config.MaxDownloadAttempts < 0 {
return fmt.Errorf("invalid max download attempts: %d", *config.MaxDownloadAttempts)
}
// validate that "default" runtime is not reset
@ -627,14 +642,6 @@ func Validate(config *Config) error {
return config.ValidatePlatformConfig()
}
// ValidateMaxDownloadAttempts validates if the max-download-attempts is within the valid range
func ValidateMaxDownloadAttempts(config *Config) error {
if config.MaxDownloadAttempts != nil && *config.MaxDownloadAttempts <= 0 {
return fmt.Errorf("invalid max download attempts: %d", *config.MaxDownloadAttempts)
}
return nil
}
// GetDefaultRuntimeName returns the current default runtime
func (conf *Config) GetDefaultRuntimeName() string {
conf.Lock()

17
daemon/config/config_test.go
View file

@ -309,15 +309,18 @@ func TestValidateConfigurationErrors(t *testing.T) {
},
expectedErr: "invalid max download attempts: -10",
},
{
name: "zero max-download-attempts",
config: &Config{
CommonConfig: CommonConfig{
MaxDownloadAttempts: intPtr(0),
// TODO(thaJeztah) temporarily excluding this test as it assumes defaults are set before validating and applying updated configs
/*
{
name: "zero max-download-attempts",
config: &Config{
CommonConfig: CommonConfig{
MaxDownloadAttempts: intPtr(0),
},
},
expectedErr: "invalid max download attempts: 0",
},
expectedErr: "invalid max download attempts: 0",
},
*/
{
name: "generic resource without =",
config: &Config{

14
daemon/reload.go
View file

@ -56,9 +56,7 @@ func (daemon *Daemon) Reload(conf *config.Config) (err error) {
}
daemon.reloadDebug(conf, attributes)
daemon.reloadMaxConcurrentDownloadsAndUploads(conf, attributes)
if err := daemon.reloadMaxDownloadAttempts(conf, attributes); err != nil {
return err
}
daemon.reloadMaxDownloadAttempts(conf, attributes)
daemon.reloadShutdownTimeout(conf, attributes)
daemon.reloadFeatures(conf, attributes)
@ -124,23 +122,17 @@ func (daemon *Daemon) reloadMaxConcurrentDownloadsAndUploads(conf *config.Config
// reloadMaxDownloadAttempts updates configuration with max concurrent
// download attempts when a connection is lost and updates the passed attributes
func (daemon *Daemon) reloadMaxDownloadAttempts(conf *config.Config, attributes map[string]string) error {
if err := config.ValidateMaxDownloadAttempts(conf); err != nil {
return err
}
// If no value is set for max-download-attempts we assume it is the default value
func (daemon *Daemon) reloadMaxDownloadAttempts(conf *config.Config, attributes map[string]string) {
// We always "reset" as the cost is lightweight and easy to maintain.
maxDownloadAttempts := config.DefaultDownloadAttempts
if conf.IsValueSet("max-download-attempts") && conf.MaxDownloadAttempts != nil {
maxDownloadAttempts = *conf.MaxDownloadAttempts
}
daemon.configStore.MaxDownloadAttempts = &maxDownloadAttempts
logrus.Debugf("Reset Max Download Attempts: %d", *daemon.configStore.MaxDownloadAttempts)
// prepare reload event attributes with updatable configurations
attributes["max-download-attempts"] = fmt.Sprintf("%d", *daemon.configStore.MaxDownloadAttempts)
return nil
logrus.Debug("Reset Max Download Attempts: ", attributes["max-download-attempts"])
}
// reloadShutdownTimeout updates configuration with daemon shutdown timeout option