Make the shim wait for the process to be started before forwarding stdin and signals #49

dlespiau · 2017-05-05T16:49:38Z

A somewhat long-winded PR that should remove the need to start the shim as stopped. We finish the work to started to synchronise runtime, shim and the process inside the VM without queuing data. It can be a bit fiddly so the code is somewhat heavily documented and tested.

coveralls · 2017-05-05T16:53:14Z

Coverage increased (+1.3%) to 71.872% when pulling af0fec4 on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 70866ba on clearcontainers:master.

coveralls · 2017-05-05T16:53:14Z

Coverage increased (+1.3%) to 71.872% when pulling af0fec4 on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 70866ba on clearcontainers:master.

coveralls · 2017-05-05T16:53:31Z

Coverage increased (+1.7%) to 72.292% when pulling af0fec4 on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 70866ba on clearcontainers:master.

sboeuf · 2017-05-05T22:39:35Z

vm.go

+
+var waitForExecTimeout = 30 * time.Second
+
+// WaitForExec will wait until the process inside the VM is fully started. If


s/WaitForExec/WaitForProcess/

sboeuf · 2017-05-05T22:45:18Z

One comment about function comment not in sync with the function name, but other than that LGTM.

coveralls · 2017-05-07T13:59:26Z

Coverage increased (+1.3%) to 71.872% when pulling 5eb191d on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 70866ba on clearcontainers:master.

dlespiau · 2017-05-07T14:06:31Z

argh, I can't merge it because the LGTM isn't at the start of the line. Grmbl (against tooling getting in the way, of course).

jodh-intel · 2017-05-08T07:57:24Z

vm.go

+	return nil
+}
+
+var waitForExecTimeout = 30 * time.Second


const? Also, should this name refer to the process (waitForProcessTimeout)?

Can't make this one const, I override the timeout in tests. But sure will rename it.

jodh-intel · 2017-05-08T07:57:47Z

proxy_test.go

-// smallWaitForShimTimeout overrides the default timeout for the tests
-const smallWaitForShimTimeout = 20 * time.Millisecond
+// smallWaitTimeout overrides the default timeout for the tests
+const smallWaitTimeout = 20 * time.Millisecond


jodh-intel · 2017-05-08T08:00:31Z

vm.go

