From bbf53f713189eb2233efa03bf3aa9c96eb79ba82 Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Thu, 5 Jan 2023 16:21:44 +0100 Subject: [PATCH 2/2] Fix stonith-watchdog-timeout validation --- CHANGELOG.md | 2 + pcs/lib/cluster_property.py | 25 ++++- pcs/lib/sbd.py | 15 ++- .../lib/commands/test_cluster_property.py | 50 ++++++++-- pcs_test/tier0/lib/test_cluster_property.py | 98 ++++++++++++++----- pcs_test/tier1/test_cluster_property.py | 14 ++- 6 files changed, 159 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47212f00..0945d727 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Graceful stopping pcsd service using `systemctl stop pcsd` command - Displaying bool and integer values in `pcs resource config` command ([rhbz#2151164], [ghissue#604]) +- Allow time values in stonith-watchdog-time property ([rhbz#2158790]) ### Changed - Resource/stonith agent self-validation of instance attributes is now @@ -22,6 +23,7 @@ [rhbz#2151164]: https://bugzilla.redhat.com/show_bug.cgi?id=2151164 [rhbz#2151524]: https://bugzilla.redhat.com/show_bug.cgi?id=2151524 [rhbz#2159454]: https://bugzilla.redhat.com/show_bug.cgi?id=2159454 +[rhbz#2158790]: https://bugzilla.redhat.com/show_bug.cgi?id=2158790 ## [0.11.4] - 2022-11-21 diff --git a/pcs/lib/cluster_property.py b/pcs/lib/cluster_property.py index 3bbc093d..d3c8a896 100644 --- a/pcs/lib/cluster_property.py +++ b/pcs/lib/cluster_property.py @@ -7,6 +7,7 @@ from lxml.etree import _Element from pcs.common import reports from pcs.common.services.interfaces import ServiceManagerInterface +from pcs.common.tools import timeout_to_seconds from pcs.common.types import StringSequence from pcs.lib import ( sbd, @@ -37,8 +38,21 @@ def _validate_stonith_watchdog_timeout_property( force: bool = False, ) -> reports.ReportItemList: report_list: reports.ReportItemList = [] + original_value = value + # if value is not empty, try to convert time interval string + if value: + seconds = timeout_to_seconds(value) + if seconds is None: + # returns empty list because this should be reported by + # ValueTimeInterval validator + return report_list + value = str(seconds) if sbd.is_sbd_enabled(service_manager): - report_list.extend(sbd.validate_stonith_watchdog_timeout(value, force)) + report_list.extend( + sbd.validate_stonith_watchdog_timeout( + validate.ValuePair(original_value, value), force + ) + ) else: if value not in ["", "0"]: report_list.append( @@ -123,9 +137,6 @@ def validate_set_cluster_properties( # unknow properties are reported by NamesIn validator continue property_metadata = possible_properties_dict[property_name] - if property_metadata.name == "stonith-watchdog-timeout": - # needs extra validation - continue if property_metadata.type == "boolean": validators.append( validate.ValuePcmkBoolean( @@ -153,9 +164,13 @@ def validate_set_cluster_properties( ) ) elif property_metadata.type == "time": + # make stonith-watchdog-timeout value not forcable validators.append( validate.ValueTimeInterval( - property_metadata.name, severity=severity + property_metadata.name, + severity=severity + if property_metadata.name != "stonith-watchdog-timeout" + else reports.ReportItemSeverity.error(), ) ) report_list.extend( diff --git a/pcs/lib/sbd.py b/pcs/lib/sbd.py index 1e3cfb37..38cd8767 100644 --- a/pcs/lib/sbd.py +++ b/pcs/lib/sbd.py @@ -1,6 +1,9 @@ import re from os import path -from typing import Optional +from typing import ( + Optional, + Union, +) from pcs import settings from pcs.common import reports @@ -392,7 +395,10 @@ def _get_local_sbd_watchdog_timeout() -> int: def validate_stonith_watchdog_timeout( - stonith_watchdog_timeout: str, force: bool = False + stonith_watchdog_timeout: Union[ + validate.TypeOptionValue, validate.ValuePair + ], + force: bool = False, ) -> reports.ReportItemList: """ Check sbd status and config when user is setting stonith-watchdog-timeout @@ -401,6 +407,7 @@ def validate_stonith_watchdog_timeout( stonith_watchdog_timeout -- value to be validated """ + stonith_watchdog_timeout = validate.ValuePair.get(stonith_watchdog_timeout) severity = reports.get_severity(reports.codes.FORCE, force) if _is_device_set_local(): return ( @@ -412,11 +419,11 @@ def validate_stonith_watchdog_timeout( ), ) ] - if stonith_watchdog_timeout not in ["", "0"] + if stonith_watchdog_timeout.normalized not in ["", "0"] else [] ) - if stonith_watchdog_timeout in ["", "0"]: + if stonith_watchdog_timeout.normalized in ["", "0"]: return [ reports.ReportItem( severity, diff --git a/pcs_test/tier0/lib/commands/test_cluster_property.py b/pcs_test/tier0/lib/commands/test_cluster_property.py index 94c0938a..781222ab 100644 --- a/pcs_test/tier0/lib/commands/test_cluster_property.py +++ b/pcs_test/tier0/lib/commands/test_cluster_property.py @@ -120,6 +120,34 @@ class StonithWatchdogTimeoutMixin(LoadMetadataMixin): ) self.env_assist.assert_reports([]) + def _set_invalid_value(self, forced=False): + self.config.remove("services.is_enabled") + self.env_assist.assert_raise_library_error( + lambda: cluster_property.set_properties( + self.env_assist.get_env(), + {"stonith-watchdog-timeout": "15x"}, + [] if not forced else [reports.codes.FORCE], + ) + ) + self.env_assist.assert_reports( + [ + fixture.error( + reports.codes.INVALID_OPTION_VALUE, + option_name="stonith-watchdog-timeout", + option_value="15x", + allowed_values="time interval (e.g. 1, 2s, 3m, 4h, ...)", + cannot_be_empty=False, + forbidden_characters=None, + ), + ] + ) + + def test_set_invalid_value(self): + self._set_invalid_value(forced=False) + + def test_set_invalid_value_forced(self): + self._set_invalid_value(forced=True) + class TestSetStonithWatchdogTimeoutSBDIsDisabled( StonithWatchdogTimeoutMixin, TestCase @@ -132,6 +160,9 @@ class TestSetStonithWatchdogTimeoutSBDIsDisabled( def test_set_zero(self): self._set_success({"stonith-watchdog-timeout": "0"}) + def test_set_zero_time_suffix(self): + self._set_success({"stonith-watchdog-timeout": "0s"}) + def test_set_not_zero_or_empty(self): self.env_assist.assert_raise_library_error( lambda: cluster_property.set_properties( @@ -231,12 +262,12 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledWatchdogOnly( def test_set_zero_forced(self): self.config.env.push_cib( crm_config=fixture_crm_config_properties( - [("cib-bootstrap-options", {"stonith-watchdog-timeout": "0"})] + [("cib-bootstrap-options", {"stonith-watchdog-timeout": "0s"})] ) ) cluster_property.set_properties( self.env_assist.get_env(), - {"stonith-watchdog-timeout": "0"}, + {"stonith-watchdog-timeout": "0s"}, [reports.codes.FORCE], ) self.env_assist.assert_reports( @@ -271,7 +302,7 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledWatchdogOnly( self.env_assist.assert_raise_library_error( lambda: cluster_property.set_properties( self.env_assist.get_env(), - {"stonith-watchdog-timeout": "9"}, + {"stonith-watchdog-timeout": "9s"}, [], ) ) @@ -281,7 +312,7 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledWatchdogOnly( reports.codes.STONITH_WATCHDOG_TIMEOUT_TOO_SMALL, force_code=reports.codes.FORCE, cluster_sbd_watchdog_timeout=10, - entered_watchdog_timeout="9", + entered_watchdog_timeout="9s", ) ] ) @@ -289,12 +320,12 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledWatchdogOnly( def test_too_small_forced(self): self.config.env.push_cib( crm_config=fixture_crm_config_properties( - [("cib-bootstrap-options", {"stonith-watchdog-timeout": "9"})] + [("cib-bootstrap-options", {"stonith-watchdog-timeout": "9s"})] ) ) cluster_property.set_properties( self.env_assist.get_env(), - {"stonith-watchdog-timeout": "9"}, + {"stonith-watchdog-timeout": "9s"}, [reports.codes.FORCE], ) self.env_assist.assert_reports( @@ -302,13 +333,13 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledWatchdogOnly( fixture.warn( reports.codes.STONITH_WATCHDOG_TIMEOUT_TOO_SMALL, cluster_sbd_watchdog_timeout=10, - entered_watchdog_timeout="9", + entered_watchdog_timeout="9s", ) ] ) def test_more_than_timeout(self): - self._set_success({"stonith-watchdog-timeout": "11"}) + self._set_success({"stonith-watchdog-timeout": "11s"}) @mock.patch("pcs.lib.sbd.get_local_sbd_device_list", lambda: ["dev1", "dev2"]) @@ -323,6 +354,9 @@ class TestSetStonithWatchdogTimeoutSBDIsEnabledSharedDevices( def test_set_to_zero(self): self._set_success({"stonith-watchdog-timeout": "0"}) + def test_set_to_zero_time_suffix(self): + self._set_success({"stonith-watchdog-timeout": "0min"}) + def test_set_not_zero_or_empty(self): self.env_assist.assert_raise_library_error( lambda: cluster_property.set_properties( diff --git a/pcs_test/tier0/lib/test_cluster_property.py b/pcs_test/tier0/lib/test_cluster_property.py index 2feb728d..8d6f90b1 100644 --- a/pcs_test/tier0/lib/test_cluster_property.py +++ b/pcs_test/tier0/lib/test_cluster_property.py @@ -83,6 +83,7 @@ FIXTURE_VALID_OPTIONS_DICT = { "integer_param": "10", "percentage_param": "20%", "select_param": "s3", + "stonith-watchdog-timeout": "0", "time_param": "5min", } @@ -96,6 +97,8 @@ FIXTURE_INVALID_OPTIONS_DICT = { "have-watchdog": "100", } +STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES = ["", "0", "0s"] + def _fixture_parameter(name, param_type, default, enum_values): return ResourceAgentParameter( @@ -239,6 +242,7 @@ class TestValidateSetClusterProperties(TestCase): sbd_enabled=False, sbd_devices=False, force=False, + valid_value=True, ): self.mock_is_sbd_enabled.return_value = sbd_enabled self.mock_sbd_devices.return_value = ["devices"] if sbd_devices else [] @@ -254,9 +258,13 @@ class TestValidateSetClusterProperties(TestCase): ), expected_report_list, ) - if "stonith-watchdog-timeout" in new_properties and ( - new_properties["stonith-watchdog-timeout"] - or "stonith-watchdog-timeout" in configured_properties + if ( + "stonith-watchdog-timeout" in new_properties + and ( + new_properties["stonith-watchdog-timeout"] + or "stonith-watchdog-timeout" in configured_properties + ) + and valid_value ): self.mock_is_sbd_enabled.assert_called_once_with( self.mock_service_manager @@ -266,7 +274,10 @@ class TestValidateSetClusterProperties(TestCase): if sbd_devices: self.mock_sbd_timeout.assert_not_called() else: - if new_properties["stonith-watchdog-timeout"] in ["", "0"]: + if ( + new_properties["stonith-watchdog-timeout"] + in STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES + ): self.mock_sbd_timeout.assert_not_called() else: self.mock_sbd_timeout.assert_called_once_with() @@ -280,6 +291,8 @@ class TestValidateSetClusterProperties(TestCase): self.mock_sbd_timeout.assert_not_called() self.mock_is_sbd_enabled.reset_mock() + self.mock_sbd_devices.reset_mock() + self.mock_sbd_timeout.reset_mock() def test_no_properties_to_set_or_unset(self): self.assert_validate_set( @@ -328,7 +341,7 @@ class TestValidateSetClusterProperties(TestCase): ) def test_unset_stonith_watchdog_timeout_sbd_disabled(self): - for value in ["0", ""]: + for value in STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES: with self.subTest(value=value): self.assert_validate_set( ["stonith-watchdog-timeout"], @@ -349,22 +362,27 @@ class TestValidateSetClusterProperties(TestCase): ) def test_set_ok_stonith_watchdog_timeout_sbd_enabled_without_devices(self): - self.assert_validate_set( - [], {"stonith-watchdog-timeout": "15"}, [], sbd_enabled=True - ) + for value in ["15", "15s"]: + with self.subTest(value=value): + self.assert_validate_set( + [], + {"stonith-watchdog-timeout": value}, + [], + sbd_enabled=True, + ) def test_set_small_stonith_watchdog_timeout_sbd_enabled_without_devices( self, ): self.assert_validate_set( [], - {"stonith-watchdog-timeout": "9"}, + {"stonith-watchdog-timeout": "9s"}, [ fixture.error( reports.codes.STONITH_WATCHDOG_TIMEOUT_TOO_SMALL, force_code=reports.codes.FORCE, cluster_sbd_watchdog_timeout=10, - entered_watchdog_timeout="9", + entered_watchdog_timeout="9s", ) ], sbd_enabled=True, @@ -387,28 +405,54 @@ class TestValidateSetClusterProperties(TestCase): force=True, ) - def test_set_not_a_number_stonith_watchdog_timeout_sbd_enabled_without_devices( + def _set_invalid_value_stonith_watchdog_timeout( + self, sbd_enabled=False, sbd_devices=False + ): + for value in ["invalid", "10x"]: + with self.subTest(value=value): + self.assert_validate_set( + [], + {"stonith-watchdog-timeout": value}, + [ + fixture.error( + reports.codes.INVALID_OPTION_VALUE, + option_name="stonith-watchdog-timeout", + option_value=value, + allowed_values="time interval (e.g. 1, 2s, 3m, 4h, ...)", + cannot_be_empty=False, + forbidden_characters=None, + ) + ], + sbd_enabled=sbd_enabled, + sbd_devices=sbd_devices, + valid_value=False, + ) + + def test_set_invalid_value_stonith_watchdog_timeout_sbd_enabled_without_devices( self, ): + self._set_invalid_value_stonith_watchdog_timeout( + sbd_enabled=True, sbd_devices=False + ) - self.assert_validate_set( - [], - {"stonith-watchdog-timeout": "invalid"}, - [ - fixture.error( - reports.codes.STONITH_WATCHDOG_TIMEOUT_TOO_SMALL, - force_code=reports.codes.FORCE, - cluster_sbd_watchdog_timeout=10, - entered_watchdog_timeout="invalid", - ) - ], - sbd_enabled=True, + def test_set_invalid_value_stonith_watchdog_timeout_sbd_enabled_with_devices( + self, + ): + self._set_invalid_value_stonith_watchdog_timeout( + sbd_enabled=True, sbd_devices=True + ) + + def test_set_invalid_value_stonith_watchdog_timeout_sbd_disabled( + self, + ): + self._set_invalid_value_stonith_watchdog_timeout( + sbd_enabled=False, sbd_devices=False ) def test_unset_stonith_watchdog_timeout_sbd_enabled_without_devices( self, ): - for value in ["0", ""]: + for value in STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES: with self.subTest(value=value): self.assert_validate_set( ["stonith-watchdog-timeout"], @@ -426,7 +470,7 @@ class TestValidateSetClusterProperties(TestCase): def test_unset_stonith_watchdog_timeout_sbd_enabled_without_devices_forced( self, ): - for value in ["0", ""]: + for value in STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES: with self.subTest(value=value): self.assert_validate_set( ["stonith-watchdog-timeout"], @@ -459,7 +503,7 @@ class TestValidateSetClusterProperties(TestCase): def test_set_stonith_watchdog_timeout_sbd_enabled_with_devices_forced(self): self.assert_validate_set( [], - {"stonith-watchdog-timeout": 15}, + {"stonith-watchdog-timeout": "15s"}, [ fixture.warn( reports.codes.STONITH_WATCHDOG_TIMEOUT_CANNOT_BE_SET, @@ -472,7 +516,7 @@ class TestValidateSetClusterProperties(TestCase): ) def test_unset_stonith_watchdog_timeout_sbd_enabled_with_devices(self): - for value in ["0", ""]: + for value in STONITH_WATCHDOG_TIMEOUT_UNSET_VALUES: with self.subTest(value=value): self.assert_validate_set( ["stonith-watchdog-timeout"], diff --git a/pcs_test/tier1/test_cluster_property.py b/pcs_test/tier1/test_cluster_property.py index 39d70b9d..cb2d8f5c 100644 --- a/pcs_test/tier1/test_cluster_property.py +++ b/pcs_test/tier1/test_cluster_property.py @@ -169,7 +169,7 @@ class TestPropertySet(PropertyMixin, TestCase): def test_set_stonith_watchdog_timeout(self): self.assert_pcs_fail( - "property set stonith-watchdog-timeout=5".split(), + "property set stonith-watchdog-timeout=5s".split(), stderr_full=( "Error: stonith-watchdog-timeout can only be unset or set to 0 " "while SBD is disabled\n" @@ -179,6 +179,18 @@ class TestPropertySet(PropertyMixin, TestCase): ) self.assert_resources_xml_in_cib(UNCHANGED_CRM_CONFIG) + def test_set_stonith_watchdog_timeout_invalid_value(self): + self.assert_pcs_fail( + "property set stonith-watchdog-timeout=5x".split(), + stderr_full=( + "Error: '5x' is not a valid stonith-watchdog-timeout value, use" + " time interval (e.g. 1, 2s, 3m, 4h, ...)\n" + "Error: Errors have occurred, therefore pcs is unable to " + "continue\n" + ), + ) + self.assert_resources_xml_in_cib(UNCHANGED_CRM_CONFIG) + class TestPropertyUnset(PropertyMixin, TestCase): def test_success(self): -- 2.39.0