buildlet: create gRPC client

As a first step to port relui over to the gRPC API, create a gRPC client
implementation. This should be suitable for use in the gomote command,
but I don't want to conflict with the giant stack in flight. Also add a
GRPCClient function to iapclient for convenience.

The gomote service was missing working directory information; added it.

I also did some refactoring/cleanup.
- The Client interface is used internally to the coordinator, and as
  such it has a lot of internal functions. The set used by remote
  clients is much more reasonable. Separate it out into a new
  RemoteClient interface.
- AddCloseFn was only used in one place. Delete it.
- DestroyVM was completely unused. Delete it.

For golang/go#54344.

Change-Id: I77344a8c27076e53f565e9af56f8d365ac89b487
Reviewed-on: https://go-review.googlesource.com/c/build/+/422095
Auto-Submit: Heschi Kreinick <[email protected]>
Reviewed-by: Carlos Amedee <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>

Author: heschi

buildlet/buildletclient.go

@@ -20,8 +20,6 @@ import (
 	"strings"
 	"sync"
 	"time"
-
-	"golang.org/x/oauth2"
 )
 
 var _ Client = (*client)(nil)
@@ -646,51 +644,6 @@ func (c *client) RemoveAll(ctx context.Context, paths ...string) error {
 	return c.doOK(req.WithContext(ctx))
 }
 
-// DestroyVM shuts down the buildlet and destroys the VM instance.
-func (c *client) DestroyVM(ts oauth2.TokenSource, proj, zone, instance string) error {
-	// TODO(bradfitz): move GCE stuff out of this package?
-	gceErrc := make(chan error, 1)
-	buildletErrc := make(chan error, 1)
-	go func() {
-		gceErrc <- DestroyVM(ts, proj, zone, instance)
-	}()
-	go func() {
-		buildletErrc <- c.Close()
-	}()
-	timeout := time.NewTimer(5 * time.Second)
-	defer timeout.Stop()
-
-	var retErr error
-	var gceDone, buildletDone bool
-	for !gceDone || !buildletDone {
-		select {
-		case err := <-gceErrc:
-			if err != nil {
-				retErr = err
-			}
-			gceDone = true
-		case err := <-buildletErrc:
-			if err != nil {
-				retErr = err
-			}
-			buildletDone = true
-		case <-timeout.C:
-			e := ""
-			if !buildletDone {
-				e = "timeout asking buildlet to shut down"
-			}
-			if !gceDone {
-				if e != "" {
-					e += " and "
-				}
-				e += "timeout asking GCE to delete builder VM"
-			}
-			return errors.New(e)
-		}
-	}
-	return retErr
-}
-
 // Status provides status information about the buildlet.
 //
 // A coordinator can use the provided information to decide what, if anything,
@@ -907,11 +860,6 @@ func (c *client) ConnectSSH(user, authorizedPubKey string) (net.Conn, error) {
 	return conn, nil
 }
 
