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

Proposed by Ponnuvel Palaniyappan on 2020-07-31
Status: Needs review
Proposed branch: lp:~pponnuvel/charm-haproxy/charm-haproxy
Merge into: lp:charm-haproxy
Diff against target: 29 lines (+9/-1)
1 file modified
hooks/hooks.py (+9/-1)
To merge this branch: bzr merge lp:~pponnuvel/charm-haproxy/charm-haproxy
Reviewer Review Type Date Requested Status
Tom Haddon 2020-07-31 Needs Fixing on 2020-08-01
Canonical IS Reviewers 2020-07-31 Pending
Review via email: mp+388506@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.

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

Tom Haddon (mthaddon) wrote :

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) wrote :

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) wrote :

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

Unmerged revisions

142. By Ponnuvel Palaniyappan on 2020-07-31

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 lp#1866408.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/hooks.py'
2--- hooks/hooks.py 2020-02-20 14:18:10 +0000
3+++ hooks/hooks.py 2020-07-31 17:57:13 +0000
4@@ -59,7 +59,10 @@
5 "deb http://archive.ubuntu.com/ubuntu %(release)s-backports "
6 "main restricted universe multiverse")
7 haproxy_preferences_path = "/etc/apt/preferences.d/haproxy"
8-
9+haproxy_montioring_check_scripts = [
10+ 'check_haproxy.sh',
11+ 'check_haproxy_queue_depth.sh',
12+ ]
13
14 dupe_options = [
15 "mode tcp",
16@@ -1174,9 +1177,14 @@
17 scripts_src = os.path.join(os.environ["CHARM_DIR"], "files",
18 "nrpe")
19 scripts_dst = "/usr/lib/nagios/plugins"
20+ config_data = config_get()
21 if not os.path.exists(scripts_dst):
22 os.makedirs(scripts_dst)
23+
24 for fname in glob.glob(os.path.join(scripts_src, "*.sh")):
25+ if os.path.basename(fname) in haproxy_montioring_check_scripts and \
26+ config_data['enable_monitoring'] is False:
27+ continue
28 shutil.copy2(fname,
29 os.path.join(scripts_dst, os.path.basename(fname)))
30

Subscribers

People subscribed via source and target branches