Storm Charm was added.

Hi Maarten! Thanks for submitting this for review, sorry it's taken a bit of time to reply. I'm on review this week and will be reviewing this soon. I took a quick look at the charm and noticed the directory structure nests both the series and charm inside of the repo. In order for this to land in the store the root of the repository will need to be the contents of "precise/storm". When you have time could you correct that? I'll report back when I've done a full review of the charm.

Thanks again for the submission!

Hi Marco,

I don't understand your comment about the series and the charm inside the
repo. Can you please clarify what it is I did wrong and how I can fix it?

thanks,
Maarten

On Mon, Jul 1, 2013 at 7:40 AM, Marco Ceppi <email address hidden> wrote:

> Hi Maarten! Thanks for submitting this for review, sorry it's taken a
> bit of time to reply. I'm on review this week and will be reviewing this
> soon. I took a quick look at the charm and noticed the directory
> structure nests both the series and charm inside of the repo. In order
> for this to land in the store the root of the repository will need to be
> the contents of "precise/storm". When you have time could you correct
> that? I'll report back when I've done a full review of the charm.
>
> Thanks again for the submission!
>
> ** Branch linked: lp:~maarten-ectors/charms/precise/storm/trunk
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1195736
>
> Title:
> Storm Charm was added.
>
> Status in Juju Charms:
> New
>
> Bug description:
> The Storm, i.e. the distributed real-time communication system that
> originated at Twitter, was submitted.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1195736/+subscriptions
>

--
Maarten Ectors
Senior Cloud Strategist
Changing the Future of Cloud
Ubuntu <http://ubuntu.com> / Canonical <http://canonical.com> UK LTD
<email address hidden>
Fixed: +44 (0) 207 630 2435

Sorry I didn't make that more clear previously. In your branch lp:~maarten-ectors/charms/precise/storm/trunk you have a precise/storm directory. If you compare that to http://bazaar.launchpad.net/~charmers/charms/precise/mysql/trunk/files You'll see that the instead of having the "precise" directory then the "charm" directory in the repository, you'll instead need to just have the contents of the storm directory (the charm itself) as the root of the repository. The series of the charm is determined by the branch name and not by a directory hierarchy inside the repository. So if you could move all the contents from precise/storm to just the root of the bazaar repository it'll match what juju expects a charm's branch to look like. Typically when charm authors are writing charms they'll do something like:

mkdir -p ~/charms/precise/<charm-name>
cd ~/charms/precise/<charm-name>
bzr init

As this is what Juju expects, in your repositories current form it will fail to deploy from the store as Juju expects the hooks directory to be at the top level of the repository and not in <series>/<charm>/hooks.

Let me know if you need anymore clarification, you can also hop in to #juju on freenode if IRC is more convenient for you.

Done.

regards,
Maarten

On Mon, Jul 1, 2013 at 8:28 AM, Marco Ceppi <email address hidden> wrote:

> Sorry I didn't make that more clear previously. In your branch lp
> :~maarten-ectors/charms/precise/storm/trunk you have a precise/storm
> directory. If you compare that to
> http://bazaar.launchpad.net/~charmers/charms/precise/mysql/trunk/files
> You'll see that the instead of having the "precise" directory then the
> "charm" directory in the repository, you'll instead need to just have
> the contents of the storm directory (the charm itself) as the root of
> the repository. The series of the charm is determined by the branch name
> and not by a directory hierarchy inside the repository. So if you could
> move all the contents from precise/storm to just the root of the bazaar
> repository it'll match what juju expects a charm's branch to look like.
> Typically when charm authors are writing charms they'll do something
> like:
>
> mkdir -p ~/charms/precise/<charm-name>
> cd ~/charms/precise/<charm-name>
> bzr init
>
> As this is what Juju expects, in your repositories current form it will
> fail to deploy from the store as Juju expects the hooks directory to be
> at the top level of the repository and not in <series>/<charm>/hooks.
>
> Let me know if you need anymore clarification, you can also hop in to
> #juju on freenode if IRC is more convenient for you.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1195736
>
> Title:
> Storm Charm was added.
>
> Status in Juju Charms:
> New
>
> Bug description:
> The Storm, i.e. the distributed real-time communication system that
> originated at Twitter, was submitted.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1195736/+subscriptions
>

--
Maarten Ectors
Senior Cloud Strategist
Changing the Future of Cloud
Ubuntu <http://ubuntu.com> / Canonical <http://canonical.com> UK LTD
<email address hidden>
Fixed: +44 (0) 207 630 2435

Now reviewing

Hi Maarten, sorry for the delay in getting this reviewed. Great work on the charm so far, I didn't find much in my review. You can find my notes below:

# Blockers

These are items that are actively preventing the charm from making it's way in to the store.

## hooks/install

