Merge lp:~pponnuvel/charm-haproxy/charm-haproxy into lp:charm-haproxy

[Ponnuvel Palaniyappan]

Don't include haproxy check scripts if monitoring is disabled.

Don't include haproxy check scripts if monitoring is disabled.

If monitoring isn't enabled (via 'enable_monitoring'), stats/metrics aren't
enabled. So, don't include the haproxy check scripts as these would be
useless in that case and can causes confusion.

Fixes launchpad bug 1866408.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tom Haddon (mthaddon)]

Some comments inline, but also see the `update_nrpe_config` function, which will likely need some updates as well.

review: Needs Fixing

[Ponnuvel Palaniyappan (pponnuvel)]

Looking at `update_nrpe_config`, I think the whole thing can be simplified with a check there..
`
def update_nrpe_config():
    config_data = config_get()
    if config_data['enable_monitoring'] is False:
        return
     ...
`
Testing this shows it works as well. Does this sound ok?

[Tom Haddon (mthaddon)]

> Looking at `update_nrpe_config`, I think the whole thing can be simplified
> with a check there..
> `
> def update_nrpe_config():
> config_data = config_get()
> if config_data['enable_monitoring'] is False:
> return
> ...
> `
> Testing this shows it works as well. Does this sound ok?

This won't quite deal with the case of monitoring having been enabled at one point and then disabled. I think you'd need to explicitly remove checks in that case (there's a `remove_check` method to the NRPE object as well). I'm not sure if it's safe to run those if the checks already don't exist, or you'd need to check the existence of the files on disk before running that - `hooks/charmhelpers/contrib/charmsupport/nrpe.py` should have the details.

[lp:~pponnuvel/charm-haproxy/charm-haproxy]

143. By Ponnuvel Palaniyappan on 2020-08-05

Handle cases when config may be enabled and then later disabled.
When 'enable_monitoring' is set to True, the scripts are copied and
wnenever it's set to False, the scripts are removed.

[Ponnuvel Palaniyappan (pponnuvel)]

Modified to handle cases when 'enable_monitoring' can be enabled/disabled in any order.

Although, `remove_check` didn't fail when the scripts don't exist, it did produce a couple of "INFO" complaining that the scripts don't exist. Just to not distract with that, I have added the check to call `remove_check` only they exist.

[Tom Haddon (mthaddon)]

review: Needs Fixing

[Ponnuvel Palaniyappan (pponnuvel)]

Hi Tom, I don't see any review/diff comments?

[Tom Haddon (mthaddon)]

Hmm, sorry, not quite sure what happened there. Re-adding.

[lp:~pponnuvel/charm-haproxy/charm-haproxy]

144. By Ponnuvel Palaniyappan on 2020-08-10

Tidy up code changes based on review comments.

[Ponnuvel Palaniyappan (pponnuvel)]

Thanks, modified as suggested with one slight change because `remove_check` looks at parameter names: https://bazaar.launchpad.net/~haproxy-team/charm-haproxy/trunk/view/head:/hooks/charmhelpers/contrib/charmsupport/nrpe.py#L283

Please have another look.

[lp:~pponnuvel/charm-haproxy/charm-haproxy]

145. By Ponnuvel Palaniyappan on 2020-08-10

Remove unnecessary calls use of 'head' command.

The change is functionally equivalent but uses fewer processes.

[Ponnuvel Palaniyappan (pponnuvel)]

Commit #145 is a minor improvement (that's totally equivalent); not related to any bugs or issues.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 142

=== modified file 'files/nrpe/check_haproxy.sh'
--- files/nrpe/check_haproxy.sh	2019-06-06 13:01:59 +0000
+++ files/nrpe/check_haproxy.sh	2020-08-10 18:21:15 +0000
@@ -11,7 +11,7 @@
 
 export LOGFILE=/var/log/nagios/check_haproxy.log
 # Exclude files starting with a dot - LP#1828529
-AUTH=$(grep -r --exclude ".*" "stats auth" /etc/haproxy | head -1 | awk '{print $4}')
+AUTH=$(grep -r --exclude ".*" "stats auth" /etc/haproxy | awk '{print $4; exit}')
 
 if [ -z "$AUTH" ]; then
     echo "CRITICAL: unable to find credentials to query the haproxy statistics page"
 
=== modified file 'files/nrpe/check_haproxy_queue_depth.sh'
--- files/nrpe/check_haproxy_queue_depth.sh	2020-04-02 13:17:59 +0000
+++ files/nrpe/check_haproxy_queue_depth.sh	2020-08-10 18:21:15 +0000
@@ -2,7 +2,7 @@
 #--------------------------------------------
 # This file is managed by Juju
 #--------------------------------------------
-#                                       
+#
 # Copyright 2009,2012 Canonical Ltd.
 # Author: Tom Haddon
 
@@ -11,7 +11,7 @@
 MAXQthrsh=100
 
 # Exclude files starting with a dot - LP#1828529
-AUTH=$(grep -r --exclude ".*" "stats auth" /etc/haproxy | head -1 | awk '{print $4}')
+AUTH=$(grep -r --exclude ".*" "stats auth" /etc/haproxy | awk '{print $4; exit}')
 
 if [ -z "$AUTH" ]; then
     echo "CRITICAL: unable to find credentials to query the haproxy statistics page"
 
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2020-02-20 14:18:10 +0000
+++ hooks/hooks.py	2020-08-10 18:21:15 +0000
@@ -59,7 +59,7 @@
     "deb http://archive.ubuntu.com/ubuntu %(release)s-backports "
     "main restricted universe multiverse")
 haproxy_preferences_path = "/etc/apt/preferences.d/haproxy"
