Merge lp:~thedac/capomastro/charm-configurable-nagios-context into lp:~ubuntuone-hackers/capomastro/charm

[David Ames]

Make nagios_context configurable rather than hardcoded so staging and production use different service/hostgroups

Fix bug missing celery description for the celery check

This does not have to be the solution but something very much like this is required.

[Daniel Manrique (roadmr)]

Thanks for contributing this to our charm :)

TL;DR We can merge this if you think the issues I comment on below are not going to affect your use case, though we should fix them down the line (and that's work that we can tackle later, no need to trouble you with this).

Now for the gory details.

I noticed one thing while testing this (which I did by running the spec, while forgetting to set the proper nagios_context). nagios_context is only used in the nrpe-external-master-relation-changed hook; so:

1) If the nagios_context is changed via a juju set, the .cfg file won't be updated.
2) If the nagios_context is changed and *then* the relation is destroyed/readded, then a .cfg file is created with the new nagios_context *but* the old one is not deleted.

I'm talking about the files in /etc/nagios/nrpe.d; I notice (and checked) that you nuke /var/lib/nagios/export/service_* in the hook, so that part is covered.

I don't know enough about nagios to tell if this is important; I also imagine that for deployment purposes you won't make the same rookie mistake I made by not setting nagios_context correctly from the getgo. I'm OK with this addition to our charm if it serves your purposes within the constraints I just mentioned, but (pending your input on whether it's needed), I'd like to later fix the hook so it does the right thing with files in /etc/ (unless you tell me they're not really needed), and so that they also get updated upon changing the option in question.

review: Needs Information

[David Ames (thedac)]

Daniel,

Thanks for looking at this.

> Thanks for contributing this to our charm :)
>
> TL;DR We can merge this if you think the issues I comment on below are not
> going to affect your use case, though we should fix them down the line (and
> that's work that we can tackle later, no need to trouble you with this).
>
> Now for the gory details.
>
> I noticed one thing while testing this (which I did by running the spec, while
> forgetting to set the proper nagios_context). nagios_context is only used in
> the nrpe-external-master-relation-changed hook; so:
>
> 1) If the nagios_context is changed via a juju set, the .cfg file won't be
> updated.

This would be ideal. That would mean the nagios configs need to be generated during a config-changed hook and not just nrpe-external-master changed. I didn't want to muck about in someone else’s charm so I changed as little as possible.

However, what you are describing is the right thing to do. Nagios config generation code should be in a common area that multiple hooks can call. I'll leave that to your team.

> 2) If the nagios_context is changed and *then* the relation is
> destroyed/readded, then a .cfg file is created with the new nagios_context
> *but* the old one is not deleted.

Right see above, due nagios configs not being generated in config-changed yet. The left over file in this case is OK, though it may make sense to name it in such a way that the nagios_context does not affect the file name.

> I'm talking about the files in /etc/nagios/nrpe.d; I notice (and checked) that
> you nuke /var/lib/nagios/export/service_* in the hook, so that part is
> covered.

Right the above is only true for /etc/nagios/nrpe.d

