mineshaftgap / d4m-nfs

Docker for Mac with NFS for performance improvements over osxfs
295 stars 26 forks source link

Bash command not found for command substitution syntax wrapper around ps and grep #1

Closed patakijv closed 7 years ago

patakijv commented 7 years ago

When running the script I get an error on line 22 where it tries to run the ps and grep commands within the $(). I can confirm this by just trying to run that same command in the shell both with and without the $()

10:18:55 ~$ $(ps auxwww |grep docker |grep vmstateevent |grep '"vmstate":"running"')
-bash: patakijv: command not found
10:18:59 ~$ ps auxwww |grep docker |grep vmstateevent |grep '"vmstate":"running"'
patakijv         51442   0.0  0.0 556624300   4868   ??  S    10:22AM   0:00.01 /Applications/Docker.app/Contents/MacOS/com.docker.frontend {"action":"vmstateevent","args":{"vmstate":"running"}}
patakijv         51441   0.0  0.0 556642232   5188   ??  S    10:22AM   0:00.01 /Applications/Docker.app/Contents/MacOS/com.docker.frontend {"action":"vmstateevent","args":{"vmstate":"running"}}

I can remove the $() wrapper around the command line in lines 22 and 29 but not yet sure if that will introduce other issues that the rest of the script requires. I am still early in the evaluation and testing of this.

Clues? Comments?

if-kenn commented 7 years ago

@patakijv I noted that last night myself. I have fixed this, made output a bit better and added some helpful notes. Let me know how it works for you.

patakijv commented 7 years ago

Indeed output is improving and the change for the $() seems sufficient. Nice! Here are some quick notes after quick glance:

1) shouldn't this while at it name it 'd4f'\n" be while at it name it 'd4m'\n"? (notice the d4f changed to dfm) Or am I missing something?

2) shouldn't the script automatically handle the case (on exit or re-run) when there are multiple screens with the same name (or prevent it from happening) - i.e. I get this after second run

There are several suitable screens on:
    52010.d4m   (Detached)
    55518.d4m   (Detached)
Use -S to specify a session.

3) if my interest in this came from developing an overall solution for a development environment using docker containers on mac instead of virtualbox (from vagrant) and noticed your post on the docker forum for addressing file system performance issues, where does this script fit into the overall solution? I don't quite get (yet) where screen comes into play as the developer who is spooling up a new developer environment (i.e. a rails app container, a redis container and mysql container)

if-kenn commented 7 years ago

@patakijv

  1. You are correct.
  2. I can later add some check if it has been run before, I really just wanted this to get out there as a proof of concept to make performance much better. It should only have to be called once. There are other changes I will probably make along those lines like making sure files under /etc don't already have the correct values.
  3. I intend this script to be used in conjunction with whatever scripts you use to bring up your stack, for example we have a script that does a number of things including calling docker-compose that uses a .yml file. This would now be run before that. Note that this script can be called if D4M is already running or can bring it up.
patakijv commented 7 years ago

Great to hear the proof of concept is coming along.

Perhaps (as you continue to get this hammered out) you can add an example of your top level script you mention to your README that would show it calling this script then the docker-compose part, etc. so it is clear where and when it is called and what other surrounding steps are required in the mix. As well, what should be done (steps or scripts) if you need to rebuild/restart/recompose the development docker container environment.

if-kenn commented 7 years ago

@patakijv I have added a few checks to avoid errors if this d4m-nfs is run again. I have also added an example directory for both straight docker run and docker-compose.

I am going to close this issue for now, since the original was on the syntax error from substitution.