cmd/coordinator: report invalid SlowBots in TRY comments
If an invalid SlowBot is requested alongside valid ones via the `TRY=`
comments only the valid builders run, while silently ignoring the
invalid ones.
This CL updates this behaviour to report any invalid builder terms via a
automated review comment.
Fixes golang/go#58902
Change-Id: Ia338c1ca0783818db6fcf954a26eb4e287afb223
GitHub-Last-Rev: 4f3c2084d4fa2e4ce2064915ce6622569a43400f
GitHub-Pull-Request: golang/build#73
Reviewed-on: https://go-review.googlesource.com/c/build/+/528055
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index 5c1865f..180eaa1 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -63,6 +63,7 @@
"golang.org/x/build/repos"
"golang.org/x/build/revdial/v2"
"golang.org/x/build/types"
+ "golang.org/x/exp/slices"
"golang.org/x/time/rate"
"google.golang.org/api/option"
"google.golang.org/grpc"
@@ -1192,11 +1193,11 @@
subBranch = work.Branch
}
tryBots := dashboard.TryBuildersForProject(work.Project, work.Branch, goBranch)
- slowBots := slowBotsFromComments(work)
+ slowBots, invalidSlowBots := slowBotsFromComments(work)
builders := joinBuilders(tryBots, slowBots)
key := tryWorkItemKey(work)
- log.Printf("Starting new trybot set for %v", key)
+ log.Printf("Starting new trybot set for %v (ignored invalid terms = %q)", key, invalidSlowBots)
ts := &trySet{
tryKey: key,
tryID: "T" + randHex(9),
@@ -1246,7 +1247,7 @@
// Start the main TryBot build using the selected builders.
// There may be additional builds, those are handled below.
if !testingKnobSkipBuilds {
- go ts.notifyStarting()
+ go ts.notifyStarting(invalidSlowBots)
}
for _, bconf := range builders {
goVersion := types.MajorMinor{Major: int(work.GoVersion[0].Major), Minor: int(work.GoVersion[0].Minor)}
@@ -1476,13 +1477,17 @@
// notifyStarting runs in its own goroutine and posts to Gerrit that
// the trybots have started on the user's CL with a link of where to watch.
-func (ts *trySet) notifyStarting() {
+func (ts *trySet) notifyStarting(invalidSlowBots []string) {
name := "TryBots"
if len(ts.slowBots) > 0 {
name = "SlowBots"
}
msg := name + " beginning. Status page: " + ts.statusPage() + "\n"
+ if len(invalidSlowBots) > 0 {
+ msg += fmt.Sprintf("Note that the following SlowBot terms didn't match any existing builder name or slowbot alias: %s.\n", strings.Join(invalidSlowBots, ", "))
+ }
+
// If any of the requested SlowBot builders
// have a known issue, give users a warning.
for _, b := range ts.slowBots {
@@ -2057,12 +2062,18 @@
// slowBotsFromComments looks at the Gerrit comments in work,
// and returns all build configurations that were explicitly
-// requested to be tested as SlowBots via the TRY= syntax.
-func slowBotsFromComments(work *apipb.GerritTryWorkItem) (builders []*dashboard.BuildConfig) {
+// requested to be tested as SlowBots via the TRY= syntax. It
+// also returns any build terms that are not a valid builder
+// or alias.
+func slowBotsFromComments(work *apipb.GerritTryWorkItem) (builders []*dashboard.BuildConfig, invalidTryTerms []string) {
tryTerms := latestTryTerms(work)
+ invalidTryTerms = slices.Clone(tryTerms)
for _, bc := range dashboard.Builders {
for _, term := range tryTerms {
if bc.MatchesSlowBotTerm(term) {
+ invalidTryTerms = slices.DeleteFunc(invalidTryTerms, func(e string) bool {
+ return e == term
+ })
builders = append(builders, bc)
break
}
@@ -2071,7 +2082,7 @@
sort.Slice(builders, func(i, j int) bool {
return builders[i].Name < builders[j].Name
})
- return builders
+ return builders, invalidTryTerms
}
type xRepoAndBuilder struct {
diff --git a/cmd/coordinator/coordinator_test.go b/cmd/coordinator/coordinator_test.go
index 9d58133..aa7e572 100644
--- a/cmd/coordinator/coordinator_test.go
+++ b/cmd/coordinator/coordinator_test.go
@@ -264,8 +264,8 @@
// Next, add try messages, and check that the SlowBot is now included.
work.TryMessage = []*apipb.TryVoteMessage{
- {Message: "TRY=linux", AuthorId: 1234, Version: 1},
- {Message: "TRY=linux-arm", AuthorId: 1234, Version: 1},
+ {Message: "linux", AuthorId: 1234, Version: 1},
+ {Message: "linux-arm", AuthorId: 1234, Version: 1},
}
ts = newTrySet(work)
hasLinuxArmBuilder = false
@@ -337,7 +337,7 @@
},
},
}
- slowBots := slowBotsFromComments(work)
+ slowBots, invalidSlowBots := slowBotsFromComments(work)
var got []string
for _, bc := range slowBots {
got = append(got, bc.Name)
@@ -346,6 +346,10 @@
if !reflect.DeepEqual(got, want) {
t.Errorf("mismatch:\n got: %q\nwant: %q\n", got, want)
}
+
+ if len(invalidSlowBots) > 0 {
+ t.Errorf("mismatch invalidSlowBots:\n got: %d\nwant: 0", len(invalidSlowBots))
+ }
}
func TestSubreposFromComments(t *testing.T) {
@@ -489,3 +493,29 @@
t.Errorf("wrong most recent TryBot thread: got %s, want %s", mostRecentTryBotThread, "aaf7aa39_658707c2")
}
}
+
+func TestInvalidSlowBots(t *testing.T) {
+ work := &apipb.GerritTryWorkItem{
+ Version: 2,
+ TryMessage: []*apipb.TryVoteMessage{
+ {
+ Version: 1,
+ Message: "aix, linux-mipps, amd64, freeebsd",
+ },
+ },
+ }
+ slowBots, invalidSlowBots := slowBotsFromComments(work)
+ var got []string
+ for _, bc := range slowBots {
+ got = append(got, bc.Name)
+ }
+ want := []string{"aix-ppc64", "linux-amd64"}
+ if !reflect.DeepEqual(got, want) {
+ t.Errorf("mismatch:\n got: %q\nwant: %q\n", got, want)
+ }
+
+ wantInvalid := []string{"linux-mipps", "freeebsd"}
+ if !reflect.DeepEqual(invalidSlowBots, wantInvalid) {
+ t.Errorf("mismatch:\n got: %q\nwant: %q\n", invalidSlowBots, wantInvalid)
+ }
+}
