cmd/coordinator: add SlowBots support, opt-in to different slow trybots

This parses TRY= comments to opt-in to slower/difference trybots.

This needs some docs/UI work yet.

Updates golang/go#34501

Change-Id: I13a835520d7ac341bc86139b0a2118235bc83e60
Reviewed-on: https://go-review.googlesource.com/c/build/+/201338
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>

Author: bradfitz

cmd/coordinator/coordinator.go

@@ -42,6 +42,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+	"unicode"
 
 	"go4.org/syncutil"
 	grpc "grpc.go4.org"
@@ -1054,7 +1055,8 @@ func (k *tryKey) ChangeTriple() string {
 type trySet struct {
 	// immutable
 	tryKey
-	tryID string // "T" + 9 random hex
+	tryID    string                   // "T" + 9 random hex
+	slowBots []*dashboard.BuildConfig // any opt-in slower builders to run in a trybot run
 
 	// wantedAsOf is guarded by statusMu and is used by
 	// findTryWork. It records the last time this tryKey was still
@@ -1105,14 +1107,19 @@ var testingKnobSkipBuilds bool
 func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 	key := tryWorkItemKey(work)
 	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...)
+
 	log.Printf("Starting new trybot set for %v", key)
 	ts := &trySet{
 		tryKey: key,
 		tryID:  "T" + randHex(9),
 		trySetState: trySetState{
 			builds: make([]*buildStatus, 0, len(builders)),
 		},
+		slowBots: slowBots,
 	}
 
 	// Defensive check that the input is well-formed.
@@ -1237,7 +1244,11 @@ func (ts *trySet) state() trySetState {
 // 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() {
-	msg := "TryBots beginning. Status page: https://farmer.golang.org/try?commit=" + ts.Commit[:8]
+	name := "TryBots"
+	if len(ts.slowBots) > 0 {
+		name = "SlowBots"
+	}
+	msg := name + " beginning. Status page: https://farmer.golang.org/try?commit=" + ts.Commit[:8]
 
 	ctx := context.Background()
 	if ci, err := gerritClient.GetChangeDetail(ctx, ts.ChangeTriple()); err == nil {
@@ -1382,20 +1393,38 @@ func (ts *trySet) noteBuildComplete(bs *buildStatus) {
 	}
 
 	if remain == 0 {
-		score, msg := 1, "TryBots are happy."
-		if numFail > 0 {
+		name := "TryBots"
+		if len(ts.slowBots) > 0 {
+			name = "SlowBots"
+		}
+
+		var buf bytes.Buffer
+		var score int
+		if numFail == 0 {
+			score = 1
+			fmt.Fprintf(&buf, "%s are happy.\n", name)
+		} else if numFail > 0 {
+			score = -1
 			ts.mu.Lock()
 			errMsg := ts.errMsg.String()
 			ts.mu.Unlock()
-			score, msg = -1, fmt.Sprintf("%d of %d TryBots failed:\n%s\n"+failureFooter,
-				numFail, len(ts.builds), errMsg)
+			fmt.Fprintf(&buf, "%d of %d %s failed:\n%s\n"+failureFooter,
+				numFail, len(ts.builds), name, errMsg)
+		}
+		if len(ts.slowBots) > 0 {
+			fmt.Fprintf(&buf, "Extra slowbot builds that ran:\n")
+			for _, c := range ts.slowBots {
+				fmt.Fprintf(&buf, "* %s\n", c.Name)
+			}
 		}
+		// TODO: provide a link in the final report that links to a permanent summary page
+		// of which builds ran, and for how long.
 		if len(benchResults) > 0 {
 			// TODO: restore this functionality
 			// msg += fmt.Sprintf("\nBenchmark results are available at:\nhttps://perf.golang.org/search?q=cl:%d+try:%s", ts.ci.ChangeNumber, ts.tryID)
 		}
 		if err := gerritClient.SetReview(context.Background(), ts.ChangeTriple(), ts.Commit, gerrit.ReviewInput{
-			Message: msg,
+			Message: buf.String(),
 			Labels: map[string]int{
 				"TryBot-Result": score,
 			},
@@ -1857,12 +1886,25 @@ func (st *buildStatus) build() error {
 
 func (st *buildStatus) isTry() bool { return st.trySet != nil }
 
+func (st *buildStatus) isSlowBot() bool {
+	if st.trySet == nil {
+		return false
+	}
+	for _, conf := range st.trySet.slowBots {
+		if st.conf == conf {
+			return true
+		}
+	}
+	return false
+}
+
 func (st *buildStatus) buildRecord() *types.BuildRecord {
 	rec := &types.BuildRecord{
 		ID:        st.buildID,
 		ProcessID: processID,
 		StartTime: st.startTime,
 		IsTry:     st.isTry(),
+		IsExtra:   st.isSlowBot(),
 		GoRev:     st.Rev,
 		Rev:       st.SubRevOrGoRev(),
 		Repo:      st.RepoOrGo(),
@@ -3634,3 +3676,59 @@ func importPathOfRepo(repo string) string {
 	}
 	return "golang.org/x/" + repo
 }
+
+// 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) {
+	tryMsg := latestTryMessage(work) // "aix, darwin, linux-386-387, arm64"
+	if tryMsg == "" {
+		return nil
+	}
+	if len(tryMsg) > 1<<10 { // arbitrary sanity
+		return nil
+	}
+
+	have := map[string]bool{}
+	for _, bc := range existing {
+		have[bc.Name] = true
+	}
+
+	tryTerms := strings.FieldsFunc(tryMsg, func(c rune) bool {
+		return !unicode.IsLetter(c) && !unicode.IsNumber(c) && c != '-' && c != '_'
+	})
+	for _, bc := range dashboard.Builders {
+		if have[bc.Name] {
+			continue
+		}
+		for _, term := range tryTerms {
+			if bc.MatchesSlowBotTerm(term) {
+				extraBuilders = append(extraBuilders, bc)
+				break
+			}
+		}
+	}
+	sort.Slice(extraBuilders, func(i, j int) bool {
+		return extraBuilders[i].Name < extraBuilders[j].Name
+	})
+	return
+}
+
+func latestTryMessage(work *apipb.GerritTryWorkItem) string {
+	// Prioritize exact version matches first
+	for i := len(work.TryMessage) - 1; i >= 0; i-- {
+		m := work.TryMessage[i]
+		if m.Version == work.Version {
+			return m.Message
+		}
+	}
+	// Otherwise the latest message at all
+	for i := len(work.TryMessage) - 1; i >= 0; i-- {
+		m := work.TryMessage[i]
+		if m.Message != "" {
+			return m.Message
+		}
+	}
+	return ""
+}

cmd/coordinator/coordinator_test.go

@@ -19,6 +19,7 @@ import (
 	"time"
 
 	"golang.org/x/build/buildenv"
+	"golang.org/x/build/dashboard"
 	"golang.org/x/build/internal/buildgo"
 	"golang.org/x/build/maintner/maintnerd/apipb"
 )
@@ -211,3 +212,41 @@ func TestBuildersJSON(t *testing.T) {
 		t.Error(buf.String())
 	}
 }
+
+func mustConf(t *testing.T, name string) *dashboard.BuildConfig {
+	conf, ok := dashboard.Builders[name]
+	if !ok {
+		t.Fatalf("unknown builder %q", name)
+	}
+	return conf
+}
+
+func TestSlowBotsFromComments(t *testing.T) {
+	existing := []*dashboard.BuildConfig{mustConf(t, "linux-amd64")}
+	work := &apipb.GerritTryWorkItem{
+		Version: 2,
+		TryMessage: []*apipb.TryVoteMessage{
+			{
+				Version: 1,
+				Message: "ios",
+			},
+			{
+				Version: 2,
+				Message: "arm64, mac aix ",
+			},
+			{
+				Version: 1,
+				Message: "aix",
+			},
+		},
+	}
+	extra := slowBotsFromComments(work, existing)
+	var got []string
+	for _, bc := range extra {
+		got = append(got, bc.Name)
+	}
+	want := []string{"aix-ppc64", "darwin-amd64-10_14", "linux-arm64-packet"}
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("mismatch:\n got: %q\nwant: %q\n", got, want)
+	}
+}

dashboard/builders.go

@@ -19,6 +19,81 @@ import (
 	"golang.org/x/build/types"
 )
 
+// slowBotAliases maps short names from TRY= comments to which builder to run.
+//
+// TODO: we'll likely expand this, or move it, or change the matching
+// syntax entirely. This is a first draft.
+var slowBotAliases = map[string]string{
+	// Known missing builders:
+	"linux-mips":    "",
+	"linux-mips64":  "",
+	"linux-mipsle":  "",
+	"mips":          "",
+	"mips64":        "",
+	"mipsle":        "",
+	"netbsd-arm64":  "",
+	"openbsd-arm":   "",
+	"openbsd-arm64": "",
+	"nacl-arm":      "",
+
+	"386":            "linux-386",
+	"aix":            "aix-ppc64",
+	"amd64":          "linux-amd64",
+	"amd64p32":       "nacl-amd64p32",
+	"android":        "android-amd64-emu",
+	"android-386":    "android-386-emu",
+	"android-amd64":  "android-amd64-emu",
+	"android-arm":    "android-arm-corellium",
+	"android-arm64":  "android-arm64-corellium",
+	"arm":            "linux-arm",
+	"arm64":          "linux-arm64-packet",
+	"arm64p32":       "nacl-amd64p32",
+	"darwin":         "darwin-amd64-10_14",
+	"darwin-386":     "darwin-386-10_14",
+	"darwin-amd64":   "darwin-amd64-10_14",
+	"darwin-arm":     "darwin-arm-mg912baios",
+	"darwin-arm64":   "darwin-arm64-corellium",
+	"dragonfly":      "dragonfly-amd64",
+	"freebsd":        "freebsd-amd64-12_0",
+	"freebsd-386":    "freebsd-386-12_0",
+	"freebsd-amd64":  "freebsd-amd64-12_0",
+	"freebsd-arm":    "freebsd-arm-paulzhol",
+	"illumos":        "illumos-amd64",
+	"ios":            "darwin-arm64-corellium",
+	"js":             "js-wasm",
+	"linux":          "linux-amd64",
+	"linux-arm64":    "linux-arm64-packet",
+	"linux-mips64le": "linux-mips64le-mengzhuo",
+	"linux-ppc64":    "linux-ppc64-buildlet",
+	"linux-ppc64le":  "linux-ppc64le-buildlet",
+	"linux-s390x":    "linux-s390x-ibm",
+	"mac":            "darwin-amd64-10_14",
+	"macos":          "darwin-amd64-10_14",
+	"mips64le":       "linux-mips64le-mengzhuo",
+	"nacl":           "nacl-amd64p32",
+	"nacl-387":       "nacl-386",
+	"nacl-arm64p32":  "nacl-amd64p32",
+	"netbsd":         "netbsd-amd64-8_0",
+	"netbsd-386":     "netbsd-386-8_0",
+	"netbsd-amd64":   "netbsd-amd64-8_0",
+	"netbsd-arm":     "netbsd-arm-bsiegert",
+	"openbsd":        "openbsd-amd64-64",
+	"openbsd-386":    "openbsd-386-64",
+	"openbsd-amd64":  "openbsd-amd64-64",
+	"plan9":          "plan9-386-0intro",
+	"plan9-386":      "plan9-386-0intro",
+	"plan9-amd64":    "plan9-amd64-9front",
+	"ppc64":          "linux-ppc64-buildlet",
+	"ppc64le":        "linux-ppc64le-buildlet",
+	"s390x":          "linux-s390x-ibm",
+	"solaris":        "solaris-amd64-oraclerel",
+	"solaris-amd64":  "solaris-amd64-oraclerel",
+	"wasm":           "js-wasm",
+	"windows":        "windows-amd64-2016",
+	"windows-386":    "windows-386-2008",
+	"windows-amd64":  "windows-amd64-2016",
+}
+
 // Builders are the different build configurations.
 // The keys are like "darwin-amd64" or "linux-386-387".
 // This map should not be modified by other packages.
@@ -787,6 +862,13 @@ func (c *BuildConfig) GOARCH() string {
 	return arch[:i]
 }
 
+// MatchesSlowBotTerm reports whether some provided term from a
+// TRY=... comment on a Run-TryBot+1 vote on Gerrit should match this
+// build config.
+func (c *BuildConfig) MatchesSlowBotTerm(term string) bool {
+	return term != "" && (term == c.Name || slowBotAliases[term] == c.Name)
+}
+
 // FilePathJoin is mostly like filepath.Join (without the cleaning) except
 // it uses the path separator of c.GOOS instead of the host system's.
 func (c *BuildConfig) FilePathJoin(x ...string) string {