Update start servers by saijel · Pull Request #163 · claritylab/lucida

saijel · 2017-04-05T03:38:06Z

list of changes:

remove the dependency of gnome-terminal in services makefiles
add the ability to start all services (secure and non-secure hosts) using a tmux session with a tmux window for each service
add the ability to make the host secure in order to add the ASR functionality by generating self-signed certificates for the host

jhauswald · 2017-04-05T06:02:46Z

 import logging


+


I'll remove all of these extra white spaces. I stupidly diff -w to ignore the whitespace.

jhauswald · 2017-04-05T06:03:16Z

+                threaded=True, ssl_context=('certs/server.crt', 'certs/server.key'))
+    # For http
+    else:
+        print 'Starting non-secure flask'


So non-secure means you can't use ASR right? Is that stated somewhere?

I'm adding a comment within this file but also I think it'll be more clear once I write out the instructions on how to start servers with and without ASR.

jhauswald · 2017-04-05T06:04:31Z


 start_server: 
-	gnome-terminal -x bash -c "./gradlew run $(filter-out $@,$(MAKECMDGOALS)); read -n1"
+	./gradlew run


can you speak to what $(filter-out $@,$(MAKECMDGOALS)); read -n1 was doing?

I honestly don't know what these were doing. Every time I run with $(filter-out $@,$(MAKECMDGOALS)); read -n1 it says these commands are not found.

According to the internet: $(filter-out PATTERN...,TEXT) returns all whitespace-separated words in TEXT that do not match any of the PATTERN words, removing the words that do match one or more. And read -n1 means read one character at a time. But MAKECMDGOALS is not defined anywhere.

jhauswald · 2017-04-05T06:04:53Z


-gnome-terminal -x bash -c "python kaldigstserver/worker.py -u ws://localhost:8081/worker/ws/speech -c sample_english_nnet2.yaml; read -n1"
+python kaldigstserver/worker.py -u ${ASR_ADDR_PORT}/worker/ws/speech -c sample_english_nnet2.yaml
+


why these extra lines?

Getting rid of the extra spaces.

jhauswald · 2017-04-05T06:05:15Z

+if [ "$1" == "secure" ]; then
+    echo "Enabling secure host"
+    # Getting the host IP address
+    export ASR_ADDR_PORT="wss://$(/sbin/ifconfig eth0 | grep 'inet addr:' | cut -d: -f2 | awk '{ print $1}'):8081"


This makes me a little nervous, so all ASR ports run on 8081 and its sort of hard coded all over the place right?

Yes, that's how it seems. Each service has a port that is hard-coded and used in multiple places.

Yes I know that I'm asking if you have an idea for a better design?

The issue is we need a common place to have a service registry that all servers and cmd center can access. In some other implementations the command center holds the registry, the microservices register themselves giving the cmd their host and port, and the cmd adds this to its service registry. This way the microservices can change their locations and just register themselves every time they are initialized.

jhauswald · 2017-04-05T06:06:08Z

+    unset SECURE_HOST
+fi
+
+declare -a commandcenter=("CMD" "$(pwd)/../lucida/commandcenter/")


I don't know what declare does, can you enlighten me?

the declare allows you to declare and initialize an array by specifying the list of elements (separated by white space) rather than initializing each element of an array separately

jhauswald · 2017-04-05T06:07:04Z

+	./DIGServer

 start_test: 
 	gnome-terminal -x bash -c "./run-test.sh $(filter-out $@,$(MAKECMDGOALS)); read -n1"


if you're removing all gnome-terminals, why not get rid of this one too?

There currently isn't a automated way to test all of the servers so when someone wants to run tests they have to go to each individual microservice folder and run start_test. In this situation it may be necessary to have a new terminal open as it won't be necessary to keep it running after the test has been run unlike a server.

I see your point but I think in that case there should be a better way to test. can you open an issue about moving start_test as unittest, similar to what we do internally? Look at the infrastructure we use

Okay, I created an issue. In the meantime let me look into our internal infrastructure.

jhauswald · 2017-04-05T06:07:47Z

@@ -19,3 +19,9 @@ local:

 start_all:


are these other make targets still relevant? In adding your features, are there any cleanups you can make that would make it easier to understand what you're adding?

Think about that across your PR. I know the make targets are defined one way, unclear whether its the best way

You see this comment?

I will remove start_all since start_all_tmux replaces that. As far as the way make targets are defined, they are defined in a consistent way across all services which makes it easy to loop through all services and start them.

sounds good

Once the PR is merged I will be sure to update the wikis/README to show the new way to start all servers (both with and without ASR).

jhauswald · 2017-04-05T06:08:46Z

@@ -0,0 +1,71 @@
+#!/bin/bash


jhauswald · 2017-04-05T06:09:52Z

