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

Proposed by Benji York
Status: Merged
Merged at revision: 175
Proposed branch: lp:~benji/charms/precise/juju-gui/bug-1284088
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 117 lines (+48/-9)
2 files modified
scripts/charmsupport/nrpe.py (+27/-2)
scripts/update-nrpe.py (+21/-7)
To merge this branch: bzr merge lp:~benji/charms/precise/juju-gui/bug-1284088
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+211781@code.launchpad.net

Commit message

Handle broken nrpe relations.

Description of the change

Handle broken nrpe relations.

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

To post a comment you must log in.
Revision history for this message
Benji York (benji) wrote :
Download full text (4.3 KiB)

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...

Read more...

Revision history for this message
Francesco Banconi (frankban) wrote :

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/

Revision history for this message
Benji York (benji) wrote :

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/

175. By Benji York

fix bugs found in review

Revision history for this message
Benji York (benji) wrote :

*** Submitted:

Handle broken nrpe relations.

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
5# Authors:5# Authors:
6# Matthew Wedgwood <matthew.wedgwood@canonical.com>6# Matthew Wedgwood <matthew.wedgwood@canonical.com>
77
8# XXX This file has been modified to add the ability to remove NRPE checks.
9# The original project that created this code has been abandoned so there is
10# no upstream to which the modifications can be sent.
11
8import subprocess12import subprocess
9import pwd13import pwd
10import grp14import grp
11import os15import os
12import re16import re
13import shlex17import shlex
18import errno
1419
15from hookenv import config, local_unit20from hookenv import config, local_unit
1621
@@ -105,6 +110,11 @@
105 subprocess.call(['juju-log', 'Check command not found: {}'.format(command[0])])110 subprocess.call(['juju-log', 'Check command not found: {}'.format(command[0])])
106 return ''111 return ''
107112
113 def service_file_name(self, hostname):
114 return '{}/service__{}_check_{}.cfg'.format(
115 NRPE.nagios_exportdir, hostname, self.shortname)
116
117
108 def write(self, nagios_context, hostname):118 def write(self, nagios_context, hostname):
109 for f in os.listdir(NRPE.nagios_exportdir):119 for f in os.listdir(NRPE.nagios_exportdir):
110 if re.search('.*check_{}.cfg'.format(self.shortname), f):120 if re.search('.*check_{}.cfg'.format(self.shortname), f):
@@ -117,8 +127,7 @@
117 'shortname': self.shortname,127 'shortname': self.shortname,
118 }128 }
119 nrpe_service_text = Check.service_template.format(**templ_vars)129 nrpe_service_text = Check.service_template.format(**templ_vars)
120 nrpe_service_file = '{}/service__{}_check_{}.cfg'.format(130 nrpe_service_file = self.service_file_name(hostname)
121 NRPE.nagios_exportdir, hostname, self.shortname)
122 with open(nrpe_service_file, 'w') as nrpe_service_config:131 with open(nrpe_service_file, 'w') as nrpe_service_config:
123 nrpe_service_config.write(str(nrpe_service_text))132 nrpe_service_config.write(str(nrpe_service_text))
124133
@@ -128,9 +137,21 @@
128 nrpe_check_config.write("command[check_{}]={}\n".format(137 nrpe_check_config.write("command[check_{}]={}\n".format(
129 self.shortname, self.check_cmd))138 self.shortname, self.check_cmd))
130139
140 def remove(self, hostname):
141 """Remove the configuration file for this check."""
142 try:
143 os.unlink(self.service_file_name(hostname))
144 except OSError, e:
145 if e.errno == errno.ENOENT:
146 # Ignore the fact that the file didn't exist.
147 pass
148 else:
149 raise
150
131 def run(self):151 def run(self):
132 subprocess.call(self.check_cmd)152 subprocess.call(self.check_cmd)
133153
154
134class NRPE(object):155class NRPE(object):
135 nagios_logdir = '/var/log/nagios'156 nagios_logdir = '/var/log/nagios'
136 nagios_exportdir = '/var/lib/nagios/export'157 nagios_exportdir = '/var/lib/nagios/export'
@@ -167,3 +188,7 @@
167188
168 if os.path.isfile('/etc/init.d/nagios-nrpe-server'):189 if os.path.isfile('/etc/init.d/nagios-nrpe-server'):
169 subprocess.call(['service', 'nagios-nrpe-server', 'reload'])190 subprocess.call(['service', 'nagios-nrpe-server', 'reload'])
191
192 def remove_checks(self):
193 for check in self.checks:
194 check.remove(self.hostname)
170195
=== 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 @@
1#!/usr/bin/env python1#!/usr/bin/env python
2from charmsupport import nrpe2import sys
3import charmsupport.nrpe
4
5
6def get_nrpe():
7 nrpe = charmsupport.nrpe.NRPE()
8 nrpe.add_check('app-is-accessible', 'Check_the_app_can_be_downloaded',
9 'check-app-access.sh')
10 return nrpe
311
412
5def update_nrpe_config():13def update_nrpe_config():
6 nrpe_compat = nrpe.NRPE()14 nrpe = get_nrpe()
7 nrpe_compat.add_check(15 nrpe.write()
8 'app-is-accessible', 'Check_the_app_can_be_downloaded',16
9 'check-app-access.sh')17
10 nrpe_compat.write()18def remove_nrpe_check():
19 nrpe = get_nrpe()
20 nrpe.remove_checks()
1121
1222
13if __name__ == '__main__':23if __name__ == '__main__':
14 update_nrpe_config()24 hook_name = sys.argv[0]
25 if 'departed' in hook_name or 'broken' in hook_name:
26 remove_nrpe_check()
27 else:
28 update_nrpe_config()

Subscribers

People subscribed via source and target branches

to all changes: