Merge pull request #133870 from pohly/build-data-race-detection

build: also support KUBE_RACE for test binaries

hack/ginkgo-e2e.sh

@@ -181,7 +181,16 @@ case "${E2E_TEST_DEBUG_TOOL:-ginkgo}" in
     if [[ -n "${GINKGO_PARALLEL_NODES:-}" ]]; then
       program+=("--nodes=${GINKGO_PARALLEL_NODES}")
     elif [[ ${GINKGO_PARALLEL} =~ ^[yY]$ ]]; then
-      program+=("--nodes=25")
+      if [[ "${KUBE_RACE:-}" == "-race" ]]; then
+        # When race detection is enabled, merely running 25 e2e.test instances was too much
+        # and the OOM killer shut down the Prow test pod because of the memory overhead.
+        #
+        # A CI job could control that via GINKGO_PARALLEL_NODES, but we should also have
+        # saner defaults which take this into account.
+        program+=("--nodes=10")
+      else
+        program+=("--nodes=25")
+      fi
     fi
     program+=("${ginkgo_args[@]:+${ginkgo_args[@]}}")
     ;;

hack/lib/golang.sh

@@ -815,7 +815,7 @@ kube::golang::build_binaries_for_platform() {
   for binary in "${binaries[@]}"; do
     if [[ "${binary}" =~ ".test"$ ]]; then
       tests+=("${binary}")
-      kube::log::info "    ${binary} (test)"
+      kube::log::info "    ${binary} (test${KUBE_RACE:+, race detection})"
     elif kube::golang::is_statically_linked "${binary}"; then
       statics+=("${binary}")
       kube::log::info "    ${binary} (static)"
@@ -848,7 +848,7 @@ kube::golang::build_binaries_for_platform() {
       -tags="${gotags:-}"
     )
     if [[ -n "${KUBE_RACE:-}" ]]; then
-        build_args+=("${KUBE_RACE}")
+      build_args+=("${KUBE_RACE}")
     fi
     kube::golang::build_some_binaries "${nonstatics[@]}"
   fi
@@ -859,13 +859,18 @@ kube::golang::build_binaries_for_platform() {
     testpkg=$(dirname "${test}")
 
     mkdir -p "$(dirname "${outfile}")"
-    go test -c \
-      ${goflags:+"${goflags[@]}"} \
-      -gcflags="${gogcflags}" \
-      -ldflags="${goldflags}" \
-      -tags="${gotags:-}" \
-      -o "${outfile}" \
-      "${testpkg}"
+    build_args=(
+      -c
+      ${goflags:+"${goflags[@]}"}
+      -gcflags="${gogcflags}"
+      -ldflags="${goldflags}"
+      -tags="${gotags:-}"
+      -o "${outfile}"
+    )
+    if [[ -n "${KUBE_RACE:-}" ]]; then
+      build_args+=("${KUBE_RACE}")
+    fi
+    go test "${build_args[@]}" "${testpkg}"
   done
 }
 

test/e2e/framework/internal/junit/junit.go

@@ -17,6 +17,9 @@ limitations under the License.
 package junit
 
 import (
+	"slices"
+	"strings"
+
 	"github.com/onsi/ginkgo/v2"
 	"github.com/onsi/ginkgo/v2/reporters"
 	"github.com/onsi/ginkgo/v2/types"
@@ -42,5 +45,20 @@ func WriteJUnitReport(report ginkgo.Report, filename string) error {
 		OmitSpecLabels: true,
 	}
 
+	// Sort specs by full name. The default is by start (or completion?) time,
+	// which is less useful in spyglass because those times are not shown
+	// and thus tests seem to be listed with no apparent order.
+	slices.SortFunc(report.SpecReports, func(a, b types.SpecReport) int {
+		res := strings.Compare(a.FullText(), b.FullText())
+		if res == 0 {
+			// Use start time as tie-breaker in the unlikely
+			// case that two specs have the same full name.
+			return a.StartTime.Compare(b.StartTime)
+		}
+		return res
+	})
+
+	detectDataRaces(report)
+
 	return reporters.GenerateJUnitReportWithConfig(report, filename, config)
 }

test/e2e/framework/internal/junit/junit_data_races.go

@@ -0,0 +1,75 @@
+//go:build race
+
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package junit
+
+import (
+	"regexp"
+	"time"
+
+	"github.com/onsi/ginkgo/v2"
+	"github.com/onsi/ginkgo/v2/types"
+)
+
+var dataRaceRE = regexp.MustCompile(`(?m)^WARNING: DATA RACE$`)
+
+func containsDataRace(output string) bool {
+	return dataRaceRE.MatchString(output)
+}
+
+// Detect data races in output and mark those tests as failed.
+// Ginkgo itself captures the warning in the output of those
+// tests where it is printed, but does not mark the tests as failed.
+// This is not exactly wrong (the reason might be in code that was
+// started elsewhere), but without a test being marked as "failed",
+// prune-junit-xml will prune the output and spyglass won't show
+// the test as failed.
+//
+// Modifies the report. Doesn't touch tests which are already failed
+// because those already need to be investigated.
+func detectDataRaces(report ginkgo.Report) {
+	for i, spec := range report.SpecReports {
+		if spec.State != types.SpecStatePassed {
+			continue
+		}
+
+		// Both variants of captured output get checked, just to be on the safe side.
+		ginkgoWriterRace := containsDataRace(spec.CapturedGinkgoWriterOutput)
+		stdoutRace := containsDataRace(spec.CapturedStdOutErr)
+		if ginkgoWriterRace || stdoutRace {
+			spec.State = types.SpecStateFailed
+			spec.Failure = types.Failure{
+				FailureNodeContext: types.FailureNodeIsLeafNode,
+				FailureNodeType:    spec.LeafNodeType,
+				Location:           types.NewCustomCodeLocation("output analysis"),
+				TimelineLocation: types.TimelineLocation{
+					Time: time.Now(),
+				},
+			}
+			// Let's move that text to the failure message.
+			if stdoutRace {
+				spec.Failure.Message = spec.CapturedStdOutErr
+				spec.CapturedStdOutErr = ""
+			} else {
+				spec.Failure.Message = spec.CapturedGinkgoWriterOutput
+				spec.CapturedGinkgoWriterOutput = ""
+			}
+			report.SpecReports[i] = spec
+		}
+	}
+}

test/e2e/framework/internal/junit/junit_no_data_races.go

@@ -0,0 +1,29 @@
+//go:build !race
+
+/*
+Copyright 2025 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package junit
+
+import (
+	"github.com/onsi/ginkgo/v2"
+)
+
+func detectDataRaces(report ginkgo.Report) {
+	// This is a NOP variant of this function which is used when the test binary is compiled
+	// without race detection. In that case there cannot be any data race reports and therefore
+	// we don't need to check for them.
+}

test/e2e/framework/test_context.go

@@ -28,7 +28,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -614,19 +613,6 @@ func AfterReadingAllFlags(t *TestContextType) {
 		}
 
 		ginkgo.ReportAfterSuite("Kubernetes e2e JUnit report", func(report ginkgo.Report) {
-			// Sort specs by full name. The default is by start (or completion?) time,
-			// which is less useful in spyglass because those times are not shown
-			// and thus tests seem to be listed with no apparent order.
-			slices.SortFunc(report.SpecReports, func(a, b types.SpecReport) int {
-				res := strings.Compare(a.FullText(), b.FullText())
-				if res == 0 {
-					// Use start time as tie-breaker in the unlikely
-					// case that two specs have the same full name.
-					return a.StartTime.Compare(b.StartTime)
-				}
-				return res
-			})
-
 			// With Ginkgo v1, we used to write one file per
 			// parallel node. Now Ginkgo v2 automatically merges
 			// all results into a report for us. The 01 suffix is