Merge lp:~matsubara/charms/precise/oops-tools/trunk into lp:charms/oops-tools

Proposed by Diogo Matsubara
Status: Merged
Merged at revision: 12
Proposed branch: lp:~matsubara/charms/precise/oops-tools/trunk
Merge into: lp:charms/oops-tools
Diff against target: 200 lines (+134/-10)
7 files modified
.bzrignore (+1/-0)
README.txt (+7/-0)
config.yaml (+33/-0)
hooks/amqp-relation-changed (+62/-0)
hooks/db-relation-changed (+12/-10)
hooks/db-relation-joined (+16/-0)
metadata.yaml (+3/-0)
To merge this branch: bzr merge lp:~matsubara/charms/precise/oops-tools/trunk
Reviewer Review Type Date Requested Status
Clint Byrum (community) Needs Fixing
Review via email: mp+112017@code.launchpad.net

Commit message

Add basic README, support to use amqp and list Diogo Matsubara as the maintainer of the charm.

Description of the change

Hi,

this branch adds a README file, myself as the maintainer and support to use an amqp queue.

charm proof doesn't return anything, so I think it's free of lint.

Cheers,

Diogo

To post a comment you must log in.
Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Diogo, this does not look right.

I see config-get's mixed with relation-get's in db-relation-changed/joined.

There is a rabbitmq charm, so I'd like to see a rabbitmq relation in addition to the config.yaml values for the external AMQP server.

Also the check for empty relation values seems to have been removed in db-relation-changed, which may cause race conditions since we may run that hook before the database server has set the values we want.

review: Needs Fixing
Revision history for this message
Diogo Matsubara (matsubara) wrote :

> Diogo, this does not look right.

Hi Clint, thanks for the review

>
> I see config-get's mixed with relation-get's in db-relation-changed/joined.

The way I understand the workflow is:

db-relation-joined sets the values for USER and DATABASE from the config values and pass those to the postgresql charm
Once postgresql charm finishes doing its job, it triggers the db-relation-changed and oops-tools sets its configs from those values. Before, oops-tools charm didn't have a db-relation-joined hook, so that's why there was a check if the user was set. Now that it has a joined and changed hooks, when the changed runs I can safely assume the db charm has been run.
Is this a misunderstanding on my part? Please help clarify so I can fix this.

>
> There is a rabbitmq charm, so I'd like to see a rabbitmq relation in addition
> to the config.yaml values for the external AMQP server.
>

Not sure I understand this, I added a amqp-relation-changed hook. What else is missing?

> Also the check for empty relation values seems to have been removed in db-
> relation-changed, which may cause race conditions since we may run that hook
> before the database server has set the values we want.

Explained above.

Cheers,

Diogo

Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Excerpts from Diogo Matsubara's message of 2012-07-02 08:40:28 UTC:
> > Diogo, this does not look right.
>
> Hi Clint, thanks for the review
>
> >
> > I see config-get's mixed with relation-get's in db-relation-changed/joined.
>
> The way I understand the workflow is:
>
> db-relation-joined sets the values for USER and DATABASE from the config values and pass those to the postgresql charm
> Once postgresql charm finishes doing its job, it triggers the db-relation-changed and oops-tools sets its configs from those values. Before, oops-tools charm didn't have a db-relation-joined hook, so that's why there was a check if the user was set. Now that it has a joined and changed hooks, when the changed runs I can safely assume the db charm has been run.

Ahh, no you cannot make this assumption. joined and changed *always*
run in series at the beginning of a relationship. You cannot assume that
the value has been set when changed runs, as it may just be running in
that initial first-pass mode before relation-set has happened on the
other side. It will be run again when the values are set.

> Is this a misunderstanding on my part? Please help clarify so I can fix this.
>
> >
> > There is a rabbitmq charm, so I'd like to see a rabbitmq relation in addition
> > to the config.yaml values for the external AMQP server.
> >
>
> Not sure I understand this, I added a amqp-relation-changed hook. What else is missing?

I think I just must have missed that. Will review again once the check
for empty relation values is restored.

