rkt: add experimental support for attachable applications #3396

lucab · 2016-11-21T16:37:12Z

This commit introduces experimental support for attachable
applications. It consists of:

a new attach subcommand
a set of per-app flags to control stdin/stdout/stderr modes
a stage1 iottymux binary for multiplexing and attaching
two new templated stage1 services, iomux and ttymux

TODO:

rebase on master once stage1: update coreos base to alpha 1235.0.0 #3388 is merged
cleanup iottymux code, many TODO pending in there
add developer documentation, as the whole mechanism is complex
add user documentation for rkt attach and --stdin, --stdout, --stderr
add smoke tests for stream and tty-attach
ungate, this is currently behind a $RKT_EXPERIMENT_ATTACH flag
signal proxying and custom detach

squeed · 2016-11-22T13:08:24Z

rkt/attach.go

+		return nil, fmt.Errorf("mode must specify at least one endpoint to attach")
+	}
+	if (attachTTYIn || attachTTYOut) && (attachStdin || attachStdout || attachStderr) {
+		return nil, fmt.Errorf("incompatibles endpoints %q", attachMode)


"Incompatible". Also, this could be clearer (e.g. "cannot attach TTY and stdIn/out/err")

squeed · 2016-11-22T13:17:11Z

Hm.
Does this mean you need to decide at app add whether or not you are going to attach a TTY? Will this make kubernetes mad?

squeed · 2016-11-22T13:18:25Z

Oh, nevermind, I get it! tty is a remote interactive process, whereas interactive is a local interactive process.

squeed · 2016-11-22T13:19:52Z

stage1/init/common/units.go

@@ -179,6 +171,131 @@ func ImmutableEnv(p *stage1commontypes.Pod, interactive bool, privateUsers strin
 	return w.Error()
 }

+// SetupAppIO prepares all properties related to streams (stdin/stdout/stderr) and TTY
+// for an application service unit
+func (w *UnitWriter) SetupAppIO(p *stage1commontypes.Pod, ra *schema.RuntimeApp, binPath string, interactive bool, debug bool, opts ...*unit.UnitOption) []*unit.UnitOption {


I know this is a WIP but I would love this to have a really good docblock, plus a reference to whatever other docs you write. Being nice to the future programmer who comes and wonders what the heck we're doing.

ack, and maybe split in separate smaller methods

squeed · 2016-11-22T13:28:52Z

stage1/iottymux/iottymux.go

+
+	ec := make(chan error)
+	for s, p := range eps {
+		conn, err := net.Dial("tcp", fmt.Sprintf("127.0.0.1:%s", p))


Why did you choose to use a network socket instead of a Unix socket?

No strong design decision, but network sockets look more flexible here as they are already scoped per-pod, they are not tied to the overabundant mount-ns magic we have in place and can play nicely with nsenter (if needed).

Slowly starting to re-consider this choice in favor of a stage1-namespaced file-backed unix socket 😊

s-urbaniak

@lucab I left a couple of comments as a first pass.

s-urbaniak · 2016-11-29T12:06:23Z

common/apps/apps.go

@@ -30,6 +30,21 @@ var (
 	ErrInvalidSeccompOverride = errors.New("invalid seccomp command-line override")
 )

+// AppIOType describes the type of an application stream at runtime (stdin/stdout/stderr).
+type AppIOType string


type AppIO string is sufficient?

s-urbaniak · 2016-11-29T12:07:45Z

rkt/rkt.go

+
+// getRktEnvExperiments returns annotations for rkt experimental/hidden
+// features
+func getRktEnvExperiments(annotations map[types.ACIdentifier]string) map[types.ACIdentifier]string {


rktEnvExperiments

I removed this code, will reconsider these features later in a better way.

s-urbaniak · 2016-11-29T12:10:03Z

rkt/rkt.go

+		if envVal == "" {
+			continue
+		}
+		annotations[v] = envVal


we are modifying annotations here. I suggest we either make a copy of it, or declare the method to be mutable and not return annotations at all.

s-urbaniak · 2016-11-29T12:19:55Z

rkt/attach.go

+	defer p.Close()
+
+	if p.State() != pkgPod.Running {
+		stderr.Printf("pod %q isn't currently running", p.UUID)


stderr.Fatalf (implying os.Exit(254))?

Everywhere else we bubble up the return code to the wrapper and I prefer to stick to it. It may make sense to return the error object instead of the exit code, but not in the scope of this PR.

👍 for bubbling up so defers happen; I don't think it needs or should be an error object either necessarily since one step up won't have more info in this case ever.

s-urbaniak · 2016-11-29T12:23:47Z

rkt/attach.go

+	attachArgs := []string{
+		fmt.Sprintf("--app=%s", appName),
+		fmt.Sprintf("--debug=%t", debug),
+	}


this is a bit unexpected to be in a parseAttachMode func, I'd expect only attach related options in this method.

Renaming to "createAttachFlags" might help

You are right, this func is misleading. I'll split the concerns, move name and debug out of here, and rename it.

s-urbaniak · 2016-11-29T12:55:36Z

stage1/iottymux/iottymux.go

+		if err != nil {
+			break
+		}
+		_, err = stdin.Write(lineIn)


if err != nil, we should probably close the connection to the client and log things too?

s-urbaniak · 2016-11-29T13:05:22Z

stage1/iottymux/iottymux.go

+	}
+	// Open FIFOs
+	var endpoints = make([]*net.Listener, 3)
+	var fifos = make([]*os.File, 3)


why do we need a slice here?

s-urbaniak · 2016-11-29T13:06:32Z

stage1/iottymux/iottymux.go

+func actionIOMux(statusFile *os.File) error {
+	var err error
+	var endpoints = make([]*net.Listener, 3)
+	var fifos = make([]*os.File, 3)


let's document the nature of these slices -> stdin, stdout, stderr

s-urbaniak · 2016-11-29T13:12:40Z

stage1/iottymux/iottymux.go

+	var logs []io.WriteCloser
+	var alivel []bool
+	var conns []io.WriteCloser
+	var alivec []bool


instead of maintaining alivec and alivel slices, you could remove them simply from the logs and conns slices in cases errors occur.

s-urbaniak · 2016-11-29T13:20:15Z

stage1/iottymux/iottymux.go

+}
+
+// acceptConn accepts a single client and queues it for further proxying
+func acceptConn(socket *net.Listener, c chan *net.Conn, stream string) {


net.Listener is an interface, please don't pass a pointer to it.

net.Conn is an interface, please don't send pointers of it via a channel, it boils down to being a two-word pair of a pointer and type already, see http://research.swtch.com/interfaces.

euank

First review pass. The documentation is nice and the code looks good overall

euank · 2016-12-07T02:07:45Z

Documentation/devel/log-attach-design.md

+```
+{
+    "name": "coreos.com/rkt/stage2/stdout",
+    "value": "log"


Copy paste error? This should be null

euank · 2016-12-07T06:15:19Z

rkt/attach.go

+	defer p.Close()
+
+	if p.State() != pkgPod.Running {
+		stderr.Printf("pod %q isn't currently running", p.UUID)


👍 for bubbling up so defers happen; I don't think it needs or should be an error object either necessarily since one step up won't have more info in this case ever.

euank · 2016-12-07T06:16:16Z

rkt/attach.go

+	}
+
+	if err = stage0.Attach(p.Path(), podPID, *appName, stage1RootFS, uuid, attachArgs); err != nil {
+		stderr.PrintE("enter failed", err)


'attach' might be clearer here

euank · 2016-12-07T06:17:45Z

rkt/attach.go

+	attachArgs := []string{
+		fmt.Sprintf("--app=%s", appName),
+		fmt.Sprintf("--debug=%t", debug),
+	}


Renaming to "createAttachFlags" might help

euank · 2016-12-07T06:21:02Z

rkt/attach.go

+		return nil, fmt.Errorf("incompatible endpoints %q, cannot simultaneously attach TTY and streams", attachMode)
+	}
+
+	attachArgs = append(attachArgs, []string{


we can drop the []string{}... bit, right?

Definitely, silly me :(

euank · 2016-12-07T06:27:56Z

stage1/attach/attach.go

+	}
+
+	args := stage1common.PrepareEnterCmd(false)
+	args = append(args, []string{


The []string{}... is redundant

lucab · 2016-12-13T17:59:23Z

PTAL. I've fixed the issues reported in previous reviews and cleaned up most of the the TODOs. There are still several in iottymux.go (especially in TTY code) but I don't want to block on those.

lucab · 2017-01-24T11:08:05Z

No, at the moment stream and tty don't log to journal. But that is probably going to change as soon as we touch into logmode support for k8s.

lucab · 2017-01-24T14:04:31Z

@squeed I added a bunch of changes for your comments and a test for sandbox-attach (killing some bugs while at it).

thomasfricke · 2017-01-25T14:19:59Z

@lucab @s-urbaniak preparing a training for I tried to work with rkt in minikube.

minikube start --vm-driver=kvm --container-runtime=rkt --network-plugin=cni

The version

rkt Version: 1.14.0

is quite outdated. Do you have some newer version, as the old one has
the issue with attaching to the tty and the networking inside does not
work?

I would like to use rkt as an alternative in demos and training, so it
would be nice if the version coming with minikube would work at least.

This command shoud work out of the box
kubectl run busybox --image=busybox sh

At the moment I work around this issue with
kubectl run busybox --image=busybox sleep 1000

and

kubectl exec -it busybox-434806045-3qcgt sh

This is ugly for demos. Any idea or hint?

Thanks in advance!

jonboulle · 2017-01-25T15:30:06Z

@thomasfricke have you filed an issue against minikube about this? Thomas Fricke <notifications@github.com> schrieb am Mi., 25. Jan. 2017, 15:20:

…

@lucab <https://github.com/lucab> @s-urbaniak <https://github.com/s-urbaniak> preparing a training for I tried to work with rkt in minikube. minikube start --vm-driver=kvm --container-runtime=rkt --network-plugin=cni The version rkt Version: 1.14.0 is quite outdated. Do you have some newer version, as the old one has the issue with attaching to the tty and the networking inside does not work? I would like to use rkt as an alternative in demos and training, so it would be nice if the version coming with minikube would work at least. This command shoud work out of the box kubectl run busybox --image=busybox sh At the moment I work around this issue with kubectl run busybox --image=busybox sleep 1000 and kubectl exec -it busybox-434806045-3qcgt sh This is ugly for demos. Any idea or hint? Thanks in advance! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3396 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACewNwlV9QPMYUwSHqeCoZWfQf0rBNL-ks5rV1oUgaJpZM4K4aqg> .

s-urbaniak · 2017-01-26T10:45:53Z

@thomasfricke I created kubernetes/minikube#1042

s-urbaniak

LGTM, the test passes for me locally, I made another pass, and tried it locally; let's ship it.

s-urbaniak

Sorry, I was just playing around with this a bit more, and attach doesn't play nice with restarting an app:

$ sudo -E rkt app add --debug $pod docker://busybox --stderr=stream --stdout=stream --name=echo --exec=/bin/sh -- -c 'while true; do echo "stdout" $(date); (>&2 echo "stderr" $(date)); sleep 1; done'
stage0: locking pod manifest
stage0: Loading image sha512-dec96773486195b575ff04628b8f816e3d7eceb608dd07f745513902d36ea82b
stage0: Writing image manifest
stage0: adding app to sandbox
stage0: stage0: app-add: systemctl daemon-reload output:
$ sudo -E rkt app start --debug $pod --app=echo
$ sudo -E rkt attach $pod
stdout Sat Jan 28 11:29:36 UTC 2017
stderr Sat Jan 28 11:29:36 UTC 2017
...
^C
$ sudo -E rkt app stop --debug $pod --app=echo
$ sudo -E rkt app start --debug $pod --app=echo
$ echo $?
0
$ sudo -E rkt attach $pod
iottymux: runtime failure: dial unix /rkt/iottymux/echo/sock-stdout: connect: connection refused
stage1-attach: error executing "iottymux": exit status 254
attach: attach failed: error executing stage1 entrypoint: exit status 254

In the sandbox I see:

[  OK  ] Started Journal Service.
[  OK  ] Reached target rkt apps target.
         Starting rkt supervisor-ready signaling...
[  OK  ] Started rkt supervisor-ready signaling.
[19486.122201] echo[12]: iottymux: Listening for stdout on /rkt/iottymux/echo/sock-stdout
[19486.122427] echo[12]: iottymux: Listening for stderr on /rkt/iottymux/echo/sock-stderr
[19493.574246] echo[49]: iottymux: runtime failure
[19493.574479] echo[49]:   └─unable to open stdout listener: listen unix /rkt/iottymux/echo/sock-stdout: bind: address already in use

I think the above can also be encoded in a functional test.

Initial analysis:
When we stop the app using app stop, the iomux@echo.service is successfully stopped too, but the socket files are still present:

$ enterpod 
entering pod 819a9dca	echo	registry-1.docker.io/library/busybox:latest	running	38 seconds ago	38 seconds ago	default:ip4=172.16.28.109
bash-4.3# systemctl status iomux@echo.service
● iomux@echo.service - Streaming I/O handler for echo
   Loaded: loaded (/usr/lib/systemd/system/iomux@.service; static; vendor preset: enabled)
   Active: inactive (dead) since Sat 2017-01-28 12:42:11 CET; 27s ago
  Process: 13 ExecStart=/iottymux --action=iomux --app=%i (code=killed, signal=TERM)
 Main PID: 13 (code=killed, signal=TERM)

Jan 28 12:41:59 rkt-819a9dca-9b56-48ed-9e7d-4b1ec93ce15a systemd[1]: Started Streaming I/O handler for echo.
Jan 28 12:41:59 rkt-819a9dca-9b56-48ed-9e7d-4b1ec93ce15a echo[13]: iottymux: Listening for stdout on /rkt/iottymux/echo/sock-stdout
Jan 28 12:41:59 rkt-819a9dca-9b56-48ed-9e7d-4b1ec93ce15a echo[13]: iottymux: Listening for stderr on /rkt/iottymux/echo/sock-stderr
Jan 28 12:42:11 rkt-819a9dca-9b56-48ed-9e7d-4b1ec93ce15a systemd[1]: Stopping Streaming I/O handler for echo...
Jan 28 12:42:11 rkt-819a9dca-9b56-48ed-9e7d-4b1ec93ce15a systemd[1]: Stopped Streaming I/O handler for echo.
bash-4.3# cd /rkt/iottymux/echo/
bash-4.3#  
endpoints    env          sock-stderr  sock-stdout

s-urbaniak · 2017-01-28T14:25:20Z

resource cleanup is not properly handled upon SIGTERM, I'll submit a fix and a test.

s-urbaniak · 2017-01-29T16:27:24Z

fix for the above is available under s-urbaniak@cb346c6

s-urbaniak · 2017-01-30T14:27:53Z

Documentation/devel/log-attach-design.md

+
+This component takes care of multiplexing TTY and receiving clients for attaching.
+
+It is started as an instance of the templatedd `ttymux@.service` service by a `After=` dependency from the application.


"of the templated"

lucab · 2017-01-30T16:50:43Z

@s-urbaniak Thanks, I picked up your commit in this branch.

lucab · 2017-02-01T09:33:08Z

ok to test

s-urbaniak

LGTM once green

This commit introduces experimental support for attachable applications. This consists of: * a new `attach` subcommand * a set of per-app flags to control stdin/stdout/stderr modes * a stage1 `iottymux` binary for multiplexing and attaching * two new templated stage1 services, `iomux` and `ttymux`

lucab · 2017-02-02T12:20:18Z

Green, merging.

@s-urbaniak

As rkt 1.24.0 supports an experimental attach command, minikube iso image should also contain the new version. See also rkt/rkt#3396 (comment) /cc @s-urbaniak

lucab added component/stage0 component/stage1 do not merge needs/tests labels Nov 21, 2016

lucab force-pushed the to-upstream/stage1-iottymux branch from cb003e9 to 6052116 Compare November 22, 2016 12:42

squeed reviewed Nov 22, 2016

View reviewed changes

lucab mentioned this pull request Nov 23, 2016

stage0: attach to the app's stdin/stdout/stderr #1799

Closed

lucab force-pushed the to-upstream/stage1-iottymux branch 3 times, most recently from add802e to 682b3fd Compare November 28, 2016 17:28

s-urbaniak suggested changes Nov 29, 2016

View reviewed changes

lucab force-pushed the to-upstream/stage1-iottymux branch 4 times, most recently from fb0d859 to 1f069cc Compare December 6, 2016 15:05

euank reviewed Dec 7, 2016

View reviewed changes

lucab force-pushed the to-upstream/stage1-iottymux branch 8 times, most recently from 1218b4d to 3dddf85 Compare December 13, 2016 17:51

lucab removed the do not merge label Dec 13, 2016

lucab force-pushed the to-upstream/stage1-iottymux branch from 0f5cc70 to ae2341b Compare January 24, 2017 11:09

s-urbaniak approved these changes Jan 27, 2017

View reviewed changes

s-urbaniak suggested changes Jan 28, 2017

View reviewed changes

s-urbaniak reviewed Jan 30, 2017

View reviewed changes

lucab force-pushed the to-upstream/stage1-iottymux branch from ae2341b to b28afd4 Compare January 30, 2017 16:49

lucab force-pushed the to-upstream/stage1-iottymux branch 2 times, most recently from b3685f2 to 86a098f Compare January 31, 2017 11:33

s-urbaniak approved these changes Feb 1, 2017

View reviewed changes

lucab and others added 5 commits February 1, 2017 13:29

tests: add a rkt-attach smoke test

e0b95f9

docs: add user doc for rkt-attach

0223674

docs/devel: add logging and attaching design doc

c0ad687

apps/attach: clean resources on termination

2b025fb

lucab force-pushed the to-upstream/stage1-iottymux branch from 86a098f to 5a3fc14 Compare February 1, 2017 13:43

tests: wait for attachable status before attaching

73ef404

lucab force-pushed the to-upstream/stage1-iottymux branch from 5a3fc14 to 73ef404 Compare February 1, 2017 15:00

lucab merged commit fefb966 into rkt:master Feb 2, 2017

omribahumi mentioned this pull request Feb 11, 2017

stdout in stream mode doesn't buffer output before attachment #3587

Open

dongsupark pushed a commit to endocode/minikube that referenced this pull request Feb 14, 2017

rkt: bump to 1.24.0

91e023a

As rkt 1.24.0 supports an experimental attach command, minikube iso image should also contain the new version. See also rkt/rkt#3396 (comment) /cc @s-urbaniak

dongsupark mentioned this pull request Feb 14, 2017

rkt: bump to 1.24.0 kubernetes/minikube#1128

Merged


		This component takes care of multiplexing TTY and receiving clients for attaching.

		It is started as an instance of the templatedd `ttymux@.service` service by a `After=` dependency from the application.

rkt: add experimental support for attachable applications #3396

rkt: add experimental support for attachable applications #3396

Conversation

lucab commented Nov 21, 2016 • edited Loading

squeed Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

squeed commented Nov 22, 2016

squeed commented Nov 22, 2016

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

s-urbaniak 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

euank 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

lucab commented Dec 13, 2016

lucab commented Jan 24, 2017

lucab commented Jan 24, 2017

thomasfricke commented Jan 25, 2017

jonboulle commented Jan 25, 2017 via email

s-urbaniak commented Jan 26, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

s-urbaniak left a comment

Choose a reason for hiding this comment

s-urbaniak commented Jan 28, 2017

s-urbaniak commented Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucab commented Jan 30, 2017

lucab commented Feb 1, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

lucab commented Feb 2, 2017

lucab commented Nov 21, 2016 •

edited

Loading

squeed Nov 22, 2016 •

edited

Loading