-// AddCloseFunc adds an optional extra code to run on close.
-func (c *client) AddCloseFunc(fn func()) {
-	c.closeFuncs = append(c.closeFuncs, fn)
-}
-
 func condRun(fn func()) {
 	if fn != nil {
 		fn()

buildlet/fakebuildletclient.go

@@ -13,33 +13,37 @@ import (
 	"net/http"
 	"os"
 	"strings"
-
-	"golang.org/x/oauth2"
 )
 
+// RemoteClient is a subset of methods that can be used by a gomote client.
+type RemoteClient interface {
+	Close() error
+	Exec(ctx context.Context, cmd string, opts ExecOpts) (remoteErr, execErr error)
+	GetTar(ctx context.Context, dir string) (io.ReadCloser, error)
+	ListDir(ctx context.Context, dir string, opts ListDirOpts, fn func(DirEntry)) error
+	Put(ctx context.Context, r io.Reader, path string, mode os.FileMode) error
+	PutTar(ctx context.Context, r io.Reader, dir string) error
+	PutTarFromURL(ctx context.Context, tarURL, dir string) error
+	ProxyTCP(port int) (io.ReadWriteCloser, error)
+	RemoteName() string
+	RemoveAll(ctx context.Context, paths ...string) error
+	WorkDir(ctx context.Context) (string, error)
+}
+
 // Client is an interface that represent the methods exposed by client. The
 // fake buildlet client should be used instead of client when testing things that
 // use the client interface.
+// This includes a number of coordinator-internal details; users outside the
+// coordinator should use RemoteClient.
 type Client interface {
-	AddCloseFunc(fn func())
-	Close() error
+	RemoteClient
 	ConnectSSH(user, authorizedPubKey string) (net.Conn, error)
-	DestroyVM(ts oauth2.TokenSource, proj, zone, instance string) error
-	Exec(ctx context.Context, cmd string, opts ExecOpts) (remoteErr, execErr error)
 	GCEInstanceName() string
-	GetTar(ctx context.Context, dir string) (io.ReadCloser, error)
 	IPPort() string
 	IsBroken() bool
-	ListDir(ctx context.Context, dir string, opts ListDirOpts, fn func(DirEntry)) error
 	MarkBroken()
 	Name() string
 	ProxyRoundTripper() http.RoundTripper
-	ProxyTCP(port int) (io.ReadWriteCloser, error)
-	Put(ctx context.Context, r io.Reader, path string, mode os.FileMode) error
-	PutTar(ctx context.Context, r io.Reader, dir string) error
-	PutTarFromURL(ctx context.Context, tarURL, dir string) error
-	RemoteName() string
-	RemoveAll(ctx context.Context, paths ...string) error
 	SetDescription(v string)
 	SetDialer(dialer func(context.Context) (net.Conn, error))
 	SetGCEInstanceName(v string)
@@ -49,7 +53,6 @@ type Client interface {
 	Status(ctx context.Context) (Status, error)
 	String() string
 	URL() string
-	WorkDir(ctx context.Context) (string, error)
 }
 
 var errUnimplemented = errors.New("unimplemented function")
@@ -63,11 +66,6 @@ type FakeClient struct {
 	name         string
 }
 
-// AddCloseFunc adds optional extra code to run on close for the fake buildlet.
-func (fc *FakeClient) AddCloseFunc(fn func()) {
-	fc.closeFuncs = append(fc.closeFuncs, fn)
-}
-
 // Close is a fake client closer.
 func (fc *FakeClient) Close() error {
 	for _, f := range fc.closeFuncs {
@@ -81,11 +79,6 @@ func (fc *FakeClient) ConnectSSH(user, authorizedPubKey string) (net.Conn, error
 	return nil, errUnimplemented
 }
 
-// DestroyVM destroys a fake VM.
-func (fc *FakeClient) DestroyVM(ts oauth2.TokenSource, proj, zone, instance string) error {
-	return errUnimplemented
-}
-
 // Exec fakes the execution.
 func (fc *FakeClient) Exec(ctx context.Context, cmd string, opts ExecOpts) (remoteErr, execErr error) {
 	if cmd == "" {
@@ -121,7 +114,7 @@ func (fc *FakeClient) IsBroken() bool { return false }
 // ListDir lists a directory on a fake buildlet.
 func (fc *FakeClient) ListDir(ctx context.Context, dir string, opts ListDirOpts, fn func(DirEntry)) error {
 	if dir == "" || fn == nil {
-		errors.New("invalid arguments")
+		return errors.New("invalid arguments")
 	}
 	var lsOutput = `drwxr-xr-x      gocache/
 drwxr-xr-x      tmp/`
@@ -148,7 +141,7 @@ func (fc *FakeClient) ProxyTCP(port int) (io.ReadWriteCloser, error) { return ni
 func (fc *FakeClient) Put(ctx context.Context, r io.Reader, path string, mode os.FileMode) error {
 	// TODO(go.dev/issue/48742) add a file system implementation which would enable proper testing.
 	if path == "" {
-		errors.New("invalid argument")
+		return errors.New("invalid argument")
 	}
 	return nil
 }
@@ -199,7 +192,9 @@ func (fc *FakeClient) String() string { return "" }
 func (fc *FakeClient) URL() string { return "" }
 
 // WorkDir is the working directory for the fake buildlet.
-func (fc *FakeClient) WorkDir(ctx context.Context) (string, error) { return "", errUnimplemented }
+func (fc *FakeClient) WorkDir(ctx context.Context) (string, error) {
+	return "/work", nil
+}
 
 // RemoveAll deletes the provided paths, relative to the work directory for a fake buildlet.
 func (fc *FakeClient) RemoveAll(ctx context.Context, paths ...string) error {

buildlet/gce.go

@@ -259,26 +259,37 @@ OpLoop:
 	if opts.OnGotInstanceInfo != nil {
 		opts.OnGotInstanceInfo(inst)
 	}
-	var closeFuncs []func()
+	var closeFunc func()
 	if opts.UseIAPTunnel {
-		localPort, closeFunc, err := createIAPTunnel(ctx, inst)
+		var localPort string
+		var err error
+		localPort, closeFunc, err = createIAPTunnel(ctx, inst)
 		if err != nil {
 			return nil, fmt.Errorf("creating IAP tunnel: %v", err)
 		}
 		buildletURL = "http://localhost:" + localPort
 		ipPort = "127.0.0.1:" + localPort
-		closeFuncs = append(closeFuncs, closeFunc)
 	}
 	client, err := buildletClient(ctx, buildletURL, ipPort, &opts)
 	if err != nil {
 		return nil, err
 	}
-	for _, cf := range closeFuncs {
-		client.AddCloseFunc(cf)
+	if closeFunc != nil {
+		return &extraCloseClient{client, closeFunc}, nil
 	}
 	return client, nil
 }
 
+type extraCloseClient struct {
+	Client
+	close func()
+}
+
+func (e *extraCloseClient) Close() error {
+	defer e.close()
+	return e.Close()
+}
+
 func createIAPTunnel(ctx context.Context, inst *compute.Instance) (string, func(), error) {
 	// Allocate a local listening port.
 	ln, err := net.Listen("tcp", "localhost:0")
@@ -350,16 +361,6 @@ func createIAPTunnel(ctx context.Context, inst *compute.Instance) (string, func(
 	return "", nil, fmt.Errorf("iap tunnel startup timed out")
 }
 
-// DestroyVM sends a request to delete a VM. Actual VM description is
-// currently (2015-01-19) very slow for no good reason. This function
-// returns once it's been requested, not when it's done.
-func DestroyVM(ts oauth2.TokenSource, proj, zone, instance string) error {
-	computeService, _ := compute.New(oauth2.NewClient(context.TODO(), ts))
-	apiGate()
-	_, err := computeService.Instances.Delete(proj, zone, instance).Do()
-	return err
-}
-
 type VM struct {
 	// Name is the name of the GCE VM instance.
 	// For example, it's of the form "mote-bradfitz-plan9-386-foo",