From 212e7159125773bd0030b8404219deb81a15ffca Mon Sep 17 00:00:00 2001 From: Herko Lategan Date: Wed, 6 Nov 2024 13:59:26 +0000 Subject: [PATCH 1/2] roachtest: minor task manager refactoring 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. Epic: None Release note: None --- .../roachtestutil/mixedversion/runner.go | 2 +- .../roachtest/roachtestutil/task/manager.go | 37 ++++++++++++------- pkg/cmd/roachtest/test_impl.go | 4 +- pkg/cmd/roachtest/test_runner.go | 2 +- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go b/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go index c29c3c8e99b6..5a5af126494f 100644 --- a/pkg/cmd/roachtest/roachtestutil/mixedversion/runner.go +++ b/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 } diff --git a/pkg/cmd/roachtest/roachtestutil/task/manager.go b/pkg/cmd/roachtest/roachtestutil/task/manager.go index d15d783d5e37..de4bbd0e3c37 100644 --- a/pkg/cmd/roachtest/roachtestutil/task/manager.go +++ b/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() diff --git a/pkg/cmd/roachtest/test_impl.go b/pkg/cmd/roachtest/test_impl.go index 0fade5bfa0d0..a58d4b726778 100644 --- a/pkg/cmd/roachtest/test_impl.go +++ b/pkg/cmd/roachtest/test_impl.go @@ -12,7 +12,6 @@ import ( "math/rand" "os" "regexp" - "runtime/debug" "strings" "sync" "time" @@ -654,8 +653,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 diff --git a/pkg/cmd/roachtest/test_runner.go b/pkg/cmd/roachtest/test_runner.go index 69d43859c58a..a8bf62bf5b57 100644 --- a/pkg/cmd/roachtest/test_runner.go +++ b/pkg/cmd/roachtest/test_runner.go @@ -1299,7 +1299,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 } From 7a663b0d2d1ec1b4f33e90e1976a5a6eb6b18419 Mon Sep 17 00:00:00 2001 From: Herko Lategan Date: Tue, 5 Nov 2024 15:22:58 +0000 Subject: [PATCH 2/2] testutils: roachtest log import linter 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 --- pkg/testutils/lint/lint_test.go | 100 ++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 23 deletions(-) diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 273cdd06e574..f2a2c7a3cc03 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/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.") + } + }) + }