From da30d06708cd376382819b6111ce23ee0b4c6d21 Mon Sep 17 00:00:00 2001 From: Ludovico de Nittis Date: Wed, 6 Sep 2023 18:47:41 +0200 Subject: [PATCH 1/5] Remove deprecated "http-timeout" and "http-error-retry" options Those two options have been deprecated 5 years ago. There is likely no reason to keep them around anymore. Signed-off-by: Ludovico de Nittis --- README.md | 2 -- cmd/desync/config.go | 6 ++---- cmd/desync/store.go | 20 -------------------- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index fe0a9e7..b731f16 100644 --- a/README.md +++ b/README.md @@ -240,8 +240,6 @@ For most use cases, it is sufficient to use the tool's default configuration not Available configuration values: -- `http-timeout` *DEPRECATED, see `store-options..timeout`* - HTTP request timeout used in HTTP stores (not S3) in nanoseconds -- `http-error-retry` *DEPRECATED, see `store-options..error-retry` - Number of times to retry failed chunk requests from HTTP stores - `s3-credentials` - Defines credentials for use with S3 stores. Especially useful if more than one S3 store is used. The key in the config needs to be the URL scheme and host used for the store, excluding the path, but including the port number if used in the store URL. The key can also contain glob patterns, and the available wildcards are `*`, `?` and `[…]`. Please refer to the [filepath.Match](https://pkg.go.dev/path/filepath#Match) documentation for additional information. It is also possible to use a [standard aws credentials file](https://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html) in order to store s3 credentials. - `store-options` - Allows customization of chunk and index stores, for example compression settings, timeouts, retry behavior and keys. Not all options are applicable to every store, some of these like `timeout` are ignored for local stores. Some of these options, such as the client certificates are overwritten with any values set in the command line. Note that the store location used in the command line needs to match the key under `store-options` exactly for these options to be used. As for the `s3-credentials`, glob patterns are also supported. A configuration file where more than one key matches a single store location, is considered invalid. - `timeout` - Time limit for chunk read or write operation in nanoseconds. Default: 1 minute. If set to a negative value, timeout is infinite. diff --git a/cmd/desync/config.go b/cmd/desync/config.go index 10b894d..c7c393c 100644 --- a/cmd/desync/config.go +++ b/cmd/desync/config.go @@ -32,10 +32,8 @@ type S3Creds struct { // Config is used to hold the global tool configuration. It's used to customize // store features and provide credentials where needed. type Config struct { - HTTPTimeout time.Duration `json:"http-timeout,omitempty"` - HTTPErrorRetry int `json:"http-error-retry,omitempty"` - S3Credentials map[string]S3Creds `json:"s3-credentials"` - StoreOptions map[string]desync.StoreOptions `json:"store-options"` + S3Credentials map[string]S3Creds `json:"s3-credentials"` + StoreOptions map[string]desync.StoreOptions `json:"store-options"` } // GetS3CredentialsFor attempts to find creds and region for an S3 location in the diff --git a/cmd/desync/store.go b/cmd/desync/store.go index ee098b7..8190a90 100644 --- a/cmd/desync/store.go +++ b/cmd/desync/store.go @@ -123,16 +123,6 @@ func storeFromLocation(location string, cmdOpt cmdStoreOptions) (desync.Store, e return nil, err } case "http", "https": - // This is for backwards compatibility only, to support http-timeout and - // http-error-retry in the config file for a bit longer. If those are - // set, and the options for the store aren't, then use the old values. - // TODO: Remove this code and drop these config options in the future. - if opt.Timeout == 0 && cfg.HTTPTimeout > 0 { - opt.Timeout = cfg.HTTPTimeout - } - if opt.ErrorRetry == 0 && cfg.HTTPErrorRetry > 0 { - opt.ErrorRetry = cfg.HTTPErrorRetry - } s, err = desync.NewRemoteHTTPStore(loc, opt) if err != nil { return nil, err @@ -252,16 +242,6 @@ func indexStoreFromLocation(location string, cmdOpt cmdStoreOptions) (desync.Ind return nil, "", err } case "http", "https": - // This is for backwards compatibility only, to support http-timeout and - // http-error-retry in the config file for a bit longer. If those are - // set, and the options for the store aren't, then use the old values. - // TODO: Remove this code and drop these config options in the future. - if opt.Timeout == 0 && cfg.HTTPTimeout > 0 { - opt.Timeout = cfg.HTTPTimeout - } - if opt.ErrorRetry == 0 && cfg.HTTPErrorRetry > 0 { - opt.ErrorRetry = cfg.HTTPErrorRetry - } s, err = desync.NewRemoteHTTPIndexStore(&p, opt) if err != nil { return nil, "", err From b400e901291903e2abab955215b8c7f7866e2a6c Mon Sep 17 00:00:00 2001 From: Ludovico de Nittis Date: Fri, 8 Sep 2023 15:49:30 +0200 Subject: [PATCH 2/5] Keep the CLI options while querying the stores in `info` Signed-off-by: Ludovico de Nittis --- cmd/desync/info.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/desync/info.go b/cmd/desync/info.go index e5806fc..33f2662 100644 --- a/cmd/desync/info.go +++ b/cmd/desync/info.go @@ -122,7 +122,7 @@ func runInfo(ctx context.Context, opt infoOptions, args []string) error { results.Unique = len(deduped) if len(opt.stores) > 0 { - store, err := multiStoreWithRouter(cmdStoreOptions{n: opt.n}, opt.stores...) + store, err := multiStoreWithRouter(opt.cmdStoreOptions, opt.stores...) if err != nil { return err } From a4ed4c15fca98bd6cb8d1ad150078ef43b4149f8 Mon Sep 17 00:00:00 2001 From: Ludovico de Nittis Date: Fri, 8 Sep 2023 17:01:35 +0200 Subject: [PATCH 3/5] Correctly set the default option values and merge them with the config When loading a config file, we need to set the options to their default values. Otherwise, when merging it with the CLI options, we could end up with unexpected results. Additionally we need to distinguish between a CLI option that is set to its default value because it was not provided as a launch parameter or if it was explicitly set by the client. This is important because in `MergedWith()` we should prefer the CLI option only if it was explicitly set. Signed-off-by: Ludovico de Nittis --- cmd/desync/config.go | 2 +- cmd/desync/options.go | 18 ++-- cmd/desync/options_test.go | 164 +++++++++++++++++++++++++++++++++++++ store.go | 20 ++++- 4 files changed, 194 insertions(+), 10 deletions(-) create mode 100644 cmd/desync/options_test.go diff --git a/cmd/desync/config.go b/cmd/desync/config.go index c7c393c..448f137 100644 --- a/cmd/desync/config.go +++ b/cmd/desync/config.go @@ -78,7 +78,7 @@ func (c Config) GetS3CredentialsFor(u *url.URL) (*credentials.Credentials, strin // config file. func (c Config) GetStoreOptionsFor(location string) (options desync.StoreOptions, err error) { found := false - options = desync.StoreOptions{} + options = desync.NewStoreOptionsWithDefaults() for k, v := range c.StoreOptions { if locationMatch(k, location) { if found { diff --git a/cmd/desync/options.go b/cmd/desync/options.go index 70981b2..e3b2082 100644 --- a/cmd/desync/options.go +++ b/cmd/desync/options.go @@ -18,28 +18,30 @@ type cmdStoreOptions struct { trustInsecure bool cacheRepair bool errorRetry int + pflag.FlagSet } -// MergeWith takes store options as read from the config, and applies command-line +// MergedWith takes store options as read from the config, and applies command-line // provided options on top of them and returns the merged result. func (o cmdStoreOptions) MergedWith(opt desync.StoreOptions) desync.StoreOptions { opt.N = o.n - if o.clientCert != "" { + + if o.FlagSet.Lookup("client-cert").Changed { opt.ClientCert = o.clientCert } - if o.clientKey != "" { + if o.FlagSet.Lookup("client-key").Changed { opt.ClientKey = o.clientKey } - if o.caCert != "" { + if o.FlagSet.Lookup("ca-cert").Changed { opt.CACert = o.caCert } if o.skipVerify { opt.SkipVerify = true } - if o.trustInsecure { + if o.FlagSet.Lookup("trust-insecure").Changed { opt.TrustInsecure = true } - if o.errorRetry > 0 { + if o.FlagSet.Lookup("error-retry").Changed { opt.ErrorRetry = o.errorRetry } return opt @@ -61,7 +63,9 @@ func addStoreOptions(o *cmdStoreOptions, f *pflag.FlagSet) { f.StringVar(&o.caCert, "ca-cert", "", "trust authorities in this file, instead of OS trust store") f.BoolVarP(&o.trustInsecure, "trust-insecure", "t", false, "trust invalid certificates") f.BoolVarP(&o.cacheRepair, "cache-repair", "r", true, "replace invalid chunks in the cache from source") - f.IntVarP(&o.errorRetry, "error-retry", "e", 3, "number of times to retry in case of network error") + f.IntVarP(&o.errorRetry, "error-retry", "e", desync.DefaultErrorRetry, "number of times to retry in case of network error") + + o.FlagSet = *f } // cmdServerOptions hold command line options used in HTTP servers. diff --git a/cmd/desync/options_test.go b/cmd/desync/options_test.go new file mode 100644 index 0000000..5faca83 --- /dev/null +++ b/cmd/desync/options_test.go @@ -0,0 +1,164 @@ +package main + +import ( + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "os" + "testing" +) + +const defaultErrorRetry = 3 + +func newTestOptionsCommand(opt *cmdStoreOptions) *cobra.Command { + cmd := &cobra.Command{} + + addStoreOptions(opt, cmd.Flags()) + return cmd +} + +func TestErrorRetryOptions(t *testing.T) { + for _, test := range []struct { + name string + args []string + cfgFileContent []byte + errorRetryStoreHit int + errorRetryStoreMiss int + }{ + {"Config with error-retry set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), + 20, defaultErrorRetry, + }, + {"Error retry via command line args", + []string{"--error-retry", "10"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), + 10, 10, + }, + {"Config without error-retry", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), + defaultErrorRetry, defaultErrorRetry, + }, + {"Config with default error-retry", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 3}}}`), + defaultErrorRetry, defaultErrorRetry, + }, + {"Config that disables error-retry", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 0}}}`), + 0, defaultErrorRetry, + }, + {"Disables error-retry via command line args", + []string{"--error-retry", "0"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), + 0, 0, + }, + {"Force the default values via command line args", + []string{"--error-retry", "3"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), + defaultErrorRetry, defaultErrorRetry, + }, + } { + t.Run(test.name, func(t *testing.T) { + f, err := os.CreateTemp("", "desync-options") + require.NoError(t, err) + defer os.Remove(f.Name()) + _, err = f.Write(test.cfgFileContent) + require.NoError(t, err) + + // Set the global config file name + cfgFile = f.Name() + + initConfig() + + var cmdOpt cmdStoreOptions + + cmd := newTestOptionsCommand(&cmdOpt) + cmd.SetArgs(test.args) + + // Execute the mock command, to load the options provided in the launch arguments + _, err = cmd.ExecuteC() + require.NoError(t, err) + + configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") + opt := cmdOpt.MergedWith(configOptions) + require.Equal(t, test.errorRetryStoreHit, opt.ErrorRetry) + + configOptions, err = cfg.GetStoreOptionsFor("/missingStore") + opt = cmdOpt.MergedWith(configOptions) + require.NoError(t, err) + require.Equal(t, test.errorRetryStoreMiss, opt.ErrorRetry) + }) + } +} + +func TestStringOptions(t *testing.T) { + for _, test := range []struct { + name string + args []string + cfgFileContent []byte + clientCertStoreHit string + clientCertStoreMiss string + clientKeyStoreHit string + clientKeyStoreMiss string + caCertStoreHit string + caCertStoreMiss string + }{ + {"Config with options set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "/foo", "", "/bar", "", "/baz", "", + }, + {"Configs set via command line args", + []string{"--client-cert", "/aa/bb", "--client-key", "/another", "--ca-cert", "/ca"}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "/aa/bb", "/aa/bb", "/another", "/another", "/ca", "/ca", + }, + {"Config without any of those string options set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), + "", "", "", "", "", "", + }, + {"Disable values from CLI args", + []string{"--client-cert", "", "--client-key", "", "--ca-cert", ""}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "", "", "", "", "", "", + }, + } { + t.Run(test.name, func(t *testing.T) { + f, err := os.CreateTemp("", "desync-options") + require.NoError(t, err) + defer os.Remove(f.Name()) + _, err = f.Write(test.cfgFileContent) + require.NoError(t, err) + + // Set the global config file name + cfgFile = f.Name() + + initConfig() + + var cmdOpt cmdStoreOptions + + cmd := newTestOptionsCommand(&cmdOpt) + cmd.SetArgs(test.args) + + // Execute the mock command, to load the options provided in the launch arguments + _, err = cmd.ExecuteC() + require.NoError(t, err) + + configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") + opt := cmdOpt.MergedWith(configOptions) + require.Equal(t, test.clientCertStoreHit, opt.ClientCert) + require.Equal(t, test.clientKeyStoreHit, opt.ClientKey) + require.Equal(t, test.caCertStoreHit, opt.CACert) + + configOptions, err = cfg.GetStoreOptionsFor("/missingStore") + opt = cmdOpt.MergedWith(configOptions) + require.NoError(t, err) + require.Equal(t, test.clientCertStoreMiss, opt.ClientCert) + require.Equal(t, test.clientKeyStoreMiss, opt.ClientKey) + require.Equal(t, test.caCertStoreMiss, opt.CACert) + }) + } +} diff --git a/store.go b/store.go index fc2496b..343a456 100644 --- a/store.go +++ b/store.go @@ -2,11 +2,14 @@ package desync import ( "context" + "encoding/json" "fmt" "io" "time" ) +const DefaultErrorRetry = 3 + // Store is a generic interface implemented by read-only stores, like SSH or // HTTP remote stores currently. type Store interface { @@ -71,7 +74,7 @@ type StoreOptions struct { Timeout time.Duration `json:"timeout,omitempty"` // Number of times object retrieval should be attempted on error. Useful when dealing - // with unreliable connections. Default: 0 + // with unreliable connections. ErrorRetry int `json:"error-retry,omitempty"` // Number of nanoseconds to wait before first retry attempt. @@ -91,12 +94,25 @@ type StoreOptions struct { Uncompressed bool `json:"uncompressed"` } +// NewStoreOptionsWithDefaults creates a new StoreOptions struct with the default values set +func NewStoreOptionsWithDefaults() (o StoreOptions) { + o.ErrorRetry = DefaultErrorRetry + return o +} + +func (o *StoreOptions) UnmarshalJSON(data []byte) error { + // Set all the default values before loading the JSON store options + o.ErrorRetry = DefaultErrorRetry + type Alias StoreOptions + return json.Unmarshal(data, (*Alias)(o)) +} + // Returns data converters that convert between plain and storage-format. Each layer // represents a modification such as compression or encryption and is applied in order // depending the direction of data. If data is written to storage, the layer's toStorage // method is called in the order they are returned. If data is read, the fromStorage // method is called in reverse order. -func (o StoreOptions) converters() []converter { +func (o *StoreOptions) converters() []converter { var m []converter if !o.Uncompressed { m = append(m, Compressor{}) From a55fbd5542a3a4b66519b3c15a0a99223bff6781 Mon Sep 17 00:00:00 2001 From: Ludovico de Nittis Date: Fri, 8 Sep 2023 18:29:58 +0200 Subject: [PATCH 4/5] Add --error-retry-base-interval as a CLI option Setting the base interval between error retries is a useful option, especially when the Internet connection might be unstable. Given that `error-retry` can already be set via the CLI, by adding an option for `error-retry-base-interval` too we can avoid the need to force users to ship a config file when its only purpose was to only change this value. Signed-off-by: Ludovico de Nittis --- cmd/desync/options.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/cmd/desync/options.go b/cmd/desync/options.go index e3b2082..69b5508 100644 --- a/cmd/desync/options.go +++ b/cmd/desync/options.go @@ -2,6 +2,7 @@ package main import ( "errors" + "time" "github.com/folbricht/desync" "github.com/spf13/pflag" @@ -10,14 +11,15 @@ import ( // cmdStoreOptions are used to pass additional options to store initialization from the // commandline. These generally override settings from the config file. type cmdStoreOptions struct { - n int - clientCert string - clientKey string - caCert string - skipVerify bool - trustInsecure bool - cacheRepair bool - errorRetry int + n int + clientCert string + clientKey string + caCert string + skipVerify bool + trustInsecure bool + cacheRepair bool + errorRetry int + errorRetryBaseInterval time.Duration pflag.FlagSet } @@ -44,6 +46,9 @@ func (o cmdStoreOptions) MergedWith(opt desync.StoreOptions) desync.StoreOptions if o.FlagSet.Lookup("error-retry").Changed { opt.ErrorRetry = o.errorRetry } + if o.FlagSet.Lookup("error-retry-base-interval").Changed { + opt.ErrorRetryBaseInterval = o.errorRetryBaseInterval + } return opt } @@ -64,6 +69,7 @@ func addStoreOptions(o *cmdStoreOptions, f *pflag.FlagSet) { f.BoolVarP(&o.trustInsecure, "trust-insecure", "t", false, "trust invalid certificates") f.BoolVarP(&o.cacheRepair, "cache-repair", "r", true, "replace invalid chunks in the cache from source") f.IntVarP(&o.errorRetry, "error-retry", "e", desync.DefaultErrorRetry, "number of times to retry in case of network error") + f.DurationVarP(&o.errorRetryBaseInterval, "error-retry-base-interval", "b", 0, "initial retry delay, increases linearly with each subsequent attempt") o.FlagSet = *f } From dfd2636194b38736ce81e2fe6236f31e51cc8f75 Mon Sep 17 00:00:00 2001 From: Ludovico de Nittis Date: Fri, 8 Sep 2023 18:39:50 +0200 Subject: [PATCH 5/5] Set a 500ms default value for error-retry-base-interval When the Internet connection is unstable, a few requests might fail. In those cases, by default, Desync retries 3 times before giving up. However, if the connection momentarily becomes unstable and you retry immediately without waiting, the chances of those attempts still failing are very high. With this commit we set a more reasonable 500ms of base wait interval to give the clients an higher chance of success. Signed-off-by: Ludovico de Nittis --- cmd/desync/options.go | 2 +- cmd/desync/options_test.go | 60 +++++++++++++++++++++----------------- store.go | 4 ++- 3 files changed, 37 insertions(+), 29 deletions(-) diff --git a/cmd/desync/options.go b/cmd/desync/options.go index 69b5508..b1f39c8 100644 --- a/cmd/desync/options.go +++ b/cmd/desync/options.go @@ -69,7 +69,7 @@ func addStoreOptions(o *cmdStoreOptions, f *pflag.FlagSet) { f.BoolVarP(&o.trustInsecure, "trust-insecure", "t", false, "trust invalid certificates") f.BoolVarP(&o.cacheRepair, "cache-repair", "r", true, "replace invalid chunks in the cache from source") f.IntVarP(&o.errorRetry, "error-retry", "e", desync.DefaultErrorRetry, "number of times to retry in case of network error") - f.DurationVarP(&o.errorRetryBaseInterval, "error-retry-base-interval", "b", 0, "initial retry delay, increases linearly with each subsequent attempt") + f.DurationVarP(&o.errorRetryBaseInterval, "error-retry-base-interval", "b", desync.DefaultErrorRetryBaseInterval, "initial retry delay, increases linearly with each subsequent attempt") o.FlagSet = *f } diff --git a/cmd/desync/options_test.go b/cmd/desync/options_test.go index 5faca83..f1c90bd 100644 --- a/cmd/desync/options_test.go +++ b/cmd/desync/options_test.go @@ -5,9 +5,11 @@ import ( "github.com/stretchr/testify/require" "os" "testing" + "time" ) const defaultErrorRetry = 3 +const DefaultErrorRetryBaseInterval = 500 * time.Millisecond func newTestOptionsCommand(opt *cmdStoreOptions) *cobra.Command { cmd := &cobra.Command{} @@ -18,46 +20,48 @@ func newTestOptionsCommand(opt *cmdStoreOptions) *cobra.Command { func TestErrorRetryOptions(t *testing.T) { for _, test := range []struct { - name string - args []string - cfgFileContent []byte - errorRetryStoreHit int - errorRetryStoreMiss int + name string + args []string + cfgFileContent []byte + errorRetryStoreHit int + errorRetryStoreMiss int + baseIntervalStoreHit time.Duration + baseIntervalStoreMiss time.Duration }{ - {"Config with error-retry set", + {"Config with the error retry and base interval set", []string{""}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), - 20, defaultErrorRetry, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 20, defaultErrorRetry, 250000000, DefaultErrorRetryBaseInterval, }, - {"Error retry via command line args", - []string{"--error-retry", "10"}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), - 10, 10, + {"Error retry and base interval via command line args", + []string{"--error-retry", "10", "--error-retry-base-interval", "1s"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 10, 10, 1000000000, 1000000000, }, - {"Config without error-retry", + {"Config without error retry nor base interval", []string{""}, []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), - defaultErrorRetry, defaultErrorRetry, + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, }, - {"Config with default error-retry", + {"Config with default error retry and base interval", []string{""}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 3}}}`), - defaultErrorRetry, defaultErrorRetry, + []byte(`{"store-options": {"/store/*/":{"error-retry": 3, "error-retry-base-interval": 500000000}}}`), + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, }, - {"Config that disables error-retry", + {"Config that disables error retry and base interval", []string{""}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 0}}}`), - 0, defaultErrorRetry, + []byte(`{"store-options": {"/store/*/":{"error-retry": 0, "error-retry-base-interval": 0}}}`), + 0, defaultErrorRetry, 0, DefaultErrorRetryBaseInterval, }, - {"Disables error-retry via command line args", - []string{"--error-retry", "0"}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), - 0, 0, + {"Disables error retry and base interval via command line args", + []string{"--error-retry", "0", "--error-retry-base-interval", "0"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 0, 0, 0, 0, }, {"Force the default values via command line args", - []string{"--error-retry", "3"}, - []byte(`{"store-options": {"/store/*/":{"error-retry": 20}}}`), - defaultErrorRetry, defaultErrorRetry, + []string{"--error-retry", "3", "--error-retry-base-interval", "500ms"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 750000000}}}`), + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, }, } { t.Run(test.name, func(t *testing.T) { @@ -84,11 +88,13 @@ func TestErrorRetryOptions(t *testing.T) { configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") opt := cmdOpt.MergedWith(configOptions) require.Equal(t, test.errorRetryStoreHit, opt.ErrorRetry) + require.Equal(t, test.baseIntervalStoreHit, opt.ErrorRetryBaseInterval) configOptions, err = cfg.GetStoreOptionsFor("/missingStore") opt = cmdOpt.MergedWith(configOptions) require.NoError(t, err) require.Equal(t, test.errorRetryStoreMiss, opt.ErrorRetry) + require.Equal(t, test.baseIntervalStoreMiss, opt.ErrorRetryBaseInterval) }) } } diff --git a/store.go b/store.go index 343a456..d4d985d 100644 --- a/store.go +++ b/store.go @@ -9,6 +9,7 @@ import ( ) const DefaultErrorRetry = 3 +const DefaultErrorRetryBaseInterval = 500 * time.Millisecond // Store is a generic interface implemented by read-only stores, like SSH or // HTTP remote stores currently. @@ -79,7 +80,6 @@ type StoreOptions struct { // Number of nanoseconds to wait before first retry attempt. // Retry attempt number N for the same request will wait N times this interval. - // Default: 0 nanoseconds ErrorRetryBaseInterval time.Duration `json:"error-retry-base-interval,omitempty"` // If SkipVerify is true, this store will not verify the data it reads and serves up. This is @@ -97,12 +97,14 @@ type StoreOptions struct { // NewStoreOptionsWithDefaults creates a new StoreOptions struct with the default values set func NewStoreOptionsWithDefaults() (o StoreOptions) { o.ErrorRetry = DefaultErrorRetry + o.ErrorRetryBaseInterval = DefaultErrorRetryBaseInterval return o } func (o *StoreOptions) UnmarshalJSON(data []byte) error { // Set all the default values before loading the JSON store options o.ErrorRetry = DefaultErrorRetry + o.ErrorRetryBaseInterval = DefaultErrorRetryBaseInterval type Alias StoreOptions return json.Unmarshal(data, (*Alias)(o)) }