Merge lp:~rcj/charms/precise/nrpe/trunk into lp:charms/nrpe

[Robert C Jennings]

nrpe charm pep8/pylint cleanup and additional testing

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-1051-results

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-1197-results

review: Needs Fixing (automated testing)

[Adam Israel (aisrael)]

Hi Robert,

Thanks for all of your efforts getting this charm cleaned up. I had the opportunity to review the charm today, and wanted to provide some feedback.

I attempted to run the tests both manually, and how the automated testing runs (using bundletester).

Bundletester, run from the charm directory, failed:

$ bundletester -l DEBUG
...
DEBUG:runner:call ['/vagrant/precise/nrpe/tests/20-local_monitors']
DEBUG:runner:
Traceback (most recent call last):
  File "/vagrant/precise/nrpe/tests/20-local_monitors", line 25, in <module>
    with open('../test-monitors.yaml', 'r') as monitors_file:
FileNotFoundError: [Errno 2] No such file or directory: '../test-monitors.yaml'

DEBUG:runner:Exit Code: 1
    20-local_monitors

The 'test-monitors.yaml' file exists in the charm root, but with bundletester run from the charm root, I suspect it's checking for that file outside of the charm, instead of relative to the test script.

Next, I reset my environment and ran the tests by hand. I still encountered an error running 20-local_monitors, however.

$ ./20-local_monitors
2014-10-29 16:02:31 Starting deployment of local
2014-10-29 16:02:31 Deploying services...
2014-10-29 16:02:38 Adding relations...
2014-10-29 16:02:39 Adding relation nrpe:local-monitors <-> apache2:local-monitors
2014-10-29 16:03:39 Deployment complete in 69.14 seconds
Expected file on nrpe not found: /etc/nagios/nrpe.d/check_true.cfg

review: Needs Fixing

[Adam Israel (aisrael)]

Here are some additional details that might help:

$ juju status
environment: local
machines:
  "0":
    agent-state: started
    agent-version: 1.20.10.1
    dns-name: localhost
    instance-id: localhost
    series: trusty
    state-server-member-status: has-vote
  "1":
    agent-state: started
    agent-version: 1.20.10.1
    dns-name: 10.0.3.171
    instance-id: vagrant-local-machine-1
    series: precise
    hardware: arch=amd64
  "2":
    agent-state: started
    agent-version: 1.20.10.1
    dns-name: 10.0.3.222
    instance-id: vagrant-local-machine-2
    series: precise
    hardware: arch=amd64
services:
  apache2:
    charm: cs:precise/apache2-25
    exposed: false
    relations:
      juju-info:
      - nrpe
      local-monitors:
      - nrpe
    units:
      apache2/0:
        agent-state: started
        agent-version: 1.20.10.1
        machine: "1"
        public-address: 10.0.3.171
        subordinates:
          nrpe/0:
            upgrading-from: cs:precise/nrpe-7
            agent-state: started
            agent-version: 1.20.10.1
            public-address: 10.0.3.171
  nagios:
    charm: cs:precise/nagios-10
    exposed: false
    relations:
      monitors:
      - nrpe
    units:
      nagios/0:
        agent-state: started
        agent-version: 1.20.10.1
        machine: "2"
        open-ports:
        - 80/tcp
        public-address: 10.0.3.222
  nrpe:
    charm: cs:precise/nrpe-7
    exposed: false
    relations:
      general-info:
      - apache2
      local-monitors:
      - apache2
      monitors:
      - nagios
    subordinate-to:
    - apache2

ubuntu@vagrant-local-machine-1:/etc/nagios/nrpe.d$ ls -la
total 16
drwxr-xr-x 1 root root 114 Oct 29 16:00 .
drwxr-xr-x 1 root root 56 Oct 29 16:00 ..
-rwxr----- 1 root root 25 Oct 29 16:00 allowed.hosts.cfg
-rw-r--r-- 1 root root 144 Oct 29 16:00 base-monitors.cfg
-rw-r--r-- 1 root root 96 Oct 29 16:02 check_mem.cfg
-rw-r--r-- 1 root root 67 Oct 29 16:02 disk__.cfg

[Review Queue (review-queue)]

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-10371-results

review: Needs Fixing (automated testing)

[Marco Ceppi (marcoceppi)]

I'm moving this back in to "work in progress" When you're ready for another review please move this merge to "needs review"

