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)