search

kubernetes-sigs / e2e-framework

A Go framework for end-to-end testing of components running in Kubernetes clusters.
Apache License 2.0
529 stars 103 forks source link

Calling `Config`'s `Client()` in tests running parallel yields a data race #352

Closed pmalek closed 4 months ago

pmalek commented 1 year ago

What happened?

Data race in pkg/envconf/config.go

go test -v -race -count 1 -trimpath .
=== RUN   TestX1
=== PAUSE TestX1
=== RUN   TestX2
=== PAUSE TestX2
=== CONT  TestX1
=== CONT  TestX2
=== RUN   TestX1/feature
=== RUN   TestX2/feature
==================
WARNING: DATA RACE
Write at 0x00c000134480 by goroutine 19:
  sigs.k8s.io/e2e-framework/pkg/envconf.(*Config).Client()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/envconf/config.go:141 +0x9c
  e2eclientresources.Setup()
      e2eclientresources/main_test.go:29 +0x38
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).executeSteps()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:435 +0x104
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTestFeature.(*testEnv).execFeature.func1()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:449 +0x13c
  testing.tRunner()
      testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      testing/testing.go:1648 +0x40

Previous write at 0x00c000134480 by goroutine 20:
  sigs.k8s.io/e2e-framework/pkg/envconf.(*Config).Client()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/envconf/config.go:141 +0x9c
  e2eclientresources.Setup()
      e2eclientresources/main_test.go:29 +0x38
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).executeSteps()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:435 +0x104
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTestFeature.(*testEnv).execFeature.func1()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:449 +0x13c
  testing.tRunner()
      testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      testing/testing.go:1648 +0x40

Goroutine 19 (running) created at:
  testing.(*T).Run()
      testing/testing.go:1648 +0x5d8
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).execFeature()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:442 +0x3ac
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTestFeature()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:215 +0x234
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTests()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:277 +0x900
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).Test()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:326 +0x60
  e2eclientresources.TestX1()
      e2eclientresources/main_test.go:44 +0xb8
  testing.tRunner()
      testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      testing/testing.go:1648 +0x40

Goroutine 20 (running) created at:
  testing.(*T).Run()
      testing/testing.go:1648 +0x5d8
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).execFeature()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:442 +0x3ac
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTestFeature()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:215 +0x234
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).processTests()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:277 +0x900
  sigs.k8s.io/e2e-framework/pkg/env.(*testEnv).Test()
      sigs.k8s.io/e2e-framework@v0.3.1-0.20231113122213-262cac32d35e/pkg/env/env.go:326 +0x60
  e2eclientresources.TestX2()
      e2eclientresources/main_test.go:49 +0xb8
  testing.tRunner()
      testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      testing/testing.go:1648 +0x40
==================
=== RUN   TestX1/feature/A
=== RUN   TestX2/feature/A
=== NAME  TestX1/feature
    testing.go:1465: race detected during execution of test
=== NAME  TestX2/feature
    testing.go:1465: race detected during execution of test
=== NAME  TestX1
    testing.go:1465: race detected during execution of test
--- FAIL: TestX1 (0.04s)
    --- FAIL: TestX1/feature (0.04s)
        --- PASS: TestX1/feature/A (0.00s)
=== NAME  TestX2
    testing.go:1465: race detected during execution of test
--- FAIL: TestX2 (0.04s)
    --- FAIL: TestX2/feature (0.04s)
        --- PASS: TestX2/feature/A (0.00s)
FAIL
FAIL    e2eclientresources      0.089s
FAIL

What did you expect to happen?

No data race

How can we reproduce it (as minimally and precisely as possible)?

package main

import (
    "context"
    "os"
    "testing"

    corev1 "k8s.io/api/core/v1"
    "sigs.k8s.io/e2e-framework/klient/conf"
    "sigs.k8s.io/e2e-framework/pkg/env"
    "sigs.k8s.io/e2e-framework/pkg/envconf"
    "sigs.k8s.io/e2e-framework/pkg/features"
)

var tenv env.Environment

func TestMain(m *testing.M) {
    cfg, err := envconf.NewFromFlags()
    if err != nil {
        panic(err)
    }
    cfg.WithKubeconfigFile(conf.ResolveKubeConfigFile())
    tenv = env.NewWithConfig(cfg)

    os.Exit(tenv.Run(m))
}

func Setup(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {
    res := c.Client().Resources()
    podList := corev1.PodList{}
    err := res.List(ctx, &podList)
    if err != nil {
        t.Fatalf("failed listing pods: %v", err)
    }
    return ctx
}

func Assess(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
    return ctx
}

func TestX1(t *testing.T) {
    t.Parallel()
    tenv.Test(t, generateFeat())
}

func TestX2(t *testing.T) {
    t.Parallel()
    tenv.Test(t, generateFeat())
}

func generateFeat() features.Feature {
    return features.New("feature").
        WithSetup("setup", Setup).
        Assess("A", Assess).
        Feature()
}

Anything elese we need to know?

No response

E2E Provider Used

kind

e2e-framework Version

v0.3.1-0.20231113122213-262cac32d35e

OS version

```console $ uname -a Darwin MBP 23.1.0 Darwin Kernel Version 23.1.0: Mon Oct 9 21:27:24 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T6000 arm64 arm Darwin ```
pmalek commented 1 year ago

If we're fine with making Config goroutine safe then I'd be happy to submit a PR to fix this.

harshanarayana commented 11 months ago

That is a perfectly valid thing to do.

cc @vladimirvivien any concerns on doing this?

vladimirvivien commented 11 months ago

@pmalek and @harshanarayana I am Ok making this concurrency safe.

pmalek commented 10 months ago

I've created https://github.com/kubernetes-sigs/e2e-framework/pull/367 to cover this. There's a question in the PR description whether we'd like to cover all Config's field or just leave it with client.

k8s-triage-robot commented 7 months ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

pmalek commented 7 months ago

Will most likely be covered by #396.

/remove-lifecycle stale