cmd/coordinator, maintner/maintnerd/maintapi: consolidate TryBot branch logic

This change reverts the coordinator side to be much simpler, as it was
before CL 167382, and consolidates all the version selection in maintapi.
This direction is chosen because at this time¹ maintapi is best suited
to perform version selection.

This is a refactor CL that leaves current behavior unmodified, and test
cases provide improved coverage. The following smaller CL will make the
desired changes to behavior.

Remove TestNewTrySetBuildRepoGo110 because it's no longer needed. It was
added when x/build started requiring module mode to build successfully,
given that Go 1.10 didn't have module mode support.

Background

At this time¹ coordinator makes use of maintapi to find TryBot work,
and it is a collaborate effort between the two components that results
in determining what builds will happen. Coordinator is ultimately
responsible for starting and running the builds, but it doesn't have
information about the branches in the Go project (the Go revision at
refs/heads/master, refs/heads/release-branch.go1.16, etc.). Maintapi
has that information via the maintner corpus. So it makes the version
information available to coordinator by populating relevant fields in
apipb.GoFindTryWorkResponse.

Issue golang/go#28891 was about wanting to test golang.org/x repo CLs
on release-branch.go1.n with the corresponding Go 1.n version, rather
than Go tip. Unfortunately, it was implemented on the coordinator side,
resulting in the logic for version selection to be more spread between
the coordinator and maintapi components. There were followup issues
like golang/go#42127 and golang/go#37512, whose fixes built on top of
the coordinator side, and increased complexity there. As a consequence,
making and testing further changes became more difficult than it needs
to be.

¹ After golang/go#34744 is done, I'd like to move all the TryBot version
  selection logic into coordinator, the component responsible for TryBots.
  But not today.

For golang/go#46154.

Change-Id: I93986acefd4bf66b27ccf0323439966122b7989a
Reviewed-on: https://go-review.googlesource.com/c/build/+/319789
Trust: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>

Author: dmitshur

cmd/coordinator/coordinator.go

@@ -1218,26 +1218,6 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 		work.GoVersion = []*apipb.MajorMinor{{}}
 	}
 
-	// GoCommit is non-empty for x/* repos (aka "subrepos"). It
-	// is the Go revision to use to build & test the x/* repo
-	// with. The first element is the master branch. We test the
-	// master branch against all the normal builders configured to
-	// do subrepos. Any GoCommit values past the first are for older
-	// release branches, but we use a limited subset of builders for those.
-	var goRev string
-	if len(work.GoCommit) > 0 {
-		// By default, use the first GoCommit, which represents Go tip (master branch).
-		goRev = work.GoCommit[0]
-	}
-	for i, goBranch := range work.GoBranch {
-		// There are two cases where we want to change goRev to work.GoCommit[i]:
-		// 1. CL branch is like "master" or "release-branch.go1.15" and matches the Go branch exactly.
-		// 2. CL branch is like "release-branch.go1.15-suffix" and its prefix matches.
-		if work.Branch == goBranch || strings.HasPrefix(work.Branch, goBranch+"-") {
-			goRev = work.GoCommit[i]
-		}
-	}
-
 	addBuilderToSet := func(bs *buildStatus, brev buildgo.BuilderRev) {
 		bs.trySet = ts
 		status[brev] = bs
@@ -1252,6 +1232,15 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 		go ts.awaitTryBuild(idx, bs, brev)
 	}
 
+	var mainBuildGoCommit string
+	if key.Project != "go" && len(work.GoCommit) > 0 {
+		// work.GoCommit is non-empty when work.Project != "go".
+		// For the main build, use the first GoCommit, which represents Go tip (master branch).
+		mainBuildGoCommit = work.GoCommit[0]
+	}
+
+	// Start the main TryBot build using the selected builders.
+	// There may be additional builds, those are handled below.
 	if !testingKnobSkipBuilds {
 		go ts.notifyStarting()
 	}
@@ -1260,7 +1249,7 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 		if goVersion.Less(bconf.MinimumGoVersion) {
 			continue
 		}
