cmd/buildlet, cmd/coordinator: delete old --reverse mode

All the buildlets have been updated to use --reverse-type with a host
type instead of a builder type.

Fixes golang/go#21260

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

Author: bradfitz

cmd/buildlet/buildlet.go

@@ -50,7 +50,6 @@ var (
 	rebootOnHalt = flag.Bool("reboot", false, "reboot system in /halt handler.")
 	workDir      = flag.String("workdir", "", "Temporary directory to use. The contents of this directory may be deleted at any time. If empty, TempDir is used to create one.")
 	listenAddr   = flag.String("listen", "AUTO", "address to listen on. Unused in reverse mode. Warning: this service is inherently insecure and offers no protection of its own. Do not expose this port to the world.")
-	reverse      = flag.String("reverse", "", "[deprecated; use --reverse-type instead] if non-empty, go into reverse mode where the buildlet dials the coordinator instead of listening for connections. The value is a comma-separated list of modes, e.g. 'darwin-arm,darwin-amd64-race'")
 	reverseType  = flag.String("reverse-type", "", "if non-empty, go into reverse mode where the buildlet dials the coordinator instead of listening for connections. The value is the dashboard/builders.go Hosts map key, naming a HostConfig. This buildlet will receive work for any BuildConfig specifying this named HostConfig.")
 	coordinator  = flag.String("coordinator", "localhost:8119", "address of coordinator, in production use farmer.golang.org. Only used in reverse mode.")
 	hostname     = flag.String("hostname", "", "hostname to advertise to coordinator for reverse mode; default is actual hostname")
@@ -157,10 +156,7 @@ func main() {
 		log.Printf("set OS rlimits.")
 	}
 
-	if *reverse != "" && *reverseType != "" {
-		log.Fatalf("can't specify both --reverse and --reverse-type")
-	}
-	isReverse := *reverse != "" || *reverseType != ""
+	isReverse := *reverseType != ""
 
 	if *listenAddr == "AUTO" && !isReverse {
 		v := defaultListenAddr()

cmd/buildlet/reverse.go

@@ -49,13 +49,6 @@ func keyForMode(mode string) (string, error) {
 		os.Remove(keyPath)
 	}
 	if err != nil {
-		if os.IsNotExist(err) && *reverse != "" && !strings.Contains(*reverse, ",") {
-			globalKeyPath := filepath.Join(homedir(), ".gobuildkey")
-			key, err = ioutil.ReadFile(globalKeyPath)
-			if err != nil {
-				return "", fmt.Errorf("cannot read either key file %q or %q: %v", keyPath, globalKeyPath, err)
-			}
-		}
 		if len(key) == 0 || err != nil {
 			return "", fmt.Errorf("cannot read key file %q: %v", keyPath, err)
 		}
@@ -77,24 +70,9 @@ func dialCoordinator() error {
 		}
 	}
 
-	var modes, keys []string
-	if *reverse != "" {
-		// Old way.
-		modes = strings.Split(*reverse, ",")
-		for _, m := range modes {
-			key, err := keyForMode(m)
-			if err != nil {
-				log.Fatalf("failed to find key for %s: %v", m, err)
-			}
-			keys = append(keys, key)
-		}
-	} else {
-		// New way.
-		key, err := keyForMode(*reverseType)
-		if err != nil {
-			log.Fatalf("failed to find key for %s: %v", *reverseType, err)
-		}
-		keys = append(keys, key)
+	key, err := keyForMode(*reverseType)
+	if err != nil {
+		log.Fatalf("failed to find key for %s: %v", *reverseType, err)
 	}
 
 	addr := *coordinator
@@ -138,13 +116,8 @@ func dialCoordinator() error {
 	if err != nil {
 		log.Fatal(err)
 	}
-	if *reverse != "" {
-		// Old way.
-		req.Header["X-Go-Builder-Type"] = modes
-	} else {
-		req.Header.Set("X-Go-Host-Type", *reverseType)
-	}
-	req.Header["X-Go-Builder-Key"] = keys
+	req.Header.Set("X-Go-Host-Type", *reverseType)
+	req.Header.Set("X-Go-Builder-Key", key)
 	req.Header.Set("X-Go-Builder-Hostname", *hostname)
 	req.Header.Set("X-Go-Builder-Version", strconv.Itoa(buildletVersion))
 	req.Header.Set("X-Revdial-Version", "2")

cmd/coordinator/reverse.go

@@ -541,14 +541,14 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "buildlet registration requires SSL", http.StatusInternalServerError)
 		return
 	}
-	// Check build keys.
 
-	// modes can be either 1 buildlet type (new way) or builder mode(s) (the old way)
-	hostType := r.Header.Get("X-Go-Host-Type")
-	modes := r.Header["X-Go-Builder-Type"] // old way
-	gobuildkeys := r.Header["X-Go-Builder-Key"]
-	buildletVersion := r.Header.Get("X-Go-Builder-Version")
-	revDialVersion := r.Header.Get("X-Revdial-Version")
+	var (
+		hostType        = r.Header.Get("X-Go-Host-Type")
+		buildKey        = r.Header.Get("X-Go-Builder-Key")
+		buildletVersion = r.Header.Get("X-Go-Builder-Version")
+		revDialVersion  = r.Header.Get("X-Revdial-Version")
+		hostname        = r.Header.Get("X-Go-Builder-Hostname")
+	)
 
 	switch revDialVersion {
 	case "":
@@ -561,34 +561,14 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Convert the new argument style (X-Go-Host-Type) into the
-	// old way, to minimize changes in the rest of this code.
+	// Check build keys.
 	if hostType != "" {
-		if len(modes) > 0 {
-			http.Error(w, "invalid mix of X-Go-Host-Type and X-Go-Builder-Type", http.StatusBadRequest)
-			return
-		}
-		modes = []string{hostType}
-	}
-	if len(modes) == 0 || len(modes) != len(gobuildkeys) {
-		http.Error(w, fmt.Sprintf("need at least one mode and matching key, got %d/%d", len(modes), len(gobuildkeys)), http.StatusPreconditionFailed)
+		http.Error(w, "missing X-Go-Host-Type; old buildlet binary?", http.StatusBadRequest)
 		return
 	}
-	hostname := r.Header.Get("X-Go-Builder-Hostname")
-
-	for i, m := range modes {
-		if gobuildkeys[i] != builderKey(m) {
-			http.Error(w, fmt.Sprintf("bad key for mode %q", m), http.StatusPreconditionFailed)
-			return
-		}
-	}
-
-	// For older builders using the buildlet's -reverse flag only,
-	// collapse their builder modes down into a singular hostType.
-	legacyNote := ""
-	if hostType == "" {
-		hostType = mapBuilderToHostType(modes)
-		legacyNote = fmt.Sprintf(" (mapped from legacy modes %q)", modes)
+	if buildKey != builderKey(hostType) {
+		http.Error(w, "invalid build key", http.StatusPreconditionFailed)
+		return
 	}
 
 	conn, bufrw, err := w.(http.Hijacker).Hijack()
@@ -603,8 +583,8 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	log.Printf("Registering reverse buildlet %q (%s) for host type %v %s; buildletVersion=%v; revDialVersion=%v",
-		hostname, r.RemoteAddr, hostType, legacyNote, buildletVersion, revDialVersion)
+	log.Printf("Registering reverse buildlet %q (%s) for host type %v; buildletVersion=%v; revDialVersion=%v",
+		hostname, r.RemoteAddr, hostType, buildletVersion, revDialVersion)
 
 	var dialer func(context.Context) (net.Conn, error)
 	var revDialerDone <-chan struct{}
@@ -659,8 +639,8 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 	tstatus := time.Now()
 	status, err := client.Status()
 	if err != nil {
-		log.Printf("Reverse connection %s/%s for modes %v did not answer status after %v: %v",
-			hostname, r.RemoteAddr, modes, time.Since(tstatus), err)
+		log.Printf("Reverse connection %s/%s for %s did not answer status after %v: %v",
+			hostname, r.RemoteAddr, hostType, time.Since(tstatus), err)
 		conn.Close()
 		return
 	}
@@ -669,7 +649,7 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 		conn.Close()
 		return
 	}
-	log.Printf("Buildlet %s/%s: %+v for %s", hostname, r.RemoteAddr, status, modes)
+	log.Printf("Buildlet %s/%s: %+v for %s", hostname, r.RemoteAddr, status, hostType)
 
 	now := time.Now()
 	b := &reverseBuildlet{
@@ -683,11 +663,8 @@ func handleReverse(w http.ResponseWriter, r *http.Request) {
 		regTime:      now,
 	}
 	reversePool.addBuildlet(b)
-	registerBuildlet(modes) // testing only
 }
 
-var registerBuildlet = func(modes []string) {} // test hook
-
 type byTypeThenHostname []*reverseBuildlet
 
 func (s byTypeThenHostname) Len() int      { return len(s) }
@@ -700,29 +677,3 @@ func (s byTypeThenHostname) Less(i, j int) bool {
 	}
 	return ti < tj
 }
-
-// mapBuilderToHostType maps from the user's Request.Header["X-Go-Builder-Type"]
-// mode list down into a single host type, or the empty string if unknown.
-func mapBuilderToHostType(modes []string) string {
-	// First, see if any of the provided modes are a host type.
-	// If so, this is an updated client.
-	for _, v := range modes {
-		if _, ok := dashboard.Hosts[v]; ok {
-			return v
-		}
-	}
-
-	// Else, it's an old client, still speaking in terms of
-	// builder names.  See if any are registered aliases. First
-	// one wins. (There are no ambiguities in the wild.)
-	for hostType, hconf := range dashboard.Hosts {
-		for _, alias := range hconf.ReverseAliases {
-			for _, v := range modes {
-				if v == alias {
-					return hostType
-				}
-			}
-		}
-	}
-	return ""
-}

dashboard/builders.go

@@ -200,15 +200,13 @@ var Hosts = map[string]*HostConfig{
 		HermeticReverse: true,
 		ExpectNum:       50,
 		env:             []string{"GOROOT_BOOTSTRAP=/usr/local/go"},
-		ReverseAliases:  []string{"linux-arm", "linux-arm-arm5"},
 		SSHUsername:     "root",
 	},
 	"host-linux-arm5spacemonkey": &HostConfig{
-		IsReverse:      true,
-		ExpectNum:      3,
-		env:            []string{"GOROOT_BOOTSTRAP=/usr/local/go"},
-		ReverseAliases: []string{"linux-arm-arm5spacemonkey"},
-		OwnerGithub:    "esnolte", // https://github.com/golang/go/issues/34973#issuecomment-543836871
+		IsReverse:   true,
+		ExpectNum:   3,
+		env:         []string{"GOROOT_BOOTSTRAP=/usr/local/go"},
+		OwnerGithub: "esnolte", // https://github.com/golang/go/issues/34973#issuecomment-543836871
 	},
 	"host-openbsd-amd64-60": &HostConfig{
 		VMImage:            "openbsd-amd64-60",
@@ -357,12 +355,11 @@ var Hosts = map[string]*HostConfig{
 		OwnerGithub: "tuxillo",
 	},
 	"host-freebsd-arm-paulzhol": &HostConfig{
-		IsReverse:      true,
-		ExpectNum:      1,
-		Notes:          "Cubiboard2 1Gb RAM dual-core Cortex-A7 (Allwinner A20), FreeBSD 11.1-RELEASE",
-		env:            []string{"GOROOT_BOOTSTRAP=/usr/home/paulzhol/go1.4"},
-		ReverseAliases: []string{"freebsd-arm-paulzhol"},
-		OwnerGithub:    "paulzhol",
+		IsReverse:   true,
+		ExpectNum:   1,
+		Notes:       "Cubiboard2 1Gb RAM dual-core Cortex-A7 (Allwinner A20), FreeBSD 11.1-RELEASE",
+		env:         []string{"GOROOT_BOOTSTRAP=/usr/home/paulzhol/go1.4"},
+		OwnerGithub: "paulzhol",
 	},
 	"host-freebsd-arm64-dmgk": &HostConfig{
 		IsReverse:   true,
@@ -452,7 +449,6 @@ var Hosts = map[string]*HostConfig{
 		env: []string{
 			"GOROOT_BOOTSTRAP=/Users/gopher/go1.4",
 		},
-		ReverseAliases:  []string{"darwin-amd64-10_8"},
 		SSHUsername:     "gopher",
 		HermeticReverse: false, // TODO: make it so, like 10.12
 	},
@@ -463,7 +459,6 @@ var Hosts = map[string]*HostConfig{
 		env: []string{
 			"GOROOT_BOOTSTRAP=/Users/gopher/go1.4",
 		},
-		ReverseAliases:  []string{"darwin-amd64-10_10"},
 		SSHUsername:     "gopher",
 		HermeticReverse: false, // TODO: make it so, like 10.12
 	},
@@ -474,7 +469,6 @@ var Hosts = map[string]*HostConfig{
 		env: []string{
 			"GOROOT_BOOTSTRAP=/Users/gopher/go1.4",
 		},
-		ReverseAliases:  []string{"darwin-amd64-10_11"},
 		SSHUsername:     "gopher",
 		HermeticReverse: false, // TODO: make it so, like 10.12
 	},
@@ -485,7 +479,6 @@ var Hosts = map[string]*HostConfig{
 		env: []string{
 			"GOROOT_BOOTSTRAP=/Users/gopher/go1.4",
 		},
-		ReverseAliases:  []string{"darwin-amd64-10_12"},
 		SSHUsername:     "gopher",
 		HermeticReverse: true, // we destroy the VM when done & let cmd/makemac recreate
 	},
@@ -500,18 +493,16 @@ var Hosts = map[string]*HostConfig{
 		HermeticReverse: true, // we destroy the VM when done & let cmd/makemac recreate
 	},
 	"host-linux-s390x": &HostConfig{
-		Notes:          "run by IBM",
-		OwnerGithub:    "mundaym",
-		IsReverse:      true,
-		env:            []string{"GOROOT_BOOTSTRAP=/var/buildlet/go-linux-s390x-bootstrap"},
-		ReverseAliases: []string{"linux-s390x-ibm"},
+		Notes:       "run by IBM",
+		OwnerGithub: "mundaym",
+		IsReverse:   true,
+		env:         []string{"GOROOT_BOOTSTRAP=/var/buildlet/go-linux-s390x-bootstrap"},
 	},
 	"host-linux-ppc64-osu": &HostConfig{
 		Notes:           "Debian jessie; run by Go team on osuosl.org",
 		IsReverse:       true,
 		ExpectNum:       5,
 		env:             []string{"GOROOT_BOOTSTRAP=/usr/local/go-bootstrap"},
-		ReverseAliases:  []string{"linux-ppc64-buildlet"},
 		SSHUsername:     "root",
 		HermeticReverse: false, // TODO: run in chroots with overlayfs? https://github.com/golang/go/issues/34830#issuecomment-543386764
 	},
@@ -723,14 +714,6 @@ type HostConfig struct {
 	Notes       string // notes for humans
 
 	SSHUsername string // username to ssh as, empty means not supported
-
-	// ReverseAliases lists alternate names for this buildlet
-	// config, for older clients doing a reverse dial into the
-	// coordinator from outside. This prevents us from updating
-	// 75+ dedicated machines/VMs atomically, switching them to
-	// the new "host-*" names.
-	// This is only applicable if IsReverse.
-	ReverseAliases []string
 }
 
 // A BuildConfig describes how to run a builder.