PoC DO NOT MERGE: Implementation of `swarmctl service exec` #2359

fntlnz · 2017-08-28T18:59:07Z

I want to address #1895 - Please beaware that this is just an experiment
to get my hands dirty with the codebase and to understand how things
works.

At the current stage this basically is a gRPC service on the managers
that connects the client to the client to the container executing the
task of a specific service.

It does not support having a tty yet.

Feedback and What I need to go on:

I need some advice in designing this and if possible code review.
Since this is just a PoC I'm not yet writing tests, this is just
something I need to understand a codebase where I'm not even that
familiar to do TDD.
There are probably, I already see some of them contexts that are not
closed.

Try it:
swarmctl service exec

(Yes I know that one expects to have sh) in the end or something like
that. It's too early

So why did you open the PR if you're still working?

Because I need feedback 😉

Signed-off-by: Lorenzo Fontana lo@linux.com

aaronlehmann

I think this is a cool proof of concept. I'm impressed at how little code it takes, though I realize this isn't streaming yet and doesn't handle shared attachments.

We've talked about a few different ways of doing attach and exec. One approach, which is probably the simplest from a design perspective, is to make something similar to "logbroker" that multiplexes the streams over gRPC. It's relatively straightforward because the design and implementation would end up very similar to the logs functionality, and there's a lot of code for logging that could be used as example or possibly reused directly. That's sort of the direction I see this PR going right now. It has a lot of things missing right now since it's in the proof of concept stage, but if all those things were added, I think it would (or should) end up looking quite similar to logbroker.

The other thing we've discussed is building this functionality on top of SSH, with the manager setting up SSH sessions between the worker and the client, but not actually sitting in between and seeing all the traffic. This has some significant performance benefits, partly because the traffic doesn't have to pass through a manager, and also because SSH is probably much better suited for interactive flows than gRPC. I believe @stevvooe did some experiments with gRPC and encountered some performance problems in the past.

