From bbf53f713189eb2233efa03bf3aa9c96eb79ba82 Mon Sep 17 00:00:00 2001

From: Miroslav Lisik <mlisik@redhat.com>

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