- L62: You download zeromq from source, but don't perform any cryptographic verification of the payload[1]. It looks as though they provide SHA1SUMS for their downloads so you'll just need to add the extra step of verifying the tarbal matches the SHA1 hash

- L86-L88: These lines breaks idempotency. If the install hook were to be run again (say it failed half way though and someone ran `juju resolved --retry`) there would be no $CHARM_DIR/hooks/storm.yaml file anymore. Consider instead using the install command or just copying it to it's destination. Also, you'll need to check to insure the group and user doesn't already exist. If it does groupadd will exit > 0 and so will useradd.

- There are inconsistencies with restart_storm, start_storm, and stop_storm. There's a condition where coworker-relation could fire before master-relation was met, meaning restart_storm would be called before service_ready nullifying the effects of start_storm. restart_storm could probably just call stop_storm and start_storm respectively instead of directly calling service commands. I may be reading the hook wrong though, so if that's not the cause carry on.

# Minor

Not blockers, just things to consider. Not required for charm store.

- I'd consider not having the "install" hook being the central symlinked file. It gives the wrong impression when looking at the directory tree. Typically people place a "hooks" (hooks.py, hooks.sh) file in the hooks directory and symlink all hooks to that file. I'd go one step further and say create a $CHARM_DIR/lib/hooks.sh (or common.sh) outside the hooks directory. Then instead of symlinks stub the calls in each hook file instead. So instead of a symlink of hooks/install -> lib/hooks.sh have the install hook include the common file and stub the commands there (remove the case call from common file):

```
#!/bin/bash

. lib/hooks.sh

configure_hosts
install_base_packages
configure_storm_base
```

This is, again just a suggestion, but it's been considered to help improve charm readability.

- Typically you'll want to avoid placing anything other than hooks in the hooks directory. The files storm.sh and storm.yaml are just files that are copied to the units, you may want to instead place them under a files directory in the $CHARM_DIR or somewhere other than hooks.

- The description for the charm is quite large and replicates a lot of what's in the README. I'd consider removing the code examples and instead making sure that information is in the README. The description is used to give an overview of what the service is and what the charm does. The REAME should be where you drill down in to use cases, etc

# Nitpicks

Seriously, these are just me splitting hairs but I feel compelled to mention anyways. You're by no means required or suggested to implement any of these fixes.

