cmd/coordinator: consider explicitly requested builders as SlowBots

Previously, a builder was considered to be "SlowBot" whenever it was
requested via the TRY= syntax, and it wasn't already a part of the
default set of trybot builders.

This change makes it so that a builder is considered a SlowBot if
it was explicitly requested via the TRY= syntax, even if it was
already going to run anyway.

The goal behind this change is to improve the experience of using
SlowBots, by making them more useful and predictable.

The UI currently reports that "SlowBots are starting ..." whenever
there is a non-zero amount of SlowBots. So if a user requested
TRY=linux-amd64, they'd likely see "TryBots are starting ...",
which can be misunderstood to mean that SlowBots are not working.

Additionally, if a user requested three builders via TRY= syntax,
but one of them happened to be a part of the default try set, then
the message at the end would be "Extra slowbot builds that ran:"
including only 2 builders. At that point, it's hard to tell whether
they got the TRY= syntax wrong for one of the builders, or if it was
omitted because it's always run anyway.

A future usability improvement in golang.org/issue/38279 is to make
SlowBots not skip any tests that normal trybots may skip for faster
results. This change is in preparation for that.

For golang/go#38279.
For golang/go#34501.

Change-Id: Idb7f1bcb1694fe72f85237976995a94f273c7c16
Reviewed-on: https://go-review.googlesource.com/c/build/+/227778
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Alexander Rakoczy <[email protected]>

Author: dmitshur

cmd/coordinator/coordinator.go

@@ -1167,9 +1167,9 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 	// TODO(golang.org/issue/38303): compute goBranch value better
 	goBranch := key.Branch // assume same as repo's branch for now
 
-	builders := dashboard.TryBuildersForProject(key.Project, key.Branch, goBranch)
-	slowBots := slowBotsFromComments(work, builders)
-	builders = append(builders, slowBots...)
+	tryBots := dashboard.TryBuildersForProject(key.Project, key.Branch, goBranch)
+	slowBots := slowBotsFromComments(work)
+	builders := joinBuilders(tryBots, slowBots)
 
 	log.Printf("Starting new trybot set for %v", key)
 	ts := &trySet{
@@ -1342,6 +1342,23 @@ func tryKeyToBuilderRev(builder string, key tryKey, goRev string) buildgo.Builde
 	}
 }
 
+// joinBuilders joins sets of builders into one set.
+// The resulting set contains unique builders sorted by name.
+func joinBuilders(sets ...[]*dashboard.BuildConfig) []*dashboard.BuildConfig {
+	byName := make(map[string]*dashboard.BuildConfig)
+	for _, set := range sets {
+		for _, bc := range set {
+			byName[bc.Name] = bc
+		}
+	}
+	var all []*dashboard.BuildConfig
+	for _, bc := range byName {
+		all = append(all, bc)
+	}
+	sort.Slice(all, func(i, j int) bool { return all[i].Name < all[j].Name })
+	return all
+}
+
 // state returns a copy of the trySet's state.
 func (ts *trySet) state() trySetState {
 	ts.mu.Lock()
@@ -1541,7 +1558,7 @@ func (ts *trySet) noteBuildComplete(bs *buildStatus) {
 				numFail, len(ts.builds), name, errMsg)
 		}
 		if len(ts.slowBots) > 0 {
-			fmt.Fprintf(&buf, "Extra slowbot builds that ran:\n")
+			fmt.Fprintf(&buf, "SlowBot builds that ran:\n")
 			for _, c := range ts.slowBots {
 				fmt.Fprintf(&buf, "* %s\n", c.Name)
 			}
@@ -2073,8 +2090,11 @@ func (st *buildStatus) useKeepGoingFlag() bool {
 	return !st.isTry() && strings.HasPrefix(st.branch, "release-branch.go")
 }
 
+// isTry reports whether the build is a part of a TryBot (pre-submit) run.
+// It may be a normal TryBot (part of the default try set) or a SlowBot.
 func (st *buildStatus) isTry() bool { return st.trySet != nil }
 
+// isSlowBot reports whether the build is an explicitly requested SlowBot.
 func (st *buildStatus) isSlowBot() bool {
 	if st.trySet == nil {
 		return false
@@ -2093,7 +2113,7 @@ func (st *buildStatus) buildRecord() *types.BuildRecord {
 		ProcessID: processID,
 		StartTime: st.startTime,
 		IsTry:     st.isTry(),
-		IsExtra:   st.isSlowBot(),
+		IsSlowBot: st.isSlowBot(),
 		GoRev:     st.Rev,
 		Rev:       st.SubRevOrGoRev(),
 		Repo:      st.RepoOrGo(),
@@ -3942,33 +3962,23 @@ func importPathOfRepo(repo string) string {
 	return r.ImportPath
 }
 
-// slowBotsFromComments looks at the TRY= comments from Gerrit (in
-// work) and returns any addition slow trybot build configuration that
-// should be tested in addition to the ones provided in the existing
-// slice.
-func slowBotsFromComments(work *apipb.GerritTryWorkItem, existing []*dashboard.BuildConfig) (extraBuilders []*dashboard.BuildConfig) {
-	have := map[string]bool{}
-	for _, bc := range existing {
-		have[bc.Name] = true
-	}
-
+// 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) {
 	tryTerms := latestTryTerms(work)
-
 	for _, bc := range dashboard.Builders {
-		if have[bc.Name] {
-			continue
-		}
 		for _, term := range tryTerms {
 			if bc.MatchesSlowBotTerm(term) {
-				extraBuilders = append(extraBuilders, bc)
+				builders = append(builders, bc)
 				break
 			}
 		}
 	}
-	sort.Slice(extraBuilders, func(i, j int) bool {
-		return extraBuilders[i].Name < extraBuilders[j].Name
+	sort.Slice(builders, func(i, j int) bool {
+		return builders[i].Name < builders[j].Name
 	})
-	return
+	return builders
 }
 
 // xReposFromComments looks at the TRY= comments from Gerrit (in

cmd/coordinator/coordinator_test.go

@@ -271,7 +271,6 @@ func mustConf(t *testing.T, name string) *dashboard.BuildConfig {
 }
 
 func TestSlowBotsFromComments(t *testing.T) {
-	existing := []*dashboard.BuildConfig{mustConf(t, "linux-amd64")}
 	work := &apipb.GerritTryWorkItem{
 		Version: 2,
 		TryMessage: []*apipb.TryVoteMessage{
@@ -289,9 +288,9 @@ func TestSlowBotsFromComments(t *testing.T) {
 			},
 		},
 	}
-	extra := slowBotsFromComments(work, existing)
+	slowBots := slowBotsFromComments(work)
 	var got []string
-	for _, bc := range extra {
+	for _, bc := range slowBots {
 		got = append(got, bc.Name)
 	}
 	want := []string{"aix-ppc64", "darwin-amd64-10_14", "linux-arm64-packet"}

types/types.go

@@ -87,7 +87,7 @@ type BuildRecord struct {
 	ProcessID     string
 	StartTime     time.Time
 	IsTry         bool // is trybot run
-	IsExtra       bool // is an extra opt-in "slowbot" builder, not on by default
+	IsSlowBot     bool // is an explicitly requested "slowbot" builder
 	GoRev         string
 	Rev           string // same as GoRev for repo "go"
 	Repo          string // "go", "net", etc.