Charm Needed: Subway IRC client/server

This isn't a real review because I'm not a reviewer, but I talked to Ben on IRC:

- needs an open-port statement for the port.
- needs some -y's around some of the apt-get's
- needs a more descriptive readme.

Since juju doesn't do subordinates yet and it's a chat client I think leaving mongo and node in this charm is fine, we don't need 4 instances to run an irc client. This charm will be slick though.

Ben when you've pushed the things we've talked about flip the status to Fix Committed to get it back in the queue, thanks!

@Jorge:

√ - open-port statement

√ - for -y flags

√ - for improved Readme

Re-submitted for Review

Hi Ben

I've had a review of the subway charm; heres some feedback

1) metadata.yaml

a) name

Currently the name is 'Subway' - best practice seems to be to keep this lower case so when someone branches 'subway' the charm has the same name and can be deployed:

juju deploy --repository . local:subway

b) requires: relations

Not sure quite what you are aiming for here:

requires: nodejs, npm, subway
  interface: http

Its not correctly formatted and both 'charm proof' and 'juju deploy' won't parse it.

I think that your charm needs to:

provides:
  subway:
    interface: http

but if its a single, standalone service unit then it might not even need that. That said this would let you put a proxy server infront of subway like haproxy - you will need to create the associated hooks to support this if so.

Have a read of https://juju.ubuntu.com/docs/charm.html#the-metadata-file and give me a ping (jamespage) on IRC if you need any help.

c) description:

I'd wrap each line of the description at 80 columns - it be a bit more readable and will look better in the charm store.

2) install hook

I fixed up the above locally and had a spin and got the following errors

23: install_subway: not found
27: git: not found

Needs some more work.

Looking at the install hook I don't think it will ever complete as the last command is:

npm install && node subway

Hooks should really complete - you could either 'nohup node subway' & or install a upstart config to run it for you - take a look at the etherpad-lite charm for some ideas.

Upstart config would be better so that if the service unit reboots subway will restart.

I'd also be tempted todo the open-ports and start of subway in the start hook.

I think thats sufficient for a first review - please make sure that you give your charm a test before re-submitting.

To resubmit for review please mark 'Fix Committed'.

Thanks

James

In my latest revision I made the following improvements:
* Re-positioned open-port to earlier in the install hook
* nohup added to final node subway deploy
*Improved MetaData Name, Desc and Indentation of other fields to prevent hanging

Bi Ben! The charm is looking great. Very few things left to change:

charm proof

I'd recommend running 'charm proof' in the charm root after most changes. It shows this right now:

I: relation subway has no hooks
E: revision file in root of charm is required

The hooks problem is actually quite important.. as the 'subway' relation needs a joined hook that pushes its hostname and port into the relationship. So you need a file

hooks/subway-relation-joined

Which at its minimum is an executable that looks like this:

#!/bin/sh
relation-set hostname=`unit-get private-address` port=80

That way users can relate your charm to a web proxy of any kind and it will inform their proxy of the port and hostname to send traffic to.

charm-helpers

# Install the totally awesome charm-helper-sh package
add-apt-repository ppa:charmers/charm-helpers
apt-get update && apt-get install -y charm-helper-sh

You don't actually use charm-helpers, so this block can be deleted.

tmp/meta

Its not clear how these directories are used. Is that used by subway itself after startup? If so, beware that the whole charm dir is cleaned up, so on 'destroy-service', the data will be wiped out (normally its left alone in case you want to backup or transfer it to another machine between destroy-service and terminate-machine).

adding nohup

This needs to go in the 'start' hook, not install. Also the corresponding way to stop the service should be in stop. Does node provide a pidfile? Even better, you could use an upstart job to be the cool kid on the block. :)

running as root

Best not to run things as root like this. I'd suggest creating a user called 'subway' with

adduser --system --group subway

-- More review later, reached EOD, but please respond to these soon. :)

@Clint:

I implemented most of your suggestions and ran charm proof and it looks pretty frosty now.... I improved start/stop hooks and discussed the stop with jcastro and this charm seems ready to ship unless I missed something else.

Ben, thanks for sticking with this, its looking better definitely. A few more things before this is ready for the charm store (and really, I'm excited, I think this is one of the cooler webapps I've looked at for the charm store):

[1] install_error on local provider:

The local provider only gives us a very bare bones system. One thing missing is 'make'. This causes npm to fail to install the needed modules because of:

2012-03-20 05:29:55,901: hook.output@ERROR: npm http 200 https://registry.npmjs.org/connect-assets/-/connect-assets-2.1.6.tgz

2012-03-20 05:29:57,555: hook.output@INFO:
> bcrypt@0.5.0 install /var/lib/juju/units/subway-1/charm/subway/node_modules/bcrypt
> make build

2012-03-20 05:29:57,559: hook.output@ERROR: sh:
2012-03-20 05:29:57,560: hook.output@ERROR: make: not found
2012-03-20 05:29:57,560: hook.output@ERROR:

So, in addition to npm, you need to install the full build suite, as some of the npm packages seem to need a C or C++ compiler, so

apt-get -y install npm build-essential

Should work and be general enough to handle most modules. It resolved the issue on my system anyway.

[2] start hook never returns

Its good that you moved the service startup to the start hook, however, the start hook never returns, so the juju unit agent will be unable to run further hooks until that happens. nohup is not sufficient to run a daemon. At the very least, you need to use start-stop-daemon to background it like this:

start-stop-daemon --start --oknodo --background --pidfile /run/subway.pid --startas /usr/bin/node -- subway

This has the added benefit of making stop more clear:

start-stop-daemon --stop --oknodo --pidfile /run/subway.pid --retry 5

Even better would be to make an upstart job. Its quite simple:

cat >> /etc/init/subway.conf <<EOF
start on runlevel [2345]
stop on runlevel [^2345]
chdir $PWD
exec node subway
EOF

Then start/stop are just

start subway || :

and

stop subway || :

--- non blockers below ---

[3] non idempotent install hook

Sometimes install needs to be run over and over again, especially if there is a transient error with one of the mirrors used to install software, somebody might need to retry the hook Right now, install will fail on the git clone when run a second time because the subway directory is there.

I'd suggest this

if [ -d subway ] ; then
  (cd subway && git pull)
else
  git clone .... subway
fi

[4] open-port before software is running

This is just a style thing, but typically you don't want to open the port until the software is fully configured and running. So I'd move the 'open-port 3000' command to the start hook, after the service is started.

One final thing.. I'm still a bit concerned at running node.js as root unnecessarily. Its a bit more work, but really, we should create a user and run nodejs as that user. Not a blocker.. but.. it would be good to address that.

I have made "some" of the improvements you suggest and covered all the blockers and re-submit it for final (hopefully) review. I do plan to transition the way the job is created to a upstart job in a later revision but for right now I feel confident this is ready to ship.

Ben, I led you slightly astray with the start-stop-daemon command, it is actually

start-stop-daemon --start --oknodo --background --make-pidfile --pidfile /run/subway.pid --chdir `pwd`/subway --startas /usr/bin/node -- subway

Note the --chdir and --make-pidfile.

This is the only change necessary to make the charm work, so I just made the change, and promulgated it.

Nicely done! :)