Merge pull request #123732 from serathius/parallel-featureflags

Fix SetFeatureGateDuringTest handling of Parallel tests

Kubernetes-commit: e062f925aec9137ca3f06704c6adb2883812e657

featuregate/testing/feature_gate.go

@@ -18,18 +18,35 @@ package testing
 
 import (
 	"fmt"
+	"strings"
+	"sync"
 	"testing"
 
 	"k8s.io/component-base/featuregate"
 )
 
-// SetFeatureGateDuringTest sets the specified gate to the specified value, and returns a function that restores the original value.
-// Failures to set or restore cause the test to fail.
+var (
+	overrideLock        sync.Mutex
+	featureFlagOverride map[featuregate.Feature]string
+)
+
+func init() {
+	featureFlagOverride = map[featuregate.Feature]string{}
+}
+
+// SetFeatureGateDuringTest sets the specified gate to the specified value for duration of the test.
+// Fails when it detects second call to the same flag or is unable to set or restore feature flag.
+// Returns empty cleanup function to maintain the old function signature that uses defer.
+// TODO: Remove defer from calls to SetFeatureGateDuringTest and update hack/verify-test-featuregates.sh when we can do large scale code change.
+//
+// WARNING: Can leak set variable when called in test calling t.Parallel(), however second attempt to set the same feature flag will cause fatal.
 //
 // Example use:
 //
 // defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.<FeatureName>, true)()
 func SetFeatureGateDuringTest(tb testing.TB, gate featuregate.FeatureGate, f featuregate.Feature, value bool) func() {
+	tb.Helper()
+	detectParallelOverrideCleanup := detectParallelOverride(tb, f)
 	originalValue := gate.Enabled(f)
 
 	// Specially handle AllAlpha and AllBeta
@@ -50,12 +67,41 @@ func SetFeatureGateDuringTest(tb testing.TB, gate featuregate.FeatureGate, f fea
 		tb.Errorf("error setting %s=%v: %v", f, value, err)
 	}
 
-	return func() {
+	tb.Cleanup(func() {
+		tb.Helper()
+		detectParallelOverrideCleanup()
 		if err := gate.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, originalValue)); err != nil {
 			tb.Errorf("error restoring %s=%v: %v", f, originalValue, err)
 		}
 		for _, cleanup := range cleanups {
 			cleanup()
 		}
+	})
+	return func() {}
+}
+
+func detectParallelOverride(tb testing.TB, f featuregate.Feature) func() {
+	tb.Helper()
+	overrideLock.Lock()
+	defer overrideLock.Unlock()
+	beforeOverrideTestName := featureFlagOverride[f]
+	if beforeOverrideTestName != "" && !sameTestOrSubtest(tb, beforeOverrideTestName) {
+		tb.Fatalf("Detected parallel setting of a feature gate by both %q and %q", beforeOverrideTestName, tb.Name())
+	}
+	featureFlagOverride[f] = tb.Name()
+
+	return func() {
+		tb.Helper()
+		overrideLock.Lock()
+		defer overrideLock.Unlock()
+		if afterOverrideTestName := featureFlagOverride[f]; afterOverrideTestName != tb.Name() {
+			tb.Fatalf("Detected parallel setting of a feature gate between both %q and %q", afterOverrideTestName, tb.Name())
+		}
+		featureFlagOverride[f] = beforeOverrideTestName
 	}
 }
+
+func sameTestOrSubtest(tb testing.TB, testName string) bool {
+	// Assumes that "/" is not used in test names.
+	return tb.Name() == testName || strings.HasPrefix(tb.Name(), testName+"/")
+}

featuregate/testing/feature_gate_test.go

@@ -19,6 +19,8 @@ package testing
 import (
 	gotest "testing"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"k8s.io/component-base/featuregate"
 )
 
@@ -67,8 +69,11 @@ func TestSpecialGates(t *gotest.T) {
 		"stable_default_off_set_on": true,
 	}
 	expect(t, gate, before)
+	t.Cleanup(func() {
+		expect(t, gate, before)
+	})
 
