Adds console resize support. #84

jterry75 · 2017-07-31T23:05:50Z

Resolves: #76

msftclas · 2017-07-31T23:05:54Z

@jterry75,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

jterry75 · 2017-07-31T23:06:45Z

I have verified this through docker for both the init process and an exec process such as VI that the console resize is indeed happening.

jstarks · 2017-07-31T23:36:21Z

What happens if the TTY relay finishes and closes master just as the resize console call is coming down?

It seems like some synchronization might be missing there.

jterry75 · 2017-08-01T16:57:01Z

I certainly could add synchronization but I didn't for two reasons.

If the resize is right before a close it will succeed. If the resize is right after a close you will get an error from the ioctl call which is no different than returning an error "invalid process" or whatever from locking and detecting the case. IE: the kernel is already a form of synchronization.
What is the case where you have a TTY exit the process and then resize it? The TTY case as far as I know is only for interactive sessions in which I am not physically fast enough to exit the process while resizing it.

All that said I am happy to add a mutex on the relay that exposes the console handle. Let me know if you still want it.

jstarks · 2017-08-01T17:25:22Z

The kernel is not a safe form of synchronization. The fd can get reused right after the close call, so you will send the ioctl to the wrong place.

Why can't the relay implement the resize method? Then you can synchronize internally without exposing a mutex.

beweedon · 2017-08-01T18:25:01Z

service/gcs/core/gcs/gcs.go

