Merge lp:~benji/charms/precise/juju-gui/bug-1284088 into lp:~juju-gui/charms/precise/juju-gui/trunk

[Benji York]

Handle broken nrpe relations.

Handle broken nrpe relations.

https://codereview.appspot.com/76860047/

[Benji York (benji)]

Reviewers: mp+211781_code.launchpad.net,

Message:
Please take a look.

Description:
Handle broken nrpe relations.

https://code.launchpad.net/~benji/charms/precise/juju-gui/bug-1284088/+merge/211781

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/76860047/

Affected files (+51, -7 lines):
   A [revision details]
   M scripts/charmsupport/nrpe.py
   M scripts/update-nrpe.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: scripts/update-nrpe.py
=== modified file 'scripts/update-nrpe.py'
--- scripts/update-nrpe.py 2014-02-18 12:18:02 +0000
+++ scripts/update-nrpe.py 2014-03-19 16:39:34 +0000
@@ -1,14 +1,29 @@
  #!/usr/bin/env python
-from charmsupport import nrpe
+import sys
+import charmsupport.nrpe
+
+
+def get_nrpe():
+ nrpe = charmsupport.nrpe.NRPE()
+ nrpe.add_check('app-is-accessible', 'Check_the_app_can_be_downloaded',
+ 'check-app-access.sh')
+ return nrpe

  def update_nrpe_config():
- nrpe_compat = nrpe.NRPE()
- nrpe_compat.add_check(
- 'app-is-accessible', 'Check_the_app_can_be_downloaded',
- 'check-app-access.sh')
- nrpe_compat.write()
+ nrpe = get_nrpe()
+ nrpe.write()
+
+
+def remove_nrpe_check():
+ nrpe = get_nrpe()
+ nrpe.remove_checks()

  if __name__ == '__main__':
- update_nrpe_config()
+ hook_name = sys.argv[0]
+ remove_nrpe_check()
+ if 'departed' in hook_name or 'broken' in hook_name:
+ remove_nrpe_check()
+ else:
+ update_nrpe_config()

Index: scripts/charmsupport/nrpe.py
=== modified file 'scripts/charmsupport/nrpe.py'
--- scripts/charmsupport/nrpe.py 2013-07-29 19:02:49 +0000
+++ scripts/charmsupport/nrpe.py 2014-03-19 16:39:34 +0000
@@ -5,12 +5,17 @@
  # Authors:
  # Matthew Wedgwood <email address hidden>

+# XXX This file has been modified to add the ability to remove NRPE checks.
+# The original project that created this code has been abandoned so there
is
+# no upstream to which the modifications can be sent.
+
  import subprocess
  import pwd
  import grp
  import os
  import re
  import shlex
+import errno

  from hookenv import config, local_unit

