Fix watchdog on Python 3 (#531)

A misunderstanding of the ioctl() call interface. If mutable=False then fcntl.ioctl() actually returns the arg buffer back.
This accidentally worked on Python2 because int and str comparison did not return an error.
Error reporting is actually done by raising IOError on Python2 and OSError on Python3.

* Properly handle errors in set_timeout(), have them result in only a warning if watchdog support is not required.

* Improve watchdog device driver name display on Python3

* Eliminate race condition in watchdog feature tests.
  The pinged/closed states were not getting reset properly if the checks ran too quickly.
  Add explicit reset points in feature test so the check is unambiguous.

features/steps/watchdog.py

@@ -31,7 +31,7 @@ def watchdog_was_closed(context, name):
     assert context.pctl.get_watchdog(name).was_closed
 
 
-@step('I wait for next {name:w} watchdog ping')
+@step('I reset {name:w} watchdog state')
 def watchdog_reset_pinged(context, name):
     context.pctl.get_watchdog(name).reset()
 

features/watchdog.feature

@@ -14,7 +14,8 @@ Feature: watchdog
     Then postgres0 watchdog has been closed
 
   Scenario: watchdog is opened and pinged after resume
-    Given I run patronictl.py resume batman
+    Given I reset postgres0 watchdog state
+    And I run patronictl.py resume batman
     Then I receive a response returncode 0
     And postgres0 watchdog has been pinged after 10 seconds
 
@@ -23,7 +24,8 @@ Feature: watchdog
     Then postgres0 watchdog has been closed
 
   Scenario: watchdog is triggered if patroni stops responding
-    Given I start postgres0 with watchdog
+    Given I reset postgres0 watchdog state
+    And I start postgres0 with watchdog
     Then postgres0 role is the primary after 10 seconds
     When postgres0 hangs for 30 seconds
     Then postgres0 watchdog is triggered after 30 seconds

patroni/watchdog/base.py

@@ -133,6 +133,7 @@ class Watchdog(object):
 
         try:
             self.impl.open()
+            actual_timeout = self._set_timeout()
         except WatchdogError as e:
             logger.warning("Could not activate %s: %s", self.impl.describe(), e)
             self.impl = NullWatchdog()
@@ -141,8 +142,6 @@ class Watchdog(object):
             logger.warning("Watchdog implementation can't be disabled."
                            " Watchdog will trigger after Patroni loses leader key.")
 
-        actual_timeout = self._set_timeout()
-
         if not self.impl.is_running or actual_timeout > self.config.timeout:
             if self.config.mode == MODE_REQUIRED:
                 if self.impl.is_null:

patroni/watchdog/linux.py

@@ -155,21 +155,25 @@ class LinuxWatchdogDevice(WatchdogBase):
     def can_be_disabled(self):
         return self.get_support().has_MAGICCLOSE
 
-    def _ioctl(self, func, arg, mutate_arg=False):
+    def _ioctl(self, func, arg):
+        """Runs the specified ioctl on the underlying fd.
+
+        Raises WatchdogError if the device is closed.
+        Raises OSError or IOError (Python 2) when the ioctl fails."""
         if self._fd is None:
             raise WatchdogError("Watchdog device is closed")
-
-        result = fcntl.ioctl(self._fd, func, arg, mutate_arg)
-        if result < 0:
-            raise IOError(result)
+        fcntl.ioctl(self._fd, func, arg, True)
 
     def get_support(self):
         if self._support_cache is None:
             info = watchdog_info()
-            self._ioctl(WDIOC_GETSUPPORT, info, True)
+            try:
+                self._ioctl(WDIOC_GETSUPPORT, info)
+            except (WatchdogError, OSError, IOError) as e:
+                raise WatchdogError("Could not get information about watchdog device: {}".format(e))
             self._support_cache = WatchdogInfo(info.options,
                                                info.firmware_version,
-                                               str(bytearray(info.identity)).rstrip('\x00'))
+                                               bytearray(info.identity).decode(errors='ignore').rstrip('\x00'))
         return self._support_cache
 
     def describe(self):
@@ -180,7 +184,7 @@ class LinuxWatchdogDevice(WatchdogBase):
             try:
                 _, version, identity = self.get_support()
                 ver_str = " (firmware {0})".format(version) if version else ""
-            except WatchdogError:  # XXX: Can it really be raise when self._fd is not None?
+            except WatchdogError:
                 pass
 
         return identity + ver_str + dev_str
@@ -199,11 +203,17 @@ class LinuxWatchdogDevice(WatchdogBase):
         timeout = int(timeout)
         if not 0 < timeout < 0xFFFF:
             raise WatchdogError("Invalid timeout {0}. Supported values are between 1 and 65535".format(timeout))
-        self._ioctl(WDIOC_SETTIMEOUT, ctypes.c_int(timeout))
+        try:
+            self._ioctl(WDIOC_SETTIMEOUT, ctypes.c_int(timeout))
+        except (WatchdogError, OSError, IOError) as e:
+            raise WatchdogError("Could not set timeout on watchdog device: {}".format(e))
 
     def get_timeout(self):
         timeout = ctypes.c_int()
-        self._ioctl(WDIOC_GETTIMEOUT, timeout, True)
+        try:
+            self._ioctl(WDIOC_GETTIMEOUT, timeout)
+        except (WatchdogError, OSError, IOError) as e:
+            raise WatchdogError("Could not get timeout on watchdog device: {}".format(e))
         return timeout.value
 
 

tests/test_watchdog.py

@@ -194,15 +194,24 @@ class TestLinuxWatchdogDevice(unittest.TestCase):
         self.assertRaises(WatchdogError, self.impl.set_timeout, -1)
 
     @patch('os.open', Mock(return_value=3))
-    @patch('fcntl.ioctl', Mock(return_value=-1))
+    @patch('fcntl.ioctl', Mock(side_effect=OSError))
     def test__ioctl(self):
         self.assertRaises(WatchdogError, self.impl.get_support)
         self.impl.open()
-        self.assertRaises(IOError, self.impl.get_support)
+        self.assertRaises(WatchdogError, self.impl.get_support)
 
     def test_is_healthy(self):
         self.assertFalse(self.impl.is_healthy)
 
+    @patch('os.open', Mock(return_value=3))
+    @patch('fcntl.ioctl', Mock(side_effect=OSError))
+    def test_error_handling(self):
+        self.impl.open()
+        self.assertRaises(WatchdogError, self.impl.get_timeout)
+        self.assertRaises(WatchdogError, self.impl.set_timeout, 10)
+        # We still try to output a reasonable string even if getting info errors
+        self.assertEquals(self.impl.describe(), "Linux watchdog device")
+
     @patch('os.open', Mock(side_effect=OSError))
     def test_open(self):
         self.assertRaises(WatchdogError, self.impl.open)