-	cleanupAlpha := SetFeatureGateDuringTest(t, gate, "AllAlpha", true)
+	SetFeatureGateDuringTest(t, gate, "AllAlpha", true)
 	expect(t, gate, map[featuregate.Feature]bool{
 		"AllAlpha": true,
 		"AllBeta":  false,
@@ -89,7 +94,7 @@ func TestSpecialGates(t *gotest.T) {
 		"stable_default_off_set_on": true,
 	})
 
-	cleanupBeta := SetFeatureGateDuringTest(t, gate, "AllBeta", true)
+	SetFeatureGateDuringTest(t, gate, "AllBeta", true)
 	expect(t, gate, map[featuregate.Feature]bool{
 		"AllAlpha": true,
 		"AllBeta":  true,
@@ -109,11 +114,6 @@ func TestSpecialGates(t *gotest.T) {
 		"stable_default_off":        false,
 		"stable_default_off_set_on": true,
 	})
-
-	// run cleanups in reverse order like defer would
-	cleanupBeta()
-	cleanupAlpha()
-	expect(t, gate, before)
 }
 
 func expect(t *gotest.T, gate featuregate.FeatureGate, expect map[featuregate.Feature]bool) {
@@ -124,3 +124,127 @@ func expect(t *gotest.T, gate featuregate.FeatureGate, expect map[featuregate.Fe
 		}
 	}
 }
+
+func TestSetFeatureGateInTest(t *gotest.T) {
+	gate := featuregate.NewFeatureGate()
+	err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{
+		"feature": {PreRelease: featuregate.Alpha, Default: false},
+	})
+	require.NoError(t, err)
+
+	assert.False(t, gate.Enabled("feature"))
+	defer SetFeatureGateDuringTest(t, gate, "feature", true)()
+	defer SetFeatureGateDuringTest(t, gate, "feature", true)()
+
+	assert.True(t, gate.Enabled("feature"))
+	t.Run("Subtest", func(t *gotest.T) {
+		assert.True(t, gate.Enabled("feature"))
+	})
+
+	t.Run("ParallelSubtest", func(t *gotest.T) {
+		assert.True(t, gate.Enabled("feature"))
+		// Calling t.Parallel in subtest will resume the main test body
+		t.Parallel()
+		assert.True(t, gate.Enabled("feature"))
+	})
+	assert.True(t, gate.Enabled("feature"))
+
+	t.Run("OverwriteInSubtest", func(t *gotest.T) {
+		defer SetFeatureGateDuringTest(t, gate, "feature", false)()
+		assert.False(t, gate.Enabled("feature"))
+	})
+	assert.True(t, gate.Enabled("feature"))
+}
+
+func TestDetectLeakToMainTest(t *gotest.T) {
+	t.Cleanup(func() {
+		featureFlagOverride = map[featuregate.Feature]string{}
+	})
+	gate := featuregate.NewFeatureGate()
+	err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{
+		"feature": {PreRelease: featuregate.Alpha, Default: false},
+	})
+	require.NoError(t, err)
+
+	// Subtest setting feature gate and calling parallel will leak it out
+	t.Run("LeakingSubtest", func(t *gotest.T) {
+		fakeT := &ignoreFatalT{T: t}
+		defer SetFeatureGateDuringTest(fakeT, gate, "feature", true)()
+		// Calling t.Parallel in subtest will resume the main test body
+		t.Parallel()
+		// Leaked false from main test
+		assert.False(t, gate.Enabled("feature"))
+	})
+	// Leaked true from subtest
+	assert.True(t, gate.Enabled("feature"))
+	fakeT := &ignoreFatalT{T: t}
+	defer SetFeatureGateDuringTest(fakeT, gate, "feature", false)()
+	assert.True(t, fakeT.fatalRecorded)
+}
+
+func TestDetectLeakToOtherSubtest(t *gotest.T) {
+	t.Cleanup(func() {
+		featureFlagOverride = map[featuregate.Feature]string{}
+	})
+	gate := featuregate.NewFeatureGate()
+	err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{
+		"feature": {PreRelease: featuregate.Alpha, Default: false},
+	})
+	require.NoError(t, err)
+
+	subtestName := "Subtest"
+	// Subtest setting feature gate and calling parallel will leak it out
+	t.Run(subtestName, func(t *gotest.T) {
+		fakeT := &ignoreFatalT{T: t}
+		defer SetFeatureGateDuringTest(fakeT, gate, "feature", true)()
+		t.Parallel()
+	})
+	// Add suffix to name to prevent tests with the same prefix.
+	t.Run(subtestName+"Suffix", func(t *gotest.T) {
+		// Leaked true
+		assert.True(t, gate.Enabled("feature"))
+
+		fakeT := &ignoreFatalT{T: t}
+		defer SetFeatureGateDuringTest(fakeT, gate, "feature", false)()
+		assert.True(t, fakeT.fatalRecorded)
+	})
+}
+
+func TestCannotDetectLeakFromSubtest(t *gotest.T) {
+	t.Cleanup(func() {
+		featureFlagOverride = map[featuregate.Feature]string{}
+	})
+	gate := featuregate.NewFeatureGate()
+	err := gate.Add(map[featuregate.Feature]featuregate.FeatureSpec{
+		"feature": {PreRelease: featuregate.Alpha, Default: false},
+	})
+	require.NoError(t, err)
+
+	defer SetFeatureGateDuringTest(t, gate, "feature", false)()
+	// Subtest setting feature gate and calling parallel will leak it out
+	t.Run("Subtest", func(t *gotest.T) {
+		defer SetFeatureGateDuringTest(t, gate, "feature", true)()
+		t.Parallel()
+	})
+	// Leaked true
+	assert.True(t, gate.Enabled("feature"))
+}
+
+type ignoreFatalT struct {
+	*gotest.T
+	fatalRecorded bool
+}
+
+func (f *ignoreFatalT) Fatal(args ...any) {
+	f.T.Helper()
+	f.fatalRecorded = true
+	newArgs := []any{"[IGNORED]"}
+	newArgs = append(newArgs, args...)
+	f.T.Log(newArgs...)
+}
+
+func (f *ignoreFatalT) Fatalf(format string, args ...any) {
+	f.T.Helper()
+	f.fatalRecorded = true
+	f.T.Logf("[IGNORED] "+format, args...)
+}

