Merge cockroachdb#134317 cockroachdb#134408

134317: testutils: roachtest log import linter r=srosenberg a=herkolategan

Previously, it was possible to add the CRDB log or the standard golang log as an
import in roachtests. This adds a new linter that prevents using incorrect
logging in roachtests. The logger supplied by the test framework to a test
should be used in all circumstances.

Epic: None
Release note: None

134408: roachtest: minor task manager refactoring r=srosenberg a=herkolategan

Implement a few refactoring changes based on recent feedback.
1. Rename the `ExpectedCancel` event field to `TriggeredByTest` as it makes more sense in the context of the flag that gets set on the event. A comment has been added to explain the reasoning behind this flag.
2. Update the commentary on task termination to indicate that it does not wait indefinitely for tasks to stop.
3. Update the test framework panic handler logging to be more inline with how panics are logged in a test's main goroutine.

Informs: cockroachdb#133263

Epic: None
Release note: None

Co-authored-by: Herko Lategan <[email protected]>

pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go

@@ -187,7 +187,7 @@ func (tr *testRunner) run() (retErr error) {
 			if event.Err == nil {
 				tr.logger.Printf("background step finished: %s", event.Name)
 				continue
-			} else if event.ExpectedCancel {
+			} else if event.TriggeredByTest {
 				tr.logger.Printf("background step canceled by test: %s", event.Name)
 				continue
 			}

pkg/cmd/roachtest/roachtestutil/task/manager.go

@@ -28,9 +28,9 @@ type (
 
 	// Event represents the result of a task execution.
 	Event struct {
-		Name           string
-		Err            error
-		ExpectedCancel bool
+		Name            string
+		Err             error
+		TriggeredByTest bool
 	}
 
 	manager struct {
@@ -59,7 +59,7 @@ func NewManager(ctx context.Context, l *logger.Logger) Manager {
 func (m *manager) defaultOptions() []Option {
 	// The default panic handler simply returns the panic as an error.
 	defaultPanicHandlerFn := func(_ context.Context, name string, l *logger.Logger, r interface{}) error {
-		return r.(error)
+		return fmt.Errorf("panic: %v", r)
 	}
 	// The default error handler simply returns the error as is.
 	defaultErrorHandlerFn := func(_ context.Context, name string, l *logger.Logger, err error) error {
@@ -98,17 +98,26 @@ func (m *manager) GoWithCancel(fn Func, opts ...Option) context.CancelFunc {
 		}
 		err = internalFunc(l)
 		event := Event{
-			Name:           opt.Name,
-			Err:            err,
-			ExpectedCancel: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(),
+			Name: opt.Name,
+			Err:  err,
+			// TriggeredByTest is set to true if the task was canceled intentionally,
+			// by the test, and we encounter an error. The assumption is that we
+			// expect the error to have been caused by the cancelation, hence the
+			// error above was not caused by a failure. This ensures we don't register
+			// a test failure if the task was meant to be canceled. It's possible that
+			// `expectedContextCancellation` could be set before the context is
+			// canceled, thus we also ensure that the context is canceled.
+			TriggeredByTest: err != nil && IsContextCanceled(groupCtx) && expectedContextCancellation.Load(),
 		}
-		select {
-		case m.events <- event:
-			// exit goroutine
-		case <-m.ctx.Done():
-			// Parent context already finished, exit goroutine.
+
+		// Do not send the event if the parent context is canceled. The test is
+		// already aware of the cancelation and sending an event would be redundant.
+		// For instance, a call to test.Fatal would already have captured the error
+		// and canceled the context.
+		if IsContextCanceled(m.ctx) {
 			return nil
 		}
+		m.events <- event
 		return err
 	})
 
@@ -129,7 +138,9 @@ func (m *manager) Go(fn Func, opts ...Option) {
 }
 
 // Terminate will call the stop functions for every task started during the
-// test. Returns when all task functions have returned.
+// test. Returns when all task functions have returned, or after a 5-minute
+// timeout, whichever comes first. If the timeout is reached, the function logs
+// a warning message and returns.
 func (m *manager) Terminate(l *logger.Logger) {
 	func() {
 		m.mu.Lock()

pkg/cmd/roachtest/test_impl.go

@@ -12,7 +12,6 @@ import (
 	"math/rand"
 	"os"
 	"regexp"
-	"runtime/debug"
 	"strings"
 	"sync"
 	"time"
@@ -679,8 +678,7 @@ func (t *testImpl) IsBuildVersion(minVersion string) bool {
 }
 
 func panicHandler(_ context.Context, name string, l *logger.Logger, r interface{}) error {
-	l.Printf("panic stack trace in task %s:\n%s", name, string(debug.Stack()))
-	return fmt.Errorf("panic (stack trace above): %v", r)
+	return fmt.Errorf("test task %s panicked: %v", name, r)
 }
 
 // GoWithCancel runs the given function in a goroutine and returns a

pkg/cmd/roachtest/test_runner.go

@@ -1316,7 +1316,7 @@ func (r *testRunner) runTest(
 				if event.Err == nil {
 					t.L().Printf("task finished: %s", event.Name)
 					continue
-				} else if event.ExpectedCancel {
+				} else if event.TriggeredByTest {
 					t.L().Printf("task canceled by test: %s", event.Name)
 					continue
 				}

pkg/testutils/lint/lint_test.go

@@ -198,6 +198,31 @@ func TestLint(t *testing.T) {
 		t.Error(err)
 	}
 
+	// Things that are package scoped are below here.
+	pkgScope := pkgVar
+	if !pkgSpecified {
+		pkgScope = "./pkg/..."
+	}
+
+	// Load packages for top-level forbidden import tests.
+	pkgPath := filepath.Join(cockroachDB, pkgScope)
+	pkgs, err := packages.Load(
+		&packages.Config{
+			Mode: packages.NeedImports | packages.NeedName,
+			Dir:  crdbDir,
+		},
+		pkgPath,
+	)
+	if err != nil {
+		t.Fatal(errors.Wrapf(err, "error loading package %s", pkgPath))
+	}
+	// NB: if no packages were found, this API confusingly
+	// returns no error, so we need to explicitly check that
+	// something was returned.
+	if len(pkgs) == 0 {
+		t.Fatalf("could not list packages under %s", pkgPath)
+	}
+
 	t.Run("TestLowercaseFunctionNames", func(t *testing.T) {
 		skip.UnderShort(t)
 		t.Parallel()
@@ -1804,12 +1829,6 @@ func TestLint(t *testing.T) {
 		}
 	})
 
-	// Things that are packaged scoped are below here.
-	pkgScope := pkgVar
-	if !pkgSpecified {
-		pkgScope = "./pkg/..."
-	}
-
 	t.Run("TestForbiddenImports", func(t *testing.T) {
 		t.Parallel()
 
@@ -1850,23 +1869,6 @@ func TestLint(t *testing.T) {
 		grepBuf.WriteString(")$")
 
 		filter := stream.FilterFunc(func(arg stream.Arg) error {
-			pkgPath := filepath.Join(cockroachDB, pkgScope)
-			pkgs, err := packages.Load(
-				&packages.Config{
-					Mode: packages.NeedImports | packages.NeedName,
-					Dir:  crdbDir,
-				},
-				pkgPath,
-			)
-			if err != nil {
-				return errors.Wrapf(err, "error loading package %s", pkgPath)
-			}
-			// NB: if no packages were found, this API confusingly
-			// returns no error, so we need to explicitly check that
-			// something was returned.
-			if len(pkgs) == 0 {
-				return errors.Newf("could not list packages under %s", pkgPath)
-			}
 			for _, pkg := range pkgs {
 				for _, s := range pkg.Imports {
 					arg.Out <- pkg.PkgPath + ": " + s.PkgPath
@@ -2794,4 +2796,56 @@ func TestLint(t *testing.T) {
 			}
 		}
 	})
+
+	// Test forbidden roachtest imports.
+	t.Run("TestRoachtestForbiddenImports", func(t *testing.T) {
+		t.Parallel()
+
+		roachprodLoggerPkg := "github.com/cockroachdb/cockroach/pkg/roachprod/logger"
+		// forbiddenImportPkg -> permittedReplacementPkg
+		forbiddenImports := map[string]string{
+			"github.com/cockroachdb/cockroach/pkg/util/log": roachprodLoggerPkg,
+			"log": roachprodLoggerPkg,
+		}
+
+		// grepBuf creates a grep string that matches any forbidden import pkgs.
+		var grepBuf bytes.Buffer
+		grepBuf.WriteByte('(')
+		for forbiddenPkg := range forbiddenImports {
+			grepBuf.WriteByte('|')
+			grepBuf.WriteString(regexp.QuoteMeta(forbiddenPkg))
+		}
+		grepBuf.WriteString(")$")
+
+		filter := stream.FilterFunc(func(arg stream.Arg) error {
+			for _, pkg := range pkgs {
+				for _, s := range pkg.Imports {
+					arg.Out <- pkg.PkgPath + ": " + s.PkgPath
+				}
+			}
+			return nil
+		})
+		numAnalyzed := 0
+		if err := stream.ForEach(stream.Sequence(
+			filter,
+			stream.Sort(),
+			stream.Uniq(),
+			stream.Grep(`cockroach/pkg/cmd/roachtest/(tests|operations): `),
+		), func(s string) {
+			pkgStr := strings.Split(s, ": ")
+			_, importedPkg := pkgStr[0], pkgStr[1]
+			numAnalyzed++
+
+			// Test that a disallowed package is not imported.
+			if replPkg, ok := forbiddenImports[importedPkg]; ok {
+				t.Errorf("\n%s <- please use %q instead of %q", s, replPkg, importedPkg)
+			}
+		}); err != nil {
+			t.Error(err)
+		}
+		if numAnalyzed == 0 {
+			t.Errorf("Empty input! Please check the linter.")
+		}
+	})
+
 }