cmd/coordinator: plumb commit time and branch from findWork down into scheduler

The branch is not yet used in this CL, but the scheduler has it now
and can use it easily in the future.

Updates golang/go#19178

Change-Id: I6abab826a8668cb091d0face8184f28d08421722
Reviewed-on: https://go-review.googlesource.com/c/build/+/208277
Reviewed-by: Bryan C. Mills <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

Author: bradfitz

cmd/coordinator/coordinator.go

@@ -357,11 +357,24 @@ func main() {
 var ignoreAllNewWork bool
 
 // addWorkTestHook is optionally set by tests.
-var addWorkTestHook func(work buildgo.BuilderRev)
+var addWorkTestHook func(buildgo.BuilderRev, *commitDetail)
+
+type commitDetail struct {
+	CommitTime string // in time.RFC3339 format
+	Branch     string
+}
 
 func addWork(work buildgo.BuilderRev) {
+	addWorkDetail(work, nil)
+}
+
+// addWorkDetail adds some work to (maybe) do, if it's not already
+// enqueued and the builders are configured to run the given repo. The
+// detail argument is optional and used for scheduling. It's currently
+// only used for post-submit builds.
+func addWorkDetail(work buildgo.BuilderRev, detail *commitDetail) {
 	if f := addWorkTestHook; f != nil {
-		f(work)
+		f(work, detail)
 		return
 	}
 	if ignoreAllNewWork || isBuilding(work) {
@@ -375,12 +388,12 @@ func addWork(work buildgo.BuilderRev) {
 		}
 		return
 	}
-	st, err := newBuild(work)
+	st, err := newBuild(work, detail)
 	if err != nil {
 		log.Printf("Bad build work params %v: %v", work, err)
-	} else {
-		st.start()
+		return
 	}
+	st.start()
 }
 
 // httpToHTTPSRedirector redirects all requests from http to https.
@@ -872,12 +885,28 @@ func findWork() error {
 
 	var goRevisions []string // revisions of repo "go", branch "master" revisions
 	seenSubrepo := make(map[string]bool)
+	commitTime := make(map[string]string)   // git rev => "2019-11-20T22:54:54Z" (time.RFC3339 from build.golang.org's JSON)
+	commitBranch := make(map[string]string) // git rev => "master"
+
+	add := func(br buildgo.BuilderRev) {
+		rev := br.SubRev
+		if br.SubRev == "" {
+			rev = br.Rev
+		}
+		addWorkDetail(br, &commitDetail{
+			CommitTime: commitTime[rev],
+			Branch:     commitBranch[rev],
+		})
+	}
+
 	for _, br := range bs.Revisions {
 		if br.Repo == "grpc-review" {
 			// Skip the grpc repo. It's only for reviews
 			// for now (using LetsUseGerrit).
 			continue
 		}
+		commitTime[br.Revision] = br.Date
+		commitBranch[br.Revision] = br.Branch
 		awaitSnapshot := false
 		if br.Repo == "go" {
 			if br.Branch == "master" {
@@ -938,7 +967,7 @@ func findWork() error {
 				}
 			}
 
-			addWork(rev)
+			add(rev)
 		}
 	}
 
@@ -950,7 +979,7 @@ func findWork() error {
 			continue
 		}
 		for _, rev := range goRevisions {
-			addWork(buildgo.BuilderRev{Name: b, Rev: rev})
+			add(buildgo.BuilderRev{Name: b, Rev: rev})
 		}
 	}
 	return nil
@@ -1160,7 +1189,7 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 			continue
 		}
 		brev := tryKeyToBuilderRev(bconf.Name, key, goRev)
-		bs, err := newBuild(brev)
+		bs, err := newBuild(brev, noCommitDetail)
 		if err != nil {
 			log.Printf("can't create build for %q: %v", brev, err)
 			continue
@@ -1190,7 +1219,7 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 				continue
 			}
 			brev := tryKeyToBuilderRev(linuxBuilder.Name, key, goRev)