+// timeout.
+func (session *ioSession) WaitForProcess(shouldReset bool) error {
+	session.vm.infof(1, "session",
+		"waiting for runtime to execute the process for token %s (timeout %s)",


Is that %s correct? Even if it is, it might look less odd if the format was specified as %v.

Yup that's correct (It'll call String() on any type that satisfies the Stringer interface).

Went digging a bit in the go source code. %s and %v will end up doing the same thing when the argument implements the stringer interface:

https://golang.org/src/fmt/print.go?s=5540:5590#L593

then both will do p.fmt.fmt_s() in https://golang.org/src/fmt/print.go?s=5540:5590#L424

dlespiau · 2017-05-08T17:08:03Z

PR updated:

Renamed waitForExecTimeout to waitForProcessTimeout.

coveralls · 2017-05-08T17:09:36Z

Coverage increased (+1.2%) to 72.292% when pulling 1124a6f on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 4db5a15 on clearcontainers:master.

amshinde · 2017-05-12T23:34:47Z

proxy.go

+	if newClient.session != nil {
+		proxy.Lock()
+		info := proxy.tokenToVM[newClient.token]
+		info.state = tokenStateAllocated


Is this session being kept round forever. If the shim disconnects, we should wait for a reconnection for a specific time, after which we should discard the session.

Yes, but that's not a problem introduced in this PR, we've always had it. It's related to #24. Currently we need a DisconnectShim to clean up shim-related data structures. I've extended issue #24 to cover this.

jodh-intel · 2017-05-15T10:11:32Z

vm.go

 // tokens. Starpod isn't handled as it's not currently use to start processes
 // and indicated as deprecated in the hyperstart API.
-func (vm *vm) relocateHyperCommand(hyper *api.Hyper) error {
+// RelocateHyperCommand will return the Session of the process in question if


Nit: The comment should refer to relocateHyperCommand.

dlespiau · 2017-05-17T11:27:02Z

PR updated:

Renamed RelocateHyperCommand to relocateHyperCommand in comment.

coveralls · 2017-05-17T11:38:15Z

Coverage increased (+1.7%) to 72.241% when pulling b9948be on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 800cc89 on clearcontainers:master.

coveralls · 2017-05-17T11:38:58Z

Coverage increased (+1.3%) to 71.823% when pulling ba3b9b5 on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 800cc89 on clearcontainers:master.

That's useful only in tests really at this point as the shim is written in C. But one never knows! Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

dlespiau · 2017-05-17T15:48:35Z

This needs to be rebased on top of the logrus port before merging.

Turns out it's really useful to know when we are hitting timeouts when debugging! Updates: clearcontainers#37 Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

We close the shim connection when something bad happens: - We receive an error trying to write to the socket (most likely because the shim died or exited). - We have an other kind of unrecoverable error for that client (we don't have one right now but we will in the near future) We now want to progress a bit on the recovery side of things. If the shim dies, we want to allow it to reconnect and re-claim the session. This commit does just that. This is tested by a subsequent unit test: TestShimSendStdinAfterExeccmd Updates: clearcontainers#4 Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

As we don't queue any data in the proxy today, we need the process inside the VM to exist before we can send stdin data to it. Updates: clearcontainers#39 Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Just like: commit 3d897a7 Author: Damien Lespiau <damien.lespiau@intel.com> Date: Fri May 5 15:57:01 2017 +0100 proxy: Don't forward stdin data until the process is started but for signals. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

dlespiau · 2017-05-18T14:13:49Z

And now rebased on top of the logrus changes.

coveralls · 2017-05-18T14:19:13Z

Coverage increased (+1.6%) to 72.904% when pulling 70af5ec on dlespiau:20170504-sync-stdin-newcontainer-execcmd into 2925d0a on clearcontainers:master.

dlespiau · 2017-05-19T17:05:28Z

@amshinde mind having another look at this one?

amshinde · 2017-05-19T17:28:24Z

lgtm

dlespiau force-pushed the 20170504-sync-stdin-newcontainer-execcmd branch from 862b9b1 to af0fec4 Compare May 5, 2017 16:50

sboeuf reviewed May 5, 2017

View reviewed changes

dlespiau force-pushed the 20170504-sync-stdin-newcontainer-execcmd branch from af0fec4 to 5eb191d Compare May 7, 2017 13:55

jodh-intel reviewed May 8, 2017

View reviewed changes

dlespiau force-pushed the 20170504-sync-stdin-newcontainer-execcmd branch from 5eb191d to 1124a6f Compare May 8, 2017 17:07

amshinde reviewed May 12, 2017

View reviewed changes

jodh-intel reviewed May 15, 2017

View reviewed changes

dlespiau force-pushed the 20170504-sync-stdin-newcontainer-execcmd branch 2 times, most recently from ba3b9b5 to b9948be Compare May 17, 2017 11:28

client: Add a SendStdin API

511ec5d

That's useful only in tests really at this point as the shim is written in C. But one never knows! Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

dlespiau added the do-not-merge label May 17, 2017

Damien Lespiau added 4 commits May 18, 2017 15:01

vm: Log if we hit the timeout

f86a43f

Turns out it's really useful to know when we are hitting timeouts when debugging! Updates: clearcontainers#37 Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

proxy: Don't forward stdin data until the process is started

d994aff

As we don't queue any data in the proxy today, we need the process inside the VM to exist before we can send stdin data to it. Updates: clearcontainers#39 Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

dlespiau force-pushed the 20170504-sync-stdin-newcontainer-execcmd branch from b9948be to 70af5ec Compare May 18, 2017 14:10

dlespiau removed the do-not-merge label May 18, 2017

dlespiau merged commit 0c71a01 into clearcontainers:master May 19, 2017


		var waitForExecTimeout = 30 * time.Second

		// WaitForExec will wait until the process inside the VM is fully started. If

Make the shim wait for the process to be started before forwarding stdin and signals #49

Make the shim wait for the process to be started before forwarding stdin and signals #49

Uh oh!

Conversation

dlespiau commented May 5, 2017

Uh oh!

coveralls commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 5, 2017

Uh oh!

coveralls commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf commented May 5, 2017

Uh oh!

coveralls commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlespiau May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlespiau commented May 8, 2017

Uh oh!

coveralls commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlespiau commented May 17, 2017

Uh oh!

coveralls commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau commented May 17, 2017

Uh oh!

dlespiau commented May 18, 2017

Uh oh!

coveralls commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau commented May 19, 2017

Uh oh!

amshinde commented May 19, 2017 • edited by sameo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented May 5, 2017 •

edited

Loading

coveralls commented May 5, 2017 •

edited

Loading

coveralls commented May 7, 2017 •

edited

Loading

dlespiau commented May 7, 2017 •

edited

Loading

dlespiau May 8, 2017 •

edited

Loading

coveralls commented May 8, 2017 •

edited

Loading

coveralls commented May 17, 2017 •

edited

Loading

coveralls commented May 17, 2017 •

edited

Loading

coveralls commented May 18, 2017 •

edited

Loading

amshinde commented May 19, 2017 •

edited by sameo

Loading