-
+nrpe_scripts_dest = "/usr/lib/nagios/plugins"
 
 dupe_options = [
     "mode tcp",
@@ -1173,20 +1173,42 @@
 def install_nrpe_scripts():
     scripts_src = os.path.join(os.environ["CHARM_DIR"], "files",
                                "nrpe")
-    scripts_dst = "/usr/lib/nagios/plugins"
-    if not os.path.exists(scripts_dst):
-        os.makedirs(scripts_dst)
+    if not os.path.exists(nrpe_scripts_dest):
+        os.makedirs(nrpe_scripts_dest)
     for fname in glob.glob(os.path.join(scripts_src, "*.sh")):
         shutil.copy2(fname,
-                     os.path.join(scripts_dst, os.path.basename(fname)))
-
-
-def update_nrpe_config():
-    install_nrpe_scripts()
+                     os.path.join(nrpe_scripts_dest, os.path.basename(fname)))
+
+
+def remove_nrpe_scripts():
+    scripts_src = os.path.join(os.environ["CHARM_DIR"], "files",
+                               "nrpe")
+    for fname in glob.glob(os.path.join(scripts_src, "*.sh")):
+        try:
+            os.remove(os.path.join(nrpe_scripts_dest,
+                      os.path.basename(fname)))
+        except OSError:
+            pass
+
+
+def  update_nrpe_config():
+    config_data = config_get()
     nrpe_compat = nrpe.NRPE()
-    nrpe_compat.add_check('haproxy', 'Check HAProxy', 'check_haproxy.sh')
-    nrpe_compat.add_check('haproxy_queue', 'Check HAProxy queue depth',
-                          'check_haproxy_queue_depth.sh')
+    checks_args = [
+        ('haproxy', 'Check HAProxy', 'check_haproxy.sh'),
+        ('haproxy_queue', 'Check HAProxy queue depth', 'check_haproxy_queue_depth.sh'),
+    ]
+    if config_data['enable_monitoring'] is True:
+        install_nrpe_scripts()
+        for check_args in checks_args:
+            nrpe_compat.add_check(*check_args)
+    else:
+        for check_args in checks_args:
+            if os.path.isfile(nrpe_scripts_dest + '/' + check_args[2]):
+                nrpe_compat.remove_check(shortname=check_args[0],
+                                         description=check_args[1],
+                                         check_cmd=check_args[2])
+        remove_nrpe_scripts()
     nrpe_compat.write()