@@ -105,6 +110,11 @@
          subprocess.call(['juju-log', 'Check command not found:
{}'.format(command[0])])
          return ''

+ def service_file_name(self, nagios_context, hostname):
+ return '{}/service__{}_check_{}.cfg'.format(
+ NRPE.nagios_exportdir, hostname, self.shortname)
+
+
      def write(self, nagios_context, hostname):
          for f in os.listdir(NRPE.nagios_exportdir):
              if re.search('.*check_{}.cfg'.format(self.shortname), f):
@@ -119,6 +129,7 @@
          nrpe_service_text = Check.service_template.format(**templ_vars)
          nrpe_service_file = '{}/service__{}_check_{}.cfg'.format(
              NRPE.nagios_exportdir, hostname, self.shortname)
+ nrpe_service_file = self.servic...

[Francesco Banconi (frankban)]

Thanks for this branch Benji: LGTM with minors.

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py
File scripts/charmsupport/nrpe.py (right):

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py#newcode113
scripts/charmsupport/nrpe.py:113: def service_file_name(self,
nagios_context, hostname):
nagios_context seems unused in this method.

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py#newcode130
scripts/charmsupport/nrpe.py:130: nrpe_service_file =
'{}/service__{}_check_{}.cfg'.format(
The nrpe_service_file is retrieved using the external method in the next
line, so I think we can safely remove this one.

https://codereview.appspot.com/76860047/diff/1/scripts/update-nrpe.py
File scripts/update-nrpe.py (right):

https://codereview.appspot.com/76860047/diff/1/scripts/update-nrpe.py#newcode25
scripts/update-nrpe.py:25: remove_nrpe_check()
So we always remove checks and then add them (or remove them again)
based on the hook name. Is the second removal needed?

https://codereview.appspot.com/76860047/

[Benji York (benji)]

Thanks for the good review. Read all the comments before replying
because there is a twist ending.

Sincerely,
M. Night Shyamalan

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py
File scripts/charmsupport/nrpe.py (right):

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py#newcode113
scripts/charmsupport/nrpe.py:113: def service_file_name(self,
nagios_context, hostname):
On 2014/03/19 17:42:04, frankban wrote:
> nagios_context seems unused in this method.

You are exactly right. Given the complete lack of tests and the fact
that this is a dead library plus the extensive testing I've done of the
code as-is, I'm going to leave it be (unless you object strenuously).

https://codereview.appspot.com/76860047/diff/1/scripts/charmsupport/nrpe.py#newcode130
scripts/charmsupport/nrpe.py:130: nrpe_service_file =
'{}/service__{}_check_{}.cfg'.format(
On 2014/03/19 17:42:04, frankban wrote:
> The nrpe_service_file is retrieved using the external method in the
next line,
> so I think we can safely remove this one.

Good catch. Again, I am loathe to touch the known-working code, even to
do something this simple.

https://codereview.appspot.com/76860047/diff/1/scripts/update-nrpe.py
File scripts/update-nrpe.py (right):

https://codereview.appspot.com/76860047/diff/1/scripts/update-nrpe.py#newcode25
scripts/update-nrpe.py:25: remove_nrpe_check()
On 2014/03/19 17:42:04, frankban wrote:
> So we always remove checks and then add them (or remove them again)
based on the
> hook name. Is the second removal needed?

Darn, this is bad code. This really is a stopper, so I'm going to
retract what I said before about changing the code and implement the two
suggestions you made as well as remove this line and then re-test.

https://codereview.appspot.com/76860047/

[lp:~benji/charms/precise/juju-gui/bug-1284088]

175. By Benji York on 2014-03-19

fix bugs found in review

[Benji York (benji)]

*** Submitted:

Handle broken nrpe relations.

R=frankban
CC=
https://codereview.appspot.com/76860047

https://codereview.appspot.com/76860047/

=== added symlink 'hooks/nrpe-external-master-relation-broken'
=== target is u'../scripts/update-nrpe.py'
=== added symlink 'hooks/nrpe-external-master-relation-departed'
=== target is u'../scripts/update-nrpe.py'
=== modified file 'scripts/charmsupport/nrpe.py'
--- scripts/charmsupport/nrpe.py	2013-07-29 19:02:49 +0000
+++ scripts/charmsupport/nrpe.py	2014-03-19 19:36:58 +0000
@@ -5,12 +5,17 @@
 # Authors:
 #  Matthew Wedgwood <[e-mail]>
 
+# XXX This file has been modified to add the ability to remove NRPE checks.
+# The original project that created this code has been abandoned so there is
+# no upstream to which the modifications can be sent.
+
 import subprocess
 import pwd
 import grp
 import os
 import re
 import shlex
+import errno
 
 from hookenv import config, local_unit
 
@@ -105,6 +110,11 @@
         subprocess.call(['juju-log', 'Check command not found: {}'.format(command[0])])
         return ''
 
+    def service_file_name(self, hostname):
+        return '{}/service__{}_check_{}.cfg'.format(
+            NRPE.nagios_exportdir, hostname, self.shortname)
+
+
     def write(self, nagios_context, hostname):
         for f in os.listdir(NRPE.nagios_exportdir):
             if re.search('.*check_{}.cfg'.format(self.shortname), f):
@@ -117,8 +127,7 @@
             'shortname': self.shortname,
         }
         nrpe_service_text = Check.service_template.format(**templ_vars)
-        nrpe_service_file = '{}/service__{}_check_{}.cfg'.format(
-            NRPE.nagios_exportdir, hostname, self.shortname)
+        nrpe_service_file = self.service_file_name(hostname)
         with open(nrpe_service_file, 'w') as nrpe_service_config:
             nrpe_service_config.write(str(nrpe_service_text))
 
@@ -128,9 +137,21 @@
             nrpe_check_config.write("command[check_{}]={}\n".format(
                 self.shortname, self.check_cmd))
 
+    def remove(self, hostname):
+        """Remove the configuration file for this check."""
+        try:
+            os.unlink(self.service_file_name(hostname))
+        except OSError, e:
+            if e.errno == errno.ENOENT:
+                # Ignore the fact that the file didn't exist.
+                pass
+            else:
+                raise
+
     def run(self):
         subprocess.call(self.check_cmd)
 
+
 class NRPE(object):
     nagios_logdir = '/var/log/nagios'
     nagios_exportdir = '/var/lib/nagios/export'
@@ -167,3 +188,7 @@
 
         if os.path.isfile('/etc/init.d/nagios-nrpe-server'):
             subprocess.call(['service', 'nagios-nrpe-server', 'reload'])
+
+    def remove_checks(self):
+        for check in self.checks:
+            check.remove(self.hostname)

=== modified file 'scripts/update-nrpe.py'
--- scripts/update-nrpe.py	2014-02-18 12:18:02 +0000
+++ scripts/update-nrpe.py	2014-03-19 19:36:58 +0000
@@ -1,14 +1,28 @@
 #!/usr/bin/env python
-from charmsupport import nrpe
+import sys
+import charmsupport.nrpe
+
+
+def get_nrpe():
+    nrpe = charmsupport.nrpe.NRPE()
+    nrpe.add_check('app-is-accessible', 'Check_the_app_can_be_downloaded',
+        'check-app-access.sh')
+    return nrpe
 
 
 def update_nrpe_config():
-    nrpe_compat = nrpe.NRPE()
-    nrpe_compat.add_check(
-        'app-is-accessible', 'Check_the_app_can_be_downloaded',
-        'check-app-access.sh')
-    nrpe_compat.write()
+    nrpe = get_nrpe()
+    nrpe.write()
+
+
+def remove_nrpe_check():
+    nrpe = get_nrpe()
+    nrpe.remove_checks()
 
 
 if __name__ == '__main__':
-    update_nrpe_config()
+    hook_name = sys.argv[0]
+    if 'departed' in hook_name or 'broken' in hook_name:
+        remove_nrpe_check()
+    else:
+        update_nrpe_config()