Merge ~paulgear/ntp-charm/+git/ntp-charm:master into ntp-charm:master

[Paul Gear]

Selectively downgrade missing sync peer to WARNING

This uses comparison of more recent and less recent polls to determine
whether reachability is improving. If so, a missing sync peer is
considered WARNING rather than CRITICAL, on the assumption that ntpd will
correct the problem shortly. Thanks to Paul Collins for the idea and
initial implementation of this.

Update the test suite to check for this, and add python3 compatibility
for print statements.

[Paul Collins (pjdc)]

review: Approve (lgtm)

[Barry Price (barryprice)]

Makes sense, +1

review: Approve

diff --git a/files/nagios/check_ntpmon.py b/files/nagios/check_ntpmon.py
index 83613dd..8d24df4 100755
--- a/files/nagios/check_ntpmon.py
+++ b/files/nagios/check_ntpmon.py
@@ -19,6 +19,8 @@
 # this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+from __future__ import print_function
+
 import argparse
 import psutil
 import re
@@ -43,6 +45,15 @@ def isipaddressy(name):
     return re.search(r'^[0-9a-f.:]*$', name) is not None
 
 
+def report_reachability(old, new):
+    if old > new:
+        return "decreasing"
+    elif new > old:
+        return "increasing"
+    else:
+        return "stable"
+
+
 class CheckNTPMonSilent(object):
 
     def __init__(self,
@@ -109,12 +120,12 @@ class CheckNTPMonSilent(object):
         return True
 
     def dump(self):
-        print "warnpeers  = %d" % (self.warnpeers)
-        print "okpeers    = %d" % (self.okpeers)
-        print "warnoffset = %g" % (self.warnoffset)
-        print "critoffset = %g" % (self.critoffset)
-        print "warnreach  = %g" % (self.warnreach)
-        print "critreach  = %g" % (self.critreach)
+        print("warnpeers  = %d" % (self.warnpeers))
+        print("okpeers    = %d" % (self.okpeers))
+        print("warnoffset = %g" % (self.warnoffset))
+        print("critoffset = %g" % (self.critoffset))
+        print("warnreach  = %g" % (self.warnreach))
+        print("critreach  = %g" % (self.critreach))
 
     @classmethod
     def clone(cls, obj):
@@ -151,7 +162,7 @@ class CheckNTPMon(CheckNTPMonSilent):
         Return 1 if the number of peers is WARNING
         Return 2 if the number of peers is CRITICAL"""
         code, msg = CheckNTPMonSilent.peers(self, n)
-        print msg
+        print(msg)
         return code
 
     def offset(self, offset):
@@ -159,7 +170,7 @@ class CheckNTPMon(CheckNTPMonSilent):
         Return 1 if the offset is WARNING
         Return 2 if the offset is CRITICAL"""
         code, msg = CheckNTPMonSilent.offset(self, offset)
-        print msg
+        print(msg)
         return code
 
     def reachability(self, percent):
@@ -168,14 +179,14 @@ class CheckNTPMon(CheckNTPMonSilent):
         Return 2 if the reachability percentage is critical
         Raise a ValueError if reachability is not a percentage"""
         code, msg = CheckNTPMonSilent.reachability(self, percent)
-        print msg
+        print(msg)
         return code
 
     def sync(self, synchost):
         """Return 0 if the synchost is non-zero in length and is a roughly valid host identifier
         Return 2 otherwise"""
         code, msg = CheckNTPMonSilent.sync(self, synchost)
-        print msg
+        print(msg)
         return code
 
     def is_silent(self):
@@ -252,6 +263,8 @@ class NTPPeers(object):
             'peers': 0,
             'offsetall': 0,
             'totalreach': 0,
+            'oldreach': 0,
+            'newreach': 0,
         }
         self.check = check
 
@@ -284,8 +297,12 @@ class NTPPeers(object):
             # reachability - this counts the number of bits set in the reachability field
             # (which is displayed in octal in the ntpq output)
             # http://stackoverflow.com/questions/9829578/fast-way-of-counting-bits-in-python
-            self.ntpdata['totalreach'] += bin(int(peerdata['reach'],
-                                                  8)).count("1")
+            reachint = int(peerdata['reach'], 8)
+            self.ntpdata['totalreach'] += bin(reachint).count("1")
+
+            # count the 4 oldest & 4 newest polls
+            self.ntpdata['oldreach'] += bin((reachint >> 4) & 0x0f).count("1")
+            self.ntpdata['newreach'] += bin(reachint & 0x0f).count("1")
 
         # average offsets
         if self.ntpdata['survivors'] > 0:
@@ -309,24 +326,29 @@ class NTPPeers(object):
 
     def dumppart(self, peertype, peertypeoffset, displaytype):
         if self.ntpdata[peertype] > 0:
-            print "%d %s peers, average offset %g ms" % (
+            print("%d %s peers, average offset %g ms" % (
                 self.ntpdata[peertype],
                 displaytype,
                 self.ntpdata[peertypeoffset],
-            )
+            ))
 
     def dump(self):
         if self.ntpdata.get('syncpeer'):
-            print "Synced to: %s, offset %g ms" % (
-                self.ntpdata['syncpeer'], self.ntpdata['offsetsyncpeer'])
+            print("Synced to: %s, offset %g ms" % (
+                self.ntpdata['syncpeer'], self.ntpdata['offsetsyncpeer']))
         else:
-            print "No remote sync peer"
+            print("No remote sync peer")
         self.dumppart('peers', 'averageoffset', 'total')
         self.dumppart('survivors', 'averageoffsetsurvivors', 'good')
         self.dumppart('backups', 'averageoffsetbackups', 'backup')
         self.dumppart('discards', 'averageoffsetdiscards', 'discarded')
-        print "Average reachability of all peers: %d%%" % (
-            self.ntpdata['reachability'])
+        print("Average reachability of all peers: %d%%" % (
+            self.ntpdata['reachability']))
+        print("Reachability is %s (%d vs %d)" % (
+            report_reachability(self.ntpdata['oldreach'], self.ntpdata['newreach']),
+            self.ntpdata['oldreach'],
+            self.ntpdata['newreach'],
+        ))
 
     def check_peers(self, check=None):
         """Check the number of usable peers"""
@@ -358,14 +380,14 @@ class NTPPeers(object):
                     msg + " (" + result[1] + ")"
                 ]
             else:
-                print msg
+                print(msg)
                 return 1 if result < 1 else result
         else:
             ret = [2, "CRITICAL: No peers for which to check offset"]
             if check.is_silent():
                 return ret
             else:
-                print ret[1]
+                print(ret[1])
                 return ret[0]
 
     def check_reachability(self, check=None):
@@ -379,11 +401,16 @@ class NTPPeers(object):
         if check is None:
             check = self.check if self.check else CheckNTPMon()
         if self.ntpdata.get('syncpeer') is None:
-            ret = [2, "CRITICAL: No sync peer"]
+            if self.ntpdata['oldreach'] < self.ntpdata['newreach']:
+                # reachability is improving, we should be in sync soon
+                ret = [1, "WARNING: No sync peer"]
+            else:
+                # reachability is decreasing or stable
+                ret = [2, "CRITICAL: No sync peer"]
             if check.is_silent():
                 return ret
             else:
-                print ret[1]
+                print(ret[1])
                 return ret[0]
         return check.sync(self.ntpdata['syncpeer'])
 
@@ -412,9 +439,9 @@ class NTPPeers(object):
                 msg = result[1]
 
         if msg is None:
-            print "%s returned no results - please report a bug" % (method)
+            print("%s returned no results - please report a bug" % (method))
             return 3
-        print msg
+        print(msg)
         return ret
 
     @staticmethod
@@ -521,14 +548,14 @@ def main():
     lines = NTPPeers.query() if not args.test else [x.rstrip() for x in sys.stdin.readlines()]
     if lines is None:
         # Unknown result
-        print "CRITICAL: Cannot get peers from ntpq.  Please check that an NTP server is installed and running."
+        print("CRITICAL: Cannot get peers from ntpq.  Please check that an NTP server is installed and running.")
         sys.exit(2)
 
     # Don't report anything other than OK until ntpd has been running for at
     # least enough time for 8 polling intervals of 64 seconds each.
     age = NTPProcess().runtime()
     if age > 0 and age <= args.run_time:
-        print "OK: ntpd still starting up (running %d seconds)" % age
+        print("OK: ntpd still starting up (running %d seconds)" % age)
         sys.exit(0)
 
     # initialise our object with the results of ntpq and our preferred check
@@ -536,7 +563,7 @@ def main():
     ntp = NTPPeers(lines, checkntpmon)
 
     if args.debug:
-        print "\n".join(lines)
+        print("\n".join(lines))
         checkntpmon.dump()
         ntp.dump()
 
diff --git a/unit_tests/test_check_ntpmon.py b/unit_tests/test_check_ntpmon.py
index 4c41fc0..3194b31 100755
--- a/unit_tests/test_check_ntpmon.py
+++ b/unit_tests/test_check_ntpmon.py
@@ -19,6 +19,8 @@
 # this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+from __future__ import print_function
+
 import argparse
 import unittest
 import sys
@@ -66,12 +68,25 @@ testdata = [
  218.189.210.3   .INIT.          16 u    -   64    0    0.000    0.000   0.000
  103.224.117.98  .INIT.          16 u    -   64    0    0.000    0.000   0.000
  118.143.17.82   .INIT.          16 u    -   64    0    0.000    0.000   0.000
- 91.189.89.199   192.93.2.20      2 u   21   64    7  336.631    0.913   0.223
+ 91.189.89.199   192.93.2.20      2 u   21   64  301  336.631    0.913   0.223
  175.45.85.97    .INIT.          16 u    -   64    0    0.000    0.000   0.000
  192.189.54.33   .INIT.          16 u    -   64    0    0.000    0.000   0.000
  129.250.35.250  .INIT.          16 u    -   64    0    0.000    0.000   0.000
  54.252.165.245  .INIT.          16 u    -   64    0    0.000    0.000   0.000
 """,
+"""
+     remote           refid      st t when poll reach   delay   offset  jitter
+==============================================================================
+ 0.ubuntu.pool.n .POOL.          16 p    -   64    0    0.000    0.000   0.000
+ 1.ubuntu.pool.n .POOL.          16 p    -   64    0    0.000    0.000   0.000
+ 2.ubuntu.pool.n .POOL.          16 p    -   64    0    0.000    0.000   0.000
+ 3.ubuntu.pool.n .POOL.          16 p    -   64    0    0.000    0.000   0.000
+ ntp.ubuntu.com  .POOL.          16 p    -   64    0    0.000    0.000   0.000
++91.189.91.157   132.246.11.231   2 u  228  256  377  264.277    1.996  27.598
++218.189.210.3   118.143.17.82    2 u   42  256  377    3.297   -3.598   1.100
+ 209.58.185.100  130.133.1.10     2 u   62   64    3    2.237   28.653   0.088
+ 203.95.213.129  193.62.22.74     2 u   67   64    3    4.002   -1.256   0.041
+""",
 ]
 
 demodata = [
@@ -342,6 +357,32 @@ class TestCheckNTPMon(unittest.TestCase):
         self.assertEqual(ntp.check_peers(), 2, 'Low peers non-critical')
         self.assertEqual(ntp.check_reachability(), 2, 'Low reachability non-critical')
 
+    def test_NTPPeer4(self):
+        # check the parsing done by NTPPeers
+        ntp = NTPPeers(testdata[4].split("\n"))
+        self.assertEqual(ntp.ntpdata.get('syncpeer'), None)
+        self.assertEqual(ntp.ntpdata.get('offsetsyncpeer'), None)
+        self.assertEqual(ntp.ntpdata['survivors'], 2)
+        self.assertEqual(ntp.ntpdata.get('averageoffsetsurvivors'), 2.7969999999999997)
+        self.assertEqual(ntp.ntpdata['discards'], 2)
+        self.assertEqual(ntp.ntpdata['averageoffsetdiscards'], 14.9545)
+        self.assertEqual(ntp.ntpdata['peers'], 4)
+        self.assertEqual(ntp.ntpdata['averageoffset'], 8.87575)
+        self.assertEqual(ntp.ntpdata['reachability'], 62.5)
+
+        # run checks on the data
+        check = CheckNTPMon()
+        self.assertEqual(check.offset(ntp.ntpdata['averageoffsetdiscards']), 1, 'Discards offset non-warning')
+        self.assertEqual(check.offset(ntp.ntpdata['averageoffset']), 0, 'Offset non-OK')
+        self.assertEqual(check.peers(ntp.ntpdata['peers']), 0, 'Peers non-OK')
+        self.assertEqual(check.reachability(ntp.ntpdata['reachability']), 1, 'Reachability non-warning')
+
+        # run overall health checks
+        self.assertEqual(ntp.check_sync(), 1, 'Missing sync peer non-warning')
+        self.assertEqual(ntp.check_offset(), 0, 'Normal offset non-OK')
+        self.assertEqual(ntp.check_peers(), 0, 'OK peer count non-OK')
+        self.assertEqual(ntp.check_reachability(), 1, 'Low reachability non-critical')
+
     def test_defaults(self):
         c = CheckNTPMon()
         self.assertEqual(c.warnpeers, 2)
@@ -463,7 +504,7 @@ def demo():
     """Duplicate of test_demos which shows full output"""
     i = 0
     for d in demodata:
-        print "Parsing demo data %d: %s" % (i, d)
+        print("Parsing demo data %d: %s" % (i, d))
         ntp = NTPPeers(d.split("\n"))
         i += 1
         ntp.dump()
@@ -472,7 +513,7 @@ def demo():
         for method in methods:
             ret = method()
             if ret not in [0, 1, 2]:
-                print "Method %s returned invalid result parsing demo data:\n%s" % (method, d)
+                print("Method %s returned invalid result parsing demo data:\n%s" % (method, d))
                 sys.exit(3)