If you want to keep prototyping this, a possible direction would be to implement streaming within your current approach (what I'm calling the logbroker method) and see whether the performance turns out to be acceptable. That might help inform the high-level design for this feature (i.e. SSH vs. gRPC).

ping @aluzzardi @stevvooe

aaronlehmann · 2017-09-02T07:07:46Z

api/control.proto

@@ -29,6 +41,9 @@ service Control {
 	rpc ListTasks(ListTasksRequest) returns (ListTasksResponse) {
 		option (docker.protobuf.plugin.tls_authorization) = { roles: "swarm-manager" };
 	};
+	rpc Attach(stream TaskExecStream) returns (stream TaskExecStream) {
+		option (docker.protobuf.plugin.tls_authorization) = { roles: "swarm-manager"};
+	}


If this PR moves forward, this should be part of a separate service instead of adding to controlapi. You can look at logs as an example. Similarly, TaskExecStream and TaskExec above should move out of this file.

Thanks for pointing me to how the logsbroker is impelemented. Probably the whole "stdout" part can be taken from here.

aaronlehmann · 2017-09-02T07:11:40Z

agent/session.go

+			AttachStdout: true,
+			AttachStderr: true,
+			AttachStdin:  true,
+			Cmd:          strings.Split(string(data.Message), " "),


I would look into making this an explicit protobuf field in the exec flow instead of reusing Message.

aaronlehmann · 2017-09-02T07:14:17Z

api/control.proto

@@ -29,6 +41,9 @@ service Control {
 	rpc ListTasks(ListTasksRequest) returns (ListTasksResponse) {
 		option (docker.protobuf.plugin.tls_authorization) = { roles: "swarm-manager" };
 	};
+	rpc Attach(stream TaskExecStream) returns (stream TaskExecStream) {
+		option (docker.protobuf.plugin.tls_authorization) = { roles: "swarm-manager"};
+	}


Also, since this is executing a task, not attaching to the existing task, you should call this Exec and formally pass the command an arguments to be exec'd.

aaronlehmann · 2017-09-02T07:19:32Z

manager/controlapi/server.go

+
+func (s *ServerTaskExecChannels) In(containerid string) chan []byte {
+	s.registerExecChannels(containerid)
+	return s.in[containerid]


This works for a proof of concept, but I think a full implementation would want to handle multiple attachments to the same stream, or at the very least keep track of which ones already have someone attached, and not return the same channel again. logbroker is a good example of how to do this.

Also, this will need locking or some other concurrency strategy.

I see that logbroker is using some kind of filtering, however if anyone can help with resources or anything else to understand how are impelemented would be useful

I haven't (yet) dealt with closing contexts and locking resources.
I'm still in the phase where I need to understand step-by-step how things are 🔨

Also I think there are multiple places where routines are just blocking in the wrong way

fntlnz · 2017-09-04T01:57:26Z

@aaronlehmann Today I managed to have some kind of working terminal by sending the stream byte by byte, the current things in to do are:

Handling multiple services (the channel map approach doesn't even work atm and for this I'll take inspiration from logbroker)
Handling a proper tty

fntlnz · 2017-09-04T02:15:45Z

cmd/swarmctl/service/exec.go

 				attachCl.Send(&api.TaskExecStream{
 					Containerid: containerId,
-					Message:     s.Bytes(),
+					Message:     []byte{curLine},


I know this isn't super neat but I'm experimenting around the possibility to handle the stream byte-per-byte

I want to address moby#1895 - Please beaware that this is just an experiment to get my hands dirty with the codebase and to understand how things works. At the current stage this basically is a gRPC service on the managers that connects the client to the client to the container executing the task of a specific service. It does not have TTYs. Feedback and What I need to go on: - I need some advice in designing this and if possible code review. - Since this is just a PoC I'm not yet writing tests, this is just something I need to understand a codebase where I'm not even that familiar to do TDD. - There are probably, I already see some of them contexts that are not closed. Try it: swarmctl service exec <service-id> (Yes I know that one expects to have `sh`) in the end or something like that. It's too early Signed-off-by: Lorenzo Fontana <lo@linux.com>

Signed-off-by: Lorenzo Fontana <lo@linux.com>

fntlnz changed the title ~~PoC DO NOT MERGE: PoC implementation~~ PoC DO NOT MERGE: PoC implementation of swarmctl service exec Aug 28, 2017

nishanttotla added the kind/feature label Aug 31, 2017

aaronlehmann reviewed Sep 2, 2017

View reviewed changes

fntlnz commented Sep 4, 2017

View reviewed changes

fntlnz changed the title ~~PoC DO NOT MERGE: PoC implementation of swarmctl service exec~~ PoC DO NOT MERGE: Implementation of swarmctl service exec Sep 4, 2017

fntlnz added 2 commits September 9, 2017 17:24

Send stdin and stout as single bytes

af3ede7

Signed-off-by: Lorenzo Fontana <lo@linux.com>

fntlnz force-pushed the feature/service-exec branch from 77ceb6c to af3ede7 Compare September 9, 2017 15:24

fntlnz closed this May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC DO NOT MERGE: Implementation of `swarmctl service exec` #2359

PoC DO NOT MERGE: Implementation of `swarmctl service exec` #2359

fntlnz commented Aug 28, 2017 •

edited

aaronlehmann left a comment

aaronlehmann Sep 2, 2017

fntlnz Sep 4, 2017

aaronlehmann Sep 2, 2017

aaronlehmann Sep 2, 2017

aaronlehmann Sep 2, 2017

fntlnz Sep 4, 2017

fntlnz Sep 4, 2017 •

edited

fntlnz Sep 4, 2017

fntlnz commented Sep 4, 2017

fntlnz Sep 4, 2017

PoC DO NOT MERGE: Implementation of swarmctl service exec #2359

PoC DO NOT MERGE: Implementation of swarmctl service exec #2359

Conversation

fntlnz commented Aug 28, 2017 • edited

So why did you open the PR if you're still working?

aaronlehmann 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

fntlnz Sep 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fntlnz commented Sep 4, 2017

Choose a reason for hiding this comment

PoC DO NOT MERGE: Implementation of `swarmctl service exec` #2359

PoC DO NOT MERGE: Implementation of `swarmctl service exec` #2359

fntlnz commented Aug 28, 2017 •

edited

fntlnz Sep 4, 2017 •

edited