Merge lp:~macgreagoir/charms/precise/nrpe-external-master/load into lp:charms/nrpe-external-master

[Mick Gregg]

[macgreagoir] Make it possible to autogenerate check_load parameters.

[Alvaro Uria (aluria)]

lgtm

review: Approve

[Charles Butler (lazypower)]

Greetings Mick Gregg

I took some time to review this MP, and I really like the thought of the NRPE master generating the proper config for checks. While this looks great, there appears to be a slight issue with the code.

When testing, I got some errors on the config-changed hook that surface around line:

 56 let "LOAD_WARN = $PROC_COUNT / 2"

When the unit only has a single processor. I've proposed a branch for merging into yours. If you feel this is a good patch to resolve the issue, feel free to merge it and ping me for a follow up express review/merge.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

review: Needs Information

[Charles Butler (lazypower)]

Ah, and I realized I didn't link to that branch, which is here: lp:~lazypower/charms/precise/nrpe-external-master/patch-auto-config

[Mick Gregg (macgreagoir)]

Charles, thanks for the review. I could only see an empty diff in your MP, sorry. I've committed a new change that I hope does something similar to what you meant to propose.

[Matt Bruzek (mbruzek)]

Hello Mick,

Thanks for submitting this fix to the nrpe-external-master charm. The LOAD_WARN now evalutes to 1 when PROC_COUNT = 1.

I ran your branch on our CI system, and the tests pass. The CI system runs instances with 2 processors. The tests for the nrpe-external-master charm are very basic stand up charm test. As a user of NRPE it would be much appreciated if you could add better tests to the amulet test on your next commit. I would suggest writing tests that at least test the value of the load were set correctly

This change looks good to me, so I am going to merge it with the recommended version of the charm in the charm store. Thanks for the contribution!

review: Approve

[Mick Gregg (macgreagoir)]

Mark,

Thanks for the review. It didn't appear to merge, so I've set the status back to 'Needs review'. Do you mind taking a look at the merge again, please.

Cheers,

Mick

[Matt Bruzek (mbruzek)]

Mick,

Sorry about the mistake, I thought the code was merged. The code is merged now. Thanks for the follow up. Many apologies for my mistake.

[Mick Gregg (macgreagoir)]

Matt, no worries at all. Thanks again.

=== modified file 'config.yaml'
--- config.yaml	2014-05-27 10:36:57 +0000
+++ config.yaml	2015-05-21 15:57:10 +0000
@@ -49,9 +49,13 @@
     description: Number of processes check or 'auto' for the charm to generate
         thresholds based on processor count
   load:
-    default: "-w 8,8,8 -c 15,15,15"
+    default: "auto"
     type: string
-    description: Load check
+    description: |
+        Load check. Parameters for the check_load script or 'auto' to allow the
+        charm to generate parameters based on the processor count.
+        For example, '-w 8,8,8 -c 15,15,15' sets the WARNING threshold to 8 and
+        CRITICAL to 15.
   users:
     default: "-w 20 -c 25"
     type: string

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2014-11-20 04:35:17 +0000
+++ hooks/config-changed	2015-05-21 15:57:10 +0000
@@ -51,6 +51,17 @@
     PROC_THRESHOLDS=$PROCS
 fi
 
+if [[ "$LOAD" == "auto" ]]; then
+    PROC_COUNT=$(nproc)
+    let "LOAD_WARN = $PROC_COUNT / 2 + $PROC_COUNT % 2"
+    let "LOAD_CRIT = $PROC_COUNT"
+    LOAD_W="-w ${LOAD_WARN},${LOAD_WARN},${LOAD_WARN}"
+    LOAD_C="-c ${LOAD_CRIT},${LOAD_CRIT},${LOAD_CRIT}"
+    LOAD_THRESHOLDS="$LOAD_W $LOAD_C"
+else
+    LOAD_THRESHOLDS=$LOAD
+fi
+
 echo "# Root disk" > /etc/nagios/nrpe.d/check_disk_root.cfg
 echo "command[check_disk_root]=/usr/lib/nagios/plugins/check_disk ${DISK_ROOT} -p /" >> /etc/nagios/nrpe.d/check_disk_root.cfg
 
@@ -61,7 +72,7 @@
 echo "command[check_total_procs]=/usr/lib/nagios/plugins/check_procs ${PROC_THRESHOLDS}" >> /etc/nagios/nrpe.d/check_total_procs.cfg
 
 echo "# System Load" > /etc/nagios/nrpe.d/check_load.cfg
-echo "command[check_load]=/usr/lib/nagios/plugins/check_load ${LOAD}" >> /etc/nagios/nrpe.d/check_load.cfg
+echo "command[check_load]=/usr/lib/nagios/plugins/check_load ${LOAD_THRESHOLDS}" >> /etc/nagios/nrpe.d/check_load.cfg
 
 echo "# Number of Users" > /etc/nagios/nrpe.d/check_users.cfg
 echo "command[check_users]=/usr/lib/nagios/plugins/check_users ${USERS}" >> /etc/nagios/nrpe.d/check_users.cfg