Merge lp:~mbruzek/charms/trusty/apache2/nagios_extra_check_https into lp:charms/trusty/apache2

[Matt Bruzek]

@pedronis

I took the time to make some of the changes I suggested to your branch.
lp:~pedronis/charms/trusty/apache2/nagios_extra_check_https

Added a section to the readme. This could use some more specific examples and perhaps more description on how to use the nagios configuration options (I am not an expert). Any more information that you could give the users would be great here.

I also fixed up the amulet tests, they needed more time to deploy and install the prerequisites.

[Samuele Pedroni (pedronis)]

thanks for improving on my branch, I was a bit clueless about the amulet tests, they were failing already

a typical params is something like this:

-S -H svc.example.com -I 127.0.0.1 -e '"200 OK"' --url=/

or so

if we want to give an example

review: Approve

[Kevin W Monroe (kwmonroe)]

Hey @mbruzek and @pedronis,

The new monitoring section of the README is fantastic. Excellent job!

However, I'm curious if we could implement this extra_http_checks differently. Rather than create a new config opt that takes yaml (which is difficult to type into the cli and juju-gui), could we make the existing nagios_check_http_params take multiple args? If so, it seems like the functionality from this MP could be achieved with:

juju set apache2 nagios_check_http_params="-H hostA -e '200 OK'; -H hostB -e '200 OK'"

The update_nrpe_checks() in hooks.py would then split nagios_check_http_params on a semicolon (or whatever delimiter makes sense) and call check_http on each of the splits. The README would also need to be updated to let users know that nagios_check_http_params could take multiple check params.

To me this is less confusing than having 2 config opts that are both passed on to check_http. Thoughts?

As an aside, when working through this review, I noticed 'make test' failed. I know you didn't introduce this failure, but I opened a bug in case you're interested in fixing :)

https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474

[Samuele Pedroni (pedronis)]

we went with a full new config because when you have many checks you really
care about being able to set sane descriptions for them, and finding a
format for that in one string I think would get unwieldy. Honestly we need
keep the original one for backward compatible/the simple case

On Thu, Oct 29, 2015 at 6:30 PM, Kevin W Monroe <email address hidden>
wrote:

> Hey @mbruzek and @pedronis,
>
> The new monitoring section of the README is fantastic. Excellent job!
>
> However, I'm curious if we could implement this extra_http_checks
> differently. Rather than create a new config opt that takes yaml (which is
> difficult to type into the cli and juju-gui), could we make the existing
> nagios_check_http_params take multiple args? If so, it seems like the
> functionality from this MP could be achieved with:
>
> juju set apache2 nagios_check_http_params="-H hostA -e '200 OK'; -H hostB
> -e '200 OK'"
>
> The update_nrpe_checks() in hooks.py would then split
> nagios_check_http_params on a semicolon (or whatever delimiter makes sense)
> and call check_http on each of the splits. The README would also need to be
> updated to let users know that nagios_check_http_params could take multiple
> check params.
>
> To me this is less confusing than having 2 config opts that are both
> passed on to check_http. Thoughts?
>
> As an aside, when working through this review, I noticed 'make test'
> failed. I know you didn't introduce this failure, but I opened a bug in
> case you're interested in fixing :)
>
> https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474
> --
>
> https://code.launchpad.net/~mbruzek/charms/trusty/apache2/nagios_extra_check_https/+merge/275095
> You are reviewing the proposed merge of
> lp:~mbruzek/charms/trusty/apache2/nagios_extra_check_https into
> lp:charms/trusty/apache2.
>

[Kevin W Monroe (kwmonroe)]

Hey Samuele, I get your point about multiple host checks becoming unwieldy in 1 string, but I'm still off-put by having 2 config opts for this. How do you feel about testing nagios_check_http_params for "yamlness", and looping over the yaml if true, or treating it as a string if false? Something like:

--- hooks/hooks.py 2015-04-15 20:23:01 +0000
+++ hooks/hooks.py 2015-10-29 18:27:20 +0000
@@ -432,11 +432,21 @@
     conf = nrpe_compat.config
     check_http_params = conf.get('nagios_check_http_params')
     if check_http_params:
- nrpe_compat.add_check(
- shortname='vhost',
- description='Check Virtual Host',
- check_cmd='check_http %s' % check_http_params
- )
+ for check_http in yaml.safe_load(check_http_params):
+ try:
+ name = check_http['name']
+ nrpe_compat.add_check(
+ shortname=name,
+ description=check_http.get('description', name),
+ check_cmd='check_http %s' % check_http['params']
+ )
+ except TypeError:
+ nrpe_compat.add_check(
+ shortname='vhost',
+ description='Check Virtual Host',
+ check_cmd='check_http %s' % check_http_params
+ )
+ break
     nrpe_compat.write()

[Kevin W Monroe (kwmonroe)]

Ouch on that formatting.. how about this: http://paste.ubuntu.com/13001287/

[Samuele Pedroni (pedronis)]

> Ouch on that formatting.. how about this: http://paste.ubuntu.com/13001287/

sorry been busy with other things, I'm not against if the ecosys team thinks is not too strange,

about that code, in the case of a simple string this works because the for check_http will be one char strings and so we get the TypeError, defensively I would put the try except just around

name = check_http['name']

or even be more explicit separating cases

Unmerged revisions

70. By Matt Bruzek on 2015-10-16

Updating readme with monitoring section and updating amulet tests.

69. By Samuele Pedroni on 2015-08-28

the spelling was confusing, rename

68. By Samuele Pedroni on 2015-08-28

support new config nagios_extra_check_https to specify extra nrpe check_http checks, useful when fronting more than one backend

=== modified file 'README.md'
--- README.md	2015-04-15 22:59:59 +0000
+++ README.md	2015-10-20 20:08:17 +0000
@@ -296,13 +296,44 @@
 Space separated list of modules to be enabled or disabled. If a module to
 be enabled cannot be found then the charm will attempt to install it.
 
-## OpenId 
+## OpenId
 
-The openid_provider option takes a comma seperated list of OpenID
+The openid_provider option takes a comma separated list of OpenID
 providers and places them in /etc/apache2/security/allowed-ops.txt. That
-file can then be refernced by the allowed-op-list-url option when using
+file can then be referenced by the allowed-op-list-url option when using
 apache_openid
 
+## Monitoring
+
+This charm can be configured for monitoring with Nagios.  This charm can be
+related to the `nrpe` subordinate and then nrpe can relate to a nagios charm.
+This charm can also use the `nrpe-external-master` charm for Nagios systems
+outside of Juju.
+
+The following is a description of the nagios configuration options:  
+
+`nagios_context` 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.
+
+`nagios_servicegroups`  A comma-separated list of nagios servicegroups.
+      If left empty, the nagios_context will be used as the servicegroup
+
+`nagios_check_http_params` The parameters to pass to the nrpe plugin check_http.  
+
+`nagios_extra_http_checks` Extra nrpe plugin check_http checks. Useful when
+fronting more than one backend. A string with a YAML sequence of mappings with
+keys name, params (like nagios_check_http_params) and optionally a description:  
+
+    - name: backend1
+      params: -H ...
+    - name: backend2
+      description: Check backend2 endpoint
+      params: -H ...
+
 ## TODO:
 
   * Document the use of balancer, nrpe, logging and website-cache

=== modified file 'config.yaml'
--- config.yaml	2015-08-11 19:06:25 +0000
+++ config.yaml	2015-10-20 20:08:17 +0000
@@ -118,6 +118,19 @@
      default: ""
      type: string
      description: The parameters to pass to the nrpe plugin check_http.