12. By Diogo Matsubara

restore check for relation values

Revision history for this message
Diogo Matsubara (matsubara) wrote :

> Excerpts from Diogo Matsubara's message of 2012-07-02 08:40:28 UTC:
> > > Diogo, this does not look right.
> >
> > Hi Clint, thanks for the review
> >
> > >
> > > I see config-get's mixed with relation-get's in db-relation-
> changed/joined.
> >
> > The way I understand the workflow is:
> >
> > db-relation-joined sets the values for USER and DATABASE from the config
> values and pass those to the postgresql charm
> > Once postgresql charm finishes doing its job, it triggers the db-relation-
> changed and oops-tools sets its configs from those values. Before, oops-tools
> charm didn't have a db-relation-joined hook, so that's why there was a check
> if the user was set. Now that it has a joined and changed hooks, when the
> changed runs I can safely assume the db charm has been run.
>
> Ahh, no you cannot make this assumption. joined and changed *always*
> run in series at the beginning of a relationship. You cannot assume that
> the value has been set when changed runs, as it may just be running in
> that initial first-pass mode before relation-set has happened on the
> other side. It will be run again when the values are set.

Thanks! I restored the check in changed.

>
> > Is this a misunderstanding on my part? Please help clarify so I can fix
> this.
> >
> > >
> > > There is a rabbitmq charm, so I'd like to see a rabbitmq relation in
> addition
> > > to the config.yaml values for the external AMQP server.
> > >
> >
> > Not sure I understand this, I added a amqp-relation-changed hook. What else
> is missing?
>
> I think I just must have missed that. Will review again once the check
> for empty relation values is restored.

Ok, let me know if anything else is needed.

Cheers,

