From b1923a4577b81280d321c48ef016b315a9dace31 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Mon, 8 Apr 2024 17:46:29 +0200 Subject: [PATCH 1/6] [env] Do not share config between tests This commit fixes a bug where the config object was shared between tests. When running tests in parallel, it was then impossible to rely on fields like the namespace because they could have been overwritten by another test. Also, it led to tests using `-race` to fail because the shared config.klient object that were updating the same fields when initializing the client. This commit creates a new `deepCopyConfig` method that allows to create a deep copy of the config object. This way, each test can have its own without impacting the others. Now the config has the following lifecycle: - It is created when a testEnv is created - Each test uses a child testEnv that inherits the main testEnv's config - Each feature uses a child testEnv that inherits the test's testEnv's config This way, a feature inherits all the changes made in BeforeEach functions while not impacting the other features. --- pkg/env/env.go | 77 +++++++++++++++++++++++++++++++++++------ pkg/env/env_test.go | 84 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 10 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index 3b1e4838..7b3f077d 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -29,6 +29,7 @@ import ( klog "k8s.io/klog/v2" + "sigs.k8s.io/e2e-framework/klient" "sigs.k8s.io/e2e-framework/pkg/envconf" "sigs.k8s.io/e2e-framework/pkg/featuregate" "sigs.k8s.io/e2e-framework/pkg/features" @@ -116,6 +117,18 @@ func newTestEnvWithParallel() *testEnv { } } +// newChildTestEnv returns a child testEnv based on the one passed as an argument. +// The child env inherits the context and actions from the parent and +// creates a deep copy of the config so that it can be mutated without +// affecting the parent's. +func newChildTestEnv(e *testEnv) *testEnv { + return &testEnv{ + ctx: e.ctx, + cfg: e.deepCopyConfig(), + actions: e.actions, + } +} + // WithContext returns a new environment with the context set to ctx. // Argument ctx cannot be nil func (e *testEnv) WithContext(ctx context.Context) types.Environment { @@ -241,7 +254,8 @@ func (e *testEnv) processFeatureActions(ctx context.Context, t *testing.T, featu // In case if the parallel run of test features are enabled, this function will invoke the processTestFeature // as a go-routine to get them to run in parallel func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallelRun bool, testFeatures ...types.Feature) context.Context { - if e.cfg.DryRunMode() { + dedicatedTestEnv := newChildTestEnv(e) + if dedicatedTestEnv.cfg.DryRunMode() { klog.V(2).Info("e2e-framework is being run in dry-run mode. This will skip all the before/after step functions configured around your test assessments and features") } if ctx == nil { @@ -251,19 +265,20 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel t.Log("No test testFeatures provided, skipping test") return ctx } - beforeTestActions := e.getBeforeTestActions() - afterTestActions := e.getAfterTestActions() + beforeTestActions := dedicatedTestEnv.getBeforeTestActions() + afterTestActions := dedicatedTestEnv.getAfterTestActions() - runInParallel := e.cfg.ParallelTestEnabled() && enableParallelRun + runInParallel := dedicatedTestEnv.cfg.ParallelTestEnabled() && enableParallelRun if runInParallel { klog.V(4).Info("Running test features in parallel") } - ctx = e.processTestActions(ctx, t, beforeTestActions) + ctx = dedicatedTestEnv.processTestActions(ctx, t, beforeTestActions) var wg sync.WaitGroup for i, feature := range testFeatures { + featureTestEnv := newChildTestEnv(dedicatedTestEnv) featureCopy := feature featName := feature.Name() if featName == "" { @@ -273,13 +288,13 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel wg.Add(1) go func(ctx context.Context, w *sync.WaitGroup, featName string, f types.Feature) { defer w.Done() - _ = e.processTestFeature(ctx, t, featName, f) + _ = featureTestEnv.processTestFeature(ctx, t, featName, f) }(ctx, &wg, featName, featureCopy) } else { - ctx = e.processTestFeature(ctx, t, featName, featureCopy) + ctx = featureTestEnv.processTestFeature(ctx, t, featName, featureCopy) // In case if the feature under test has failed, skip reset of the features // that are part of the same test - if e.cfg.FailFast() && t.Failed() { + if featureTestEnv.cfg.FailFast() && t.Failed() { break } } @@ -287,7 +302,7 @@ func (e *testEnv) processTests(ctx context.Context, t *testing.T, enableParallel if runInParallel { wg.Wait() } - return e.processTestActions(ctx, t, afterTestActions) + return dedicatedTestEnv.processTestActions(ctx, t, afterTestActions) } // TestInParallel executes a series a feature tests from within a @@ -572,7 +587,49 @@ func (e *testEnv) requireProcessing(kind, testName string, requiredRegexp, skipR return skip, message } -// deepCopyFeature just copies the values from the Feature but creates a deep +// deepCopyConfig just copies the values from the Config to create a deep +// copy to avoid mutation when we just want an informational copy. +func (e *testEnv) deepCopyConfig() *envconf.Config { + // Basic copy which takes care of all the basic types (str, bool...) + configCopy := *e.cfg + + // Manually setting fields that are struct types + if e.cfg.KubeconfigFile() != "" { // No kubeconfig means than no client could have been created + clientCopy, _ := klient.New(e.cfg.Client().RESTConfig()) + configCopy.WithClient(clientCopy) + } + if e.cfg.AssessmentRegex() != nil { + configCopy.WithAssessmentRegex(e.cfg.AssessmentRegex().String()) + } + if e.cfg.FeatureRegex() != nil { + configCopy.WithFeatureRegex(e.cfg.FeatureRegex().String()) + } + if e.cfg.SkipAssessmentRegex() != nil { + configCopy.WithSkipAssessmentRegex(e.cfg.SkipAssessmentRegex().String()) + } + if e.cfg.SkipFeatureRegex() != nil { + configCopy.WithSkipFeatureRegex(e.cfg.SkipFeatureRegex().String()) + } + + labels := make(map[string][]string, len(e.cfg.Labels())) + for k, vals := range e.cfg.Labels() { + copyVals := make([]string, len(vals)) + copyVals = append(copyVals, vals...) + labels[k] = copyVals + } + configCopy.WithLabels(labels) + + skipLabels := make(map[string][]string, len(e.cfg.SkipLabels())) + for k, vals := range e.cfg.SkipLabels() { + copyVals := make([]string, len(vals)) + copyVals = append(copyVals, vals...) + skipLabels[k] = copyVals + } + configCopy.WithSkipLabels(e.cfg.SkipLabels()) + return &configCopy +} + +// deepCopyFeature just copies the values from the Feature to create a deep // copy to avoid mutation when we just want an informational copy. func deepCopyFeature(f types.Feature) types.Feature { fcopy := features.New(f.Name()) diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index d0a024d3..757d4c5e 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -726,6 +726,90 @@ func TestTestEnv_TestInParallel(t *testing.T) { } } +// Create a dedicated env that can be used to test the parallel execution of tests and features to make sure +// they don't share the same config object but they inherit the one from the parent env. +// Meaning that each test inherit the global testEnv and each feature inherit the testEnv of the test. +// This env is used for the 3 tests below. +var envTForChildCfgParallelTesting = NewWithConfig(envconf.New().WithParallelTestEnabled().WithNamespace("child-cfg")) + +func TestTestEnv_ChildCfgInParallel(t *testing.T) { + + envTForChildCfgParallelTesting.BeforeEachTest(func(ctx context.Context, config *envconf.Config, t *testing.T) (context.Context, error) { + t.Logf("Running before each test for test %s", t.Name()) + t.Parallel() + + // Check that namespace was inherited from the global config + if config.Namespace() != "child-cfg" { + t.Errorf("Expected namespace to be %s but got %s", t.Name(), config.Namespace()) + } + config.WithNamespace(t.Name()) + return ctx, nil + }) + + envTForChildCfgParallelTesting.AfterEachTest(func(ctx context.Context, config *envconf.Config, t *testing.T) (context.Context, error) { + t.Logf("Running after each test for test %s", t.Name()) + if config.Namespace() != t.Name() { + t.Errorf("Expected namespace to be %s but got %s", t.Name(), config.Namespace()) + } + return ctx, nil + }) + + envTForChildCfgParallelTesting.BeforeEachFeature(func(ctx context.Context, config *envconf.Config, t *testing.T, feature types.Feature) (context.Context, error) { + t.Logf("Running before each feature for feature %s", feature.Name()) + + // Check that namespace was inherited from the test + if config.Namespace() != t.Name() { + t.Errorf("Namespace in feature was not inherited from the test. Expected %s but got %s", t.Name(), config.Namespace()) + } + config.WithNamespace(feature.Name()) + return ctx, nil + }) + + envTForChildCfgParallelTesting.AfterEachFeature(func(ctx context.Context, config *envconf.Config, t *testing.T, feature types.Feature) (context.Context, error) { + t.Logf("Running after each feature for feature %s", feature.Name()) + if config.Namespace() != feature.Name() { + t.Errorf("Expected namespace in feature to be %s but got %s", feature.Name(), config.Namespace()) + } + return ctx, nil + }) +} + +func TestTestEnv_ChildCfgInParallelOne(t *testing.T) { + // Here, each feature sleeps for a different amount of time. This is done to ensure that both features can + // run their BeforeEachFeature that modify the config.Namespace BEFORE they run their AfterEachFeature that + // checks the config.Namespace. If the deepcopy did not work, the config.Namespace from 1 feature would be + // overwritten by the other feature. + f1 := features.New("test-parallel-feature1"). + Assess("sleep more", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context { + t.Log("sleeping more to be sure feature 2 finishes first") + time.Sleep(5 * time.Second) + return ctx + }) + + f2 := features.New("test-parallel-feature2"). + Assess("sleep less", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context { + t.Log("sleeping less to be sure feature 1 starts") + time.Sleep(1 * time.Second) + return ctx + }) + + _ = envTForChildCfgParallelTesting.TestInParallel(t, f1.Feature(), f2.Feature()) +} + +func TestTestEnv_ChildCfgInParallelTwo(t *testing.T) { + // Here, the only feature just sleeps a bit to make sure both tests will have run their BeforeEachTest + // before either runs their AfterEachTest. If the deepcopy did not work, the config.Namespace from 1 test + // would be overwritten by the other test. + f1 := features.New("test-parallel-feature1"). + Assess("sleep less", func(ctx context.Context, t *testing.T, config *envconf.Config) context.Context { + t.Log("sleeping less to be sure test 1 starts") + time.Sleep(1 * time.Second) + return ctx + }) + + _ = envTForChildCfgParallelTesting.Test(t, f1.Feature()) +} + // TestTParallelMultipleFeaturesInParallel runs multple features in parallel with a dedicated Parallel environment, // just to check there are no race conditions with this setting func TestTParallelMultipleFeaturesInParallel(t *testing.T) { From 0c0954ea8a94c06354c5ddaa3ff7f73c2acde7e2 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Fri, 12 Apr 2024 15:12:02 +0200 Subject: [PATCH 2/6] [env] NewChildTestEnv also copies ctx and actions --- pkg/env/env.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index 7b3f077d..c6639d25 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -117,15 +117,18 @@ func newTestEnvWithParallel() *testEnv { } } +type ctxName string + // newChildTestEnv returns a child testEnv based on the one passed as an argument. // The child env inherits the context and actions from the parent and // creates a deep copy of the config so that it can be mutated without // affecting the parent's. func newChildTestEnv(e *testEnv) *testEnv { + childCtx := context.WithValue(e.ctx, ctxName("parent"), fmt.Sprintf("%s", e.ctx)) return &testEnv{ - ctx: e.ctx, + ctx: childCtx, cfg: e.deepCopyConfig(), - actions: e.actions, + actions: append([]action{}, e.actions...), } } From 64b5e514f3146ec81590326ba23494720c5e54cb Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Tue, 16 Apr 2024 16:30:02 +0200 Subject: [PATCH 3/6] [env] Create proper getter for config client to solve race condition --- pkg/env/env.go | 5 +++-- pkg/envconf/config.go | 5 +++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index c6639d25..130bce8f 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -597,8 +597,9 @@ func (e *testEnv) deepCopyConfig() *envconf.Config { configCopy := *e.cfg // Manually setting fields that are struct types - if e.cfg.KubeconfigFile() != "" { // No kubeconfig means than no client could have been created - clientCopy, _ := klient.New(e.cfg.Client().RESTConfig()) + if client := e.cfg.GetClient(); client != nil { + // Need to recreate the underlying client because client.Resource is not thread safe + clientCopy, _ := klient.New(client.RESTConfig()) configCopy.WithClient(clientCopy) } if e.cfg.AssessmentRegex() != nil { diff --git a/pkg/envconf/config.go b/pkg/envconf/config.go index f2267ae2..9d91e9d1 100644 --- a/pkg/envconf/config.go +++ b/pkg/envconf/config.go @@ -107,6 +107,11 @@ func (c *Config) WithClient(client klient.Client) *Config { return c } +// GetClient returns the client for the environment +func (c *Config) GetClient() klient.Client { + return c.client +} + // NewClient is a constructor function that returns a previously // created klient.Client or create a new one based on configuration // previously set. Will return an error if unable to do so. From 8d60627c1ba4b2cf2aaae8a205a6bfa7ac332f8a Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Wed, 17 Apr 2024 18:43:35 +0200 Subject: [PATCH 4/6] [config] Remove client caching --- pkg/envconf/config.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/envconf/config.go b/pkg/envconf/config.go index 9d91e9d1..2dde5863 100644 --- a/pkg/envconf/config.go +++ b/pkg/envconf/config.go @@ -116,10 +116,6 @@ func (c *Config) GetClient() klient.Client { // created klient.Client or create a new one based on configuration // previously set. Will return an error if unable to do so. func (c *Config) NewClient() (klient.Client, error) { - if c.client != nil { - return c.client, nil - } - client, err := klient.NewWithKubeConfigFile(c.kubeconfig) if err != nil { return nil, fmt.Errorf("envconfig: client failed: %w", err) @@ -135,10 +131,6 @@ func (c *Config) NewClient() (klient.Client, error) { // are confident in the configuration or call NewClient() to ensure its // safe creation. func (c *Config) Client() klient.Client { - if c.client != nil { - return c.client - } - client, err := klient.NewWithKubeConfigFile(c.kubeconfig) if err != nil { panic(fmt.Errorf("envconfig: client failed: %w", err).Error()) From 95976f707086be0a10437c1bf1c80a97e76f7370 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Mon, 22 Apr 2024 14:25:10 +0200 Subject: [PATCH 5/6] [config] Refactor Client() to call NewClient() --- pkg/envconf/config.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/envconf/config.go b/pkg/envconf/config.go index 2dde5863..b3edf66d 100644 --- a/pkg/envconf/config.go +++ b/pkg/envconf/config.go @@ -131,12 +131,11 @@ func (c *Config) NewClient() (klient.Client, error) { // are confident in the configuration or call NewClient() to ensure its // safe creation. func (c *Config) Client() klient.Client { - client, err := klient.NewWithKubeConfigFile(c.kubeconfig) + client, err := c.NewClient() if err != nil { - panic(fmt.Errorf("envconfig: client failed: %w", err).Error()) + panic(err) } - c.client = client - return c.client + return client } // WithNamespace updates the environment namespace value From 56e4a65651019ba911d30ffa0f6a1058104a41d8 Mon Sep 17 00:00:00 2001 From: Baptiste Girard-Carrabin Date: Fri, 26 Apr 2024 12:39:52 +0200 Subject: [PATCH 6/6] [env] Fix go linting --- pkg/env/env.go | 6 +++++- pkg/env/env_test.go | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/env/env.go b/pkg/env/env.go index 130bce8f..103ec5c5 100644 --- a/pkg/env/env.go +++ b/pkg/env/env.go @@ -599,7 +599,11 @@ func (e *testEnv) deepCopyConfig() *envconf.Config { // Manually setting fields that are struct types if client := e.cfg.GetClient(); client != nil { // Need to recreate the underlying client because client.Resource is not thread safe - clientCopy, _ := klient.New(client.RESTConfig()) + // Panic on error because this should never happen since the client was built once already + clientCopy, err := klient.New(client.RESTConfig()) + if err != nil { + panic(err) + } configCopy.WithClient(clientCopy) } if e.cfg.AssessmentRegex() != nil { diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index 757d4c5e..4e4d7aee 100644 --- a/pkg/env/env_test.go +++ b/pkg/env/env_test.go @@ -733,7 +733,6 @@ func TestTestEnv_TestInParallel(t *testing.T) { var envTForChildCfgParallelTesting = NewWithConfig(envconf.New().WithParallelTestEnabled().WithNamespace("child-cfg")) func TestTestEnv_ChildCfgInParallel(t *testing.T) { - envTForChildCfgParallelTesting.BeforeEachTest(func(ctx context.Context, config *envconf.Config, t *testing.T) (context.Context, error) { t.Logf("Running before each test for test %s", t.Name()) t.Parallel()