dashboard: move buildlet exec code from coordinator to client package

Change-Id: I778ac78ed02be9f67436ec045a3816dfc24afda3
Reviewed-on: https://go-review.googlesource.com/2923
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

dashboard/builders.go

@@ -47,6 +47,21 @@ func (c *BuildConfig) GOARCH() string {
 	return arch[:i]
 }
 
+// AllScript returns the relative path to the operating system's script to
+// do the build and run its standard set of tests.
+// Example values are "src/all.bash", "src/all.bat", "src/all.rc".
+func (c *BuildConfig) AllScript() string {
+	if strings.HasPrefix(c.Name, "windows-") {
+		return "src/all.bat"
+	}
+	if strings.HasPrefix(c.Name, "plan9-") {
+		return "src/all.rc"
+	}
+	// TODO(bradfitz): race.bash, etc, once the race builder runs
+	// via the buildlet.
+	return "src/all.bash"
+}
+
 func (c *BuildConfig) UsesDocker() bool { return c.VMImage == "" }
 func (c *BuildConfig) UsesVM() bool     { return c.VMImage != "" }
 

dashboard/buildlet/buildletclient.go

@@ -9,10 +9,12 @@
 package buildlet // import "golang.org/x/tools/dashboard/buildlet"
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
 	"net/http"
+	"net/url"
 	"strings"
 )
 
@@ -54,6 +56,8 @@ func (c *Client) URL() string {
 	return "https://" + strings.TrimSuffix(c.ipPort, ":443")
 }
 