review: Needs Information

[Robert C Jennings (rcj)]

I have corrected the file path issue and rebased against the current tree.

[Robert C Jennings (rcj)]

Please review with latest changes and retest. Thank you.

review: Needs Resubmitting

[Adam Israel (aisrael)]

Hi Robert,

I took a look at this late last week. I'm happy to report that it merges cleanly and all tests are passing, against local and amazon providers. Thanks again for all of your efforts on this!

+1

review: Approve

[Marco Ceppi (marcoceppi)]

Merged

review: Approve

=== renamed file 'README' => 'README.md'
--- README	2012-07-26 23:39:54 +0000
+++ README.md	2015-04-02 19:42:00 +0000
@@ -1,10 +1,39 @@
+# Overview
+
 This charm provides an nrpe subordinate to integrate with nagios logging.
 
 Adding the charm as a subordinate to a service will run an nrpe server
 on the unit and allow for nagios to query the service using that server.
 
+# Usage
+
+Deploy the charm as a subordinate of an existing service.  In this example
+we will deploy a Trusty version of the Apache2 charm and add nrpe to it.
+The relationship, `general-monitors`, provides basic monitoring data.
+
+    juju deploy nagios
+    juju deploy cs:precise/apache2
+    juju deploy cs:precise/nrpe
+    juju add-relation nrpe:general-monitors apache2:juju-info
+    juju add-relation nrpe:monitors nagios:monitors
+    
+In this second example we will use the `local-monitors` relationship with
+Apache2.  The Apache2 charm provides a `local-monitors` relationship that
+will add additional monitoring checks.
+
+    juju deploy nagios
+    juju deploy cs:precise/apache2
+    juju deploy cs:precise/nrpe
+    juju add-relation nrpe:local-monitors apache2: local-monitors
+    juju add-relation nrpe:monitors nagios:monitors
+
+Note that a series for nagios is not specified as nrpe can communicate
+with nagios charms of different series.  
+
+# Charm relationship data
+
 The service provides a 'local-monitors' container scoped interface
-to define the plugins that the nrpe server can use. The charm should
+to define the plug-ins that the nrpe server can use. The charm should
 'relation-set monitors=monitors' with a monitors.yaml in the same
 format as the 'monitors' relation seen in the nagios charm. This will
 be merged with the base monitors and any monitors configured via the

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-07-26 23:37:43 +0000
+++ hooks/config-changed	2015-04-02 19:42:00 +0000
@@ -11,13 +11,16 @@
 
 PLUGIN_PATH = '/usr/lib/nagios/plugins'
 
-base_yaml = RecursiveDictionary(yaml.safe_load(open('base-monitors.yaml','r')))
+with open('base-monitors.yaml', 'r') as base_monitors:
+    base_yaml = RecursiveDictionary(yaml.safe_load(base_monitors))
 yamls = []
 if len(sys.argv) > 1:
     for f in sys.argv[1:]:
         yamls.append(yaml.safe_load(open(f)))
 else:
-    cmon = json.loads(subprocess.check_output(['config-get','monitors','--format=json']))
+    cmon = json.loads(subprocess.check_output(['config-get',
+                                               'monitors',
+                                               '--format=json']))
     if cmon is not None and type(cmon) != dict:
         cmon = yaml.safe_load(cmon)
     if cmon is not None:
@@ -32,25 +35,27 @@
 
 base_yaml = dict(base_yaml)
 
+
 def make_command(mclass, mon):
     cmd_name = 'check_true'
     cmd_line = ['/usr/bin/true']
 
     if mclass == 'procrunning' or mclass == 'processcount':
-        executable = mon.get('executable','')
-        cmd_name = '%s_%s' % (mclass, executable.replace('/','_'))
-        range_arg = '%s:%s' % (mon.get('min',''), mon.get('max',''))
+        executable = mon.get('executable', '')
+        cmd_name = '%s_%s' % (mclass, executable.replace('/', '_'))
+        range_arg = '%s:%s' % (mon.get('min', ''), mon.get('max', ''))
         cmd = os.path.join(PLUGIN_PATH, 'check_procs')
-        cmd_line = [cmd,'-w',range_arg,'-c',range_arg]
+        cmd_line = [cmd, '-w', range_arg, '-c', range_arg]
         if executable:
-            cmd_line.extend(('-C',executable))
+            cmd_line.extend(('-C', executable))
     elif mclass == 'mem':
         cmd_name = 'check_mem'
