Add exec-agent #2163

voievodin · 2016-08-18T14:55:36Z

What does this PR do?

Adds a golang based implementation of exec-agent(machine-agent described in #1943).
The main goal of the exec-agent is to reduce API instance load by putting commands execution/output streaming/logs management on the side of machines.

Exec-agent consist of two main parts

che-websocket-terminal(copied from che-lib)
Commands execution/logs streaming

Available API:

Exec-agent is deployed in a couple with terminal and can be accessed via the same host and port.

Go dependencies are managed by Godep, so the PR contains vendor sources as well.

@skabashnyuk, @garagatyi, @akorneta, @tolusha check this out

codenvy-ci · 2016-08-18T14:58:21Z

Build # 143 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/143/ to view the results.

benoitf · 2016-08-18T15:04:45Z

question:

is that clients will still be able to use
/api/workspace/<workspace-id>/machine/<machine-id>/command
(few days ago it was changed as it was /api/machine/machine-id/command)

or it means clients will require to be changed to call ws-agent REST-API directly ?

benoitf · 2016-08-18T15:06:14Z

.gitignore

@@ -17,7 +17,6 @@ test-output/

 # Compiled source #
 ###################
-*.com


do we need .com files ?

no we don't but we need to keep vendor/github.com directory

(to authorize directories with *com) !*.com/

i'll fix it thank you

voievodin · 2016-08-18T16:13:07Z

@benoitf check this out there are several issues which describe the process of moving from current approach to a new one, but as a result i think yes the clients will have to use exec-agent's API

codenvy-ci · 2016-08-19T08:03:59Z

Build # 153 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/153/ to view the results.

garagatyi · 2016-09-12T08:33:18Z

exec-agent/.gitignore

@@ -0,0 +1,3 @@
+logs/
+exec-agent
+g


Please add new line in the end of file. Also is the rule g really needed?

garagatyi · 2016-09-12T08:35:42Z

Please rebase on tom of master

garagatyi · 2016-09-12T08:37:28Z

exec-agent/auth/auth.go

@@ -0,0 +1,39 @@
+// Moved from term/server.go


Is this comment needed?

I think we need license headers on all new files

codenvy-ci · 2016-09-14T21:33:36Z

Build # 401 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/401/ to view the results.

codenvy-ci · 2016-09-15T23:29:33Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/410/

codenvy-ci · 2016-09-16T08:08:39Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/411/

codenvy-ci · 2016-09-19T15:18:08Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/433/

codenvy-ci · 2016-09-27T09:30:42Z

Build # 524 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/524/ to view the results.

codenvy-ci · 2016-09-29T20:15:01Z

Build # 571 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/571/ to view the results.

codenvy-ci · 2016-09-29T22:34:55Z

Build # 572 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/572/ to view the results.

codenvy-ci · 2016-10-03T01:15:04Z

Build # 578 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/578/ to view the results.

codenvy-ci · 2016-10-03T01:29:06Z

Build # 579 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/579/ to view the results.

codenvy-ci · 2016-10-03T03:49:52Z

Build # 580 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/580/ to view the results.

garagatyi · 2016-10-16T20:17:17Z

exec-agent/auth/auth.go

+	if tokenParam == "" {
+		return rest.Unauthorized(errors.New("Authentication failed: missing 'token' query parameter"))
+	}
+	req, err := http.NewRequest("GET", ApiEndpoint+"/machine/token/user/"+tokenParam, nil)


I think spaces near plus signs would be more standart syntax, no?

golang fmt formats it in this way

voievodin · 2016-10-27T08:56:16Z

Waiting for CQ

garagatyi

This PR is huge amount of work and it looks really awesome. I'm eager to see it merged.
I'm still reviewing but please consider my my comments. Maybe some of them are dummy, sorry if so.

garagatyi · 2016-11-16T09:45:24Z

exec-agent/assembly/assembly.xml

+          xsi:schemaLocation="http://maven.apache.org/plugins/maven-assembly-plugin/assembly/1.1.0 http://maven.apache.org/xsd/assembly-1.1.0.xsd">
+    <id>${item}</id>
+    <includeBaseDirectory>true</includeBaseDirectory>
+    <baseDirectory>terminal</baseDirectory>


Maybe rename this one?

Maybe, i haven't changed it yet because:

executable name should be also changed

agent name should be also changed

i think that the most suitable name is exec-agent but it is another, not really small bunch of work which i think should be done separately

garagatyi · 2016-11-16T10:56:49Z

exec-agent/docs/rest_api.md

+- `channel`(optional) - the id of the channel which should be subscribed to the process events
+- `types`(optional) - works only in couple with specified `channel`, defines
+the events which will be sent by the process to the `channel`. Several values may be specified,
+e.g. `channel=channel-1&types=stderr,stdout`. Possible type values:


Can you describe default behavior here

garagatyi · 2016-11-16T10:58:35Z

exec-agent/docs/rest_api.md

+
+```json
+{
+    "pid": 1,


Is there a description in the docs that this is not a real PID but our API ID of a process?

No, i thought it was obvious from the process.start method, which goes first in the doc and contains both nativePid and pid at the same time. But i can add a small description to the documentation about response data.

yes, please add

garagatyi · 2016-11-16T12:47:52Z

exec-agent/docs/rest_api.md

+
+_GET /process_
+
+- `all`(optional) - if `true` then all the processes including _dead_ ones will be returned(respecting paging ofc), otherwise if `all` is `false`, or not specified or invalid then only _alive_ processes will be returned


I think if you say otherwise you can omit ifallisfalse, or not specified or invalid

garagatyi · 2016-11-16T12:56:25Z

exec-agent/docs/ws_api.md

+
+- __name__ - the name of the command
+- __commandLine__ - command line to execute
+- __type__(optional) - command type


Should we describe in this doc that it is not used in any way by the API?

Who knows, we don't use it right now but we may use it in future, so i would probably let it as it is

garagatyi · 2016-11-16T13:09:09Z

exec-agent/docs/ws_api.md

+  "id": "0x12345",
+  "params": {
+    "pid": 2,
+    "eventTypes": "stdout,stderr",


What if channel is not subscribed to one(or all) of the event types?

garagatyi · 2016-11-16T13:10:08Z

exec-agent/docs/ws_api.md

+  "id": "0x12345",
+  "result": {
+    "pid": 2,
+    "text": "Successfully unsubscribed"


How error response looks like? How it differs from success response?

garagatyi · 2016-11-16T13:11:21Z

exec-agent/docs/rest_api.md

+_POST /process_
+
+- `channel`(optional) - the id of the channel which should be subscribed to the process events
+- `types`(optional) - works only in couple with specified `channel`, defines


In other places docs states comma separated types, maybe this one should do the same?

garagatyi · 2016-11-16T13:12:34Z

exec-agent/docs/ws_api.md

+  "id": "0x12345",
+  "params": {
+    "pid": 2,
+    "eventTypes": "process_status"


How error response looks like?
How it differs from success response?
What if channel is not subscribed to one(or all) of the event types?

i will document error responses as well

garagatyi · 2016-11-16T13:15:53Z

exec-agent/docs/ws_api.md

+
+##### Response
+
+For the command `printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10`, the result will look like


Is response limited in size? Is it chunked with websocket protocol? Does it have paging? Does it show only logs produced before this request got to the server?

Request section for this response covers your questions.

Does it show only logs produced before this request got to the server?

It will use logs [first logged message, time.Now()] where time.Now is a time when server receives request

ok. And what about size of response in bytes, is it limited? Does websocket protocol handle that somehow?

codenvy-ci · 2016-11-16T21:27:39Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1041/

garagatyi · 2016-11-17T15:24:58Z

exec-agent/assembly/assembly.xml

+          xsi:schemaLocation="http://maven.apache.org/plugins/maven-assembly-plugin/assembly/1.1.0 http://maven.apache.org/xsd/assembly-1.1.0.xsd">
+    <id>${item}</id>
+    <includeBaseDirectory>true</includeBaseDirectory>
+    <baseDirectory>terminal</baseDirectory>


garagatyi · 2016-11-17T15:28:52Z

exec-agent/auth/auth.go

+	if tokenParam == "" {
+		return rest.Unauthorized(errors.New("Authentication failed: missing 'token' query parameter"))
+	}
+	req, err := http.NewRequest("GET", ApiEndpoint+"/machine/token/user/"+tokenParam, nil)


garagatyi · 2016-11-17T15:29:30Z

exec-agent/docs/rest_api.md

+
+```json
+{
+    "pid": 1,


yes, please add

garagatyi · 2016-11-17T15:30:04Z

exec-agent/docs/ws_api.md

+  "id": "0x12345",
+  "params": {
+    "pid": 2,
+    "eventTypes": "process_status"


garagatyi · 2016-11-17T15:32:10Z

exec-agent/docs/ws_api.md

+
+##### Response
+
+For the command `printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10`, the result will look like


ok. And what about size of response in bytes, is it limited? Does websocket protocol handle that somehow?

garagatyi · 2016-11-17T15:35:33Z

exec-agent/docs/ws_api.md

+```
+
+
+#### Get process logs


Fix heading please

garagatyi · 2016-11-17T15:47:35Z

exec-agent/main.go

+	flag.BoolVar(&auth.Enabled, "enable-auth", false, "Whether authenticate on workspace master or not")
+	flag.StringVar(&auth.ApiEndpoint,
+		"auth-api-endpoint",
+		os.Getenv("CHE_API_ENDPOINT"),


this env var was recently renamed to CHE_API. Please also rename --auth-api-endpoint parameter

garagatyi · 2016-11-17T15:49:14Z

exec-agent/main.go

+	flag.StringVar(&auth.ApiEndpoint,
+		"auth-api-endpoint",
+		os.Getenv("CHE_API_ENDPOINT"),
+		"Auth api-endpoint, by default 'CHEAPI-ENDPOINT' environment variable is used for this")


fix description of default value

garagatyi · 2016-11-17T15:50:16Z

exec-agent/main.go

+		"Auth api-endpoint, by default 'CHEAPI-ENDPOINT' environment variable is used for this")
+
+	// Process configuration
+	flag.IntVar(&process.CleanupPeriodInMinutes, "process-cleanup-period", 2, "How often processs cleanup will happen(in minutes)")


Why not use ENV var to set all of the params? It is getting popular after refactoring of Che CLI

garagatyi · 2016-11-17T15:51:03Z

exec-agent/main.go

+	// Process configuration
+	flag.IntVar(&process.CleanupPeriodInMinutes, "process-cleanup-period", 2, "How often processs cleanup will happen(in minutes)")
+	flag.IntVar(&process.CleanupThresholdInMinutes,
+		"process-lifetime",


I think it is more process cleanup than lifetime

codenvy-ci · 2016-11-18T11:25:19Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1063/

garagatyi · 2016-11-21T15:58:46Z

exec-agent/main.go

+	}
+
+	serverAddress string
+	staticFlag    string


Can you explain why these variables are called flags? usually that term is used with boolean variables

garagatyi · 2016-11-21T16:00:17Z

exec-agent/main.go

+	// Server configuration
+	flag.StringVar(&serverAddress, "addr", ":9000", "IP:PORT or :PORT the address to start the server on")
+	flag.StringVar(&staticFlag, "static", "./static/", "path to static content")
+	flag.StringVar(&pathFlag, "path", "", "Path of the pty server. Go regexp syntax is suported.")


Is this whole path to pty or just prefix?

garagatyi · 2016-11-21T16:04:52Z

exec-agent/main.go

+	flag.Parse()
+
+	// print configuration
+	fmt.Println("Exec-agent configuration")


I think code can be cleaner if you split it into methods print summary, normalize base path, cleanup resources, setup routes, etc

garagatyi · 2016-11-21T16:08:25Z

exec-agent/pom.xml

+                <artifactId>license-maven-plugin</artifactId>
+                <configuration>
+                    <excludes>
+                        <exclude>*.*</exclude>


Why we need to exclude all the files from license header check?

This is temporary solution, i will remove it and handle go files as well

garagatyi · 2016-11-21T16:15:24Z

exec-agent/process/common.go

+	for _, t := range strings.Split(types, ",") {
+		switch strings.ToLower(strings.TrimSpace(t)) {
+		case "stderr":
+			mask |= StderrBit


If this code is in common file, then maybe it would be a good idea to put these constants here too?

This file is rather webscoket & rest apis commons, i will rename it to service_commons.go
in accordance to rest_service.go, ws_service.go names

garagatyi · 2016-11-22T09:31:06Z

exec-agent/process/file_logger_test.go

@@ -0,0 +1,114 @@
+package process_test


Does maven run these tests on its test phase?

Nope, i am going to add tests phase

garagatyi · 2016-11-22T10:29:41Z

exec-agent/rpc/model.go

+//
+// Request.
+// It's a message from the physical websocket connection client to the exec-agent server.
+// Request(in it's origin form) is considered is considered to be unidirectional.


Typo - is considered is used twice

garagatyi · 2016-11-22T10:45:53Z

...e/src/main/java/org/eclipse/che/api/workspace/server/launcher/TerminalAgentLauncherImpl.java

    @Inject
    public TerminalAgentLauncherImpl(@Named("che.agent.dev.max_start_time_ms") long agentMaxStartTimeMs,
-                                     @Named("che.agent.dev.ping_delay_ms") long agentPingDelayMs) {
+                                     @Named("che.agent.dev.ping_delay_ms") long agentPingDelayMs,
+                                     @Named("machine.terminal_agent.run_command") String runCommand) {


It is exec agent and this property is new, maybe we can call it as exec agent run command?

I don't think so, it may be confusing for people, as long as executable is named che-websocket-terminal and agent name is terminal, the command should be named appropriately.

codenvy-ci · 2016-11-29T19:52:44Z

Build # 1157 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1157/ to view the results.

codenvy-ci · 2016-11-30T15:10:38Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1174/

codenvy-ci · 2016-11-30T17:51:24Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1175/

codenvy-ci · 2016-12-02T00:15:41Z

Build # 1194 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1194/ to view the results.

voievodin · 2016-12-02T09:04:53Z

Gorilla mux was replaced with httprouter, waiting for CQ

codenvy-ci · 2016-12-02T10:33:58Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1198/

codenvy-ci · 2016-12-05T10:41:24Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1211/

codenvy-ci · 2016-12-05T14:25:55Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1216/

codenvy-ci · 2016-12-05T20:48:25Z

Build # 1222 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/1222/ to view the results.

codenvy-ci · 2016-12-06T15:49:33Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1231/

codenvy-ci · 2016-12-08T15:33:31Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1263/

voievodin added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 18, 2016

voievodin self-assigned this Aug 18, 2016

benoitf reviewed Aug 18, 2016
View reviewed changes

voievodin force-pushed the exec-agent branch from 380309c to 9be9ad1 Compare August 19, 2016 07:59

garagatyi reviewed Sep 12, 2016
View reviewed changes

exec-agent/.gitignore

@@ -0,0 +1,3 @@

logs/

exec-agent

g

Copy link

garagatyi Sep 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add new line in the end of file. Also is the rule g really needed?

garagatyi reviewed Sep 12, 2016
View reviewed changes

garagatyi reviewed Oct 16, 2016

View reviewed changes

dkuleshov mentioned this pull request Oct 27, 2016

Rework clients to use new command API of new exec agent #1947

Closed

garagatyi suggested changes Nov 16, 2016

View reviewed changes

garagatyi suggested changes Nov 17, 2016

View reviewed changes

garagatyi suggested changes Nov 21, 2016

View reviewed changes

garagatyi suggested changes Nov 22, 2016

View reviewed changes

voievodin force-pushed the exec-agent branch 2 times, most recently from 19a4927 to a34f616 Compare December 1, 2016 16:55

voievodin force-pushed the exec-agent branch from 68917c2 to c8f1977 Compare December 6, 2016 11:49

garagatyi approved these changes Dec 8, 2016

View reviewed changes

voievodin force-pushed the exec-agent branch from c8f1977 to a79f9ba Compare December 8, 2016 12:27

Add exec-agent

6fba10b

voievodin force-pushed the exec-agent branch from a79f9ba to 6fba10b Compare December 8, 2016 13:36

voievodin mentioned this pull request Dec 8, 2016

Remove lib module and add axec-agent codenvy/codenvy#1307

Merged

voievodin merged commit ab9b094 into master Dec 8, 2016

voievodin deleted the exec-agent branch January 17, 2017 08:23

skabashnyuk mentioned this pull request Feb 10, 2017

Remove usage of the Machine API in the Chedir #3967

Closed

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

Add exec-agent (eclipse-che#2163)

dee7744

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 2, 2017


		_GET /process_

		- `all`(optional) - if `true` then all the processes including _dead_ ones will be returned(respecting paging ofc), otherwise if `all` is `false`, or not specified or invalid then only _alive_ processes will be returned


		##### Response

		For the command `printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10`, the result will look like

Add exec-agent #2163

Add exec-agent #2163

Conversation

voievodin commented Aug 18, 2016 • edited Loading

What does this PR do?

codenvy-ci commented Aug 18, 2016

benoitf commented Aug 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf Aug 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voievodin commented Aug 18, 2016 • edited Loading

codenvy-ci commented Aug 19, 2016

Choose a reason for hiding this comment

garagatyi commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Sep 14, 2016

codenvy-ci commented Sep 15, 2016

codenvy-ci commented Sep 16, 2016

codenvy-ci commented Sep 19, 2016

codenvy-ci commented Sep 27, 2016

codenvy-ci commented Sep 29, 2016

codenvy-ci commented Sep 29, 2016

codenvy-ci commented Oct 3, 2016

codenvy-ci commented Oct 3, 2016

codenvy-ci commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voievodin commented Oct 27, 2016

garagatyi 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

codenvy-ci commented Nov 16, 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

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

codenvy-ci commented Nov 18, 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

voievodin Nov 29, 2016 • edited Loading

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

voievodin Nov 29, 2016 • edited Loading

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

codenvy-ci commented Nov 29, 2016

voievodin commented Aug 18, 2016 •

edited

Loading

benoitf Aug 18, 2016 •

edited

Loading

voievodin commented Aug 18, 2016 •

edited

Loading

voievodin Nov 29, 2016 •

edited

Loading

voievodin Nov 29, 2016 •

edited

Loading