[full ci] Attach support for exec'ed session #4339

caglar10ur · 2017-03-22T19:50:23Z

Fixes #4263
Fixes #4367
Fixes #4598
Fixes #4410
Fixes #4339
Fixes #4881

Requires #4288

vburenin · 2017-03-23T23:14:25Z

cmd/tether/attach.go

@@ -148,6 +154,19 @@ func (t *attachServerSSH) start() error {
 	}

 	if t.Enabled() {
+		// HACK
+		if t.serverConn.ServerConn != nil {
+			log.Infof("asking portlayer to get container ids")


I think all these three infos has to be debugs.

Subject says WIP :)

Haha, I get that, I just want to make sure it is not missed.

vburenin · 2017-03-24T00:42:44Z

cmd/tether/attach.go

 	for req := range reqchan {
-		var pendingFn func()
-		var payload []byte
+		payload := []byte{}


this causes allocation of a byte array pointer using var payload []byte is actually better especially if there is a chance that no data will be added.

vburenin · 2017-03-24T00:45:21Z

cmd/tether/attach.go

-				i++
+			for k, v := range t.config.Sessions {
+				if v.Active {
+


unnecessary whitespace line

vburenin · 2017-03-24T00:46:48Z

cmd/tether/attach.go

 		ok := true

 		log.Infof("received global request type %v", req.Type)

 		switch req.Type {
 		case msgs.ContainersReq:
-			keys := make([]string, len(t.config.Sessions))
+			keys := make([]string, len(t.config.Sessions)+len(t.config.Execs))


are you sure you want to allocate all strings? you will end up with an array that has many empty elements at the end if there are not active elements exist.

vburenin · 2017-03-24T00:48:54Z

lib/apiservers/engine/backends/container.go

+	err = c.containerProxy.AttachStreams(context.Background(), vc, eid, stdin, stdout, stderr, ca)
+	if err != nil {
+		if _, ok := err.(DetachError); ok {
+			log.Infof("Detach detected, tearing down connection")


This chunk of logic wants to be moved out into a separate method, current method is heavily overloaded already.

vburenin · 2017-03-24T00:50:36Z

lib/apiservers/engine/backends/container.go

@@ -1583,7 +1630,7 @@ func (c *Container) containerAttach(name string, ca *backend.ContainerAttachConf

 			handle, ok = unbind.Payload.Handle.(string)
 			if !ok {
-				return InternalServerError("type assertion failed")
+				return InternalServerError("type assOertion failed")


I think you want to rollback this line.

vburenin · 2017-03-24T14:51:20Z

lib/communication/interactor.go

@@ -12,13 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-package attach
+package communication


please name it comm.

Why? Comm sounds like common

communication is too long. using word common for naming packages is very bad, so comm can lead to communication only. I just want it, nothing else. Feel free to resist, but not too much ;)

hickeng

First pass review - added comments, but likely easiest to talk about it in the morning.

hickeng · 2017-03-23T20:35:05Z

cmd/tether/attach.go

@@ -148,6 +154,19 @@ func (t *attachServerSSH) start() error {
 	}

 	if t.Enabled() {
+		// HACK
+		if t.serverConn.ServerConn != nil {


presume this is going to be replaced with a portlayer symmetric container-ids call on the global command channel?

hickeng · 2017-03-23T20:41:51Z

cmd/tether/attach.go

-				serverConn.Unlock()
+				t.serverConn.Lock()
+				// set serverConn to nil if not an exec session
+				if _, ok = t.config.Sessions[sessionid]; ok {


I don't recall why we were clearing this - do we need ref counting if we ever have multiple sessions?

so that we can see whether the connection is established or not, this value being nil is the looping condition

hickeng · 2017-03-23T20:49:25Z

cmd/tether/msgs/messages.go

@@ -142,3 +142,6 @@ func (s *ContainersMsg) Marshal() []byte {
 func (s *ContainersMsg) Unmarshal(payload []byte) error {
 	return ssh.Unmarshal(payload, s)
 }
+
+// AskContainersReq
+const AskContainersReq = "ask-container-ids"


Presume this is going to be switched out with the direct reporting of current ID set when attach.Reload is called rather than this solicitation of a request for the updated set?

hickeng · 2017-03-28T10:36:10Z

lib/portlayer/exec/commit.go

@@ -172,6 +172,7 @@ func Commit(ctx context.Context, sess *session.Session, h *Handle, waitTime *int
 				if err != nil {
 					// NOTE: not sure how to handle this error - the change is already applied, it's just not picked up in the container
 				}
+				h.reload = false


I remember you asking about this - but handles should never be reused, so I do not understand why this is present.
We've got worse problems if we're somehow attempting to reuse a handle.

I think this is a debugging artifact, will remove it (and see what will happen)

hickeng · 2017-03-28T10:42:46Z

lib/apiservers/portlayer/restapi/handlers/task_handlers.go

@@ -74,6 +76,7 @@ func (handler *TaskHandlersImpl) JoinHandler(params tasks.JoinParams) middleware
 		Tty:       params.Config.Tty,
 		Attach:    params.Config.Attach,
 		OpenStdin: params.Config.OpenStdin,
+		RunBlock:  params.Config.Attach,


Check out this change @sflxn had to make in a pending PR. compose sets all of config.AttachStdin || config.AttachStdout || config.AttachStderr to false if running with -d, which prevents us from attaching later.

We need a different mechanism of populating RunBlocking - we were using the ordering of the attach and start calls to determine if this was required previously - has that changed or is this just how it was being communicated?

Good point. This is lacking a FIXME comment and we need to fix Bind to handle Exec'ed sessions

hickeng · 2017-03-28T11:06:24Z

lib/apiservers/portlayer/restapi/handlers/interaction_handlers.go

@@ -146,7 +147,7 @@ func (i *InteractionHandlersImpl) UnbindHandler(params interaction.InteractionUn
 // ContainerResizeHandler calls resize
 func (i *InteractionHandlersImpl) ContainerResizeHandler(params interaction.ContainerResizeParams) middleware.Responder {
 	// See whether there is an active session to the container
-	session, err := i.attachServer.Get(context.Background(), params.ID, 0)
+	session, err := i.server.Interaction(context.Background(), params.ID, interactionTimeout)


The timeout here was set to 0 previously because we were finding resize getting called either at somewhat arbitrary times and it was adding a 30s delay on the CLI end (IIRC it was the stop path - this may have changed given the 1.13 use of events, but needs confirming).

Grabbed this change from Hasan's PR :)

hickeng · 2017-03-28T11:10:07Z

lib/portlayer/attach/join.go

+	if !ok {
+		return nil, fmt.Errorf("Type assertion failed for %#+v", handle)
+	}
+


This needs to pull in the changes from the vSPC work:
https://github.com/vmware/vic/commit/23ed0b0a41dec1c7e91288db2c37e072595cb931

I think I already rebased on top of that, what's missing?

hickeng · 2017-03-28T11:33:36Z

lib/portlayer/attach/communication/connector.go

-	var ids []string
-	ids, err = SSHls(client)
+	// ask the IDs
+	ids, err := ContainerIDs(clientConn)


I think this should be below the registration of the mux handlers - I'm uncertain whether it's significant in this case but I'm very wary of potentially making a call that expects a reply without having the mux's running - I seem to recall that leads to potential deadlocks.

i believe it blocks until someone handles the request

hickeng · 2017-03-28T11:35:59Z

lib/portlayer/attach/communication/connector.go

+	go c.chans(chans)
+
+	// create the connections
+	c.ids(clientConn, ids)


ah - this may be why the runblocking on all sessions isn't impact us currently. I'm unsure what happens if we don't drain the stdout streams of the "un-attached" connections in this model

We should think about, then document, what the behaviour should be in the case of multiple sessions where we only care about output from one of them.

#4950 created to track this

hickeng · 2017-03-28T11:38:26Z

lib/tether/tether.go

@@ -470,6 +470,8 @@ func (t *tether) Start() error {
 			return err
 		}

+		// Danger, Will Robinson! There is a strict ordering here.


Unfortunately t.extensions is a map - with no ordering guarantees. What is the impact of this ordering being incorrect?
We cannot merge this unless either the comment is wrong, we resolve the dependency, or we enforce the ordering.

The ordering is between attach server and ProcessSession, will clarify it

sgairo · 2017-03-30T16:57:14Z

lib/portlayer/attach/communication/interactor.go

@@ -172,3 +166,27 @@ func (t *attachSSH) Resize(cols, rows, widthpx, heightpx uint32) error {
 	}
 	return nil
 }
+
+// Ping checks the liveleness of the connection


"liveliness"

mdubya66

Vendor changes approved. Still requires code review approval.

hickeng · 2017-03-28T15:14:16Z

lib/portlayer/attach/communication/connector.go

+}
+
+// URL returns the listener's URL
+func (c *Connector) URL() string {


I don't see this being used anywhere any more - it's would/should be called by the vSPC code if it's going to be useful.

@kreamyx do you remember how you are constructing the URL in the vps code?

@caglar10ur are you talking about this?
https://github.com/vmware/vic/blob/master/lib/vspc/helpers.go#L85

hickeng · 2017-03-28T15:17:27Z

lib/portlayer/attach/communication/connector.go

+		c.mutex.RUnlock()
+
+		if ok {
+			continue


should there be a ping test here? we'll never reject a dead connection at this point, is this covered elsewhere?

hickeng · 2017-03-28T15:37:44Z

lib/portlayer/attach/communication/connector.go

-		connection := &Connection{
-			spty: si,
-			id:   id,
+		if err := si.Ping(); err != nil {


I think it's viable to trust that the connection is solid if we've only just created it - if we release the runblock after the reader/writers are bound in channel setup instead of via an explicit message then we can move this to be validation only.

hickeng · 2017-04-06T14:51:53Z

lib/apiservers/engine/backends/container.go

+		CloseStdin:            true,
+	}
+
+	err = c.containerProxy.AttachStreams(context.Background(), ac, stdin, stdout, stderr)


Don't know if we should call this if all of the UseX are false (-d case). AttachStreams shuts down stdin on return which will kill the transport to the client. I don't know if we need that transport to return launch errors - my guess is we do.

Suggest we check for successful launch immediately after taskinspect (ec.Running for success/failure, ec.ProcessConfig.ErrMessage for detail) and only perform the attach prep if launch was successful.

caglar10ur · 2017-04-10T23:19:56Z

I know this PR is big so here are the main changes. I'm also willing to answer any questions you may have.

Implements exec functionality for personality

Those changes live in lib/apiservers/engine/backends/ directory. There are changes in container_proxy functions since exec and attach shares some common code paths. We started to pass ids instead of viccontainer structs around to them to support both workflows. AttachStreams function splits the cancelation logic into different parts. Closing stdin cancels other active streams. We also ensure that the API client connection is shutdown on exit.

Implements exec support for portlayer rest api

Those changes live in lib/apiservers/portlayer/ directory. One important change there is context changes. In the past we were calculating the timeouts manually and then passing timeouts as a parameter to Get call.
session, err := attachServer.Get(context.Background(), params.ID, timeout)

now each stream is creating its own context and passing it to the function - eliminating manual calculations and redundant timeout value.

session, err := i.server.Interaction(ctx, params.ID)

Portlayer attach subsystem re-organization

Those changes live in lib/portlayer/attach/. Some of them just renames and splitting monolithic file (they are now bind.go/join.go etc.) into pieces. Connector has new set of features. We now have a different unblock signal for signaling tether (called unblock) for starting the processes inside the container. We also now have a SSH channel mux running on client side to receive container ids from tether. Normally portlayer initiates the connection and asks container ids. This is still the case for the primary session. But after that primary session start event, any exec'ed session will cause tether to restart and as a part of that tether will push container ids to the portlayer. Additionally we now started to use channel mux to evict closed connections from the connection map (#4367). Lastly we started to use singleflight to make sure that there is only one call in-flight for each ID at any given time.

tether support for exec

As noted above, tether now pushes container ids to portlayer when it receives reload signal. It supports new unblock signal. Fixes a bug caused by wrong usage of cleanup functions. Fixes races causing incorrect behaviors.

dio changes

dio reader now has a PropagateEOF function toggles whether to return EOF when all readers return EOF. Setting this to true will result in an EOF if there are no readers available when Read is next called. dio writer now calls CloseWrite if the writes implements CloseWriter.

keramix · 2017-04-13T20:09:05Z

lib/vspc/handlers.go

+	cvm, exists := h.vspc.cvmFromTelnetConnUnlocked(tc)
+	if exists {
+		cvm.Lock()
+		log.Debugf("(vspc) detected closed connection for VM %s", cvm)


can you use cvm.vmUUID instead of cvm?

I added a String() method on cvm instead

keramix · 2017-04-13T20:09:20Z

lib/vspc/handlers.go

+		log.Debugf("(vspc) detected closed connection for VM %s", cvm)
+		log.Debugf("(vspc) closing connection to the AttachServer")
+		cvm.remoteConn.Close()
+		log.Debugf("(vspc) deleting vm records from the vm manager %s", cvm)


can you use cvm.vmUUID instead of cvm?

jakedsouza · 2017-04-13T22:50:02Z

lib/portlayer/attach/communication/server.go


 	addr, err := net.ResolveTCPAddr("tcp", fmt.Sprintf("%s:%d", n.ip, n.port))
+	if err != nil {
+		return fmt.Errorf("Attach server error %s: %s", addr, errors.ErrorStack(err))


Should probably print n.ip and n.port here

@hmahmood

* Attach support for exec'ed session Includes @hmahmood's and @hickeng's various fixed to dio/persona and portlayer Fixes vmware#4263 Fixes vmware#4367 Fixes vmware#4598 Fixes vmware#4410 Fixes vmware#4339 Fixes vmware#4881 Requires vmware#4288 Implements exec functionality for personality Those changes live in lib/apiservers/engine/backends/ directory. There are changes in container_proxy functions since exec and attach shares some common code paths. We started to pass ids instead of viccontainer structs around to them to support both workflows. AttachStreams function splits the cancelation logic into different parts. Closing stdin cancels other active streams. We also ensure that the API client connection is shutdown on exit. Implements exec support for portlayer rest api Those changes live in lib/apiservers/portlayer/ directory. One important change there is context changes. In the past we were calculating the timeouts manually and then passing timeouts as a parameter to Get call. session, err := attachServer.Get(context.Background(), params.ID, timeout) now each stream is creating its own context and passing it to the function - eliminating manual calculations and redundant timeout value. session, err := i.server.Interaction(ctx, params.ID) Portlayer attach subsystem re-organization Those changes live in lib/portlayer/attach/. Some of them just renames and splitting monolithic file (they are now bind.go/join.go etc.) into pieces. Connector has new set of features. We now have a different unblock signal for signaling tether (called unblock) for starting the processes inside the container. We also now have a SSH channel mux running on client side to receive container ids from tether. Normally portlayer initiates the connection and asks container ids. This is still the case for the primary session. But after that primary session start event, any exec'ed session will cause tether to restart and as a part of that tether will push container ids to the portlayer. Additionally we now started to use channel mux to evict closed connections from the connection map (vmware#4367). Lastly we started to use singleflight to make sure that there is only one call in-flight for each ID at any given time. tether support for exec As noted above, tether now pushes container ids to portlayer when it receives reload signal. It supports new unblock signal. Fixes a bug caused by wrong usage of cleanup functions. Fixes races causing incorrect behaviors. dio changes dio reader now has a PropagateEOF function toggles whether to return EOF when all readers return EOF. Setting this to true will result in an EOF if there are no readers available when Read is next called. dio writer now calls CloseWrite if the writes implements CloseWriter.

@hmahmood

* Attach support for exec'ed session Includes @hmahmood's and @hickeng's various fixed to dio/persona and portlayer Fixes vmware#4263 Fixes vmware#4367 Fixes vmware#4598 Fixes vmware#4410 Fixes vmware#4339 Fixes vmware#4881 Requires vmware#4288 Implements exec functionality for personality Those changes live in lib/apiservers/engine/backends/ directory. There are changes in container_proxy functions since exec and attach shares some common code paths. We started to pass ids instead of viccontainer structs around to them to support both workflows. AttachStreams function splits the cancelation logic into different parts. Closing stdin cancels other active streams. We also ensure that the API client connection is shutdown on exit. Implements exec support for portlayer rest api Those changes live in lib/apiservers/portlayer/ directory. One important change there is context changes. In the past we were calculating the timeouts manually and then passing timeouts as a parameter to Get call. session, err := attachServer.Get(context.Background(), params.ID, timeout) now each stream is creating its own context and passing it to the function - eliminating manual calculations and redundant timeout value. session, err := i.server.Interaction(ctx, params.ID) Portlayer attach subsystem re-organization Those changes live in lib/portlayer/attach/. Some of them just renames and splitting monolithic file (they are now bind.go/join.go etc.) into pieces. Connector has new set of features. We now have a different unblock signal for signaling tether (called unblock) for starting the processes inside the container. We also now have a SSH channel mux running on client side to receive container ids from tether. Normally portlayer initiates the connection and asks container ids. This is still the case for the primary session. But after that primary session start event, any exec'ed session will cause tether to restart and as a part of that tether will push container ids to the portlayer. Additionally we now started to use channel mux to evict closed connections from the connection map (vmware#4367). Lastly we started to use singleflight to make sure that there is only one call in-flight for each ID at any given time. tether support for exec As noted above, tether now pushes container ids to portlayer when it receives reload signal. It supports new unblock signal. Fixes a bug caused by wrong usage of cleanup functions. Fixes races causing incorrect behaviors. dio changes dio reader now has a PropagateEOF function toggles whether to return EOF when all readers return EOF. Setting this to true will result in an EOF if there are no readers available when Read is next called. dio writer now calls CloseWrite if the writes implements CloseWriter.

vmwclabot added the cla-not-required label Mar 22, 2017

vburenin reviewed Mar 23, 2017

View reviewed changes

vburenin reviewed Mar 24, 2017

View reviewed changes

caglar10ur changed the title ~~[WIP] Attach support for exec'ed session~~ [WIP] [skip ci] Attach support for exec'ed session Mar 24, 2017

vburenin reviewed Mar 24, 2017

View reviewed changes

caglar10ur changed the title ~~[WIP] [skip ci] Attach support for exec'ed session~~ [WIP] Attach support for exec'ed session Mar 26, 2017

hickeng requested changes Mar 28, 2017

View reviewed changes

hickeng mentioned this pull request Mar 28, 2017

[WIP] [full ci] Removing cancel() calls after reading either stdin or stdout or stderr #4352

Closed

caglar10ur changed the title ~~[WIP] Attach support for exec'ed session~~ [skip ci] [WIP] Attach support for exec'ed session Mar 29, 2017

caglar10ur changed the title ~~[skip ci] [WIP] Attach support for exec'ed session~~ [WIP] Attach support for exec'ed session Mar 29, 2017

sgairo reviewed Mar 30, 2017

View reviewed changes

caglar10ur changed the title ~~[WIP] Attach support for exec'ed session~~ [WIP] [DO NOT REVIEW] [full ci] Attach support for exec'ed session Apr 5, 2017

caglar10ur mentioned this pull request Apr 5, 2017

[attach] could it be that the exit event is being returned to the client before the output and the client terminates at that point #4166

Closed

mdubya66 approved these changes Apr 5, 2017

View reviewed changes

corrieb mentioned this pull request Apr 5, 2017

docker exec -d often just stops working #4598

Closed

hickeng requested changes Apr 6, 2017

View reviewed changes

caglar10ur changed the title ~~[WIP] [DO NOT REVIEW] [full ci] Attach support for exec'ed session~~ [full ci] Attach support for exec'ed session Apr 10, 2017

cgtexmex mentioned this pull request Apr 10, 2017

Fixed race and added support for all option in docker stats #4671

Merged

hickeng mentioned this pull request Apr 13, 2017

[tether] make SSH server loop more resilient #2626

Open

keramix reviewed Apr 13, 2017

View reviewed changes

jakedsouza reviewed Apr 13, 2017

View reviewed changes

caglar10ur added area/docker Support for the Docker operations component/persona/docker component/portlayer labels Apr 18, 2017

caglar10ur added 22 commits April 27, 2017 09:55

change vspc connection handling for closed connections

a064ff5

Use localhost for tests - Oscar goes to Jake

6339ee6

Dump the git rev

64714b8

remove detach leftovers from rebase

00fc743

propagate errors back (also fix the err checks in AttachStream)

6e031f1

use the callers context

a874e31

in lieu of connection closures

51e6370

self review fixes

4a0edcc

address George's toggle comment

8204679

fix

130dd63

Drop not needed change

4361864

address Mike's review comments

2c2451b

Address Emma's comment

02f055f

add mrrobot target to clean robot artifacts

d7003d2

Address Hasan's comment [handle upgrades]

2f8f147

ssh message cannot handle int so make it uint32

b760fc5

retry if we get ConflictError

4c7b1e2

address Hasan's comment

99b10c9

fix comments

b3ab3d9

George's comments

087ffc7

More changes to address George's comments

9b5419e

TaskWait -> TaskWaitToStart

d8aa0ac

This was referenced Apr 27, 2017

[portlayer] refactor task.Wait #4945

Open

[docker] ContainerExecCreate does state checks instead it should rely on portlayer to do the right thing #4946

Open

hickeng approved these changes Apr 27, 2017

View reviewed changes

Merge branch 'master' into attach

6664002

caglar10ur merged commit 88d2270 into vmware:master Apr 27, 2017

caglar10ur deleted the attach branch May 31, 2017 22:53

[full ci] Attach support for exec'ed session #4339

[full ci] Attach support for exec'ed session #4339

Conversation

caglar10ur commented Mar 22, 2017 • edited

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

hickeng 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

mdubya66 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

caglar10ur commented Apr 10, 2017 • edited

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

caglar10ur commented Mar 22, 2017 •

edited

caglar10ur commented Apr 10, 2017 •

edited