-        cmd = os.path.abspath(os.path.join(os.path.dirname(os.path.abspath(__file__)),
-                '..', 'plugins', 'check_mem.pl'))
+        cmd = os.path.abspath(os.path.join(
+            os.path.dirname(os.path.abspath(__file__)),
+            '..', 'plugins', 'check_mem.pl'))
         cmd_line = [cmd, '-C', '-u', '-w', '85', '-c', '90']
     elif mclass == 'disk':
-        path = mon.get('path','/')
+        path = mon.get('path', '/')
         cmd_name = 'disk_%s' % (path.replace('/', '_'))
         cmd = os.path.join(PLUGIN_PATH, 'check_disk')
         cmd_line = [cmd, '-w', '20', '-c', '10', '-p', path]
@@ -65,8 +70,8 @@
     return cmd_name
 
 # Now transform local's into remote nrpe's
-local = base_yaml['monitors'].get('local',{})
-out_remotes = base_yaml['monitors'].get('remote',{})
+local = base_yaml['monitors'].get('local', {})
+out_remotes = base_yaml['monitors'].get('remote', {})
 for mclass, mons in local.iteritems():
     if 'nrpe' not in out_remotes:
         out_remotes['nrpe'] = {}
@@ -93,7 +98,7 @@
     print yaml.safe_dump(base_yaml)
     sys.exit(0)
 
-with open('monitors.yaml','w') as end_yaml:
+with open('monitors.yaml', 'w') as end_yaml:
     yaml.safe_dump(base_yaml, end_yaml)
 
-subprocess.call(['service','nagios-nrpe-server','restart'])
+subprocess.call(['service', 'nagios-nrpe-server', 'restart'])

=== modified file 'hooks/local-monitors-relation-changed'
--- hooks/local-monitors-relation-changed	2012-07-27 00:18:58 +0000
+++ hooks/local-monitors-relation-changed	2015-04-02 19:42:00 +0000
@@ -9,13 +9,17 @@
     print 'Need JUJU_REMOTE_UNIT to run'
     sys.exit(0)
 
-primary_id=os.environ['JUJU_REMOTE_UNIT'].replace('/','-')
+primary_id = os.environ['JUJU_REMOTE_UNIT'].replace('/', '-')
+
 if not os.path.exists('data'):
     os.mkdir('data')
-with open('data/primary_id','w') as primid:
+with open('data/primary_id', 'w') as primid:
     primid.write(primary_id)
 
-monitors = json.loads(subprocess.check_output(['relation-get','--format=json','monitors']))
+monitors = json.loads(subprocess.check_output(['relation-get',
+                                               '--format=json',
+                                               'monitors']))
+
 if monitors is None or len(monitors) == 0:
     sys.exit(0)
 

=== modified file 'hooks/recursive_dictionary.py'
--- hooks/recursive_dictionary.py	2012-07-04 19:04:20 +0000
+++ hooks/recursive_dictionary.py	2015-04-02 19:42:00 +0000
@@ -6,17 +6,17 @@
 # which makes it easier to deal with yaml dump/load
 #
 # Copyright (c) 2009 Jannis Andrija Schnitzer
-# 
+#
 # Permission is hereby granted, free of charge, to any person obtaining a copy
 # of this software and associated documentation files (the "Software"), to deal
 # in the Software without restriction, including without limitation the rights
 # to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 # copies of the Software, and to permit persons to whom the Software is
 # furnished to do so, subject to the following conditions:
-# 
+#
 # The above copyright notice and this permission notice shall be included in
 # all copies or substantial portions of the Software.
-# 
+#
 # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 # FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
@@ -27,6 +27,7 @@
 
 __author__ = '[e-mail] (Jannis Andrija Schnitzer)'
 
+
 class RecursiveDictionary(dict):
     """RecursiveDictionary provides the methods rec_update and iter_rec_update
     that can be used to update member dictionaries rather than overwriting
@@ -34,7 +35,7 @@
     def rec_update(self, other, **third):
         """Recursively update the dictionary with the contents of other and
         third like dict.update() does - but don't overwrite sub-dictionaries.
-        
+
         Example:
         >>> d = RecursiveDictionary({'foo': {'bar': 42}})
         >>> d.rec_update({'foo': {'baz': 36}})
