internal/pool: move the reverse buildlet pool into a pool package

This is a set in a series of steps which will move everything buildlet
pool related into a pool package.

Updates golang/go#36841
Updates golang/go#38337

Change-Id: Ic7a0ccd7838345036df2e72b13084070541cb63c
Reviewed-on: https://go-review.googlesource.com/c/build/+/227769
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alexander Rakoczy <alex@golang.org>

cmd/coordinator/coordinator.go

@@ -254,6 +254,11 @@ func main() {
 
 	mustInitMasterKeyCache(sc)
 
+	// TODO(golang.org/issue/38337): remove package level variables where possible.
+	// TODO(golang.org/issue/36841): remove after key functions are moved into
+	// a shared package.
+	pool.SetBuilderMasterKey(masterKey())
+
 	err := pool.InitGCE(sc, vmDeleteTimeout, testFiles, &basePinErr, isGCERemoteBuildlet, *buildEnvName, *mode)
 	if err != nil {
 		if *mode == "" {
@@ -317,12 +322,12 @@ func main() {
 	http.HandleFunc("/debug/goroutines", handleDebugGoroutines)
 	http.HandleFunc("/builders", handleBuilders)
 	http.HandleFunc("/temporarylogs", handleLogs)
-	http.HandleFunc("/reverse", handleReverse)
+	http.HandleFunc("/reverse", pool.HandleReverse)
 	http.Handle("/revdial", revdialv2.ConnHandler())
 	http.HandleFunc("/style.css", handleStyleCSS)
 	http.HandleFunc("/try", serveTryStatus(false))
 	http.HandleFunc("/try.json", serveTryStatus(true))
-	http.HandleFunc("/status/reverse.json", reversePool.ServeReverseStatusJSON)
+	http.HandleFunc("/status/reverse.json", pool.ReversePool().ServeReverseStatusJSON)
 	http.HandleFunc("/status/post-submit-active.json", handlePostSubmitActiveJSON)
 	http.Handle("/dashboard", dh)
 	http.Handle("/buildlet/create", requireBuildletProxyAuth(http.HandlerFunc(handleBuildletCreate)))
@@ -494,7 +499,7 @@ func mayBuildRev(rev buildgo.BuilderRev) bool {
 	if pool.GCEBuildEnv().MaxBuilds > 0 && numCurrentBuilds() >= pool.GCEBuildEnv().MaxBuilds {
 		return false
 	}
-	if buildConf.IsReverse() && !reversePool.CanBuild(buildConf.HostType) {
+	if buildConf.IsReverse() && !pool.ReversePool().CanBuild(buildConf.HostType) {
 		return false
 	}
 	return true
@@ -866,7 +871,7 @@ func findWorkLoop() {
 		}
 		const debugArm = false
 		if debugArm {
-			for !reversePool.CanBuild("host-linux-arm") {
+			for !pool.ReversePool().CanBuild("host-linux-arm") {
 				log.Printf("waiting for ARM to register.")
 				time.Sleep(time.Second)
 			}
@@ -1634,7 +1639,7 @@ func poolForConf(conf *dashboard.HostConfig) pool.Buildlet {
 			return pool.KubePool()
 		}
 	case conf.IsReverse:
-		return reversePool
+		return pool.ReversePool()
 	default:
 		panic(fmt.Sprintf("no buildlet pool for host type %q", conf.HostType))
 	}
@@ -1742,7 +1747,7 @@ func (st *buildStatus) expectedBuildletStartDuration() time.Duration {
 			return 2 * time.Minute
 		}
 		return time.Minute
-	case *reverseBuildletPool:
+	case *pool.ReverseBuildletPool:
 		goos, arch := st.conf.GOOS(), st.conf.GOARCH()
 		if goos == "darwin" {
 			if arch == "arm" || arch == "arm64" {
@@ -2430,6 +2435,8 @@ func (st *buildStatus) distTestList() (names []string, remoteErr, err error) {
 	return names, nil, nil
 }
 
+type token struct{}
+
 // newTestSet returns a new testSet given the dist test names (strings from "go tool dist test -list")
 // and benchmark items.
 func (st *buildStatus) newTestSet(testStats *buildstats.TestStats, distTestNames []string, benchmarks []*buildgo.BenchmarkItem) (*testSet, error) {

cmd/coordinator/debug.go

@@ -31,7 +31,7 @@ func handleDoSomeWork(work chan<- buildgo.BuilderRev) func(w http.ResponseWriter
 		if r.Method == "GET" {
 			w.Header().Set("Content-Type", "text/html; charset=utf-8")
 			buf := new(bytes.Buffer)
-			if err := tmplDoSomeWork.Execute(buf, reversePool.HostTypes()); err != nil {
+			if err := tmplDoSomeWork.Execute(buf, pool.ReversePool().HostTypes()); err != nil {
 				http.Error(w, fmt.Sprintf("dosomework: %v", err), http.StatusInternalServerError)
 			}
 			buf.WriteTo(w)

cmd/coordinator/metrics.go

@@ -38,7 +38,7 @@ func reportMetrics(ctx context.Context) {
 func reportReverseCountMetrics(ctx context.Context) error {
 	m := metrics.ReverseCount
 	// 1. Gather # buildlets up per reverse builder type
-	totals := reversePool.hostTypeCount()
+	totals := pool.ReversePool().HostTypeCount()
 	// 2. Write counts to Stackdriver
 	ts := []*monpb.TimeSeries{}
 	now := ptypes.TimestampNow()

cmd/coordinator/remote.go

@@ -216,7 +216,7 @@ func handleBuildletCreate(w http.ResponseWriter, r *http.Request) {
 	// the higher priority gomote user.
 	isReverse := hconf.IsReverse
 	if isReverse {
-		if hs := reversePool.buildReverseStatusJSON().HostTypes[hconf.HostType]; hs == nil {
+		if hs := pool.ReversePool().BuildReverseStatusJSON().HostTypes[hconf.HostType]; hs == nil {
 			sendText(fmt.Sprintf("host type %q is not elastic; no machines are connected", hconf.HostType))
 		} else {
 			sendText(fmt.Sprintf("host type %q is not elastic; %d of %d machines connected, %d busy",

cmd/coordinator/status.go

@@ -410,15 +410,7 @@ func fetchMakeMacStatus() (errs, warns []string) {
 func hostTypeChecker(hostType string) func(cw *checkWriter) {
 	want := expectedHosts(hostType)
 	return func(cw *checkWriter) {
-		p := reversePool
-		p.mu.Lock()
-		defer p.mu.Unlock()
-		n := 0
-		for _, b := range p.buildlets {
-			if b.hostType == hostType {
-				n++
-			}
-		}
+		n := pool.ReversePool().SingleHostTypeCount(hostType)
 		if n < want {
 			cw.errorf("%d connected; want %d", n, want)
 		}
@@ -512,18 +504,22 @@ func reverseHostChecker(hosts []string) func(cw *checkWriter) {
 		hostSet[v] = true
 	}
 
+	// TODO(amedee): rethink how this is implemented. It has been
+	// modified due to golang.org/issues/36841
+	// instead of a single lock being held while all of the
+	// operations are performed, there is now a lock held
+	// durring each BuildletLastSeen call and again when
+	// the buildlet host names are retrieved.
 	return func(cw *checkWriter) {
-		p := reversePool
-		p.mu.Lock()
-		defer p.mu.Unlock()
+		p := pool.ReversePool()
 
 		now := time.Now()
 		wantGoodSince := now.Add(-recentThreshold)
 		numMissing := 0
 		numGood := 0
 		// Check last good times
 		for _, host := range hosts {
-			lastGood, ok := p.hostLastGood[host]
+			lastGood, ok := p.BuildletLastSeen(host)
 			if ok && lastGood.After(wantGoodSince) {
 				numGood++
 				continue
@@ -553,9 +549,9 @@ func reverseHostChecker(hosts []string) func(cw *checkWriter) {
 		// And check that we don't have more than 1
 		// connected of any type.
 		count := map[string]int{}
-		for _, b := range p.buildlets {
-			if hostSet[b.hostname] {
-				count[b.hostname]++
+		for _, hostname := range p.BuildletHostnames() {
+			if hostSet[hostname] {
+				count[hostname]++
 			}
 		}
 		for name, n := range count {
@@ -666,7 +662,7 @@ func handleStatus(w http.ResponseWriter, r *http.Request) {
 	data.KubePoolStatus = template.HTML(buf.String())
 	buf.Reset()
 
-	reversePool.WriteHTMLStatus(&buf)
+	pool.ReversePool().WriteHTMLStatus(&buf)
 	data.ReversePoolStatus = template.HTML(buf.String())
 
 	data.SchedState = sched.state()