Bug #1473509 “Mobicents RestComm Juju Charm” : Bugs : Juju Charms Collection

Revision history for this message

Jean Deruelle (jean-deruelle) wrote on 2015-08-14:

#1

Any news on this ?

Revision history for this message

Jean Deruelle (jean-deruelle) wrote on 2015-08-18:

#2

and now also at https://code.launchpad.net/~jean-deruelle/charms/trusty/mobicents-restcomm-charm/trunk

Revision history for this message

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

#3

Download full text (5.6 KiB)

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...

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.

Revision history for this message

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

#4

Moving to In Progress for now. Jean, please set this bug status back to "New" or "Fix Committed" when you're ready for additional feedback/review. Thank you!

Changed in charms:
status:	New → In Progress

Revision history for this message

Jean Deruelle (jean-deruelle) wrote on 2015-08-21:

#5

Thanks Kevin. I started to make modifications based on your comments. It may take a while for me to go through everything. I'll ping you back when I'm done. Thanks again for your valuable feedback.

Revision history for this message

Artur Tyloch (artur-tyloch) wrote on 2015-08-21:

#6

Kevin
yes, we should review Jean's mobicents-restcomm-charm as it contains latest and greatest version of restcomm from restcomm developers.

Revision history for this message

Nicolas Thomas (thomnico) wrote on 2015-08-25: Re: [Bug 1473509] Re: Mobicents RestComm Juju Charm

#7

Download full text (6.8 KiB)

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I do confirm that the thmonico branch has been used only for demos
(pre-CPP time) .. please move forward with Jean submission..

Should I destroy mine to avoid confusion ??

On 20/08/2015 20:48, Kevin W Monroe wrote:
> 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 th...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I do confirm that the thmonico branch has been used only for demos
(pre-CPP time) .. please move forward with Jean submission..

Should I destroy mine to avoid confusion ??

On 20/08/2015 20:48, Kevin W Monroe wrote:
> 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/hea
d:/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.
>

- -- 
Best Regards,
Nicolas Thomas  -  Solution Architect   -   Canonical
http://insights.ubuntu.com/?p=889
Planned absence: 10 to 21st of August 2015
GPG FPR: D592 4185 F099 9031 6590 6292 492F C740 F03A 7EB9
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJV3IriAAoJEEkvx0DwOn65cZ4IALGGobKURwa37tTnWeMuKaa4
pGRWLm+gEdEC235NeMGB2dkyOUfRqisOQSkUR58B5mUfZ7gheSZiMQeSiowQvLPu
fdJvkOomNg2OiP2lhl+LsmyJD/DHXHBxbeu+vEVIPRT4oYU/bprlq37/lBfochTJ
hu9y3Jtnsxs6SM34/nLyV1BFuZlAs4WvuhAv1I3VMhBwKsasdl9GnjBNPm8K5su2
rX4OquNRFdgUQDqOOpfRid+ypUcWzo3NhhDqu7ZQX7ekgyk1LyrAyhDJyIs59yRn
dr2PHhMe4vtg4xgyIQHCjS8ft2iLadNCfQnJSWT6P6M8iBrn6PYf+yIOiSEhlgM=
=wBii
-----END PGP SIGNATURE-----

Juju Charms Collection

Mobicents RestComm Juju Charm

Bug Description

Related branches

Other bug subscribers

Remote bug watches