-		brev := tryKeyToBuilderRev(bconf.Name, key, goRev)
+		brev := tryKeyToBuilderRev(bconf.Name, key, mainBuildGoCommit)
 		bs, err := newBuild(brev, noCommitDetail)
 		if err != nil {
 			log.Printf("can't create build for %q: %v", brev, err)
@@ -1269,17 +1258,16 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 		addBuilderToSet(bs, brev)
 	}
 
-	// For subrepos on the "master" branch, test against prior releases of Go too.
-	if key.Project != "go" && key.Branch == "master" {
-		// linuxBuilder is the standard builder we run for when testing x/* repos against
-		// the past two Go releases.
+	// If this is a golang.org/x repo and there's more than one GoCommit,
+	// that means we're testing against prior releases of Go too.
+	// The version selection logic is currently in maintapi's GoFindTryWork implementation.
+	if key.Project != "go" && len(work.GoCommit) >= 2 {
+		// linuxBuilder is the standard builder for this purpose.
 		linuxBuilder := dashboard.Builders["linux-amd64"]
 
-		// If there's more than one GoCommit, that means this is an x/* repo
-		// and we're testing against previous releases of Go.
 		for i, goRev := range work.GoCommit {
 			if i == 0 {
-				// Skip the i==0 element, which is handled above.
+				// Skip the i==0 element, which was already handled above.
 				continue
 			}
 			branch := work.GoBranch[i]

cmd/coordinator/coordinator_test.go

@@ -161,23 +161,23 @@ func TestStagingClusterBuilders(t *testing.T) {
 func TestIssue28891(t *testing.T) {
 	testingKnobSkipBuilds = true
 
-	work := &apipb.GerritTryWorkItem{ // Roughly based on https://go-review.googlesource.com/c/tools/+/150577/1.
-		Project:   "tools",
-		Branch:    "release-branch.go1.11",
-		ChangeId:  "Ice719ab807ce3922b885a800ac873cdbf165a8f7",
-		Commit:    "9d66f1bfdbed72f546df963194a19d56180c4ce7",
-		GoCommit:  []string{"a2e79571a9d3dbe3cf10dcaeb1f9c01732219869", "e39e43d7349555501080133bb426f1ead4b3ef97", "f5ff72d62301c4e9d0a78167fab5914ca12919bd"},
-		GoBranch:  []string{"master", "release-branch.go1.11", "release-branch.go1.10"},
-		GoVersion: []*apipb.MajorMinor{{1, 12}, {1, 11}, {1, 10}},
+	work := &apipb.GerritTryWorkItem{ // Based on what maintapi's GoFindTryWork does for x/net CL 258478.
+		Project:   "net",
+		Branch:    "release-branch.go1.15",
+		ChangeId:  "I546597cedf3715e6617babcb3b62140bf1857a27",
+		Commit:    "a5fa9d4b7c91aa1c3fecbeb6358ec1127b910dd6",
+		GoCommit:  []string{"72ccabc99449b2cb5bb1438eb90244d55f7b02f5"},
+		GoBranch:  []string{"release-branch.go1.15"},
+		GoVersion: []*apipb.MajorMinor{{1, 15}},
 	}
 	ts := newTrySet(work)
 	if len(ts.builds) == 0 {
 		t.Fatal("no builders in try set, want at least 1")
 	}
 	for i, bs := range ts.builds {
-		const go111Revision = "e39e43d7349555501080133bb426f1ead4b3ef97"
-		if bs.BuilderRev.Rev != go111Revision {
-			t.Errorf("build[%d]: %s: x/tools on release-branch.go1.11 branch should be tested with Go 1.11, but isn't", i, bs.NameAndBranch())
+		const go115Revision = "72ccabc99449b2cb5bb1438eb90244d55f7b02f5"
+		if bs.BuilderRev.Rev != go115Revision {
+			t.Errorf("build[%d]: %s: x/net on release-branch.go1.15 branch should be tested with Go 1.15, but isn't", i, bs.NameAndBranch())
 		}
 	}
 }
@@ -188,63 +188,40 @@ func TestIssue28891(t *testing.T) {
 func TestIssue42127(t *testing.T) {
 	testingKnobSkipBuilds = true
 
-	work := &apipb.GerritTryWorkItem{ // Roughly based on https://go-review.googlesource.com/c/net/+/264058/1.
+	work := &apipb.GerritTryWorkItem{ // Based on what maintapi's GoFindTryWork does for x/net CL 264058.
 		Project:   "net",
 		Branch:    "release-branch.go1.15-bundle",
 		ChangeId:  "I546597cedf3715e6617babcb3b62140bf1857a27",
-		Commit:    "286322bb8662ddff3686e42a01c33a1d47d25153",
-		GoCommit:  []string{"b2a8317b31d652b3ee293a313269b8290bcdf96c", "3b1f07fff774f86f13316f7bec6552566568fc10", "768b64711ae4292bd9a02c9cc8d44282f5fac66b"},
-		GoBranch:  []string{"master", "release-branch.go1.15", "release-branch.go1.14"},
-		GoVersion: []*apipb.MajorMinor{{1, 16}, {1, 15}, {1, 14}},
+		Commit:    "abf26a14a65b111d492067f407f32455c5b1048c",
+		GoCommit:  []string{"72ccabc99449b2cb5bb1438eb90244d55f7b02f5"},
+		GoBranch:  []string{"release-branch.go1.15"},
+		GoVersion: []*apipb.MajorMinor{{1, 15}},
 	}
 	ts := newTrySet(work)
 	if len(ts.builds) == 0 {
 		t.Fatal("no builders in try set, want at least 1")
 	}
 	for i, bs := range ts.builds {
-		const go115Revision = "3b1f07fff774f86f13316f7bec6552566568fc10"
+		const go115Revision = "72ccabc99449b2cb5bb1438eb90244d55f7b02f5"
 		if bs.BuilderRev.Rev != go115Revision {
 			t.Errorf("build[%d]: %s: x/net on release-branch.go1.15-bundle branch should be tested with Go 1.15, but isn't", i, bs.NameAndBranch())
 		}
 	}
 }
 
-// tests that we don't test Go 1.10 for the build repo
-func TestNewTrySetBuildRepoGo110(t *testing.T) {
-	testingKnobSkipBuilds = true
-
-	work := &apipb.GerritTryWorkItem{
-		Project:   "build",
-		Branch:    "master",
-		ChangeId:  "I6f05da2186b38dc8056081252563a82c50f0ce05",
-		Commit:    "a62e6a3ab11cc9cc2d9e22a50025dd33fc35d22f",
-		GoCommit:  []string{"a2e79571a9d3dbe3cf10dcaeb1f9c01732219869", "e39e43d7349555501080133bb426f1ead4b3ef97", "f5ff72d62301c4e9d0a78167fab5914ca12919bd"},
-		GoBranch:  []string{"master", "release-branch.go1.11", "release-branch.go1.10"},
-		GoVersion: []*apipb.MajorMinor{{1, 12}, {1, 11}, {1, 10}},
-	}
-	ts := newTrySet(work)
-	for i, bs := range ts.builds {
-		v := bs.NameAndBranch()
-		if strings.Contains(v, "Go 1.10.x") {
-			t.Errorf("unexpected builder: %v", v)
-		}
-		t.Logf("build[%d]: %s", i, v)
-	}
-}
-
 // Tests that TryBots run on branches of the x/ repositories, other than
 // "master" and "release-branch.go1.N". See golang.org/issue/37512.
 func TestXRepoBranches(t *testing.T) {
 	testingKnobSkipBuilds = true
 
-	work := &apipb.GerritTryWorkItem{
+	work := &apipb.GerritTryWorkItem{ // Based on what maintapi's GoFindTryWork does for x/tools CL 227356.
 		Project:   "tools",
 		Branch:    "gopls-release-branch.0.4",
 		ChangeId:  "Ica799fcf117bf607c0c59f41b08a78552339dc53",
-		Commit:    "6af4ce83c61d0f3e616b410b53b51982798c4d73",
-		GoVersion: []*apipb.MajorMinor{{1, 15}},
-		GoCommit:  []string{"74d6de03fd7db2c6faa7794620a9bcf0c4f018f2"},
+		Commit:    "13af72af5ccdfe6f1e75b57b02cfde3bb0a77a76",
+		GoCommit:  []string{"9995c6b50aa55c1cc1236d1d688929df512dad53"},
 		GoBranch:  []string{"master"},
+		GoVersion: []*apipb.MajorMinor{{1, 17}},
 	}
 	ts := newTrySet(work)
 	for i, bs := range ts.builds {

maintner/maintnerd/maintapi/api.go

@@ -104,7 +104,14 @@ var tryCommentRx = regexp.MustCompile(`(?m)^TRY=(.*)$`)
 
 // tryWorkItem creates a GerritTryWorkItem for
 // the Gerrit CL specified by cl, ci, comments.
-func tryWorkItem(cl *maintner.GerritCL, ci *gerrit.ChangeInfo, comments map[string][]gerrit.CommentInfo) *apipb.GerritTryWorkItem {
+//
+// goProj is the state of the main Go repository.
+// develVersion is the version of Go in development at HEAD of master branch.
+// supportedReleases are the supported Go releases per https://golang.org/doc/devel/release.html#policy.
+func tryWorkItem(
+	cl *maintner.GerritCL, ci *gerrit.ChangeInfo, comments map[string][]gerrit.CommentInfo,
+	goProj refer, develVersion apipb.MajorMinor, supportedReleases []*apipb.GoRelease,
+) (*apipb.GerritTryWorkItem, error) {
 	w := &apipb.GerritTryWorkItem{
 		Project:  cl.Project.Project(),
 		Branch:   strings.TrimPrefix(cl.Branch(), "refs/heads/"),
@@ -116,6 +123,7 @@ func tryWorkItem(cl *maintner.GerritCL, ci *gerrit.ChangeInfo, comments map[stri
 		w.Commit = ci.CurrentRevision
 		w.Version = int32(ci.Revisions[ci.CurrentRevision].PatchSetNumber)
 	}
+
 	// Look for "TRY=" comments. Only consider messages that are accompanied
 	// by a Run-TryBot+1 vote, as a way of confirming the comment author has
 	// Trybot Access (see https://golang.org/wiki/GerritAccess#trybot-access-may-start-trybots).
@@ -153,7 +161,64 @@ func tryWorkItem(cl *maintner.GerritCL, ci *gerrit.ChangeInfo, comments map[stri
 			})
 		}
 	}
-	return w
+
+	// Populate GoCommit, GoBranch, GoVersion fields
+	// according to what's being tested. Coordinator
+	// will use these to run corresponding tests.
+	if w.Project == "go" {
+		// TryBot on Go repo. Set the GoVersion field based on branch name.
+		if major, minor, ok := parseReleaseBranchVersion(w.Branch); ok {
+			// A release branch like release-branch.goX.Y.
+			// Use the major-minor Go version determined from the branch name.
+			w.GoVersion = []*apipb.MajorMinor{{major, minor}}
+		} else {
+			// A branch that is not release-branch.goX.Y: maybe
+			// "master" or a development branch like "dev.link".
+			// There isn't a way to determine the version from its name,
+			// so use the development Go version until we need to do more.
+			// TODO(golang.org/issue/42376): This can be made more precise.
+			w.GoVersion = []*apipb.MajorMinor{&develVersion}
+		}
+	} else {
+		// TryBot on a subrepo.
+		trimBundleSuffix := func(branch string) string {
+			// There was only one branch with a suffix, release-branch.go1.15-bundle in x/net, so just hardcode it.
+			// TODO: This special case can be removed when Go 1.17 is out and 1.15 is no longer supported.
+			return strings.TrimSuffix(branch, "-bundle")
+		}
+		if major, minor, ok := parseReleaseBranchVersion(trimBundleSuffix(w.Branch)); ok {
+			// An release-branch.goX.Y (or one with a -suffix) branch is used for internal needs
+			// of goX.Y only, so no reason to test it on other Go versions.
+			goBranch := fmt.Sprintf("release-branch.go%d.%d", major, minor)
+			goCommit := goProj.Ref("refs/heads/" + goBranch)
+			if goCommit == "" {
+				return nil, fmt.Errorf("branch %q doesn't exist", goBranch)
+			}
+			w.GoCommit = []string{goCommit.String()}
+			w.GoBranch = []string{goBranch}
+			w.GoVersion = []*apipb.MajorMinor{{major, minor}}
+		} else if w.Branch == "master" {
+			// For subrepos on the "master" branch, use the default policy
+			// of testing it with Go tip and the supported releases.
+			w.GoCommit = []string{goProj.Ref("refs/heads/master").String()}
+			w.GoBranch = []string{"master"}
+			w.GoVersion = []*apipb.MajorMinor{&develVersion}
+			for _, r := range supportedReleases {
+				w.GoCommit = append(w.GoCommit, r.BranchCommit)
+				w.GoBranch = append(w.GoBranch, r.BranchName)
+				w.GoVersion = append(w.GoVersion, &apipb.MajorMinor{r.Major, r.Minor})
+			}
+		} else {
+			// A branch that is neither release-branch.goX.Y nor "master":
+			// maybe some custom branch like "dev.go2go".
+			// Test it against Go tip only until we want to do more.
+			w.GoCommit = []string{goProj.Ref("refs/heads/master").String()}
+			w.GoBranch = []string{"master"}
+			w.GoVersion = []*apipb.MajorMinor{&develVersion}
+		}
+	}
+
+	return w, nil
 }
 
 func firstLine(s string) string {
@@ -251,7 +316,8 @@ func goFindTryWork(ctx context.Context, gerritc *gerrit.Client, maintc *maintner
 		return nil, err
 	}
 	// If Go X.Y is the latest supported release, the version in development is likely Go X.(Y+1).
-	develVersion := &apipb.MajorMinor{
+	// TODO(golang.org/issue/42376): This can be made more precise.
+	develVersion := apipb.MajorMinor{
 		Major: supportedReleases[0].Major,
 		Minor: supportedReleases[0].Minor + 1,
 	}
@@ -273,31 +339,10 @@ func goFindTryWork(ctx context.Context, gerritc *gerrit.Client, maintc *maintner
 		if err != nil {
 			return nil, fmt.Errorf("gerritc.ListChangeComments(ctx, %q): %v", changeID, err)
 		}
-		work := tryWorkItem(cl, ci, comments)
-		if work.Project == "go" {
-			// Trybot on Go repo. Set the GoVersion field based on branch name.
-			if major, minor, ok := parseReleaseBranchVersion(work.Branch); ok {
-				// A release branch like release-branch.goX.Y.
-				// Use the major-minor Go version determined from the branch name.
-				work.GoVersion = []*apipb.MajorMinor{{major, minor}}
-			} else {
-				// A branch that is not release-branch.goX.Y: maybe
-				// "master" or a development branch like "dev.link".
-				// There isn't a way to determine the version from its name,
-				// so use the development Go version until we need to do more.
-				// TODO(golang.org/issue/42376): This can be made more precise.
-				work.GoVersion = []*apipb.MajorMinor{develVersion}
-			}
-		} else {
-			// Trybot on a subrepo. Set the Go fields to master and the supported releases.
-			work.GoCommit = []string{goProj.Ref("refs/heads/master").String()}
-			work.GoBranch = []string{"master"}
-			work.GoVersion = []*apipb.MajorMinor{develVersion}
-			for _, r := range supportedReleases {
-				work.GoCommit = append(work.GoCommit, r.BranchCommit)
-				work.GoBranch = append(work.GoBranch, r.BranchName)
-				work.GoVersion = append(work.GoVersion, &apipb.MajorMinor{r.Major, r.Minor})
-			}
+		work, err := tryWorkItem(cl, ci, comments, goProj, develVersion, supportedReleases)
+		if err != nil {
+			log.Printf("goFindTryWork: skipping CL %v because %v\n", ci.ChangeNumber, err)
+			continue
 		}
 		res.Waiting = append(res.Waiting, work)
 	}
@@ -355,6 +400,17 @@ func (s apiService) ListGoReleases(ctx context.Context, req *apipb.ListGoRelease
 	}, nil
 }
 
+// refer is implemented by *maintner.GerritProject,
+// or something that acts like it for testing.
+type refer interface {
+	// Ref returns a non-change ref, such as "HEAD", "refs/heads/master",
+	// or "refs/tags/v0.8.0",
+	// Change refs of the form "refs/changes/*" are not supported.
+	// The returned hash is the zero value (an empty string) if the ref
+	// does not exist.
+	Ref(ref string) maintner.GitHash
+}
+
 // nonChangeRefLister is implemented by *maintner.GerritProject,
 // or something that acts like it for testing.
 type nonChangeRefLister interface {