Juju Charms Collection

Bug #1473509
Comment #7

Comment 7 for bug 1473509

Revision history for this message

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

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

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