@@ -47,7 +48,7 @@
             iterator = other
         self.iter_rec_update(iterator)
         self.iter_rec_update(third.iteritems())
-        
+
     def iter_rec_update(self, iterator):
         for (key, value) in iterator:
             if key in self and \
@@ -58,6 +59,6 @@
                 self[key] = dict(self[key])
             else:
                 self[key] = value
-    
+
     def __repr__(self):
         return super(self.__class__, self).__repr__()

=== modified file 'tests/00-setup'
--- tests/00-setup	2014-11-06 21:21:58 +0000
+++ tests/00-setup	2015-04-02 19:42:00 +0000
@@ -2,4 +2,4 @@
 
 sudo add-apt-repository ppa:juju/stable -y
 sudo apt-get update
-sudo apt-get install amulet python3-requests -y
+sudo apt-get install amulet -y

=== added file 'tests/10-monitors'
--- tests/10-monitors	1970-01-01 00:00:00 +0000
+++ tests/10-monitors	2015-04-02 19:42:00 +0000
@@ -0,0 +1,64 @@
+#!/usr/bin/python3
+
+import amulet
+import subprocess
+
+
+def file_exists(unit, unit_name, filename):
+    try:
+        unit.file(filename)
+    except:
+        amulet.raise_status(amulet.FAIL,
+            'Expected file on {} not found: {}'.format(unit_name,
+                                                       filename))
+
+
+d = amulet.Deployment(series='precise')
+
+# Use known charms for the deploy.
+d.add('nagios', charm='cs:precise/nagios-10')
+d.add('apache2', charm='cs:precise/apache2-25')
+
+# The nrpe charm will be subordinate to apache2.
+d.add('nrpe')
+
+# NRPE monitors apache2 and answers to Nagios
+d.relate('nrpe:monitors', 'nagios:monitors')
+d.relate('nrpe:general-info', 'apache2:juju-info')
+
+try:
+    d.setup(timeout=1200)
+    d.sentry.wait(timeout=1200)
+except amulet.helpers.TimeoutError:
+    amulet.raise_status(amulet.SKIP, msg="Environment wasn't stood up in time")
+except:
+    raise
+
+nrpe = d.sentry.unit['apache2/0']
+nagios = d.sentry.unit['nagios/0']
+
+# Check for nrpe configration files
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/base-monitors.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/check_mem.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/disk__.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/allowed.hosts.cfg')
+
+# Check for nagios configration files from nrpe
+base_nagios = '/etc/nagios3/conf.d/commands'
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__ccheck_mem.cfg')))
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__cdisk__.cfg')))
+
+# Check that nagios knows the IP of of the unit with nrpe
+apache_private_address = subprocess.check_output(['juju', 'run',
+                                                  '-e', d.juju_env,
+                                                  '--unit', 'apache2/0',
+                                                  'unit-get private-address'])
+nagios_charm_cfg = nagios.file_contents('/etc/nagios3/conf.d/charm.cfg')
+apache_private_address = apache_private_address.decode('utf-8')
+if nagios_charm_cfg.find(apache_private_address) == -1:
+    amulet.raise_status(amulet.FAIL,
+                        msg='IP for nrpe host not in nagios config')
+
+amulet.raise_status(amulet.PASS, msg='monitors<->juju-info relationship pass')

