Charm needed: opentsdb

Hi Robert, thanks for submitting. Obviously we would require at least some kind of commitment to maintain this charm long term or we can't accept it into the charm store. That means listing yourself as maintainer and being responsive to bug reports and questions.

Anyway, ignoring that, here is a review of the content of the charm:

** This is great, and I really love the detailed instructions in README

1. git:// urls from github are not safe, and can be MITM attacked. You can however use the https urls and at least be covered for MITM with that.

2. start should indeed start anything that needs to be running at the time it is run, so it should inspect system state and start any daemons, even if they are started elsewhere in the charm.

3. stop maybe needs --oknodo since opentsdb might not be running at the moment stop is called.

4. zookeeper-relation-changed: its only psychological, as the change doesn't actually happen until the hook exits successfully, but open-port shoudl come after the port that you're opening is serviced, since it is meant to be used as a way to say "this is now ready" not just a marker for what to expose.

5. rather than calling start-stop-daemon directly, you should arrange for the daemon to start up if the box is rebooted. An upstart job running basically the same command would be good

6. Do you mean to run tsdb as root? It will run as root right now, and thats usually not cool.

On Fri, Jun 29, 2012 at 10:42 AM, Clint Byrum <email address hidden> wrote:
> Hi Robert, thanks for submitting. Obviously we would require at least
> some kind of commitment to maintain this charm long term or we can't
> accept it into the charm store. That means listing yourself as
> maintainer and being responsive to bug reports and questions.

So, I'm happy to list myself as maintainer (how do I do that), and
respond to bug reports and q's. I just can't commit to /doing/
anything about them until/unless we were to use it in production...

> Anyway, ignoring that, here is a review of the content of the charm:
>
> ** This is great, and I really love the detailed instructions in README
>
> 1. git:// urls from github are not safe, and can be MITM attacked. You
> can however use the https urls and at least be covered for MITM with
> that.

Will do.

> 2. start should indeed start anything that needs to be running at the
> time it is run, so it should inspect system state and start any daemons,
> even if they are started elsewhere in the charm.

Is there a stock way to factor this out to avoid duplication?

> 3. stop maybe needs --oknodo since opentsdb might not be running at the
> moment stop is called.

ok.

> 4. zookeeper-relation-changed: its only psychological, as the change
> doesn't actually happen until the hook exits successfully, but open-port
> shoudl come after the port that you're opening is serviced, since it is
> meant to be used as a way to say "this is now ready" not just a marker
> for what to expose.

Sure, cando.

> 5. rather than calling start-stop-daemon directly, you should arrange
> for the daemon to start up if the box is rebooted. An upstart job
> running basically the same command would be good

Is there something I can cargo cult? Why doesn't juju take care of
restoring state when the box is restart? I mean - it knows where we
are meant to be...

> 6. Do you mean to run tsdb as root? It will run as root right now, and
> thats usually not cool.

To a degree 'meh'. Packaging is where I would really prefer all that
detail to go, but its not packaged, and the investment in doing that
is (again, for now) disproportionate to the benefits. I'll add an
adduser call to install, and sudo to the the user at appropriate
places.

-Rob

I've pushed up some changes.

On Fri, Jun 29, 2012 at 11:09 AM, Robert Collins
<email address hidden> wrote:
> On Fri, Jun 29, 2012 at 10:42 AM, Clint Byrum <email address hidden> wrote:
>> Hi Robert, thanks for submitting. Obviously we would require at least
>> some kind of commitment to maintain this charm long term or we can't
>> accept it into the charm store. That means listing yourself as
>> maintainer and being responsive to bug reports and questions.
>
> So, I'm happy to list myself as maintainer (how do I do that), and
> respond to bug reports and q's. I just can't commit to /doing/
> anything about them until/unless we were to use it in production...
>
>> Anyway, ignoring that, here is a review of the content of the charm:
>>
>> ** This is great, and I really love the detailed instructions in README
>>
>> 1. git:// urls from github are not safe, and can be MITM attacked. You
>> can however use the https urls and at least be covered for MITM with
>> that.
>
> Will do.

