From ff7a48564ca267ea263676de7881f1e8ba309ec8 Mon Sep 17 00:00:00 2001 From: Kristina Pathak Date: Fri, 3 May 2024 04:47:38 -0700 Subject: [PATCH] [builder] make retries configurable for faster tests (#10035) #### Description When running tests, waiting for `downloadModules()` to fail 3 times when that's expected adds time to the test run. This updates tests to only attempt downloading once. Note: if there's a network failure that could cause `downloadModules()` to fail when it should normally succeed. Also the wording here is `retries` when in actuality it's the number of attempts. I didn't change this to keep the log wording the same, but I can change the wording if that's preferable. #### Link to tracking issue this will help for adding tests for https://github.com/open-telemetry/opentelemetry-collector/issues/9252 and https://github.com/open-telemetry/opentelemetry-collector/issues/9896 #### Testing Tests ran --------- Co-authored-by: Pablo Baeyens --- cmd/builder/internal/builder/config.go | 14 ++++++++++ cmd/builder/internal/builder/main.go | 9 +++---- cmd/builder/internal/builder/main_test.go | 33 ++++++++++++++--------- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/cmd/builder/internal/builder/config.go b/cmd/builder/internal/builder/config.go index 50e8b4e76c3..886c02bce79 100644 --- a/cmd/builder/internal/builder/config.go +++ b/cmd/builder/internal/builder/config.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" "github.com/hashicorp/go-version" "go.uber.org/multierr" @@ -41,6 +42,8 @@ type Config struct { Providers *[]Module `mapstructure:"providers"` Replaces []string `mapstructure:"replaces"` Excludes []string `mapstructure:"excludes"` + + downloadModules retry `mapstructure:"-"` } // Distribution holds the parameters for the final binary @@ -66,6 +69,11 @@ type Module struct { Path string `mapstructure:"path"` // an optional path to the local version of this module } +type retry struct { + numRetries int + wait time.Duration +} + // NewDefaultConfig creates a new config, with default values func NewDefaultConfig() Config { log, err := zap.NewDevelopment() @@ -85,6 +93,12 @@ func NewDefaultConfig() Config { OtelColVersion: defaultOtelColVersion, Module: "go.opentelemetry.io/collector/cmd/builder", }, + // basic retry if error from go mod command (in case of transient network error). + // retry 3 times with 5 second spacing interval + downloadModules: retry{ + numRetries: 3, + wait: 5 * time.Second, + }, } } diff --git a/cmd/builder/internal/builder/main.go b/cmd/builder/internal/builder/main.go index c8516056475..3d36059af36 100644 --- a/cmd/builder/internal/builder/main.go +++ b/cmd/builder/internal/builder/main.go @@ -189,15 +189,12 @@ func GetModules(cfg Config) error { func downloadModules(cfg Config) error { cfg.Logger.Info("Getting go modules") - // basic retry if error from go mod command (in case of transient network error). This could be improved - // retry 3 times with 5 second spacing interval - retries := 3 failReason := "unknown" - for i := 1; i <= retries; i++ { + for i := 1; i <= cfg.downloadModules.numRetries; i++ { if _, err := runGoCommand(cfg, "mod", "download"); err != nil { failReason = err.Error() - cfg.Logger.Info("Failed modules download", zap.String("retry", fmt.Sprintf("%d/%d", i, retries))) - time.Sleep(5 * time.Second) + cfg.Logger.Info("Failed modules download", zap.String("retry", fmt.Sprintf("%d/%d", i, cfg.downloadModules.numRetries))) + time.Sleep(cfg.downloadModules.wait) continue } return nil diff --git a/cmd/builder/internal/builder/main_test.go b/cmd/builder/internal/builder/main_test.go index 6178c2b2f89..7ddd7ff7408 100644 --- a/cmd/builder/internal/builder/main_test.go +++ b/cmd/builder/internal/builder/main_test.go @@ -33,8 +33,15 @@ require ( )`) ) -func newInitializedConfig(t *testing.T) Config { +func newTestConfig() Config { cfg := NewDefaultConfig() + cfg.downloadModules.wait = 0 + cfg.downloadModules.numRetries = 1 + return cfg +} + +func newInitializedConfig(t *testing.T) Config { + cfg := newTestConfig() // Validate and ParseModules will be called before the config is // given to Generate. assert.NoError(t, cfg.Validate()) @@ -66,7 +73,7 @@ func TestVersioning(t *testing.T) { { description: "defaults", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.Go = "go" cfg.Replaces = append(cfg.Replaces, replaces...) return cfg @@ -76,7 +83,7 @@ func TestVersioning(t *testing.T) { { description: "require otelcol", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.Go = "go" cfg.Distribution.RequireOtelColModule = true cfg.Replaces = append(cfg.Replaces, replaces...) @@ -87,7 +94,7 @@ func TestVersioning(t *testing.T) { { description: "only gomod file, skip generate", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() tempDir := t.TempDir() err := makeModule(tempDir, goModTestFile) require.NoError(t, err) @@ -101,7 +108,7 @@ func TestVersioning(t *testing.T) { { description: "old otel version", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Verbose = true cfg.Distribution.Go = "go" cfg.Distribution.OtelColVersion = "0.97.0" @@ -133,7 +140,7 @@ func TestVersioning(t *testing.T) { { description: "old component version", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.Go = "go" cfg.Exporters = []Module{ { @@ -149,7 +156,7 @@ func TestVersioning(t *testing.T) { { description: "old component version without strict mode", cfgBuilder: func() Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.Go = "go" cfg.SkipStrictVersioning = true cfg.Exporters = []Module{ @@ -200,7 +207,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "Default Configuration Compilation", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) return cfg @@ -209,7 +216,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "LDFlags Compilation", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) cfg.LDFlags = `-X "test.gitVersion=0743dc6c6411272b98494a9b32a63378e84c34da" -X "test.gitTag=local-testing" -X "test.goVersion=go version go1.20.7 darwin/amd64"` @@ -219,7 +226,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "Debug Compilation", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) cfg.Logger = zap.NewNop() @@ -230,7 +237,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "No providers", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) cfg.Providers = &[]Module{} @@ -240,7 +247,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "Pre-confmap factories", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) cfg.Distribution.OtelColVersion = "0.98.0" @@ -251,7 +258,7 @@ func TestGenerateAndCompile(t *testing.T) { { testCase: "With confmap factories", cfgBuilder: func(t *testing.T) Config { - cfg := NewDefaultConfig() + cfg := newTestConfig() cfg.Distribution.OutputPath = t.TempDir() cfg.Replaces = append(cfg.Replaces, replaces...) cfg.Distribution.OtelColVersion = "0.99.0"