+// PutTarball writes files to the remote buildlet.
+// The Reader must be of a tar.gz file.
 func (c *Client) PutTarball(r io.Reader) error {
 	req, err := http.NewRequest("PUT", c.URL()+"/writetgz", r)
 	if err != nil {
@@ -70,3 +74,64 @@ func (c *Client) PutTarball(r io.Reader) error {
 	}
 	return nil
 }
+
+// ExecOpts are options for a remote command invocation.
+type ExecOpts struct {
+	// Output is the output of stdout and stderr.
+	// If nil, the output is discarded.
+	Output io.Writer
+
+	// OnStartExec is an optional hook that runs after the 200 OK
+	// response from the buildlet, but before the output begins
+	// writing to Output.
+	OnStartExec func()
+}
+
+// Exec runs cmd on the buildlet.
+//
+// Two errors are returned: one is whether the command succeeded
+// remotely (remoteErr), and the second (execErr) is whether there
+// were system errors preventing the command from being started or
+// seen to completition. If execErr is non-nil, the remoteErr is
+// meaningless.
+func (c *Client) Exec(cmd string, opts ExecOpts) (remoteErr, execErr error) {
+	res, err := http.PostForm(c.URL()+"/exec", url.Values{"cmd": {cmd}})
+	if err != nil {
+		return nil, err
+	}
+	defer res.Body.Close()
+	if res.StatusCode != http.StatusOK {
+		slurp, _ := ioutil.ReadAll(io.LimitReader(res.Body, 4<<10))
+		return nil, fmt.Errorf("buildlet: HTTP status %v: %s", res.Status, slurp)
+	}
+	condRun(opts.OnStartExec)
+
+	// Stream the output:
+	out := opts.Output
+	if out == nil {
+		out = ioutil.Discard
+	}
+	if _, err := io.Copy(out, res.Body); err != nil {
+		return nil, fmt.Errorf("error copying response: %v", err)
+	}
+
+	// Don't record to the dashboard unless we heard the trailer from
+	// the buildlet, otherwise it was probably some unrelated error
+	// (like the VM being killed, or the buildlet crashing due to
+	// e.g. https://golang.org/issue/9309, since we require a tip
+	// build of the buildlet to get Trailers support)
+	state := res.Trailer.Get("Process-State")
+	if state == "" {
+		return nil, errors.New("missing Process-State trailer from HTTP response; buildlet built with old (<= 1.4) Go?")
+	}
+	if state != "ok" {
+		return errors.New(state), nil
+	}
+	return nil, nil
+}
+
+func condRun(fn func()) {
+	if fn != nil {
+		fn()
+	}
+}

dashboard/buildlet/gce.go

@@ -19,6 +19,7 @@ import (
 	"google.golang.org/api/compute/v1"
 )
 
+// VMOpts control how new VMs are started.
 type VMOpts struct {
 	// Zone is the GCE zone to create the VM in. Required.
 	Zone string
@@ -149,9 +150,7 @@ func StartNewVM(ts oauth2.TokenSource, instName, builderType string, opts VMOpts
 	if err != nil {
 		return nil, fmt.Errorf("Failed to create instance: %v", err)
 	}
-	if fn := opts.OnInstanceRequested; fn != nil {
-		fn()
-	}
+	condRun(opts.OnInstanceRequested)
 	createOp := op.Name
 
 	// Wait for instance create operation to succeed.
@@ -177,9 +176,7 @@ OpLoop:
 			return nil, fmt.Errorf("Unknown create status %q: %+v", op.Status, op)
 		}
 	}
-	if fn := opts.OnInstanceCreated; fn != nil {
-		fn()
-	}
+	condRun(opts.OnInstanceCreated)
 
 	inst, err := computeService.Instances.Get(projectID, zone, instName).Do()
 	if err != nil {
@@ -207,9 +204,7 @@ OpLoop:
 		buildletURL = "http://" + ip
 		ipPort = ip + ":80"
 	}
-	if fn := opts.OnGotInstanceInfo; fn != nil {
-		fn()
-	}
+	condRun(opts.OnGotInstanceInfo)
 
 	const timeout = 90 * time.Second
 	var alive bool

dashboard/cmd/coordinator/coordinator.go

@@ -769,47 +769,29 @@ func buildInVM(conf dashboard.BuildConfig, st *buildStatus) (retErr error) {
 	}
 	st.logEventTime("end_write_tar")
 
-	// TODO(bradfitz): add an Exec method to buildlet.Client and update this code.
-	// Run the builder
-	cmd := "all.bash"
-	if strings.HasPrefix(st.name, "windows-") {
-		cmd = "all.bat"
-	} else if strings.HasPrefix(st.name, "plan9-") {
-		cmd = "all.rc"
-	}
 	execStartTime := time.Now()
-	st.logEventTime("start_exec")
-	res, err := http.PostForm(bc.URL()+"/exec", url.Values{"cmd": {"src/" + cmd}})
-	if !goodRes(res, err, "running "+cmd) {
-		return
-	}
-	defer res.Body.Close()
-	st.logEventTime("running_exec")
-	// Stream the output:
-	if _, err := io.Copy(st, res.Body); err != nil {
-		return fmt.Errorf("error copying response: %v", err)
+	st.logEventTime("pre_exec")
+
+	remoteErr, err := bc.Exec(conf.AllScript(), buildlet.ExecOpts{
+		Output:      st,
+		OnStartExec: func() { st.logEventTime("running_exec") },
+	})
+	if err != nil {
+		return err
 	}
 	st.logEventTime("done")
-
-	// Don't record to the dashboard unless we heard the trailer from
-	// the buildlet, otherwise it was probably some unrelated error
-	// (like the VM being killed, or the buildlet crashing due to
-	// e.g. https://golang.org/issue/9309, since we require a tip
-	// build of the buildlet to get Trailers support)
-	state := res.Trailer.Get("Process-State")
-	if state == "" {
-		return errors.New("missing Process-State trailer from HTTP response; buildlet built with old (<= 1.4) Go?")
-	}
-
 	var log string
-	if state != "ok" {
+	if remoteErr != nil {
 		log = st.logs()
 	}
-	if err := recordResult(st.name, state == "ok", st.rev, log, time.Since(execStartTime)); err != nil {
-		return fmt.Errorf("Status was %q but failed to report it to the dashboard: %v", state, err)
+	if err := recordResult(st.name, remoteErr == nil, st.rev, log, time.Since(execStartTime)); err != nil {
+		if remoteErr != nil {
+			return fmt.Errorf("Remote error was %q but failed to report it to the dashboard: %v", remoteErr, err)
+		}
+		return fmt.Errorf("Build succeeded but failed to report it to the dashboard: %v", err)
 	}
-	if state != "ok" {
-		return fmt.Errorf("%s failed: %v", cmd, state)
+	if remoteErr != nil {
+		return fmt.Errorf("%s failed: %v", conf.AllScript(), remoteErr)
 	}
 	return nil
 }
@@ -1081,10 +1063,6 @@ func cleanZoneVMs(zone string) error {
 		return fmt.Errorf("listing instances: %v", err)
 	}
 	for _, inst := range list.Items {
-		if !strings.HasPrefix(inst.Name, "buildlet-") {
-			// We only delete ones we created.
-			continue
-		}
 		if inst.Metadata == nil {
 			// Defensive. Not seen in practice.
 			continue
@@ -1103,7 +1081,12 @@ func cleanZoneVMs(zone string) error {
 				}
 			}
 		}
-		if sawDeleteAt && !vmIsBuilding(inst.Name) {
+		// Delete buildlets (things we made) from previous
+		// generations.  Thenaming restriction (buildlet-*)
+		// prevents us from deleting buildlet VMs used by
+		// Gophers for interactive development & debugging
+		// (non-builder users); those are named "mote-*".
+		if sawDeleteAt && strings.HasPrefix(inst.Name, "buildlet-") && !vmIsBuilding(inst.Name) {
 			log.Printf("Deleting VM %q in zone %q from an earlier coordinator generation ...", inst.Name, zone)
 			deleteVM(zone, inst.Name)
 		}