-			bs, err := newBuild(brev)
+			bs, err := newBuild(brev, noCommitDetail)
 			if err != nil {
 				log.Printf("can't create build for %q: %v", brev, err)
 				continue
@@ -1221,7 +1250,7 @@ func newTrySet(work *apipb.GerritTryWorkItem) *trySet {
 				SubName: project,
 				SubRev:  rev,
 			}
-			bs, err := newBuild(brev)
+			bs, err := newBuild(brev, noCommitDetail)
 			if err != nil {
 				log.Printf("can't create x/%s trybot build for go/master commit %s: %v", project, rev, err)
 				return nil
@@ -1338,7 +1367,7 @@ func (ts *trySet) awaitTryBuild(idx int, bs *buildStatus, brev buildgo.BuilderRe
 		if !ts.wanted() {
 			return
 		}
-		bs, _ = newBuild(brev)
+		bs, _ = newBuild(brev, noCommitDetail)
 		bs.trySet = ts
 		bs.goBranch = old.goBranch
 		go bs.start()
@@ -1588,7 +1617,12 @@ func poolForConf(conf *dashboard.HostConfig) BuildletPool {
 	}
 }
 
-func newBuild(rev buildgo.BuilderRev) (*buildStatus, error) {
+// noCommitDetail is just a nice name for nil at call sites.
+var noCommitDetail *commitDetail = nil
+
+// newBuild constructs a new *buildStatus from rev and an optional detail.
+// If detail is nil, the scheduler just has less information to work with.
+func newBuild(rev buildgo.BuilderRev, detail *commitDetail) (*buildStatus, error) {
 	// Note: can't acquire statusMu in newBuild, as this is called
 	// from findTryWork -> newTrySet, which holds statusMu.
 
@@ -1600,6 +1634,19 @@ func newBuild(rev buildgo.BuilderRev) (*buildStatus, error) {
 		return nil, fmt.Errorf("required field Rev is empty; got %+v", rev)
 	}
 
+	var branch string
+	var commitTime time.Time
+	if detail != nil {
+		branch = detail.Branch
+		if detail.CommitTime != "" {
+			var err error
+			commitTime, err = time.Parse(time.RFC3339, detail.CommitTime)
+			if err != nil {
+				return nil, fmt.Errorf("invalid commit time %q, for %+v: %err", detail.CommitTime, rev, err)
+			}
+		}
+	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	return &buildStatus{
 		buildID:    "B" + randHex(9),
@@ -1608,6 +1655,8 @@ func newBuild(rev buildgo.BuilderRev) (*buildStatus, error) {
 		startTime:  time.Now(),
 		ctx:        ctx,
 		cancel:     cancel,
+		commitTime: commitTime,
+		branch:     branch,
 	}, nil
 }
 
@@ -1717,6 +1766,8 @@ func (st *buildStatus) onceInitHelpersFunc() {
 		BuilderRev: st.BuilderRev,
 		HostType:   st.conf.HostType,
 		IsTry:      st.isTry(),
+		CommitTime: st.commitTime,
+		Branch:     st.branch,
 	}
 	st.helpers = getBuildlets(st.ctx, st.conf.NumTestHelpers(st.isTry()), schedTmpl, st)
 }
@@ -1799,6 +1850,8 @@ func (st *buildStatus) getBuildlet() (*buildlet.Client, error) {
 		HostType:   st.conf.HostType,
 		IsTry:      st.trySet != nil,
 		BuilderRev: st.BuilderRev,
+		CommitTime: st.commitTime,
+		Branch:     st.branch,
 	}
 	st.mu.Lock()
 	st.schedItem = schedItem
@@ -2110,6 +2163,8 @@ func (st *buildStatus) crossCompileMakeAndSnapshot(config *dashboard.CrossCompil
 		HostType:   config.CompileHostType,
 		IsTry:      st.trySet != nil,
 		BuilderRev: st.BuilderRev,
+		CommitTime: st.commitTime,
+		Branch:     st.branch,
 	})
 	sp.Done(err)
 	if err != nil {
@@ -3370,11 +3425,13 @@ type eventAndTime struct {
 type buildStatus struct {
 	// Immutable:
 	buildgo.BuilderRev
-	buildID   string // "B" + 9 random hex
-	goBranch  string // non-empty for subrepo trybots if not go master branch
-	conf      *dashboard.BuildConfig
-	startTime time.Time // actually time of newBuild (~same thing); TODO(bradfitz): rename this createTime
-	trySet    *trySet   // or nil
+	buildID    string // "B" + 9 random hex
+	goBranch   string // non-empty for subrepo trybots if not go master branch
+	conf       *dashboard.BuildConfig
+	startTime  time.Time // actually time of newBuild (~same thing)
+	trySet     *trySet   // or nil
+	commitTime time.Time // non-zero for post-submit builders
+	branch     string    // non-empty for post-submit work
 
 	onceInitHelpers sync.Once // guards call of onceInitHelpersFunc
 	helpers         <-chan *buildlet.Client

cmd/coordinator/coordinator_test.go

@@ -189,8 +189,8 @@ func TestFindWork(t *testing.T) {
 		return false
 	}
 
-	addWorkTestHook = func(work buildgo.BuilderRev) {
-		t.Logf("Got: %v", work)
+	addWorkTestHook = func(work buildgo.BuilderRev, d *commitDetail) {
+		t.Logf("Got: %v, %+v", work, d)
 	}
 	defer func() { addWorkTestHook = nil }()
 

cmd/coordinator/sched.go

@@ -284,15 +284,19 @@ func (s *Scheduler) waiterState(waiter *SchedItem) (ws types.BuildletWaitStatus)
 	return ws
 }
 
-// schedLess reports whether scheduled item ia is "less" (more
-// important) than scheduled item ib.
+// schedLess reports whether the scheduler item ia is "less" (more
+// important) than scheduler item ib.
 func schedLess(ia, ib *SchedItem) bool {
 	// TODO: flesh out this policy more. For now this is much
 	// better than the old random policy.
 	// For example, consider IsHelper? Figure out a policy.
+	// TODO: consider SchedItem.Branch.
+	// TODO: pass in a context to schedLess that includes current time and current
+	// top of various branches. Then we can use that in decisions rather than doing
+	// lookups or locks in a less function.
 
-	// Gomote is most important, then TryBots, then FIFO for
-	// either Gomote/Try, else LIFO for post-submit builds.
+	// Gomote is most important, then TryBots (FIFO for either), then
+	// post-submit builds (LIFO, by commit time)
 	if ia.IsGomote != ib.IsGomote {
 		return ia.IsGomote
 	}
@@ -307,8 +311,11 @@ func schedLess(ia, ib *SchedItem) bool {
 		// time. But time works for now.
 		return ia.requestTime.Before(ib.requestTime)
 	}
-	// Post-submit builds are LIFO.
-	return ib.requestTime.Before(ia.requestTime)
+
+	// Post-submit builds are LIFO by commit time, not necessarily
+	// when the coordinator's findWork loop threw them at the
+	// scheduler.
+	return ia.CommitTime.After(ib.CommitTime)
 }
 
 // SchedItem is a specification of a requested buildlet in its
@@ -320,15 +327,15 @@ type SchedItem struct {
 	IsGomote           bool
 	IsTry              bool
 	IsHelper           bool
+	CommitTime         time.Time
+	Branch             string
 
 	// The following unexported fields are set by the Scheduler in
 	// Scheduler.GetBuildlet.
 
 	s           *Scheduler
 	requestTime time.Time
-	commitTime  time.Time // TODO: populate post-submit commit time from maintnerd
-	branch      string    // TODO: populate from maintnerd
-	tryFor      string    // TODO: which user. (user with 1 trybot >> user with 50 trybots)
+	tryFor      string // TODO: which user. (user with 1 trybot >> user with 50 trybots)
 	pool        BuildletPool
 	ctxDone     <-chan struct{}
 

cmd/coordinator/sched_test.go

@@ -87,20 +87,24 @@ func TestSchedLess(t *testing.T) {
 		{
 			name: "reg LIFO, less",
 			a: &SchedItem{
-				requestTime: t2,
+				CommitTime:  t2,
+				requestTime: t1, // shouldn't be used
 			},
 			b: &SchedItem{
-				requestTime: t1,
+				CommitTime:  t1,
+				requestTime: t2, // shouldn't be used
 			},
 			want: true,
 		},
 		{
 			name: "reg LIFO, greater",
 			a: &SchedItem{
-				requestTime: t1,
+				CommitTime:  t1,
+				requestTime: t2, // shouldn't be used
 			},
 			b: &SchedItem{
-				requestTime: t2,
+				CommitTime:  t2,
+				requestTime: t1, // shouldn't be used
 			},
 			want: false,
 		},