go.mod

@@ -25,8 +25,8 @@ require (
 	go.uber.org/zap v1.26.0
 	golang.org/x/sys v0.17.0
 	gopkg.in/yaml.v2 v2.4.0
-	k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5
-	k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af
+	k8s.io/apimachinery v0.0.0-20240307171817-d82afe1e363a
+	k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a
 	k8s.io/klog/v2 v2.120.1
 	k8s.io/utils v0.0.0-20230726121419-3b25d923346b
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd
@@ -76,14 +76,10 @@ require (
 	google.golang.org/protobuf v1.33.0 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
-	k8s.io/api v0.0.0-20240306165540-05aa4bceed70 // indirect
+	k8s.io/api v0.0.0-20240311194616-96558b97565e // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
 
-replace (
-	k8s.io/api => k8s.io/api v0.0.0-20240306165540-05aa4bceed70
-	k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5
-	k8s.io/client-go => k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af
-)
+replace k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20240307160843-0407311be590

go.sum

@@ -210,12 +210,12 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
-k8s.io/api v0.0.0-20240306165540-05aa4bceed70 h1:Kw6GufAvqr668v56lckDWoGHFG5wwjQRjYuf+PMt1us=
-k8s.io/api v0.0.0-20240306165540-05aa4bceed70/go.mod h1:S69aw/5045kbDeBVmy89EQxSM7v5kHdLNzO4wt3OZ2o=
-k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5 h1:YRP8FbAab9hlobsEfyUG7P6dC6hbVTLw0eFY/AnewmY=
-k8s.io/apimachinery v0.0.0-20240306164812-cbfe0a1feaa5/go.mod h1:wEJvNDlfxMRaMhyv38SIHIEC9hah/xuzqUUhxIyUv7Y=
-k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af h1:hU8WcNO9vZrUXLzKbyXj6FUlMuTecjBr7MmbGxfCkzM=
-k8s.io/client-go v0.0.0-20240306170515-0cdc0ce850af/go.mod h1:g+S/ljjD+b0SS7OkKeZ6IBio03Ot9Q8s19hN3jHsl/Y=
+k8s.io/api v0.0.0-20240311194616-96558b97565e h1:Z2GqjWWNuM6kr7OiUdpQvs+x7vUU96a+7YdYilq3Ku8=
+k8s.io/api v0.0.0-20240311194616-96558b97565e/go.mod h1:RzL8aPQw9ZdVXCdY+Iz3AXnVX+jFyQNqcmzmS+2/Ur0=
+k8s.io/apimachinery v0.0.0-20240307160843-0407311be590 h1:YFg0j+PVfNLayHtZ3gdTeW12q7HECwhvZm9fWZpXyXo=
+k8s.io/apimachinery v0.0.0-20240307160843-0407311be590/go.mod h1:wEJvNDlfxMRaMhyv38SIHIEC9hah/xuzqUUhxIyUv7Y=
+k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a h1:EMeP7Xj22C0bpj+nV6cxqBExDijGzRKs66AY4NDStYw=
+k8s.io/client-go v0.0.0-20240309200420-7ebe0ea60e0a/go.mod h1:QDFa4l0JGh2EmATyNXW9l58yiTy4DIoVjrfyNgOxfOQ=
 k8s.io/klog/v2 v2.120.1 h1:QXU6cPEOIslTGvZaXvFWiP9VKyeet3sawzTOvdXb4Vw=
 k8s.io/klog/v2 v2.120.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=