> I don't know enough about nagios to tell if this is important; I also imagine
> that for deployment purposes you won't make the same rookie mistake I made by
> not setting nagios_context correctly from the getgo. I'm OK with this addition
> to our charm if it serves your purposes within the constraints I just
> mentioned, but (pending your input on whether it's needed), I'd like to later
> fix the hook so it does the right thing with files in /etc/ (unless you tell
> me they're not really needed), and so that they also get updated upon changing
> the option in question.

Ultimately we (IS) just need this functionality added. This MP was as much an example as a requested MP. So feel free to dump this MP and have your team work on a complete solution. Having said that, this MP will fix our immediate problem and could be a stepping stone.

[Daniel Manrique (roadmr)]

Gotcha, OK, I'll merge this now to unblock you guys, as it won't break anything *we* depend on and it fixes things for you. But I'll file a bug in our project to improve this down the road.

Thanks again!

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2014-12-16 16:53:12 +0000
+++ config.yaml	2015-02-19 01:03:00 +0000
@@ -47,3 +47,13 @@
     type: string
     default: "ppa:ce-infrastructure/capomastro"
     description: "The PPA address or private repository line for apt-add-repository"
+  nagios_context:
+    default: "juju"
+    type: string
+    description: >
+        Used by the nrpe-external-master subordinate charm.
+        A string that will be prepended to instance name to set the host name
+        in nagios. So for instance the hostname would be something like:
+            juju-postgresql-0
+        If you're running multiple environments with the same services in them
+        this allows you to differentiate between them.

=== modified file 'hooks/nrpe-external-master-relation-changed'
--- hooks/nrpe-external-master-relation-changed	2014-12-19 11:00:12 +0000
+++ hooks/nrpe-external-master-relation-changed	2015-02-19 01:03:00 +0000
@@ -11,22 +11,25 @@
 	# a very opinionated charm by providing hardcoded values
 	# that possibly no one would need to change, hence we'll
 	# have a less cluttered config.yaml file
+	export t_nagios_context="$(config-get nagios_context)"
 	export t_nagios_project="PES"
-	export t_nagios_instance_type="production"
-	export t_nagios_servicegroup="pes-prodstack-cert-infra"
-	export t_nagios_hostname="pes-prodstack-cert-infra-${unit_name//\//-}"
+	export t_nagios_servicegroup="$t_nagios_context"
+	export t_nagios_hostname="${t_nagios_context}-${unit_name//\//-}"
+
+	# Clean slate
+	rm -f /var/lib/nagios/export/service_*
 
 	# check for the actual web page being available or not
 	export t_nagios_service_description="Capomastro build server"
-	export t_nagios_command_name="check_pes_capomastro_app_${t_nagios_instance_type}"
+	export t_nagios_command_name="check_${t_nagios_context}_capomastro_app"
 	export t_nagios_command="/usr/lib/nagios/plugins/check_http -I ${private_address} -p ${port} -u / -e '302 FOUND' -u / -d 'http://capomastro/accounts/login/'"
 	
 	cheetah fill --env -p templates/nrpe_command_file.tmpl > /etc/nagios/nrpe.d/${t_nagios_command_name}.cfg
 	cheetah fill --env -p templates/nrpe_service_file.tmpl > /var/lib/nagios/export/service_${t_nagios_hostname}_${t_nagios_command_name}.cfg
 
 	# check the status of the celery workers
-	export t_service_description="Capomastro celery workers"
-	export t_nagios_command_name="check_pes_capomastro_celery_${t_nagios_instance_type}"
+	export t_nagios_service_description="Capomastro celery workers"
+	export t_nagios_command_name="check_${t_nagios_context}_capomastro_celery"
 	export t_nagios_command="/usr/local/lib/nagios/plugins/check_status_file.sh /var/log/capomastro/capomastro-celery-status"
 
 	cheetah fill --env -p templates/nrpe_command_file.tmpl > /etc/nagios/nrpe.d/${t_nagios_command_name}.cfg

=== modified file 'templates/nrpe_service_file.tmpl'
--- templates/nrpe_service_file.tmpl	2014-11-04 19:33:38 +0000
+++ templates/nrpe_service_file.tmpl	2015-02-19 01:03:00 +0000
@@ -2,7 +2,7 @@
 define service {
     use                             active-service
     host_name                       ${t_nagios_hostname}
-    service_description             ${t_nagios_hostname} ${t_nagios_project} ${t_nagios_instance_type} ${t_nagios_service_description}
+    service_description             ${t_nagios_hostname} ${t_nagios_project} ${t_nagios_context} ${t_nagios_service_description}
     check_command                   check_nrpe!${t_nagios_command_name}
     servicegroups                   ${t_nagios_servicegroup},
 }