Remote SSH connection support #1898

AWoloszyn · 2018-05-22T15:22:52Z

This is still somewhat in the experimental phase, it has no UI present yet, and can only be
enabled by command-line flags, but it adds functional remote SSH support.

This depends on both #1896 #1897 to be useful.

AWoloszyn · 2018-05-22T15:23:18Z

core/app/layout/layout.go

@@ -88,6 +90,10 @@ func withLibraryPlatformSuffix(lib string, os device.OSKind) string {
 	}
 }

+func GetLibraryName(lib LibraryType, abi *device.ABI) string {


TODO: Doc strings everywhere

ben-clayton · 2018-05-22T17:53:52Z

cmd/gapis/main.go

@@ -101,13 +105,31 @@ func run(ctx context.Context) error {
 		r.SetDeviceProperty(ctx, host, client.LaunchArgsKey, text.SplitArgs(*gapirArgStr))
 	}

-	deviceScanDone, onDeviceScanDone := task.NewSignal()
+	numAsyncScans := 0


More common to use a sync.WaitGroup:

wg := sync.WaitGroup{} if *scanAndroidDevs { wg.Add(1) crash.Go(func() { monitorAndroidDevices(ctx, r, wg.Done) }) } ... crash.Go(func(){ wg.Wait() onDeviceScanDone(ctx) })

ben-clayton · 2018-05-22T18:42:20Z

core/app/layout/layout.go

+func (l pkgLayout) Gapir(ctx context.Context, abi *device.ABI) (file.Path, error) {
+	if hostOS(ctx) == abi.OS {
+		return l.root.Join(withExecutablePlatformSuffix("gapir", hostOS(ctx))), nil
+	} else {


No else when the true branch always returns.

ben-clayton · 2018-05-22T18:42:43Z

core/app/layout/layout.go

+func (l pkgLayout) Library(ctx context.Context, lib LibraryType, abi *device.ABI) (file.Path, error) {
+	if hostOS(ctx) == abi.OS {
+		return l.root.Join("lib", withLibraryPlatformSuffix(libTypeToName[lib], hostOS(ctx))), nil
+	} else {


ben-clayton · 2018-05-22T18:42:56Z

core/app/layout/layout.go

+func (l pkgLayout) DeviceInfo(ctx context.Context, os device.OSKind) (file.Path, error) {
+	if hostOS(ctx) == os {
+		return l.root.Join(withExecutablePlatformSuffix("device_info", os)), nil
+	} else {


ben-clayton · 2018-05-22T18:43:24Z

core/app/layout/layout.go

+func (l *ZipLayout) Gapir(ctx context.Context, abi *device.ABI) (*zip.File, error) {
+	if l.os == abi.OS {
+		return l.file(withExecutablePlatformSuffix("gapir", l.os))
+	} else {


ben-clayton · 2018-05-22T19:31:43Z

core/vulkan/loader/loader.go

 	if err != nil {
-		return func() {}, err
+		return func(ctx context.Context) {}, err


I think it would be better to call cleanup here and return nil, err.

ben-clayton · 2018-05-22T19:33:11Z

core/vulkan/loader/loader.go

 	}
-	return setupJSON(lib, json, env)
+
+	err = setupJSON(ctx, lib, json, setup, tempdir, env)


Again, I think it would be better to check err, and if not nil, call cleanup and return nil, err.

ben-clayton · 2018-05-22T19:33:24Z

core/vulkan/loader/loader.go

-	return setupJSON(lib, json, env)
+
+	err = setupJSON(ctx, lib, json, setup, tempdir, env)
+	return cleanup, err


Then this becomes return cleanup, nil

ben-clayton · 2018-05-22T19:34:12Z

core/vulkan/loader/loader.go

 }

-func findLibraryAndJSON(ctx context.Context, libType layout.LibraryType) (file.Path, file.Path, error) {
-	lib, err := layout.Library(ctx, libType)
+func findLibraryAndJSON(ctx context.Context, rs DeviceReplaySetup, tempdir string, libType layout.LibraryType) (string, file.Path, error) {


Although this is private, would be great to document the return values.

ben-clayton · 2018-05-22T19:35:31Z

gapir/client/session.go

-		err = s.newHost(ctx, d, launchArgs)
-	} else if d, ok := d.(adb.Device); ok {
-		err = s.newADB(ctx, d, abi)
+		err = s.newHost(ctx, d, abi, launchArgs)


Again, it's a shame with the device abstractions that host and remote can't share the same code.

Agreed.
I think I might be able to clean this up a bit. I wanted to see how the gapit changes shook out before I tried unifying things like this. Since they will be similar in nature.

This will print the device information on stdout, as either json or a binary protobuf.

This allows an ssh configuration file to be created, and used to detect a remote SSH device.

We were writing both stdout and stderr to the same buffer in a very thread-unsafe way.

ben-clayton

There's a few comments I've made that don't seem to have been addressed. Please can you mark those you've done with a comment so I know what's still being looked at vs got skipped?

ben-clayton · 2018-05-23T14:31:56Z

cmd/gapis/main.go

@@ -142,6 +162,21 @@ func monitorAndroidDevices(ctx context.Context, r *bind.Registry, onDeviceScanDo
 	}
 }

+func getRemoteSSHDevices(ctx context.Context, r *bind.Registry, f io.Reader, scanDone func()) {
+	// Populate the registry with all the existing devices.
+	func() {


What's the point of this func?

The function was originally longer, so it was for the defer(), but no longer needed.

ben-clayton · 2018-05-23T14:33:06Z

core/app/layout/layout.go

@@ -88,6 +90,11 @@ func withLibraryPlatformSuffix(lib string, os device.OSKind) string {
 	}
 }

+// GetLibraryName returns the filename of the given Library.
+func GetLibraryName(lib LibraryType, abi *device.ABI) string {


nit: drop Get unless it collides with something else.

ben-clayton · 2018-05-23T14:43:16Z

core/os/device/remotessh/commands.go

+// remoteProcess is the interface to a running process, as started by a Target.
+type remoteProcess struct {
+	session    *ssh.Session
+	wg		   sync.WaitGroup


ben-clayton · 2018-05-23T14:44:03Z

core/os/device/remotessh/commands.go

+		if err != nil {
+			return nil, err
+		}
+		go func() {


ben-clayton · 2018-05-23T14:44:13Z

core/os/device/remotessh/commands.go

+	}
+	p := &remoteProcess{
+		session:    session,
+		wg:    sync.WaitGroup{},


ben-clayton · 2018-05-23T14:56:17Z

core/os/device/remotessh/commands.go

+			if err != nil {
+				return
+			}
+			b.doTunnel(ctx, local, remotePort)


Should you be leaving this loop if doTunnel returns an error?

Likely yes thanks. If we can't connect once, its not likely the next attempt will work.

ben-clayton · 2018-05-23T15:14:21Z

core/os/device/remotessh/commands.go

+	if err != nil {
+		return 0, err
+	}
+	crash.Go( func() {


You probably want to spin up (yet) another go-routine to monitor the cancellation of ctx.

crash.Go(func() { <-task.ShouldStop(ctx) listener.Close() })

Alternatively:

type TimeoutErr interface { Timeout() bool } for !task.Stopped(ctx) { listener.(*net.TCPListener).SetDeadline(time.Now().Add(time.Second)) local, err := listener.Accept() if err != nil { if t, ok := err.(TimeoutErr); ok && t.Timeout() { continue } break // not a timeout } b.doTunnel(ctx, local, remotePort) }

ben-clayton · 2018-05-23T15:17:27Z

core/os/device/remotessh/configuration.go

+// UnmarshalJSON is used by json.Unmashall, this allows us to set
+// up a default configuration, so that unknown parameters have
+// sane defaults.
+func (c *Configuration) UnmarshalJSON(data []byte) error {


Please see my previous comments here.

Yeah, the reason is very much for default values.
I was hoping to make sane defaults, but it seemed less clean (to me at least) to have a bunch of

func (c Configuration) Port() int { if Port == 0 { return 22; } }

For every parameter.

okay, fair enough

ben-clayton · 2018-05-23T15:19:02Z

core/os/device/remotessh/device.go

+	WriteFile(ctx context.Context, contents io.Reader, mode string, destPath string) error
+	// ForwardPort forwards the remote port. It automatically selects an open
+	// local port.
+	ForwardPort(ctx context.Context, remoteport int) (int, error)


This is actually a more convenient API than the Forward which was shared with ADB. I do wonder if we should have a common interface here (by adopting this new interface).

ben-clayton · 2018-05-23T15:21:08Z

core/vulkan/loader/loader.go

+type deviceReplaySetup interface {
+	// MakeTempDir returns a path to a created temporary. The returned function
+	// can be called to clean up the temporary directory.
+	MakeTempDir(ctx context.Context) (string, func(ctx context.Context), error)


Public methods in a private interface only really makes sense if the implementations are on public types.

ben-clayton · 2018-05-23T16:39:54Z

core/os/device/remotessh/commands.go

+				return
+			}
+			if err = b.doTunnel(ctx, local, remotePort); err != nil {
+				break


nit: Inconsistent mix of return and break

ben-clayton · 2018-05-23T16:40:22Z

core/os/device/remotessh/configuration.go

+	KnownHosts string
+}
+
+// UnmarshalJSON is used by json.Unmashall, this allows us to set


nit: Unmarshal

ben-clayton · 2018-05-23T16:44:40Z

core/os/device/remotessh/configuration.go

+// ReadConfiguration reads a set of configurations from then
+// given reader, and returns the configurations to the user.
+func ReadConfiguration(r io.Reader) ([]Configuration, error) {
+	cfg := []Configuration{}


cfgs := []Configuration{} d := json.Decoder(r) for d.More() { cfg := { User: u.Username, Port: 22, Keyfile: u.HomeDir + "/.ssh/id_rsa", KnownHosts: u.HomeDir + "/.ssh/known_hosts", } if err := d.Decode(&cfg); err != nil { return nil, err } cfgs = append(cfgs, cfg) } return cfgs, nil

ben-clayton

Perfect, thank you for all the changes!

AWoloszyn · 2018-05-23T18:26:10Z

Thanks for the careful reviews!

AWoloszyn requested review from Qining, ben-clayton and pmuetschard May 22, 2018 15:22

AWoloszyn commented May 22, 2018

View reviewed changes

ben-clayton reviewed May 22, 2018

View reviewed changes

AWoloszyn added 5 commits May 23, 2018 09:48

Added device_info executable.

bd4ef25

This will print the device information on stdout, as either json or a binary protobuf.

Added Experimental Remote SSH replay support.

078fb8c

This allows an ssh configuration file to be created, and used to detect a remote SSH device.

Hide the remoteSSHConfig flag by default.

2bd9dc0

A few minor cleanups and some additional documentation.

ca133e3

Fixes based on feedback.

bd0fe70

AWoloszyn force-pushed the remote-ssh branch from c03d352 to bd0fe70 Compare May 23, 2018 14:02

AWoloszyn added 2 commits May 23, 2018 11:06

More fixes based on comments.

8d8cbb2

Fixed a bug with the command output.

d4467ba

We were writing both stdout and stderr to the same buffer in a very thread-unsafe way.

ben-clayton reviewed May 23, 2018

View reviewed changes

More updates for comments

f999dd9

ben-clayton reviewed May 23, 2018

View reviewed changes

More comment updates.

29181fc

ben-clayton approved these changes May 23, 2018

View reviewed changes

Fix to windows header includes

3d2362d

AWoloszyn merged commit db72e9f into google:master May 23, 2018

AWoloszyn deleted the remote-ssh branch May 23, 2018 18:40

Remote SSH connection support #1898

Remote SSH connection support #1898

Conversation

AWoloszyn commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-clayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ben-clayton left a comment

Choose a reason for hiding this comment

AWoloszyn commented May 23, 2018