Merge lp:~paulgear/charm-helpers/nrpe-remove-checks into lp:charm-helpers

[Paul Gear]

This branch adds support for removing nrpe checks, reduces duplicated paths, and reduces the chance of a small (rather unlikely) bug whereby one check could remove another's export files.

[Stuart Bishop (stub)]

Looks good, except for one bug commented on inline (I think you wrote 'pass' when you meant 'return' in _remove_service_files).

I tend to prefer foo.endswith instead of re.search('.*...'), but whatever floats your boat.

review: Needs Fixing

[Paul Gear (paulgear)]

Hi Stuart - thanks for reviewing. The pass vs. return bug is fixed (I found it while testing as well); the re.search was just moving the existing code, but endswith seems like it would be a lot more efficient. Will fix that shortly.

[Stuart Bishop (stub)]

I have two failing tests, directly related to this change. The second one probably just needs the '3' changed to '4' to account for an extra call to exists. The first failure might be more serious, as an expected call to os.remove is not happening any more:

======================================================================
FAIL: tests.contrib.charmsupport.test_nrpe.NRPECheckTestCase.test_write_removes_existing_config
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/stub/charms/charm-helpers/trunk/tests/contrib/charmsupport/test_nrpe.py", line 209, in test_write_removes_existing_config
    self.patched['remove'].assert_called_once_with(expected)
  File "/home/stub/charms/charm-helpers/trunk/.venv/local/lib/python2.7/site-packages/mock.py", line 845, in assert_called_once_with
    raise AssertionError(msg)
AssertionError: Expected to be called once. Called 0 times.

======================================================================
FAIL: tests.contrib.charmsupport.test_nrpe.NRPETestCase.test_update_nrpe
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/stub/charms/charm-helpers/trunk/tests/contrib/charmsupport/test_nrpe.py", line 176, in test_update_nrpe
    relation_ids=2, relation_set=2)
  File "/home/stub/charms/charm-helpers/trunk/tests/contrib/charmsupport/test_nrpe.py", line 48, in check_call_counts
    self.assertEqual(expected, patcher.call_count, attr)
  File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 348, in assertEqual
    self.assertThat(observed, matcher, message)
  File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 433, in assertThat
    raise mismatch_error
MismatchError: 3 != 4: exists

review: Needs Fixing

[Paul Gear (paulgear)]

I don't know what I was doing wrong previously, but I reproduced the test failures on my system, updated the test suite, and it passes on both my branch and my branch merged into an up-to-date copy of devel.

=== modified file 'charmhelpers/contrib/charmsupport/nrpe.py'
--- charmhelpers/contrib/charmsupport/nrpe.py	2015-10-12 20:55:04 +0000
+++ charmhelpers/contrib/charmsupport/nrpe.py	2015-10-29 23:52:51 +0000
@@ -148,6 +148,13 @@
         self.description = description
         self.check_cmd = self._locate_cmd(check_cmd)
 
