-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jetmon: Dockerize app #5
Conversation
Dockerfile
Outdated
|
||
COPY . . | ||
|
||
CMD [ "npm", "run", "rebuild-run"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this a lot. Should the node addon (jetmon) be built during the image build? Should we approach it with multi staged build?
When I tried that the build
folder was not added locally. Also since we are developing and using the addon here it seemed better to do so when we run the container 🤷
Could really use some confirmation on this approach though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the approach is good for now. I pushed up some changes that kept a similar approach that I'll discuss below.
The new `run-jetmon.sh` script is now the command run by `Dockerfile`. It ensures that the directories are present and generates the key and cert if they are not present. Changes were made to the code in the repo to apply the changes as described in the testing instructions as they are necessary to run the codebase currently.
The logs/ and stats/ dirs are now tracked by git, but they have their own `.gitignore` files to ensure that files generated in them are not tracked.
I pushed up a couple of changes that should ease testing for now. It simplifies the testing instructions in the top post down to:
I added a There's more refinements to make, but it should ease testing. If these changes aren't wanted, I won't be hurt if some or all of them are reverted. |
@@ -123,6 +123,7 @@ var configuration = { | |||
if ( 1 == arrSettings[ WRITE_MASTER ] ) { | |||
db_config['multipleStatements'] = true; | |||
poolCluster.add( 'MISC_MASTER', db_config ); | |||
poolCluster.add( 'MISC_SLAVE' + slaveUniqueCount++, db_config ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a hack I did to overcome the parsing issues with db-config.conf
, shouldn't be committed imo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I waffled on that. It's weird to have the db-config.conf
file in the repo at all isn't it? Usually confs like that are a local file system thing and not committed.
@@ -4,6 +4,7 @@ | |||
|
|||
#include <cstdlib> | |||
#include <string> | |||
#include <cstring> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also hesitant to add this change till we make sure we are using the correct node
and node-gyp
versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured we could make adjustments once we get to see the production systems. For now, it makes testing easier without having to stash/unstash local changes.
@@ -0,0 +1,14 @@ | |||
#!/usr/bin/env bash | |||
|
|||
mkdir -p logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider adding the logs and stats related directories and files on build instead :)
I'm still not sure these should be part of the image build though or it's better to create them on our local envs.
Same with certs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed that up in my second commit where I just ensured that both directories are part of the repo. At this point, the script just acts as a sanity check to ensure that the directories are present. I think it works better set up this way (as part of the repo by default) as opposed to some Dockerfile
build step as the build process could result in directories that exist in the docker filesystem but not the local filesystem, which would make testing and log inspection more difficult.
We don't have to stick with the logic I added in the run-jetmon.sh
script (or keep the script at all). I mainly wanted to throw it out there to show that we can launch using a script that contains whatever logic we want rather than being stuck with just running the npm
command to launch the container.
I was able to run the Veriflier Dockerfile without errors. I have not done testing to ensure that Veriflier is running as expected yet.
Closing in favour of the most recent one (after merging the production updates): #8 |
First attempt at dockerizing Jetmon.
Testing instructions
jetmon.log
,status-change.log
sitespersec
,sitesqueue
,totals
config/db-config.conf
[TBA once we figure out how to populate this file properly]var _watcher = require( 'jetmon.node' );
withvar _watcher = require( '../build/Release/jetmon.node' );
inhttpcheck.js
line 21docker compose build
docker compose up -d
jetmon.log
Helper note to populate the DB (todo automate this):
docker compose exec -it mysqldb sh
mysql --user=root --password=123456 jetmon_db
jetpack_monitor_subscription
table