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 <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
diff --git a/cmd/coordinator/coordinator.go b/cmd/coordinator/coordinator.go
index c3de70c..d067c4e 100644
--- a/cmd/coordinator/coordinator.go
+++ b/cmd/coordinator/coordinator.go
@@ -357,11 +357,24 @@
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 @@
}
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 @@
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 @@
}
}
- addWork(rev)
+ add(rev)
}
}
@@ -950,7 +979,7 @@
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 @@
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 @@
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 @@
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 @@
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 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 @@
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 @@
startTime: time.Now(),
ctx: ctx,
cancel: cancel,
+ commitTime: commitTime,
+ branch: branch,
}, nil
}
@@ -1717,6 +1766,8 @@
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 @@
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 @@
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 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
diff --git a/cmd/coordinator/coordinator_test.go b/cmd/coordinator/coordinator_test.go
index 4974698..e852be1 100644
--- a/cmd/coordinator/coordinator_test.go
+++ b/cmd/coordinator/coordinator_test.go
@@ -189,8 +189,8 @@
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 }()
diff --git a/cmd/coordinator/sched.go b/cmd/coordinator/sched.go
index 332053d..f66e0e7 100644
--- a/cmd/coordinator/sched.go
+++ b/cmd/coordinator/sched.go
@@ -284,15 +284,19 @@
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 @@
// 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 @@
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{}
diff --git a/cmd/coordinator/sched_test.go b/cmd/coordinator/sched_test.go
index ebcab67..2636069 100644
--- a/cmd/coordinator/sched_test.go
+++ b/cmd/coordinator/sched_test.go
@@ -87,20 +87,24 @@
{
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,
},