+    def _get_check_filename(self):
+        return os.path.join(NRPE.nrpe_confdir, '{}.cfg'.format(self.command))
+
+    def _get_service_filename(self, hostname):
+        return os.path.join(NRPE.nagios_exportdir,
+                            'service__{}_{}.cfg'.format(hostname, self.command))
+
     def _locate_cmd(self, check_cmd):
         search_path = (
             '/usr/lib/nagios/plugins',
@@ -163,9 +170,21 @@
         log('Check command not found: {}'.format(parts[0]))
         return ''
 
+    def _remove_service_files(self):
+        if not os.path.exists(NRPE.nagios_exportdir):
+            return
+        for f in os.listdir(NRPE.nagios_exportdir):
+            if f.endswith('_{}.cfg'.format(self.command)):
+                os.remove(os.path.join(NRPE.nagios_exportdir, f))
+
+    def remove(self, hostname):
+        nrpe_check_file = self._get_check_filename()
+        if os.path.exists(nrpe_check_file):
+            os.remove(nrpe_check_file)
+        self._remove_service_files()
+
     def write(self, nagios_context, hostname, nagios_servicegroups):
-        nrpe_check_file = '/etc/nagios/nrpe.d/{}.cfg'.format(
-            self.command)
+        nrpe_check_file = self._get_check_filename()
         with open(nrpe_check_file, 'w') as nrpe_check_config:
             nrpe_check_config.write("# check {}\n".format(self.shortname))
             nrpe_check_config.write("command[{}]={}\n".format(
@@ -180,9 +199,7 @@
 
     def write_service_config(self, nagios_context, hostname,
                              nagios_servicegroups):
-        for f in os.listdir(NRPE.nagios_exportdir):
-            if re.search('.*{}.cfg'.format(self.command), f):
-                os.remove(os.path.join(NRPE.nagios_exportdir, f))
+        self._remove_service_files()
 
         templ_vars = {
             'nagios_hostname': hostname,
@@ -192,8 +209,7 @@
             'command': self.command,
         }
         nrpe_service_text = Check.service_template.format(**templ_vars)
-        nrpe_service_file = '{}/service__{}_{}.cfg'.format(
-            NRPE.nagios_exportdir, hostname, self.command)
+        nrpe_service_file = self._get_service_filename(hostname)
         with open(nrpe_service_file, 'w') as nrpe_service_config:
             nrpe_service_config.write(str(nrpe_service_text))
 
@@ -228,6 +244,22 @@
     def add_check(self, *args, **kwargs):
         self.checks.append(Check(*args, **kwargs))
 
+    def remove_check(self, *args, **kwargs):
+        if kwargs.get('shortname') is None:
+            raise ValueError('shortname of check must be specified')
+
+        # Use sensible defaults if they're not specified - these are not
+        # actually used during removal, but they're required for constructing
+        # the Check object; check_disk is chosen because it's part of the
+        # nagios-plugins-basic package.
+        if kwargs.get('check_cmd') is None:
+            kwargs['check_cmd'] = 'check_disk'
+        if kwargs.get('description') is None:
+            kwargs['description'] = ''
+
+        check = Check(*args, **kwargs)
+        check.remove(self.hostname)
+
     def write(self):
         try:
             nagios_uid = pwd.getpwnam('nagios').pw_uid

=== modified file 'tests/contrib/charmsupport/test_nrpe.py'
--- tests/contrib/charmsupport/test_nrpe.py	2015-03-16 17:54:34 +0000
+++ tests/contrib/charmsupport/test_nrpe.py	2015-10-29 23:52:51 +0000
@@ -172,7 +172,7 @@
         ]
         self.patched['relation_set'].assert_has_calls(relation_set_calls, any_order=True)
         self.check_call_counts(config=1, getpwnam=1, getgrnam=1,
-                               exists=3, open=2, listdir=1,
+                               exists=4, open=2, listdir=1,
                                relation_ids=2, relation_set=2)
 
 
@@ -200,14 +200,14 @@
 
     def test_write_removes_existing_config(self):
         self.patched['listdir'].return_value = [
-            'foo', 'bar.cfg', 'check_shortname.cfg']
+            'foo', 'bar.cfg', '_check_shortname.cfg']
         check = nrpe.Check('shortname', 'description', '/some/command')
 
         self.assertEqual(None, check.write('testctx', 'hostname', 'testsgrp'))
 
-        expected = '/var/lib/nagios/export/check_shortname.cfg'
+        expected = '/var/lib/nagios/export/_check_shortname.cfg'
         self.patched['remove'].assert_called_once_with(expected)
-        self.check_call_counts(exists=2, remove=1, open=2, listdir=1)
+        self.check_call_counts(exists=3, remove=1, open=2, listdir=1)
 
     def test_check_write_nrpe_exportdir_not_accessible(self):
         self.patched['exists'].return_value = False