+    facerecognition)
+
+# Create the session
+tmux new-session -s ${SESSION_NAME} -n vim -d


Tmux is an okay way of doing this, have you thought/investigated other ways of deploying/running web services? I know about supervisord. What else is out there?

@saijel you get a chance to think about this?

I've gotten as far seeing different options out there. This article has a good comparison of different options (http://centos-vn.blogspot.com/2014/06/daemon-showdown-upstart-vs-runit-vs.html). I'll need to do more research about what would be best for our situation.

jhauswald · 2017-04-15T16:08:09Z

I still need to test all this out but there are some discussion points as well

jhauswald · 2017-04-17T12:29:33Z

After testing:

what's the first vim window for?
can you add a check if I already have a window named lucida?
can you add a check to not nest tmux sessions if i'm already in tmux? This is generally bad practice.
how do i got about adding a new service? is that automatic or do I need to edit the bash script everytime? Is this related to your registry idea?

saijel · 2017-04-24T21:42:03Z

Answers below.

…

--- Saijel Mokashi https://clinc.com

On Mon, Apr 17, 2017 at 8:29 AM, Johann Hauswald ***@***.***> wrote: After testing: - what's the first vim window for? *I added this window to do development in. I guess it is not completely necessary. Should I remove it?* - can you add a check if I already have a window named lucida? *Yes, I added this check and updated the pull request.* - can you add a check to not nest tmux sessions if i'm already in tmux? This is generally bad practice. *Fixed in the above statement* - how do i got about adding a new service? is that automatic or do I need to edit the bash script everytime? Is this related to your registry idea?* Currently, you'd have to edit the bash script every time. I think the entire process of adding a microservice needs to be planned out to try to make it more automated. * — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#163 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AZQku_nKUCzHuCW5sYZF0_cuWpvT4R4fks5rw1s2gaJpZM4MztZ6> .

jhauswald · 2017-04-27T18:36:40Z

Went through all your responses, yes I would remove the development window from tmux. Agreed on everything else.

jhauswald · 2017-04-27T18:38:58Z

+# use the following command:
+# $tmux a -t lucida
+
+SESSION_NAME="lucida"


where's the check to see if you're already in a tmux session? What if I'm in a tmux session that is not called lucida?

jhauswald · 2017-04-27T18:40:11Z

Also what about the fixes that you have? can you make a quick MR for those?

jhauswald · 2017-05-01T12:24:38Z

+    facerecognition)
+
+# First window (0) -- vim
+tmux send-keys -t ${SESSION_NAME} 'vim' C-m


are you still generating a vim window? I don't think there's much use...

jhauswald · 2017-05-01T12:24:55Z

+    echo "Session ${SESSION_NAME} already exists."
+    exit 0;
+elif [ -n "$TMUX" ]; then
+    echo "Already in a tmux session"


saijel

@jhauswald removing the first vim window

This reverts commit d32ddd4.

update start servers

356927f

jhauswald reviewed Apr 5, 2017

View reviewed changes

removed whitespace and added ASR comments

2c94f72

replacing start_all.sh with start_all_tmux.sh

7e19001

jhauswald mentioned this pull request Apr 17, 2017

Adding a weather microservice #166

Closed

saijel added 2 commits April 24, 2017 21:38

adding check for exisiting tmux session

c876c57

fixing comment

ce75537

saijel added 2 commits April 25, 2017 18:50

updating environment variables and removing weather service

f02c3e5

stop tracking certs folder

f2a6da1

saijel assigned jhauswald Apr 26, 2017

saijel added help wanted and removed help wanted labels Apr 26, 2017

saijel closed this Apr 26, 2017

saijel reopened this Apr 26, 2017

saijel requested a review from jhauswald April 26, 2017 01:48

jhauswald reviewed Apr 27, 2017

View reviewed changes

jhauswald assigned saijel Apr 27, 2017

preventing tmux inception

eb2881e

jhauswald reviewed May 1, 2017

View reviewed changes

removing the first vim window

e4b41bd

saijel commented May 1, 2017

View reviewed changes

jhauswald merged commit d32ddd4 into claritylab:fixes May 5, 2017

jhauswald added a commit that referenced this pull request May 5, 2017

Revert "Update start servers (#163)"

90a195c

This reverts commit d32ddd4.

saijel mentioned this pull request May 5, 2017

Fixes start servers #174

Closed

saijel mentioned this pull request May 8, 2017

New start_all functionality and allowing asr #177

Merged


		gnome-terminal -x bash -c "python kaldigstserver/worker.py -u ws://localhost:8081/worker/ws/speech -c sample_english_nnet2.yaml; read -n1"
		python kaldigstserver/worker.py -u ${ASR_ADDR_PORT}/worker/ws/speech -c sample_english_nnet2.yaml

Conversation

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!

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!

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!

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!