=== added file 'tests/20-local_monitors'
--- tests/20-local_monitors	1970-01-01 00:00:00 +0000
+++ tests/20-local_monitors	2015-04-02 19:42:00 +0000
@@ -0,0 +1,75 @@
+#!/usr/bin/python3
+
+import amulet
+import subprocess
+
+
+def file_exists(unit, unit_name, filename):
+    try:
+        unit.file(filename)
+    except:
+        amulet.raise_status(amulet.FAIL,
+            'Expected file on {} not found: {}'.format(unit_name,
+                                                       filename))
+
+
+d = amulet.Deployment(series='precise')
+
+# Use known charms for the deploy.
+d.add('nagios', charm='cs:precise/nagios-10')
+d.add('apache2', charm='cs:precise/apache2-25')
+
+# The nrpe charm will be subordinate to apache2.
+d.add('nrpe')
+
+with open('test-monitors.yaml', 'r') as monitors_file:
+    monitors_cfg = monitors_file.read()
+# Configure nrpe with some test monitors
+d.configure('nrpe', options={'monitors': monitors_cfg})
+
+# NRPE monitors apache2 and answers to Nagios
+d.relate('nrpe:monitors', 'nagios:monitors')
+d.relate('nrpe:local-monitors', 'apache2:local-monitors')
+
+try:
+    d.setup(timeout=1200)
+    d.sentry.wait(timeout=1200)
+except amulet.helpers.TimeoutError:
+    amulet.raise_status(amulet.SKIP, msg="Environment wasn't stood up in time")
+except:
+    raise
+
+nrpe = d.sentry.unit['apache2/0']
+nagios = d.sentry.unit['nagios/0']
+
+# Check for nrpe configration files
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/allowed.hosts.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/base-monitors.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/check_mem.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/check_true.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/disk__.cfg')
+file_exists(nrpe, 'nrpe', '/etc/nagios/nrpe.d/processcount_.cfg')
+
+# Check for nagios configration files from nrpe
+base_nagios = '/etc/nagios3/conf.d/commands'
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__ccheck_mem.cfg')))
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__ccheck_true.cfg')))
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__cdisk__.cfg')))
+file_exists(nagios, 'nagios',
+    '/'.join((base_nagios, 'check_nrpe_H_HOSTADDRESS__cprocesscount_.cfg')))
+
+# Check that nagios knows the IP of of the unit with nrpe
+apache_private_address = subprocess.check_output(['juju', 'run',
+                                                  '-e', d.juju_env,
+                                                  '--unit', 'apache2/0',
+                                                  'unit-get private-address'])
+apache_private_address = apache_private_address.decode('utf-8')
+nagios_charm_cfg = nagios.file_contents('/etc/nagios3/conf.d/charm.cfg')
+if nagios_charm_cfg.find(apache_private_address) == -1:
+    amulet.raise_status(amulet.FAIL,
+                        msg='IP for nrpe host not in nagios config')
+
+amulet.raise_status(amulet.PASS, msg='local-monitors relationship pass')

=== removed file 'tests/99-autogen'
--- tests/99-autogen	2014-11-20 19:45:39 +0000
+++ tests/99-autogen	1970-01-01 00:00:00 +0000
@@ -1,55 +0,0 @@
-#!/usr/bin/env python3
-
-import amulet
-import requests
-import unittest
-
-
-class TestDeployment(unittest.TestCase):
-    @classmethod
-    def setUpClass(cls):
-        cls.deployment = amulet.Deployment(series='precise')
-
-        cls.deployment.add('nrpe')
-        cls.deployment.add('nagios')
-        cls.deployment.add('mysql')
-        cls.deployment.add('ubuntu')
-        cls.deployment.relate('nrpe:monitors', 'nagios:monitors')
-        cls.deployment.relate('nrpe:local-monitors', 'mysql:local-monitors')
-        cls.deployment.relate('nrpe:general-info', 'ubuntu:juju-info')
-
-        try:
-            cls.deployment.setup(timeout=900)
-            cls.deployment.sentry.wait()
-        except amulet.helpers.TimeoutError:
-            amulet.raise_status(amulet.SKIP, msg="Environment wasn't stood up in time")
-        except:
-            raise
-
-    def test_case(self):
-        # Now you can use self.deployment.sentry.unit[UNIT] to address each of
-        # the units and perform more in-depth steps.  You can also reference
-        # the first unit as self.unit.
-        # There are three test statuses that can be triggered with
-        # amulet.raise_status():
-        #   - amulet.PASS
-        #   - amulet.FAIL
-        #   - amulet.SKIP
-        # Each unit has the following methods:
-        #   - .info - An array of the information of that unit from Juju
-        #   - .file(PATH) - Get the details of a file on that unit
-        #   - .file_contents(PATH) - Get plain text output of PATH file from that unit
-        #   - .directory(PATH) - Get details of directory
-        #   - .directory_contents(PATH) - List files and folders in PATH on that unit
-        #   - .relation(relation, service:rel) - Get relation data from return service
-        #          add tests here to confirm service is up and working properly
-        # For example, to confirm that it has a functioning HTTP server:
-        #     page = requests.get('http://{}'.format(self.unit.info['public-address']))
-        #     page.raise_for_status()
-        # More information on writing Amulet tests can be found at:
-        #     https://juju.ubuntu.com/docs/tools-amulet.html
-        pass
-
-
-if __name__ == '__main__':
-    unittest.main()