From 32b0768631fcd1f8a75b69627e225a19bece4923 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Fri, 29 Sep 2017 10:27:10 +0200 Subject: [PATCH] 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 | 2 +- features/watchdog.feature | 6 ++++-- patroni/watchdog/base.py | 3 +-- patroni/watchdog/linux.py | 30 ++++++++++++++++++++---------- tests/test_watchdog.py | 13 +++++++++++-- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/features/steps/watchdog.py b/features/steps/watchdog.py index dda54441..58f50e5e 100644 --- a/features/steps/watchdog.py +++ b/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() diff --git a/features/watchdog.feature b/features/watchdog.feature index 4f8215f6..216b0c4e 100644 --- a/features/watchdog.feature +++ b/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 diff --git a/patroni/watchdog/base.py b/patroni/watchdog/base.py index 51fdb14a..0007dee1 100644 --- a/patroni/watchdog/base.py +++ b/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: diff --git a/patroni/watchdog/linux.py b/patroni/watchdog/linux.py index 41c99746..311dcd19 100644 --- a/patroni/watchdog/linux.py +++ b/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 diff --git a/tests/test_watchdog.py b/tests/test_watchdog.py index 85e45a91..42434b9d 100644 --- a/tests/test_watchdog.py +++ b/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)