Done.

>> 2. start should indeed start anything that needs to be running at the
>> time it is run, so it should inspect system state and start any daemons,
>> even if they are started elsewhere in the charm.
>
> Is there a stock way to factor this out to avoid duplication?

Don't know how to:
12:24 < lifeless> how do you interrogate a specific relation from a charm hook?
12:24 < lifeless> (I want to see if the zk relation is setup in
opentsdb in the start hook)
12:24 < imbrandon> relation-get ?
12:25 < lifeless> so, in the zookeeper-relation-changed hook, I do
zk_host=`relation-get private-address`
12:25 < imbrandon> yup
12:25 < lifeless> but that is missing the context in the start hook.
12:25 < lifeless> isn't it? Or am I misunderstanding teh whole thing
12:26 < imbrandon> right, you only have access to it in the relation* hooks
12:26 < lifeless> so thats my quesiton
12:26 < imbrandon> ahh as far as i konw you dont
12:26 < imbrandon> or cant
12:27 < lifeless> SpamapS: ^ your review, how can I do this thing?
12:30 < imbrandon> but yea as far as the relation stuff , i dont think
there is a way iirc, other than some questionable measures
12:30 < imbrandon> like using the juju charm from the cli of a hook
12:30 < imbrandon> or similar

>> 3. stop maybe needs --oknodo since opentsdb might not be running at the
>> moment stop is called.
>
> ok.

Done.

>> 4. zookeeper-relation-changed: its only psychological, as the change
>> doesn't actually happen until the hook exits successfully, but open-port
>> shoudl come after the port that you're opening is serviced, since it is
>> meant to be used as a way to say "this is now ready" not just a marker
>> for what to expose.
>
> Sure, cando.

Done.

>> 5. rather than calling start-stop-daemon directly, you should arrange
>> for the daemon to start up if the box is rebooted. An upstart job
>> running basically the same command would be good
>
> Is there something I can cargo cult? Why doesn't juju take care of
> restoring state when the box is restart? I mean - it knows where we
> are meant to be...

Not sure how to (and this really feels like something that should be
an abstracted service not hand crafted).

>> 6. Do you mean to ...

Regarding

#2

Quote
"""
Don't know how to:
12:24 < lifeless> how do you interrogate a specific relation from a charm hook?
12:24 < lifeless> (I want to see if the zk relation is setup in
opentsdb in the start hook)
"""

You should be able to use
  $ relation-ids relation-name

and then pass those values into relation-get to interrogate specific relations from outside of those relations hooks.

5. an upstart job is the easiest way to do this. and agreed juju should restart this on reboot, but currently i don't think it does if its already listed as started atm.

also fwiw, thanks for charming, this is also something i've been wanting to play with. reconnoiter, kafka ? ;-)

Hi Robert:

did you get the relation-ids relation-name issue solved?

I don't see any updates on the branch.

Ping me if/when you work on this or if I can help.

Thanks,

Juan

Assigning bug so that nobody thinks it is up for grabs and so that it does not expire in 60 days.

(Minor nit: at no point was this bug incomplete in the sense LP uses incomplete.)

I've pushed up an update with a working start hook and an upstart job.

@Juan, thanks for the offer, I think I got it all sorted.

Ok, starting with a static review:

# things to change

- add key "maintainer: $name <$email>" to metadata.yaml please

# discussion/recommendations

- how should the opentsdb code and/or schema handle upgrades? We should have an `upgrade-charm` hook

- nice start guard... handles delaying start until after zk relates _and_ allow juju control of the service afterwards. Might fail if zk relation goes away... perhaps we should have some zk-relation-{departed,broken} hooks at some point

- maybe turn the upstart job into a real template or at least a bash here-doc so it's easier to read

- maybe clean out boilerplate code from hooks? might be confusing to future readers of the charm

looks good... spinning things up now

approve

ready to promulgate as soon as I either have:

    - branch with maintainer update

or

    - just tell me the email you'd like to use for maintainer and I'll add it in

Thanks for sharing the charm!