+  nagios_extra_http_checks:
+     default: ""
+     type: string
+     description: |
+         Extra nrpe plugin check_http checks. Useful when fronting more than
+         one backend. A string with a YAML sequence of mappings with keys
+         name, params (like nagios_check_http_params) and optionally
+         description:
+             - name: backend1
+               params: -H ...
+             - name: backend2
+               description: Check backend2 endpoint
+               params: -H ...
   logrotate_rotate:
     type: string
     description: daily, weekly, monthly, or yearly?

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2015-04-15 20:23:01 +0000
+++ hooks/hooks.py	2015-10-20 20:08:17 +0000
@@ -437,6 +437,15 @@
             description='Check Virtual Host',
             check_cmd='check_http %s' % check_http_params
         )
+    extra_http_checks = conf.get('nagios_extra_http_checks')
+    if extra_http_checks:
+        for check_http in yaml.safe_load(extra_http_checks):
+            name = check_http['name']
+            nrpe_compat.add_check(
+                shortname=name,
+                description=check_http.get('description', 'Check %s' % name),
+                check_cmd='check_http %s' % check_http['params']
+            )
     nrpe_compat.write()
 
 

=== modified file 'hooks/tests/test_nrpe_hooks.py'
--- hooks/tests/test_nrpe_hooks.py	2015-03-09 16:32:43 +0000
+++ hooks/tests/test_nrpe_hooks.py	2015-10-20 20:08:17 +0000
@@ -1,5 +1,5 @@
 from testtools import TestCase
-from mock import patch
+from mock import call, patch
 
 import hooks
 
@@ -28,3 +28,30 @@
         hooks.update_nrpe_checks()
         self.assertFalse(nrpe.add_check.called)
         nrpe.write.assert_called_once_with()
+
+    @patch('hooks.nrpe.NRPE')
+    def test_update_nrpe_with_extra_http_checks(self, mock_nrpe):
+        nrpe = mock_nrpe.return_value
+        nrpe.config = {
+            'nagios_extra_http_checks': """
+- name: backend1
+  params: -H bar1
+- name: backend2
+  description: Check backend2 endpoint
+  params: -H bar2
+""",
+        }
+        hooks.update_nrpe_checks()
+        add_check_calls = nrpe.add_check.mock_calls
+        self.assertEqual(len(add_check_calls), 2)
+        self.assertEqual(add_check_calls[0], call(
+            shortname='backend1',
+            description='Check backend1',
+            check_cmd='check_http -H bar1'
+        ))
+        self.assertEqual(add_check_calls[1], call(
+            shortname='backend2',
+            description='Check backend2 endpoint',
+            check_cmd='check_http -H bar2'
+        ))
+        nrpe.write.assert_called_once_with()

=== modified file 'tests/00-setup'
--- tests/00-setup	2014-10-29 00:10:14 +0000
+++ tests/00-setup	2015-10-20 20:08:17 +0000
@@ -1,6 +1,6 @@
 #!/bin/bash
 
-set -x 
+set -x
 
 # Check if amulet is installed before adding repository and updating apt-get.
 dpkg -s amulet
@@ -11,3 +11,6 @@
 fi
 
 # Install any additional python packages or software here.
+sudo apt-get install -y python-software-properties python-flake8 python-nose \
+python-coverage python-testtools python-pyasn1 python-pyasn1-modules \
+python-openssl python-apt python-virtualenv python-jinja2

=== modified file 'tests/20-mpm-test.py'
--- tests/20-mpm-test.py	2015-03-10 10:56:19 +0000
+++ tests/20-mpm-test.py	2015-10-20 20:08:17 +0000
@@ -12,7 +12,7 @@
         """ Set up an amulet deployment using the bundle. """
         d = amulet.Deployment(series='trusty')
         d.add('apache2', os.path.join(os.path.dirname(__file__), os.pardir))
-        d.setup(180)
+        d.setup(600)
         cls.d = d
         cls.unit = d.sentry.unit['apache2/0']
         output, code = cls.unit.run('curl localhost')