-	if !ok {
-		entry, ok = c.externalProcessCache[pid]
-		if !ok {
+	if id == "" {


Do we actually expect the HCS to send an empty containerID string for external processes? I didn't know about this behavior, just making sure this is something we can definitely rely on.

ContainerID is not a required field. The HCS will not send any parameter and the unmarshal call from json will default "". I could be wrong but this seems safe.

beweedon · 2017-08-01T18:26:49Z

service/gcs/core/mockcore/mockcore.go

@@ -106,7 +115,7 @@ func (c *MockCore) SignalContainer(id string, signal oslayer.Signal) error {
 }

 // SignalProcess captures its arguments and returns a nil error.
-func (c *MockCore) SignalProcess(pid int, options prot.SignalProcessOptions) error {
+func (c *MockCore) SignalProcess(id string, pid int, options prot.SignalProcessOptions) error {
 	c.LastSignalProcess = SignalProcessCall{


Add ID field to SignalProcessCall

And update tests to match.

beweedon · 2017-08-01T18:27:01Z

service/gcs/core/mockcore/mockcore.go

@@ -159,11 +168,23 @@ func (c *MockCore) RegisterContainerExitHook(id string, exitHook func(oslayer.Pr

 // RegisterProcessExitHook captures its arguments, runs the given exit hook on
 // a process exit state with exit code 103, and returns a nil error.
-func (c *MockCore) RegisterProcessExitHook(pid int, exitHook func(oslayer.ProcessExitState)) error {
+func (c *MockCore) RegisterProcessExitHook(id string, pid int, exitHook func(oslayer.ProcessExitState)) error {


Add ID field to RegisterProcessExitHookCall

And update tests.

beweedon · 2017-08-01T18:29:29Z

service/gcs/runtime/runc/runc.go

@@ -528,8 +536,8 @@ func (c *container) startProcess(tempProcessDir string, hasTerminal bool, stdioS
 	}

 	var relay *stdio.TtyRelay
+	var master *os.File


I'm probably missing something, but why move this to a higher scope?

Was a remnant before we used the TtyRelay to do the work. Put back now.

beweedon · 2017-08-01T18:30:22Z

service/gcs/stdio/stdio.go


 	"github.com/Microsoft/opengcs/service/gcs/transport"
 	"github.com/Sirupsen/logrus"
 	"github.com/pkg/errors"
+
+	"golang.org/x/sys/unix"


Nit: To keep with the pattern we've started following (using goimports to perform formatting), this import should probably go with the above github.com imports.

jstarks · 2017-08-01T19:31:02Z

service/gcs/stdio/stdio.go

+		return errors.New("error resizing console pty is closed")
+	}
+
+	if _, _, err := syscall.Syscall(syscall.SYS_IOCTL, r.pty.Fd(), uintptr(unix.TIOCSWINSZ), uintptr(unsafe.Pointer(&consoleSize{Height: height, Width: width}))); err != 0 {


Syscall [](start = 25, length = 7)

Probably you should make a standalone function in tty.go that calls this IOCTL that can be used or tested separately from the relay.

jstarks · 2017-08-01T19:34:45Z

service/gcs/stdio/stdio.go

+	// small window that ResizeConsole could still get an invalid Fd. We call wait again to
+	// enusure that no ResizeConsole call came in before actualling closing the pty.
+	atomic.StoreInt32(&r.closing, 1)
+	r.wg.Wait()


wg [](start = 3, length = 2)

I think you need to use a separate mutex unfortunately; according to the docs, you can't legally increment the wait group once Wait has been called:

go doc sync.WaitGroup.Add
func (wg *WaitGroup) Add(delta int)
Add adds delta, which may be negative, to the WaitGroup counter. If the
counter becomes zero, all goroutines blocked on Wait are released. If the
counter goes negative, Add panics.

Note that calls with a positive delta that occur when the counter is zero must happen before a Wait. Calls with a negative delta, or calls with a positive delta that start when the counter is greater than zero, may happen at any time. Typically this means the calls to Add should execute before the statement creating the goroutine or other event to be waited for. If a WaitGroup is reused to wait for several independent sets of events, new Add calls must happen after all previous Wait calls have returned. See the WaitGroup example.

jstarks · 2017-08-01T21:23:54Z

service/gcs/stdio/stdio.go

+
+// ResizeConsole sends the appropriate resize to a pTTY FD
+// Synchronization of pty should be handled in the callers context.
+func ResizeConsole(pty *os.File, height, width uint16) error {


ResizeConsole [](start = 5, length = 13)

put this in tty.go

jstarks · 2017-08-01T21:25:03Z

service/gcs/stdio/stdio.go

+
+	if pty == nil {
+		return errors.New("error resizing console for nil pty fd")
+	}


Is this really necessary?

beweedon · 2017-08-01T21:29:32Z

service/gcs/core/gcs/gcs.go

-			containerEntry.ExitStatus = state
-			for _, hook := range containerEntry.ExitHooks {
-				hook(state)
-			}
 			delete(c.containerCache, id)


Why get rid of this?

This was a mistake for sure. Not sure how I missed that

beweedon

Looks good! I just have two more quick suggestions!

beweedon · 2017-08-01T22:51:49Z

service/gcs/bridge/bridge_test.go

+				It("should receive the correct values", func() {
+					Expect(callArgs.Pid).To(Equal(102))
+					Expect(callArgs.Height).To(Equal(uint16(80)))
+					Expect(callArgs.Width).To(Equal(uint16(80)))


Oops, forgot to mention this on my last review: If we're getting rid of the id=="" check in this PR, we should probably get rid of this test.

beweedon · 2017-08-01T22:53:24Z

service/gcs/stdio/stdio.go

 	r.s.Close()
+


Nit: unnecessary newline

Pushed updates.

jstarks · 2017-08-03T00:00:21Z

service/gcs/core/gcs/gcs.go

-	// entry.
-	processCache map[int]*processCacheEntry
-
-	externalProcessCacheMutex sync.RWMutex
 	// externalProcessCache stores information about external processes which


externalProcessCache [](start = 4, length = 20)

comment needs updating

jstarks · 2017-08-03T00:01:21Z

service/gcs/runtime/runtime.go

@@ -43,6 +43,7 @@ type Process interface {
 	Wait() (oslayer.ProcessExitState, error)
 	Pid() int
 	Delete() error
+	Tty() *stdio.TtyRelay


Tty [](start = 1, length = 3)

Maybe better to just expose the Resize method directly here? This seems to leak an implementation detail.

I will open an issue to unify the runtime Process and the oslayer Process interfaces. The issue is that for container/external processes we store them quite differently and the cache cant support both. We really should be thinking about them the same with a single Process interface from the GCS perspective. Will this work for now?

jstarks · 2017-08-03T00:02:14Z

🕐

Resolves: #76

Removes the per container cache for processes as they are all external pid's Changes to use a mutex instead of the wait group for ResizeConsole so we are confident of order. Puts back some of the containerID overloads because HCS always sends this even for external processes.

jterry75 · 2017-08-07T20:19:14Z

Anymore feedback here? Would like to get this checked in

beweedon

Looks good to me!

jterry75 requested review from jstarks, soccerGB and beweedon July 31, 2017 23:05

msftclas added the cla-not-required label Jul 31, 2017

jterry75 force-pushed the console_resize branch 4 times, most recently from a57d421 to 7026cdf Compare August 1, 2017 18:12

beweedon suggested changes Aug 1, 2017

View reviewed changes

jterry75 force-pushed the console_resize branch from 7026cdf to b055df3 Compare August 1, 2017 18:54

jstarks reviewed Aug 1, 2017

View reviewed changes

beweedon suggested changes Aug 1, 2017

View reviewed changes

jterry75 force-pushed the console_resize branch from e7abf55 to b3eb8d7 Compare August 1, 2017 22:48

beweedon previously requested changes Aug 1, 2017

View reviewed changes

jterry75 force-pushed the console_resize branch from b3eb8d7 to 37ae80c Compare August 2, 2017 16:52

jstarks reviewed Aug 3, 2017

View reviewed changes

jterry75 added 3 commits August 4, 2017 09:12

Adds console resize support.

7556251

Resolves: #76

Adding console resize bridge tests.

5691c4e

Synchronizing ResizeConsole

ab71b3f

jterry75 added 2 commits August 4, 2017 09:12

Updating comment

32fdcad

jterry75 force-pushed the console_resize branch from 37ae80c to 32fdcad Compare August 4, 2017 16:12

jterry75 mentioned this pull request Aug 4, 2017

We should unify the runtime.Process and oslayer.Process interfaces #96

Open

beweedon approved these changes Aug 7, 2017

View reviewed changes

soccerGB merged commit 874b671 into master Aug 8, 2017

beweedon deleted the console_resize branch August 8, 2017 00:45

Adds console resize support. #84

Adds console resize support. #84

Conversation

jterry75 commented Jul 31, 2017

msftclas commented Jul 31, 2017

jterry75 commented Jul 31, 2017

jstarks commented Jul 31, 2017

jterry75 commented Aug 1, 2017

jstarks commented Aug 1, 2017

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

Choose a reason for hiding this comment

beweedon 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

jstarks commented Aug 3, 2017

jterry75 commented Aug 7, 2017

beweedon left a comment

Choose a reason for hiding this comment