Diogo

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file '.bzrignore'
--- .bzrignore 1970-01-01 00:00:00 +0000
+++ .bzrignore 2012-07-03 07:24:22 +0000
@@ -0,0 +1,1 @@
1local.yaml
02
=== added file 'README.txt'
--- README.txt 1970-01-01 00:00:00 +0000
+++ README.txt 2012-07-03 07:24:22 +0000
@@ -0,0 +1,7 @@
1To deploy:
2
3juju deploy --repository=. local:oops-tools
4juju deploy --repository=. local:postgresql
5juju add-relation postgresql:db oops-tools
6juju add-relation rabbitmq oops-tools
7juju expose oops-tools
08
=== added file 'config.yaml'
--- config.yaml 1970-01-01 00:00:00 +0000
+++ config.yaml 2012-07-03 07:24:22 +0000
@@ -0,0 +1,33 @@
1options:
2 oops-web-amqp-host:
3 type: string
4 default: "localhost"
5 description: The rabbitMQ server.
6 oops-web-amqp-user:
7 type: string
8 default: 'oopstools'
9 description: The user to log into the rabbitMQ server.
10 oops-web-amqp-password:
11 type: string
12 default: 'guest'
13 description: The password for the user to log into the rabbitMQ server.
14 oops-web-amqp-vhost:
15 type: string
16 default: '/'
17 description: The vhost in the rabbitMQ server.
18 oops-web-amqp-exchange:
19 type: string
20 default: 'oopses'
21 description: The exchange to use on the rabbitMQ server.
22 oops-web-amqp-routing:
23 type: string
24 default: ''
25 description: The routing key to use on the rabbitMQ server.
26 db-user:
27 type: string
28 default: 'oopstools'
29 description: The database user for oops-tools
30 db-name:
31 type: string
32 default: 'oopstools'
33 description: The database name for oops-tools
034
=== added file 'hooks/amqp-relation-changed'
--- hooks/amqp-relation-changed 1970-01-01 00:00:00 +0000
+++ hooks/amqp-relation-changed 2012-07-03 07:24:22 +0000
@@ -0,0 +1,62 @@
1#!/bin/bash
2set -eux
3OOPS_HOME="/var/www/oops-tools"
4OOPS_DIR="$OOPS_HOME/logs/oops-amqp/"
5if [[ ! -d $OOPS_DIR ]]; then
6 mkdir -p $OOPS_DIR
7fi
8RABBIT_USER=`config-get oops-web-amqp-user`
9RABBIT_VHOST=`config-get oops-web-amqp-vhost`
10RABBIT_EXCHANGE=`config-get oops-web-amqp-exchange`
11RABBIT_ROUTING=`config-get oops-web-amqp-exchange`
12
13juju-log "amqp_joined: requesting credentials for $RABBIT_USER"
14echo "amqp_joined: requesting credentials for $RABBIT_USER"
15relation-set username=$RABBIT_USER
16relation-set vhost=$RABBIT_VHOST
17
18RABBIT_HOST=`relation-get private-address`
19RABBIT_PASSWORD=`relation-get password`
20if [[ -z $RABBIT_HOST ]] || [[ -z $RABBIT_PASSWORD ]] ; then
21 echo "amqp_changed: RABBIT_HOST||RABBIT_PASSWORD not set. Peer not ready? Exit 0 and retry"
22 exit 0
23fi
24echo "amqp_changed: Setting rabbit config on production.cfg: $RABBIT_HOST $RABBIT_USER $RABBIT_PASSWORD"
25# Use # as the separator since $OOPS_DIR has slashes on it
26sed -i "s#^oopsdir =.*#oopsdir = $OOPS_DIR#g" $OOPS_HOME/production.cfg
27sed -i "s/^oops-web-amqp-host =.*$/oops-web-amqp-host = $RABBIT_HOST/g" \
28 $OOPS_HOME/production.cfg
29sed -i "s/^oops-web-amqp-user =.*$/oops-web-amqp-user = $RABBIT_USER/g" \
30 $OOPS_HOME/production.cfg
31sed -i "s/^oops-web-amqp-password =.*$/oops-web-amqp-password = $RABBIT_PASSWORD/g" \
32 $OOPS_HOME/production.cfg
33# Use # as the separator since $RABBIT_VHOST has slashes on it
34sed -i "s#^oops-web-amqp-vhost =.*#oops-web-amqp-vhost = $RABBIT_VHOST#g" \
35 $OOPS_HOME/production.cfg
36sed -i \
37 "s/^oops-web-amqp-exchange =.*$/oops-web-amqp-exchange = $RABBIT_EXCHANGE/g" \
38 $OOPS_HOME/production.cfg
39sed -i \
40 "s/^oops-web-amqp-routing =.*$/oops-web-amqp-routing = $RABBIT_ROUTING/g" \
41 $OOPS_HOME/production.cfg
42# Remove old settings.py
43rm -f $OOPS_HOME/src/oopstools/settings.py
44# Update settings.py from production.cfg
45$OOPS_HOME/bin/buildout -c $OOPS_HOME/production.cfg
46# Reload oops-tools
47service apache2 restart
48# Setup the amqp2disk script.
49# First kill any amqp2disk scripts running.
50AMQP2DISK_PID=`ps afx|grep amqp2disk | sed '/grep/d' | awk '{print $1}'`
51if [[ -n $AMQP2DISK_PID ]]; then
52 kill -9 $AMQP2DISK_PID
53fi
54
55# Now run it with the updated config.
56$OOPS_HOME/bin/amqp2disk --host $RABBIT_HOST \
57 --output $OOPS_DIR \
58 --username $RABBIT_USER \
59 --password $RABBIT_PASSWORD \
60 --vhost $RABBIT_VHOST \
61 --queue $RABBIT_EXCHANGE \
62 --bind-to $RABBIT_EXCHANGE &
063
=== modified file 'hooks/db-relation-changed'
--- hooks/db-relation-changed 2012-01-19 00:54:52 +0000
+++ hooks/db-relation-changed 2012-07-03 07:24:22 +0000
@@ -1,22 +1,24 @@
1#!/bin/bash1#!/bin/bash
2set -eux # -x for verbose logging to juju debug-log2set -eux # -x for verbose logging to juju debug-log
3hooksdir=$PWD3hooksdir=$PWD
4user=`relation-get user`4USER=`relation-get user`
5password=`relation-get password`5DATABASE=`relation-get database`
6host=`relation-get host`6HOST=`relation-get host`
7database=`relation-get database`7PASSWORD=`relation-get password`
8
8# All values are set together, so checking on a single value is enough9# All values are set together, so checking on a single value is enough
9# If $user is not set, DB is still setting itself up, we exit awaiting next run10# If $user is not set, DB is still setting itself up, we exit awaiting next run
10[ -z "$user" ] && exit 011[ -z "$user" ] && exit 0
11juju-log "Setting up Oops tools for the first time"12
12# Write production.cfg13# Update oops-tools config with db relation values.
13sed -i "s/^db-name =.*$/db-name = $database/g" /var/www/oops-tools/production.cfg14sed -i "s/^db-name =.*$/db-name = $DATABASE/g" /var/www/oops-tools/production.cfg
14sed -i "s/^db-host =.*$/db-host = $host/g" /var/www/oops-tools/production.cfg15sed -i "s/^db-host =.*$/db-host = $HOST/g" /var/www/oops-tools/production.cfg
15sed -i "s/^db-user =.*$/db-user = $user/g" /var/www/oops-tools/production.cfg16sed -i "s/^db-user =.*$/db-user = $USER/g" /var/www/oops-tools/production.cfg
16sed -i "s/^db-password =.*$/db-password = $password/g" /var/www/oops-tools/production.cfg17sed -i "s/^db-password =.*$/db-password = $PASSWORD/g" /var/www/oops-tools/production.cfg
17sed -i "s/^db-port =.*$/db-port = 5432/g" /var/www/oops-tools/production.cfg18sed -i "s/^db-port =.*$/db-port = 5432/g" /var/www/oops-tools/production.cfg
18cd /var/www/oops-tools19cd /var/www/oops-tools
19rm -f src/oopstools/settings.py20rm -f src/oopstools/settings.py
21# Update settings.py
20bin/buildout -c production.cfg22bin/buildout -c production.cfg
21bin/django syncdb --noinput23bin/django syncdb --noinput
22bin/django migrate24bin/django migrate
2325
=== added file 'hooks/db-relation-joined'
--- hooks/db-relation-joined 1970-01-01 00:00:00 +0000
+++ hooks/db-relation-joined 2012-07-03 07:24:22 +0000
@@ -0,0 +1,16 @@
1#!/bin/bash
2set -eux # -x for verbose logging to juju debug-log
3hooksdir=$PWD
4OOPS_HOME=/var/www/oops-tools
5USER=`config-get db-user`
6DATABASE=`config-get db-name`
7relation-set user=$USER
8relation-set database=$DATABASE
9$HOST=`relation-get host`
10$PASSWORD=`relation-get password`
11
12sed -i "s/^db-name =.*$/db-name = $DATABASE/g" $OOPS_HOME/production.cfg
13sed -i "s/^db-host =.*$/db-host = $HOST/g" $OOPS_HOME/production.cfg
14sed -i "s/^db-user =.*$/db-user = $USER/g" $OOPS_HOME/production.cfg
15sed -i "s/^db-password =.*$/db-password = $PASSWORD/g" $OOPS_HOME/production.cfg
16sed -i "s/^db-port =.*$/db-port = 5433/g" $OOPS_HOME/production.cfg
017
=== modified file 'metadata.yaml'
--- metadata.yaml 2012-01-30 18:54:42 +0000
+++ metadata.yaml 2012-07-03 07:24:22 +0000
@@ -1,10 +1,13 @@
1name: oops-tools1name: oops-tools
2maintainer: Diogo Matsubara <diogo.matsubara@canonical.com>
2summary: "Oops tools"3summary: "Oops tools"
3description: |4description: |
4 Django app to analyse and aggregate OOPS reports5 Django app to analyse and aggregate OOPS reports
5requires:6requires:
6 db:7 db:
7 interface: pgsql8 interface: pgsql
9 amqp:
10 interface: rabbitmq
8provides:11provides:
9 website:12 website:
10 interface: http13 interface: http

Subscribers

People subscribed via source and target branches

to all changes: