pcs/bz2158790-01-fix-stonith-watchdog-timeout-validation.patch

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