diff --git a/crmsh/sbd.py b/crmsh/sbd.py index 7b09a820b3..07f2261efe 100644 --- a/crmsh/sbd.py +++ b/crmsh/sbd.py @@ -756,6 +756,7 @@ def check_and_fix(self) -> CheckResult: else: raise FixFailure(f"Failed to fix {name} issue") + watchdog.Watchdog.warn_if_using_softdog() SBDManager.warn_diskless_sbd() return SBDConfigChecker._return_helper(check_res_list) diff --git a/crmsh/ui_sbd.py b/crmsh/ui_sbd.py index 7ae0073623..e1a6df6e43 100644 --- a/crmsh/ui_sbd.py +++ b/crmsh/ui_sbd.py @@ -672,14 +672,14 @@ def _print_sbd_status(self): def _print_watchdog_info(self): padding = 2 max_node_len = max(len(node) for node in self.cluster_nodes) + padding - watchdog_sbd_re = r"\[[0-9]+\] (/dev/.*)\nIdentity: Busy: .*sbd.*\nDriver: (.*)" device_list, driver_list, kernel_timeout_list = [], [], [] for node in self.cluster_nodes: - out = self.cluster_shell.get_stdout_or_raise_error("sbd query-watchdog", node) - res = re.search(watchdog_sbd_re, out) - if res: - device, driver = res.groups() + out = self.cluster_shell.get_stdout_or_raise_error(watchdog.Watchdog.QUERY_CMD, node) + watchdog_info = watchdog.Watchdog.get_watchdog_info(out, sbd_only=True) + if watchdog_info: + device = next(iter(watchdog_info)) + driver = watchdog_info[device] kernel_timeout = self.cluster_shell.get_stdout_or_raise_error("cat /proc/sys/kernel/watchdog_thresh", node) else: device, driver, kernel_timeout = "N/A", "N/A", "N/A" diff --git a/crmsh/utils.py b/crmsh/utils.py index 76d5bb55d7..febc11e814 100644 --- a/crmsh/utils.py +++ b/crmsh/utils.py @@ -1662,7 +1662,7 @@ def list_cluster_nodes(no_reg=False) -> list[str]: return [x.uname for x in cibquery.get_cluster_nodes(cib)] -def cluster_run_cmd(cmd, node_list=[]): +def cluster_run_cmd(cmd, node_list=None): """ Run cmd in cluster nodes """ diff --git a/crmsh/watchdog.py b/crmsh/watchdog.py index 31c23b5ea0..5d3fc81e85 100644 --- a/crmsh/watchdog.py +++ b/crmsh/watchdog.py @@ -1,17 +1,24 @@ +import logging import re + from . import utils from .sh import ShellUtils from . import sh from . import sbd +logger = logging.getLogger(__name__) + + class Watchdog(object): """ Class to find valid watchdog device name """ WATCHDOG_CFG = "/etc/modules-load.d/watchdog.conf" QUERY_CMD = "sudo sbd query-watchdog" - DEVICE_FIND_REGREX = "\\[[0-9]+\\] (/dev/.*)\n.*\nDriver: (.*)" + # output format might like: + # [1] /dev/watchdog\nIdentity: Software Watchdog\nDriver: softdog\n + DEVICE_FIND_REGREX = r"[ \t]*\[[0-9]+\] (/dev/[^\n]+)\n[ \t]*Identity: ([^\n]+)\n[ \t]*Driver: ([^\n]+)" def __init__(self, _input=None, remote_user=None, peer_host=None): """ @@ -28,25 +35,23 @@ def watchdog_device_name(self): return self._watchdog_device_name @staticmethod - def verify_watchdog_device(dev, ignore_error=False): + def verify_watchdog_device(dev): """ Use wdctl to verify watchdog device """ - rc, _, err = ShellUtils().get_stdout_stderr("wdctl {}".format(dev)) + rc, _, err = ShellUtils().get_stdout_stderr(f"wdctl {dev}") if rc != 0: - if ignore_error: - return False - else: - utils.fatal("Invalid watchdog device {}: {}".format(dev, err)) + utils.fatal(f"Invalid watchdog device {dev}: {err}") return True @staticmethod - def _load_watchdog_driver(driver): + def _load_watchdog_driver(driver, join=False): """ Load specific watchdog driver """ - ShellUtils().get_stdout_stderr(f"echo {driver} > {Watchdog.WATCHDOG_CFG}") - ShellUtils().get_stdout_stderr("systemctl restart systemd-modules-load") + cmd = f"echo {driver} > {Watchdog.WATCHDOG_CFG} && systemctl restart systemd-modules-load" + node_list = utils.this_node() if join else None + utils.cluster_run_cmd(cmd, node_list) @staticmethod def get_watchdog_device_from_sbd_config(): @@ -64,6 +69,49 @@ def _driver_is_loaded(driver): _, out, _ = ShellUtils().get_stdout_stderr("lsmod") return re.search("\n{}\\s+".format(driver), out) + @staticmethod + def _get_configured_watchdog_driver(): + """ + Get watchdog driver name from modules-load config. + """ + try: + with open(Watchdog.WATCHDOG_CFG) as f: + return f.readline().strip() + except OSError: + return None + + @classmethod + def get_watchdog_info(cls, out, sbd_only=False): + """ + Parse sbd query-watchdog output into {device_name: driver_name}. + """ + if not out: + return {} + + watchdog_info = {} + for device, identity, driver in re.findall(cls.DEVICE_FIND_REGREX, out): + if sbd_only and not re.search(r"Busy: .*sbd", identity): + continue + if driver == "": + configured_driver = cls._get_configured_watchdog_driver() + if configured_driver and cls._driver_is_loaded(configured_driver): + driver = configured_driver + watchdog_info[device] = driver + return watchdog_info + + @staticmethod + def warn_if_using_softdog(): + """ + Warn if SBD is using softdog as watchdog driver. + """ + rc, out, err = ShellUtils().get_stdout_stderr(Watchdog.QUERY_CMD) + if rc != 0 or not out: + logger.debug("Failed to run %s: %s", Watchdog.QUERY_CMD, err) + return + + if "softdog" in Watchdog.get_watchdog_info(out, sbd_only=True).values(): + logger.warning("It's not recommended to use softdog as watchdog driver in production environment") + def _set_watchdog_info(self): """ Set watchdog info through sbd query-watchdog command @@ -71,9 +119,7 @@ def _set_watchdog_info(self): """ rc, out, err = ShellUtils().get_stdout_stderr(self.QUERY_CMD) if rc == 0 and out: - # output format might like: - # [1] /dev/watchdog\nIdentity: Software Watchdog\nDriver: softdog\n - self._watchdog_info_dict = dict(re.findall(self.DEVICE_FIND_REGREX, out)) + self._watchdog_info_dict = self.get_watchdog_info(out) else: utils.fatal("Failed to run {}: {}".format(self.QUERY_CMD, err)) @@ -92,39 +138,24 @@ def _get_driver_through_device_remotely(self, dev_name): """ rc, out, err = sh.cluster_shell().get_rc_stdout_stderr_without_input(self._peer_host, self.QUERY_CMD) if rc == 0 and out: - # output format might like: - # [1] /dev/watchdog\nIdentity: Software Watchdog\nDriver: softdog\n - device_driver_dict = dict(re.findall(self.DEVICE_FIND_REGREX, out)) - if device_driver_dict and dev_name in device_driver_dict: + device_driver_dict = self.get_watchdog_info(out) + if dev_name in device_driver_dict: return device_driver_dict[dev_name] else: return None else: utils.fatal("Failed to run {} remotely: {}".format(self.QUERY_CMD, err)) - def _get_first_unused_device(self): - """ - Get first unused watchdog device name - """ - for dev in self._watchdog_info_dict: - if self.verify_watchdog_device(dev, ignore_error=True): - return dev - return None - def _set_input(self): - """ - If self._input was not provided by option: - 1. Try to get it from sbd config file - 2. Try to get the first valid device from result of sbd query-watchdog - 3. Set the self._input as softdog - """ - if not self._input: - dev = self.get_watchdog_device_from_sbd_config() - if dev and self.verify_watchdog_device(dev, ignore_error=True): + if self._input: + return + + for dev, driver in self._watchdog_info_dict.items(): + if driver != "softdog": self._input = dev return - first_unused = self._get_first_unused_device() - self._input = first_unused if first_unused else "softdog" + + self._input = "softdog" def _valid_device(self, dev): """ @@ -136,7 +167,7 @@ def _valid_device(self, dev): def join_watchdog(self): """ - In join proces, get watchdog device from config + In join process, get watchdog device from config If that device not exist, get driver name from init node, and load that driver """ self._set_watchdog_info() @@ -148,7 +179,7 @@ def join_watchdog(self): if not self._valid_device(self._input): driver = self._get_driver_through_device_remotely(self._input) - self._load_watchdog_driver(driver) + self._load_watchdog_driver(driver, join=True) def init_watchdog(self): """ diff --git a/test/unittests/test_sbd.py b/test/unittests/test_sbd.py index fe7d010380..967183b5fb 100644 --- a/test/unittests/test_sbd.py +++ b/test/unittests/test_sbd.py @@ -392,11 +392,12 @@ def test_check_and_fix_sbd_inconsistent(self, mock_service_manager, mock_check_a self.instance_check._check_config_consistency.assert_called_once() @patch('crmsh.sbd.SBDConfigChecker._check_deprecated_property') + @patch("crmsh.watchdog.Watchdog.warn_if_using_softdog") @patch('crmsh.sbd.SBDManager.warn_diskless_sbd') @patch('crmsh.utils.list_cluster_nodes_except_me') @patch('crmsh.utils.check_all_nodes_reachable') @patch('crmsh.sbd.ServiceManager') - def test_check_and_fix_not_fix(self, mock_service_manager, mock_check_all_nodes_reachable, mock_list_cluster_nodes_except_me, mock_warn_diskless_sbd, mock_check_deprecated_property): + def test_check_and_fix_not_fix(self, mock_service_manager, mock_check_all_nodes_reachable, mock_list_cluster_nodes_except_me, mock_warn_diskless_sbd, mock_warn_if_using_softdog, mock_check_deprecated_property): mock_service_manager_inst = Mock() mock_service_manager.return_value = mock_service_manager_inst mock_service_manager_inst.service_is_active = Mock(return_value=True) @@ -424,6 +425,7 @@ def test_check_and_fix_not_fix(self, mock_service_manager, mock_check_all_nodes_ self.instance_check._check_config_consistency.assert_called_once() self.instance_check._load_configurations_from_runtime.assert_called_once() self.instance_check._check_sbd_disk_metadata.assert_called_once() + mock_warn_if_using_softdog.assert_called_once_with() @patch('crmsh.utils.list_cluster_nodes_except_me') @patch('crmsh.utils.check_all_nodes_reachable') @@ -444,11 +446,12 @@ def test_check_and_fix_fix_failure(self, mock_service_manager, mock_check_all_no self.assertTrue("Failed to fix SBD disk metadata" in str(context.exception)) @patch('crmsh.sbd.SBDConfigChecker._check_deprecated_property') + @patch("crmsh.watchdog.Watchdog.warn_if_using_softdog") @patch('crmsh.sbd.SBDManager.warn_diskless_sbd') @patch('crmsh.utils.list_cluster_nodes_except_me') @patch('crmsh.utils.check_all_nodes_reachable') @patch('crmsh.sbd.ServiceManager') - def test_check_and_fix_fix_success(self, mock_service_manager, mock_check_all_nodes_reachable, mock_list_cluster_nodes_except_me, mock_warn_diskless_sbd, mock_check_deprecated_property): + def test_check_and_fix_fix_success(self, mock_service_manager, mock_check_all_nodes_reachable, mock_list_cluster_nodes_except_me, mock_warn_diskless_sbd, mock_warn_if_using_softdog, mock_check_deprecated_property): mock_service_manager_inst = Mock() mock_service_manager.return_value = mock_service_manager_inst mock_service_manager_inst.service_is_active = Mock(return_value=True) @@ -484,6 +487,7 @@ def test_check_and_fix_fix_success(self, mock_service_manager, mock_check_all_no self.instance_fix._check_fencing_timeout.assert_called_once() self.instance_fix._check_sbd_delay_start_unset_dropin.assert_called_once() + mock_warn_if_using_softdog.assert_called_once_with() @patch('logging.Logger.error') @patch('logging.Logger.warning') @patch('crmsh.utils.remote_diff_this') diff --git a/test/unittests/test_watchdog.py b/test/unittests/test_watchdog.py index 4c48b9ffb3..647d0f8423 100644 --- a/test/unittests/test_watchdog.py +++ b/test/unittests/test_watchdog.py @@ -44,13 +44,6 @@ def test_watchdog_device_name(self): res = self.watchdog_inst.watchdog_device_name assert res is None - @mock.patch('crmsh.sh.ShellUtils.get_stdout_stderr') - def test_verify_watchdog_device_ignore_error(self, mock_run): - mock_run.return_value = (1, None, "error") - res = self.watchdog_inst.verify_watchdog_device("/dev/watchdog", True) - self.assertEqual(res, False) - mock_run.assert_called_once_with("wdctl /dev/watchdog") - @mock.patch('crmsh.utils.fatal') @mock.patch('crmsh.sh.ShellUtils.get_stdout_stderr') def test_verify_watchdog_device_error(self, mock_run, mock_error): @@ -67,13 +60,11 @@ def test_verify_watchdog_device(self, mock_run): res = self.watchdog_inst.verify_watchdog_device("/dev/watchdog") self.assertEqual(res, True) - @mock.patch('crmsh.sh.ShellUtils.get_stdout_stderr') + @mock.patch('crmsh.utils.cluster_run_cmd') def test_load_watchdog_driver(self, mock_run): self.watchdog_inst._load_watchdog_driver("softdog") - mock_run.assert_has_calls([ - mock.call(f"echo softdog > {watchdog.Watchdog.WATCHDOG_CFG}"), - mock.call("systemctl restart systemd-modules-load") - ]) + mock_run.assert_called_once_with( + f"echo softdog > {watchdog.Watchdog.WATCHDOG_CFG} && systemctl restart systemd-modules-load", None) @mock.patch('crmsh.utils.parse_sysconfig') def test_get_watchdog_device_from_sbd_config(self, mock_parse): @@ -96,6 +87,81 @@ def test_driver_is_loaded(self, mock_run): assert res is not None mock_run.assert_called_once_with("lsmod") + @mock.patch("crmsh.watchdog.Watchdog._driver_is_loaded") + def test_get_watchdog_info(self, mock_driver_is_loaded): + output = """ +Discovered 2 watchdog devices: + +[1] /dev/watchdog +Identity: Busy: PID 3120 (sbd) +Driver: softdog +CAUTION: Not recommended for use with sbd. + +[2] /dev/watchdog1 +Identity: iTCO_wdt +Driver: iTCO_wdt + """ + res = watchdog.Watchdog.get_watchdog_info(output) + self.assertEqual(res, {"/dev/watchdog": "softdog", "/dev/watchdog1": "iTCO_wdt"}) + mock_driver_is_loaded.assert_not_called() + + def test_get_watchdog_info_sbd_only(self): + output = """ +[1] /dev/watchdog +Identity: Busy: PID 3120 (sbd) +Driver: softdog + +[2] /dev/watchdog1 +Identity: iTCO_wdt +Driver: iTCO_wdt + """ + res = watchdog.Watchdog.get_watchdog_info(output, sbd_only=True) + self.assertEqual(res, {"/dev/watchdog": "softdog"}) + + @mock.patch("builtins.open", new_callable=mock.mock_open, read_data="iTCO_wdt\n") + def test_get_configured_watchdog_driver(self, mock_open): + res = watchdog.Watchdog._get_configured_watchdog_driver() + self.assertEqual(res, "iTCO_wdt") + mock_open.assert_called_once_with(watchdog.Watchdog.WATCHDOG_CFG) + + @mock.patch("builtins.open", side_effect=OSError) + def test_get_configured_watchdog_driver_error(self, mock_open): + res = watchdog.Watchdog._get_configured_watchdog_driver() + self.assertEqual(res, None) + mock_open.assert_called_once_with(watchdog.Watchdog.WATCHDOG_CFG) + + @mock.patch("crmsh.watchdog.Watchdog._driver_is_loaded") + @mock.patch("crmsh.watchdog.Watchdog._get_configured_watchdog_driver") + def test_get_watchdog_info_unknown_configured_driver(self, mock_configured_driver, mock_driver_is_loaded): + output = """ +[1] /dev/watchdog +Identity: Busy: PID 3120 (sbd) +Driver: + """ + mock_configured_driver.return_value = "iTCO_wdt" + mock_driver_is_loaded.return_value = True + res = watchdog.Watchdog.get_watchdog_info(output) + self.assertEqual(res, {"/dev/watchdog": "iTCO_wdt"}) + + mock_configured_driver.assert_called_once_with() + mock_driver_is_loaded.assert_called_once_with("iTCO_wdt") + + @mock.patch("crmsh.watchdog.Watchdog._driver_is_loaded") + @mock.patch("crmsh.watchdog.Watchdog._get_configured_watchdog_driver") + def test_get_watchdog_info_unknown_unloaded_driver(self, mock_configured_driver, mock_driver_is_loaded): + output = """ +[1] /dev/watchdog +Identity: Busy: PID 3120 (sbd) +Driver: + """ + mock_configured_driver.return_value = "iTCO_wdt" + mock_driver_is_loaded.return_value = False + res = watchdog.Watchdog.get_watchdog_info(output) + self.assertEqual(res, {"/dev/watchdog": ""}) + + mock_configured_driver.assert_called_once_with() + mock_driver_is_loaded.assert_called_once_with("iTCO_wdt") + @mock.patch('crmsh.utils.fatal') @mock.patch('crmsh.sh.ShellUtils.get_stdout_stderr') def test_set_watchdog_info_error(self, mock_run, mock_error): @@ -184,39 +250,24 @@ def test_get_driver_through_device_remotely(self, mock_cluster_shell): self.assertEqual(res, "softdog") mock_cluster_shell().get_rc_stdout_stderr_without_input.assert_called_once_with("node1", watchdog.Watchdog.QUERY_CMD) - def test_get_first_unused_device_none(self): - res = self.watchdog_inst._get_first_unused_device() - self.assertEqual(res, None) - - @mock.patch('crmsh.watchdog.Watchdog.verify_watchdog_device') - def test_get_first_unused_device(self, mock_verify): - mock_verify.return_value = True - self.watchdog_inst._watchdog_info_dict = {'/dev/watchdog': 'softdog', '/dev/watchdog0': 'softdog', '/dev/watchdog1': 'iTCO_wdt'} - res = self.watchdog_inst._get_first_unused_device() - self.assertEqual(res, "/dev/watchdog") - mock_verify.assert_called_once_with("/dev/watchdog", ignore_error=True) + def test_set_input_keep_existing(self): + self.watchdog_inst._input = "/dev/watchdog" + self.watchdog_inst._set_input() + self.assertEqual(self.watchdog_inst._input, "/dev/watchdog") - @mock.patch('crmsh.watchdog.Watchdog._get_first_unused_device') - @mock.patch('crmsh.watchdog.Watchdog.verify_watchdog_device') - @mock.patch('crmsh.watchdog.Watchdog.get_watchdog_device_from_sbd_config') - def test_set_input_from_config(self, mock_from_config, mock_verify, mock_first): - mock_from_config.return_value = "/dev/watchdog" - mock_verify.return_value = True + def test_set_input_softdog_when_no_device(self): self.watchdog_inst._set_input() - mock_first.assert_not_called() - mock_from_config.assert_called_once_with() + self.assertEqual(self.watchdog_inst._input, "softdog") - @mock.patch('crmsh.watchdog.Watchdog._get_first_unused_device') - @mock.patch('crmsh.watchdog.Watchdog.verify_watchdog_device') - @mock.patch('crmsh.watchdog.Watchdog.get_watchdog_device_from_sbd_config') - def test_set_input(self, mock_from_config, mock_verify, mock_first): - mock_from_config.return_value = None - mock_first.return_value = None + def test_set_input_softdog_when_only_softdog(self): + self.watchdog_inst._watchdog_info_dict = {'/dev/watchdog': 'softdog', '/dev/watchdog0': 'softdog'} self.watchdog_inst._set_input() self.assertEqual(self.watchdog_inst._input, "softdog") - mock_from_config.assert_called_once_with() - mock_verify.assert_not_called() - mock_first.assert_called_once_with() + + def test_set_input_prefer_non_softdog(self): + self.watchdog_inst._watchdog_info_dict = {'/dev/watchdog': 'softdog', '/dev/watchdog1': 'iTCO_wdt'} + self.watchdog_inst._set_input() + self.assertEqual(self.watchdog_inst._input, "/dev/watchdog1") def test_valid_device_false(self): res = self.watchdog_inst._valid_device("test") @@ -257,7 +308,7 @@ def test_join_watchdog(self, mock_set_info, mock_from_config, mock_valid, mock_g mock_from_config.assert_called_once_with() mock_valid.assert_called_once_with("/dev/watchdog") mock_get_driver_remotely.assert_called_once_with("/dev/watchdog") - mock_load.assert_called_once_with("softdog") + mock_load.assert_called_once_with("softdog", join=True) @mock.patch('crmsh.sh.ShellUtils.get_stdout_stderr') @mock.patch('crmsh.watchdog.Watchdog._valid_device')