There are some odd formatting issues, indents not being intented to match braces (L4...

Hi Marco,

I made the updates. Storm is unfortunately not ready for prime-time yet,
since the supervisors/workers seem to stop working when something happens
with Nimbus and I was not able to validate DRPC works. However there is
good news. I found a storm expert, whose book I am currently reviewing to
take ownership and bring the storm charm up to standard and use it for his
book/blog. I will write him an email with you in copy. Not sure if you need
to do something more before he can start working on it. As soon as he says
he is ready, we can do the transfer and he becomes the owner. Ok?

thanks,
Maarten

Thanks,
Maarten Ectors
Senior Cloud Strategist
Changing the Future of Cloud
Ubuntu <http://ubuntu.com> / Canonical <http://canonical.com> UK LTD
<email address hidden>
Fixed: +44 (0) 207 630 2435
Mobile: +44 (0) 791 860 8145

On Mon, Jul 8, 2013 at 4:09 PM, Marco Ceppi <email address hidden> wrote:

> Hi Maarten, sorry for the delay in getting this reviewed. Great work on
> the charm so far, I didn't find much in my review. You can find my notes
> below:
>
> # Blockers
>
> These are items that are actively preventing the charm from making it's
> way in to the store.
>
> ## hooks/install
>
> - L62: You download zeromq from source, but don't perform any
> cryptographic verification of the payload[1]. It looks as though they
> provide SHA1SUMS for their downloads so you'll just need to add the
> extra step of verifying the tarbal matches the SHA1 hash
>
> - L86-L88: These lines breaks idempotency. If the install hook were to
> be run again (say it failed half way though and someone ran `juju
> resolved --retry`) there would be no $CHARM_DIR/hooks/storm.yaml file
> anymore. Consider instead using the install command or just copying it
> to it's destination. Also, you'll need to check to insure the group and
> user doesn't already exist. If it does groupadd will exit > 0 and so
> will useradd.
>
> - There are inconsistencies with restart_storm, start_storm, and
> stop_storm. There's a condition where coworker-relation could fire
> before master-relation was met, meaning restart_storm would be called
> before service_ready nullifying the effects of start_storm.
> restart_storm could probably just call stop_storm and start_storm
> respectively instead of directly calling service commands. I may be
> reading the hook wrong though, so if that's not the cause carry on.
>
> # Minor
>
> Not blockers, just things to consider. Not required for charm store.
>
> - I'd consider not having the "install" hook being the central symlinked
> file. It gives the wrong impression when looking at the directory tree.
> Typically people place a "hooks" (hooks.py, hooks.sh) file in the hooks
> directory and symlink all hooks to that file. I'd go one step further
> and say create a $CHARM_DIR/lib/hooks.sh (or common.sh) outside the
> hooks directory. Then instead of symlinks stub the calls in each hook
> file instead. So instead of a symlink of hooks/install -> lib/hooks.sh
> have the install hook include the common file and stub the commands
> there (remove the case call from common file):
>
> ```
> #!/bin/bash
>
> . lib/hooks.sh
>
> configure_hosts
> install_base_pa...

Hi Maarten,

Yes, that's fine. Just make sure the maintainer field in metadata.yaml is updated to reflect the maintainer and he links his code branch here for review. As far as "transfering" ownership he can just change the bug ownership and that's about it. There is no real formal process for transferring ownership especially since the charm hasn't even landed in the store yet. Thanks for your work on the charm thus far and the update!

I'm marking this as incomplete until there is an update to the charm for review (and the expert takes over). When the time comes move this bug back to New and it'll be added back to the review queue.

Feel free to reply here, <email address hidden>, or #juju on freenode if you have any questions regarding this.

Thanks!

Maarten,

Thank you for updating this charm and resubmitting it for review, and apologies for it taking so long to get the update reviewed. The updated charm looks good but for a few issues:

It seems that you moved the files into a subdirectory ("storm"), which causes it to fail to deploy. (All of the files need to be in the top level of the charm for it to work correctly.) I also noticed that you're not validating the SHA512 nor MD5 for the apache-storm-$version.zip file that's being downloaded.

With the subdirectory moved up, the charm passed proof without any warnings or errors, and I was able to deploy it according to the example configuration from the README. However, I think I might have run into the issue you mentioned in your previous comment regarding nimbus.

The first time I tried to load the page, I got a java traceback due to a connection error, and it seemed that the storm-nimbus daemon had died due to not being able to connect to the zookeeper service; I was able to get it working with "sudo storm restart" on stormmaster/0, though, so it seems like it was just a transient error. I think just adding the --respawn option to the storm-nimbus daemon line might be sufficient, but converting the init.d script to a couple of Upstart config files would probably make them quite a bit cleaner and easier to maintain.

I look forward to seeing the issues addressed so that I can add my +1 for getting this charm into the store!

solved the problems. Can you recheck?

Hello Maarten!

Thanks again for your submission of the Storm charm to the Charm Store. I think it will definitely be a great addition! I have done a review of the charm and here is what I found.

# charm proof

The charm passes `charm proof` without any errors. That is awesome! Let's move on.

# Deployment

I was able to complete the deployment without any errors by following the instructions found on the README file. I did get to the web interface after a little while after it said the unit was started, and I am assuming this is because the service took a bit starting.

Storm seems to be running under the 'storm' user and not root. Awesome!

# Configuration Options

I see that currently the port can not be set to a number fewer than 1024. This is because the service is not running as root, correct? In this case the `setcap` command may be able to help you in order to accept ports lower than 1024. On the other hand, I do not see any verification to check if the port is 1024 or lower. If the user sets port 80, for example, then the service will just continue to display as 'started' with port 80 opened, but the user will not be able to connect. Probably a good idea to add a verification or use setcap to be able to set ports lower than 80.

# Relations

As I mentioned before, I was able to fully deploy the service and access a working UI. But when I did `juju destroy-relation stormmaster:master stormworker:worker` I was not able to access the UI even though `juju status` listed the port opened. It may be a good idea to close the port once the relation is broken if it is expected to have no UI. Also, when I re-added the relation, the UI went back up, but "Supervisor Summary" is now empty, which had one unit listed in there before I destroyed the relation.

I destroyed the relation between stormmaster and zookeeper and the UI went down. When re-added, it came back up, as expected. On the other hand, when I destroyed the relation between stormworker and zookeeper nothing seemed to happen. Is that expected?

# Knitpicks

There are some thingies that I'd like to see for the charm to be even more awesome.

First of all, it would be nice to have consistency in terms of line-wrapping. I suggest having 80-line wrapping in all the README, config.yaml and metadata.yaml files. And on the README files, you should give 4-line intending to get it displayed as code. There is some code that has some extra spaces. Also, the README file is a bit slim. I would suggest doing 'charm add readme' and filling in all the info. There are lots of parts of the template that can just be filled in by copying the info you already have, so you're halfway through that one :) The README file should be renamed to README.md in order for it to be correctly displayed on the GUI - otherwise, even if it's on Markdown, it will not display correctly. The revision file can be deleted - it's not something that is used now. Finally, on the metadata.yaml file, you set the category as 'misc'. Is there any reason for not having it listed as an application?

Again, thanks a lot for your submission of the charm to the Charm Store. I am really eager to see the charm being promulgated. For now, ...