Juju Charms Collection

Bug #1473509
Comment #3

Comment 3 for bug 1473509

Revision history for this message

Kevin W Monroe (kwmonroe) wrote on 2015-08-20:

Hi Jean, thanks so much for the submission! I didn't know much about Mobicents prior to this review, but it seems like an interesting multimedia/communication platform. It would be a very welcomed addition to the charm store!

I am a bit confused as there seems to be 2 charms for mobicents, one is mobicents-restcomm (by thomnico), and the other is mobicents-restcomm-charm (by you). The source is almost identical, but I wanted to confirm that your charm is the one that we should be reviewing.

Assuming yours is the right charm, I'm happy to say that 'charm proof' completed without issue and your icon.svg looks good. However, I did find a few issues that we'd like to see polished prior to recommending this for the charm store:

+ Superflous files
The 'revision' file is obsolete and can be removed from your charm source without causing any problems. I also found the following .bak and ~ files that can be removed (i'm guessing these backup files snuck into your repo with 'bzr add'):

│   ├── mediaserver-relation-joined.bak
│   ├── restcomm-relation-changed.bak
│   ├── restcomm-relation-joined.bak
├── metadata.yaml~
├── revision~

+ README.md
As mentioned earlier, I wasn't familiar with Mobicents prior to this review. It would help to add an Introduction section that summarizes what Mobicents does and what users can expect after deploying this charm. Adding a Resources section that links to the mobicents.org site and other references (bug trackers, mailing lists, docs, etc) is also recommended to help users like myself become familiar with this offering.

The deployment instructions for this charm require a local copy to be present. You can deploy directly from your namespace in the charm store, which would eliminate that requirement. In the README, I suggest changing this:

juju deploy -u --repository=../../ local:trusty/mobicents-restcomm

to this:

juju deploy cs:~jean-deruelle/trusty/mobicents-restcomm-charm

You have quite a few relations to this charm, which is great! Part of the power of Juju comes from the ability to relate services to one another. You show how to use the database relation in the README, but not the cscf, clearwater-ellis, or load-balancer relations. I'm unclear on how/when I would use those, so a more detailed Deployment section talking about these relations would help.

In the Test section, you list 2 URLs to try, yet they are identical. Is there a different port/path to try besides :8080/restcomm-management?

+ Hooks
- The 'clearwater-ellis-relation-changed' hooks is mostly commented out. Is that intentional? I noticed other hooks (e.g. cscf-relation-changed) stop and start the service. I am curious if a similar restart needs to happen on a clearwater relation change.

- The 'database-relation-changed' hook logs the database connection information. I would at least remove the password in case juju logs are shared with people that should not know this info:

juju-log "user $user password $password host $host database $database"

- I suggest creating a 'database-relation-departed' hook that removes the mysql db in case the relation to mysql is ever removed. This will help anyone maintaining mysql in their environment by ensuring only active databases are present.

- There is a 'restcomm-relation-joined' hook, but no such relation defined in 'metadata.yaml'. If that hook isn't used, it should be removed. If it is needed, you'll need to add that to 'metadata.yaml' like you've done for the other hooks.

+ Deployment / Configuration
- I was excited to see that this charm installed and related to mysql without problem. However, I was unable to access the :8080/restcomm-management URL. A closer look at the debug log revealed this:

machine-0: 2015-08-20 15:31:53 ERROR juju.worker runner.go:223 exited "firewaller": cannot change firewall ports: cannot open ports: The maximum number of rules per security group has been reached. (RulesPerSecurityGroupLimitExceeded)

I'm guessing the 8080 port was never actually opened due to a constraint in my Amazon environment. I found the loop that was opening ports in ./lib/mobicents/restcomm-utils.sh and see an interesting comment:

      # XXX: Highly restrict this range so we can only open 10 ports
      # for ec2. This should be a 100 port range
      local lowPort=65434
      local highPort=65535

So I'm guessing this charm isn't meant to deploy on Amazon by default. I suggest making lowPort and highPort configurable by adding options to config.yaml. The defaults would be 65434 and 65535, but anyone attempting an AWS deployment could alter those and restrict the range to be opened (documenting this in a Limitation section of the README, of course :)

+ Tests
This charm is missing tests, which are required so we can include this in our automated test runner. Automated testing is nice so you can be alerted if your charm ever fails to deploy on a variety of substrates. Fortunately, adding tests is simple with charm tools! See the "Add Tests" section here for more details:

https://jujucharms.com/docs/devel/tools-charm-tools

I'll also point to an example test that may be helpful for you:

http://bazaar.launchpad.net/~joe/charms/trusty/suitecrm/trunk/view/head:/tests/100-deploy-suitecrm

This simply deploys and relates the required charms, then makes sure that the server is listening to http requests. That could easily be modified to make sure that :8080/restcomm-management is up.

While this charm has a lot of great potential, we need to address the above issues before we can recommend it for the store. I look forward to iterating on this to get that recommended status! Thanks again for your work thus far.

Hi Jean, thanks so much for the submission!  I didn't know much about Mobicents prior to this review, but it seems like an interesting multimedia/communication platform.  It would be a very welcomed addition to the charm store!

I am a bit confused as there seems to be 2 charms for mobicents, one is mobicents-restcomm (by thomnico), and the other is mobicents-restcomm-charm (by you).  The source is almost identical, but I wanted to confirm that your charm is the one that we should be reviewing.

Assuming yours is the right charm, I'm happy to say that 'charm proof' completed without issue and your icon.svg looks good.  However, I did find a few issues that we'd like to see polished prior to recommending this for the charm store:

+ Superflous files
The 'revision' file is obsolete and can be removed from your charm source without causing any problems.  I also found the following .bak and ~ files that can be removed (i'm guessing these backup files snuck into your repo with 'bzr add'):

│   ├── mediaserver-relation-joined.bak
│   ├── restcomm-relation-changed.bak
│   ├── restcomm-relation-joined.bak
├── metadata.yaml~
├── revision~

+ README.md
As mentioned earlier, I wasn't familiar with Mobicents prior to this review. It would help to add an Introduction section that summarizes what Mobicents does and what users can expect after deploying this charm.  Adding a Resources section that links to the mobicents.org site and other references (bug trackers, mailing lists, docs, etc) is also recommended to help users like myself become familiar with this offering.

The deployment instructions for this charm require a local copy to be present.  You can deploy directly from your namespace in the charm store, which would eliminate that requirement.  In the README, I suggest changing this:

juju deploy -u --repository=../../ local:trusty/mobicents-restcomm

to this:

juju deploy cs:~jean-deruelle/trusty/mobicents-restcomm-charm

You have quite a few relations to this charm, which is great!  Part of the power of Juju comes from the ability to relate services to one another.  You show how to use the database relation in the README, but not the cscf, clearwater-ellis, or load-balancer relations.  I'm unclear on how/when I would use those, so a more detailed Deployment section talking about these relations would help.

In the Test section, you list 2 URLs to try, yet they are identical.  Is there a different port/path to try besides :8080/restcomm-management?

+ Hooks
- The 'clearwater-ellis-relation-changed' hooks is mostly commented out.  Is that intentional?  I noticed other hooks (e.g. cscf-relation-changed) stop and start the service.  I am curious if a similar restart needs to happen on a clearwater relation change.

- The 'database-relation-changed' hook logs the database connection information.  I would at least remove the password in case juju logs are shared with people that should not know this info:

juju-log "user $user password $password host $host database $database"

- I suggest creating a 'database-relation-departed' hook that removes the mysql db in case the relation to mysql is ever removed.  This will help anyone maintaining mysql in their environment by ensuring only active databases are present.

- There is a 'restcomm-relation-joined' hook, but no such relation defined in 'metadata.yaml'.  If that hook isn't used, it should be removed.  If it is needed, you'll need to add that to 'metadata.yaml' like you've done for the other hooks.

+ Deployment / Configuration
- I was excited to see that this charm installed and related to mysql without problem.  However, I was unable to access the :8080/restcomm-management URL.  A closer look at the debug log revealed this:

I'm guessing the 8080 port was never actually opened due to a constraint in my Amazon environment.  I found the loop that was opening ports in ./lib/mobicents/restcomm-utils.sh and see an interesting comment:

# XXX: Highly restrict this range so we can only open 10 ports
      # for ec2. This should be a 100 port range
      local lowPort=65434
      local highPort=65535

So I'm guessing this charm isn't meant to deploy on Amazon by default.  I suggest making lowPort and highPort configurable by adding options to config.yaml.  The defaults would be 65434 and 65535, but anyone attempting an AWS deployment could alter those and restrict the range to be opened (documenting this in a Limitation section of the README, of course :)

+ Tests
This charm is missing tests, which are required so we can include this in our automated test runner.  Automated testing is nice so you can be alerted if your charm ever fails to deploy on a variety of substrates.  Fortunately, adding tests is simple with charm tools!  See the "Add Tests" section here for more details:

https://jujucharms.com/docs/devel/tools-charm-tools

I'll also point to an example test that may be helpful for you:

http://bazaar.launchpad.net/~joe/charms/trusty/suitecrm/trunk/view/head:/tests/100-deploy-suitecrm

This simply deploys and relates the required charms, then makes sure that the server is listening to http requests.  That could easily be modified to make sure that :8080/restcomm-management is up.

While this charm has a lot of great potential, we need to address the above issues before we can recommend it for the store.  I look forward to iterating on this to get that recommended status!  Thanks again for your work thus far.