Skip to content

Commit ea594f0

Browse files
craig[bot]herkolategan
andcommitted
Merge #148445
148445: testutils: add roachtest analyzer r=DarrylWong a=herkolategan Previously, there was no linter that would detect use of the `go` keyword in roachtest. All roachtest goroutines must be launched through the `Task` interface provided by the roachtest framework. This change reuses a linter from the `lint passes forbiddenmethod` package to detect use of the `go` keyword in roachtest. The roachtest go analyzer only operates on the roachtest package, with a filter hardcoded in the analyzer. Thus we can still add excludes in the nogo config to allow goroutines in some roachtest packages that are part of the framework. Epic: None Release note: None Co-authored-by: Herko Lategan <[email protected]>
2 parents 989f0a0 + 7743555 commit ea594f0

File tree

12 files changed

+141
-79
lines changed

12 files changed

+141
-79
lines changed

BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,5 +208,6 @@ nogo(
208208
"//pkg/testutils/lint/passes/returnerrcheck",
209209
"//pkg/testutils/lint/passes/shadow",
210210
"//pkg/testutils/lint/passes/unconvert",
211+
"//pkg/testutils/lint/passes/roachtest",
211212
],
212213
)

build/bazelutil/nogo_config.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@
1515
"pkg/.*_generated\\.go$": "generated code"
1616
}
1717
},
18+
"roachtestgo": {
19+
"exclude_files": {
20+
"pkg/cmd/roachtest/roachtestutil/task": "task manager implements goroutine management",
21+
"pkg/cmd/roachtest/roachtestutil/mixedversion": "part of the roachtest framework",
22+
"pkg/cmd/roachtest/operations": "operations do not run under the managed roachtest framework",
23+
"pkg/cmd/roachtest/[^/]+\\.go$": "roachtest framework",
24+
"_test\\.go$": "tests"
25+
}
26+
},
1827
"deepequalerrors": {
1928
"exclude_files": {
2029
"external/": "exclude all third-party code for all analyzers",

pkg/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2454,6 +2454,7 @@ GO_TARGETS = [
24542454
"//pkg/testutils/lint/passes/returncheck:returncheck",
24552455
"//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck",
24562456
"//pkg/testutils/lint/passes/returnerrcheck:returnerrcheck_test",
2457+
"//pkg/testutils/lint/passes/roachtest:roachtest",
24572458
"//pkg/testutils/lint/passes/shadow:shadow",
24582459
"//pkg/testutils/lint/passes/staticcheck:staticcheck",
24592460
"//pkg/testutils/lint/passes/unconvert:unconvert",

pkg/cmd/roachtest/roachtestutil/BUILD.bazel

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ go_library(
2222
"//pkg/cmd/roachtest/cluster",
2323
"//pkg/cmd/roachtest/option",
2424
"//pkg/cmd/roachtest/roachtestflags",
25+
"//pkg/cmd/roachtest/roachtestutil/task",
2526
"//pkg/cmd/roachtest/spec",
2627
"//pkg/cmd/roachtest/test",
2728
"//pkg/kv/kvpb",
@@ -32,7 +33,6 @@ go_library(
3233
"//pkg/testutils",
3334
"//pkg/testutils/sqlutils",
3435
"//pkg/util",
35-
"//pkg/util/ctxgroup",
3636
"//pkg/util/httputil",
3737
"//pkg/util/humanizeutil",
3838
"//pkg/util/protoutil",
@@ -46,7 +46,6 @@ go_library(
4646
"@com_github_prometheus_common//expfmt",
4747
"@com_github_stretchr_testify//require",
4848
"@org_golang_x_exp//maps",
49-
"@org_golang_x_sync//errgroup",
5049
],
5150
)
5251

pkg/cmd/roachtest/roachtestutil/profile.go

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,13 @@ import (
2020

2121
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
2222
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
23+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/task"
2324
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
2425
"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
25-
"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
2626
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
2727
"github.com/cockroachdb/errors"
2828
"github.com/google/pprof/profile"
2929
"github.com/stretchr/testify/require"
30-
"golang.org/x/sync/errgroup"
3130
)
3231

3332
type profileOptions struct {
@@ -277,9 +276,9 @@ func MeasureQPS(
277276
}
278277
// Count the inserts before sleeping.
279278
var total atomic.Int64
280-
group := ctxgroup.WithContext(ctx)
279+
g := t.NewErrorGroup(task.WithContext(ctx))
281280
for _, db := range dbs {
282-
group.Go(func() error {
281+
g.Go(func(ctx context.Context, l *logger.Logger) error {
283282
var v float64
284283
if err := db.QueryRowContext(
285284
ctx, `SELECT sum(value) FROM crdb_internal.node_metrics WHERE name in ('sql.select.count', 'sql.insert.count')`,
@@ -291,7 +290,7 @@ func MeasureQPS(
291290
})
292291
}
293292

294-
require.NoError(t, group.Wait())
293+
require.NoError(t, g.WaitE())
295294
return int(total.Load())
296295
}
297296

@@ -408,33 +407,33 @@ func getProfileSingleNode(
408407
// Supported profile types are: {"cpu", "allocs", "mutex", "heap"}.
409408
func GetProfile(
410409
ctx context.Context,
410+
t task.GroupProvider,
411411
cluster cluster.Cluster,
412-
logger *logger.Logger,
413412
profileType string,
414413
duration time.Duration,
415414
nodes option.NodeListOption,
416415
) ([]*profile.Profile, error) {
417416
profiles := make([]*profile.Profile, len(nodes))
418417

419418
// Create an error group to manage concurrent profile collection.
420-
g, ctx := errgroup.WithContext(ctx)
419+
g := t.NewErrorGroup(task.WithContext(ctx))
421420

422421
for i, nodeId := range nodes {
423-
g.Go(func() error {
422+
g.Go(func(ctx context.Context, l *logger.Logger) error {
424423
var err error
425-
profiles[i], err = getProfileSingleNode(ctx, cluster, logger, profileType,
424+
profiles[i], err = getProfileSingleNode(ctx, cluster, l, profileType,
426425
nodeId, duration)
427426

428427
if err != nil {
429-
logger.Printf("error getting profile for node %d: %s", nodeId, err)
428+
l.Printf("error getting profile for node %d: %s", nodeId, err)
430429
return errors.Wrapf(err, "getting profile for n%d", nodeId)
431430
}
432431
return nil
433432
})
434433
}
435434

436435
// Wait for all profiles to complete or first error
437-
if err := g.Wait(); err != nil {
436+
if err := g.WaitE(); err != nil {
438437
return nil, err
439438
}
440439

pkg/cmd/roachtest/tests/ldap_connection_scale.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@ import (
1414
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
1515
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
1616
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
17+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/task"
1718
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
1819
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
1920
"github.com/cockroachdb/cockroach/pkg/roachprod/install"
21+
"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
2022
"github.com/jackc/pgx/v5"
2123
"github.com/stretchr/testify/require"
2224
)
@@ -315,10 +317,11 @@ func testParallelConnections(
315317
t.L().Printf("Creating connection: %d.", i+1)
316318
semaphore <- struct{}{}
317319

318-
go func(index int) {
320+
t.Go(func(ctx context.Context, _ *logger.Logger) error {
319321
defer func() { <-semaphore }()
320-
createConnectionFunc(index)
321-
}(i)
322+
createConnectionFunc(i)
323+
return nil
324+
}, task.Name(fmt.Sprintf("create-connection-%d", i)))
322325
}
323326

324327
// Ensure all connections have been created.

pkg/cmd/roachtest/tests/sysbench.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ func runSysbench(ctx context.Context, t test.Test, c cluster.Cluster, opts sysbe
299299
m.Go(
300300
func(ctx context.Context, l *logger.Logger) error {
301301
var err error
302-
profiles[typ], err = roachtestutil.GetProfile(ctx, c, l, typ,
302+
profiles[typ], err = roachtestutil.GetProfile(ctx, t, c, typ,
303303
collectionDuration, c.CRDBNodes())
304304
return err
305305
},

pkg/cmd/roachtest/tests/tpcc.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
2626
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
2727
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/mixedversion"
28+
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/task"
2829
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
2930
"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
3031
"github.com/cockroachdb/cockroach/pkg/roachprod/install"
@@ -2511,9 +2512,8 @@ func runTPCCPublished(
25112512
// Run all the queries in parallel to find the total pending count.
25122513
found := make(chan int)
25132514
for _, nodeID := range crdbNodes {
2514-
nodeID := nodeID
2515-
go func() {
2516-
db := c.Conn(ctx, t.L(), nodeID)
2515+
t.Go(func(ctx context.Context, l *logger.Logger) error {
2516+
db := c.Conn(ctx, l, nodeID)
25172517
defer db.Close()
25182518
var n int
25192519
require.NoError(t,
@@ -2522,7 +2522,8 @@ func runTPCCPublished(
25222522
"SELECT value FROM crdb_internal.node_metrics WHERE name = 'queue.replicate.pending'",
25232523
).Scan(&n))
25242524
found <- n
2525-
}()
2525+
return nil
2526+
}, task.Name(fmt.Sprintf("check-replication-pending-%d", nodeID)))
25262527
}
25272528
var total int
25282529
// Wait until they have all completed.

pkg/testutils/lint/lint_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2854,7 +2854,8 @@ func TestLint(t *testing.T) {
28542854
}
28552855
})
28562856

2857-
// Test forbidden roachtest imports.
2857+
// Test forbidden roachtest imports. The mixedversion and task packages are
2858+
// allowed because they are part of the roachtest framework.
28582859
t.Run("TestRoachtestForbiddenImports", func(t *testing.T) {
28592860
t.Parallel()
28602861

@@ -2890,7 +2891,8 @@ func TestLint(t *testing.T) {
28902891
filter,
28912892
stream.Sort(),
28922893
stream.Uniq(),
2893-
stream.Grep(`cockroach/pkg/cmd/roachtest/(tests|operations): `),
2894+
stream.Grep(`cockroach/pkg/cmd/roachtest/.*: `),
2895+
stream.GrepNot(`cockroach/pkg/cmd/roachtest/roachtestutil/(mixedversion|task): `),
28942896
), func(s string) {
28952897
pkgStr := strings.Split(s, ": ")
28962898
_, importedPkg := pkgStr[0], pkgStr[1]

pkg/testutils/lint/passes/forbiddenmethod/naked_go.go

Lines changed: 68 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -16,72 +16,84 @@ import (
1616
"golang.org/x/tools/go/ast/inspector"
1717
)
1818

19-
const nakedGoPassName = "nakedgo"
20-
2119
// NakedGoAnalyzer prevents use of the `go` keyword.
22-
var NakedGoAnalyzer = &analysis.Analyzer{
23-
Name: nakedGoPassName,
24-
Doc: "Prevents direct use of the 'go' keyword. Goroutines should be launched through Stopper.",
25-
Requires: []*analysis.Analyzer{inspect.Analyzer},
26-
Run: func(pass *analysis.Pass) (interface{}, error) {
27-
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
28-
inspect.Preorder([]ast.Node{
29-
(*ast.GoStmt)(nil),
30-
}, func(n ast.Node) {
31-
node := n.(*ast.GoStmt)
20+
var NakedGoAnalyzer = NewNakedGoAnalyzer(
21+
"nakedgo",
22+
"Use of go keyword not allowed, use a Stopper instead",
23+
"Prevents direct use of the 'go' keyword. Goroutines should be launched through Stopper.",
24+
nil,
25+
)
3226

33-
const debug = false
27+
type FilterFunc func(pass *analysis.Pass) bool
3428

35-
// NB: we're not using passesutil.HasNoLintComment because it
36-
// has false negatives (i.e. comments apply to infractions that
37-
// they were clearly not intended for).
38-
//
39-
// The approach here is inspired by `analysistest.check` - the
40-
// nolint comment has to be on the same line as the *end* of the
41-
// `*GoStmt`.
42-
f := passesutil.FindContainingFile(pass, n)
43-
cm := ast.NewCommentMap(pass.Fset, node, f.Comments)
44-
var cc *ast.Comment
45-
for _, cg := range cm[n] {
46-
for _, c := range cg.List {
47-
if c.Pos() < node.Go {
48-
// The current comment is "before" the `go` invocation, so it's
49-
// not relevant for silencing the lint.
50-
continue
51-
}
52-
if cc == nil || cc.Pos() > node.Go {
53-
// This comment is after, but closer to the `go` invocation than
54-
// previous candidate.
55-
cc = c
56-
if debug {
57-
fmt.Printf("closest comment now %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
29+
func NewNakedGoAnalyzer(name, message, doc string, filter FilterFunc) *analysis.Analyzer {
30+
return &analysis.Analyzer{
31+
Name: name,
32+
Doc: doc,
33+
Requires: []*analysis.Analyzer{inspect.Analyzer},
34+
Run: func(pass *analysis.Pass) (interface{}, error) {
35+
if filter != nil && !filter(pass) {
36+
return nil, nil
37+
}
38+
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
39+
inspect.Preorder([]ast.Node{
40+
(*ast.GoStmt)(nil),
41+
}, func(n ast.Node) {
42+
node := n.(*ast.GoStmt)
43+
44+
const debug = false
45+
46+
// NB: we're not using passesutil.HasNoLintComment because it
47+
// has false negatives (i.e. comments apply to infractions that
48+
// they were clearly not intended for).
49+
//
50+
// The approach here is inspired by `analysistest.check` - the
51+
// nolint comment has to be on the same line as the *end* of the
52+
// `*GoStmt`.
53+
f := passesutil.FindContainingFile(pass, n)
54+
cm := ast.NewCommentMap(pass.Fset, node, f.Comments)
55+
var cc *ast.Comment
56+
for _, cg := range cm[n] {
57+
for _, c := range cg.List {
58+
if c.Pos() < node.Go {
59+
// The current comment is "before" the `go` invocation, so it's
60+
// not relevant for silencing the lint.
61+
continue
62+
}
63+
if cc == nil || cc.Pos() > node.Go {
64+
// This comment is after, but closer to the `go` invocation than
65+
// previous candidate.
66+
cc = c
67+
if debug {
68+
fmt.Printf("closest comment now %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
69+
}
5870
}
5971
}
6072
}
61-
}
62-
if cc != nil && strings.Contains(cc.Text, "nolint:"+nakedGoPassName) {
63-
if debug {
64-
fmt.Printf("GoStmt at: %d-%d\n", n.Pos(), n.End())
65-
fmt.Printf("GoStmt.Go at: %d\n", node.Go)
66-
fmt.Printf("GoStmt.Call at: %d-%d\n", node.Call.Pos(), node.Call.End())
67-
}
73+
if cc != nil && strings.Contains(cc.Text, "nolint:"+name) {
74+
if debug {
75+
fmt.Printf("GoStmt at: %d-%d\n", n.Pos(), n.End())
76+
fmt.Printf("GoStmt.Go at: %d\n", node.Go)
77+
fmt.Printf("GoStmt.Call at: %d-%d\n", node.Call.Pos(), node.Call.End())
78+
}
6879

69-
goPos := pass.Fset.Position(node.End())
70-
cmPos := pass.Fset.Position(cc.Pos())
80+
goPos := pass.Fset.Position(node.End())
81+
cmPos := pass.Fset.Position(cc.Pos())
7182

72-
if goPos.Line == cmPos.Line {
73-
if debug {
74-
fmt.Printf("suppressing lint because of %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
83+
if goPos.Line == cmPos.Line {
84+
if debug {
85+
fmt.Printf("suppressing lint because of %d-%d: %s\n", cc.Pos(), cc.End(), cc.Text)
86+
}
87+
return
7588
}
76-
return
7789
}
78-
}
7990

80-
pass.Report(analysis.Diagnostic{
81-
Pos: n.Pos(),
82-
Message: "Use of go keyword not allowed, use a Stopper instead",
91+
pass.Report(analysis.Diagnostic{
92+
Pos: n.Pos(),
93+
Message: message,
94+
})
8395
})
84-
})
85-
return nil, nil
86-
},
96+
return nil, nil
97+
},
98+
}
8799
}

0 commit comments

Comments
 (0)