From 0b9ff6ff00394a79e99b1eb89ac5985ace05636b Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Mon, 25 May 2026 19:56:27 +0200 Subject: [PATCH 1/6] impove operations validation in pcs commands * pcs resource update * pcs resource op add --- pcs/lib/cib/resource/operations.py | 33 ++-- pcs/resource.py | 141 ++++++++++-------- .../tier0/lib/cib/test_resource_operations.py | 9 +- .../tier1/cib_resource/test_operation_add.py | 6 +- pcs_test/tier1/legacy/test_resource.py | 71 +++++---- 5 files changed, 153 insertions(+), 107 deletions(-) diff --git a/pcs/lib/cib/resource/operations.py b/pcs/lib/cib/resource/operations.py index a9b9ed382..1d80520fe 100644 --- a/pcs/lib/cib/resource/operations.py +++ b/pcs/lib/cib/resource/operations.py @@ -109,16 +109,16 @@ def prepare( allowed_operation_name_list -- operation names defined by a resource agent allow_invalid -- flag for validation skipping """ - operations_to_validate = _operations_to_normalized(raw_operation_list) + operations_to_validate = operations_to_normalized(raw_operation_list) report_list: ReportItemList = [] report_list.extend( - _validate_operation_list( + validate_operation_list( operations_to_validate, allowed_operation_name_list, allow_invalid ) ) - operation_list = _normalized_to_operations( + operation_list = normalized_to_operations( operations_to_validate, new_role_names_supported ) @@ -146,7 +146,7 @@ def prepare( ] -def _operations_to_normalized( +def operations_to_normalized( raw_operation_list: Iterable[ResourceOperationFilteredIn], ) -> List[validate.TypeOptionNormalizedMap]: return [ @@ -154,7 +154,7 @@ def _operations_to_normalized( ] -def _normalized_to_operations( +def normalized_to_operations( normalized_pairs: Iterable[validate.TypeOptionNormalizedMap], new_role_names_supported: bool, ) -> List[ResourceOperationFilteredOut]: @@ -170,7 +170,7 @@ def _replace_role(op_dict): ] -def _validate_operation_list( +def validate_operation_list( operation_list, allowed_operation_name_list, allow_invalid=False ): severity = reports.item.get_severity(reports.codes.FORCE, allow_invalid) @@ -179,12 +179,21 @@ def _validate_operation_list( validators = [ validate.NamesIn(ATTRIBUTES, option_type=option_type), validate.IsRequiredAll(["name"], option_type=option_type), - validate.ValueIn( - "name", - allowed_operation_name_list, - option_name_for_report="operation name", - severity=severity, - ), + ] + # None means agent metadata failed to load in pcs resource update or + # pcs resource op add - skip name validation for backward compatibility. + # pcs resource create always passes a list - empty for void agent when + # agent loading error is forced. + if allowed_operation_name_list is not None: + validators.append( + validate.ValueIn( + "name", + allowed_operation_name_list, + option_name_for_report="operation name", + severity=severity, + ), + ) + validators += [ validate.ValueIn("role", const.PCMK_ROLES), validate.ValueDeprecated( "role", diff --git a/pcs/resource.py b/pcs/resource.py index 5edafdda6..90c7f027c 100644 --- a/pcs/resource.py +++ b/pcs/resource.py @@ -57,12 +57,8 @@ from pcs.common.pacemaker.resource.operations import ( OCF_CHECK_LEVEL_INSTANCE_ATTRIBUTE_NAME, ) -from pcs.common.str_tools import ( - format_list, - format_list_custom_last_separator, - format_plural, -) -from pcs.lib.cib.resource import guest_node, primitive +from pcs.common.str_tools import format_list, format_plural +from pcs.lib.cib.resource import guest_node, operations, primitive from pcs.lib.cib.tools import get_resources from pcs.lib.commands.resource import ( _get_nodes_to_validate_against, @@ -452,8 +448,37 @@ def resource_op_add(argv: Argv, modifiers: InputModifiers) -> None: if dom is None: dom = utils.get_cib_dom() - # add the requested operation - utils.replace_cib_configuration(resource_operation_add(dom, res_id, argv)) + res_el = utils.dom_get_resource(dom, res_id) + if not res_el: + utils.err("Unable to find resource: %s" % res_id) + + allowed_operation_name_list = None + agent_name = _get_resource_agent_name_from_rsc_el(res_el) + try: + agent_facade = _get_resource_agent_facade(agent_name) + allowed_operation_name_list = [ + op.name for op in agent_facade.metadata.actions + ] + except lib_ra.ResourceAgentError as e: + # Do not fail with an error to keep backward compatibility. + # NOTE: Reconsider when moving operation commands to pcs.lib. + process_library_reports( + [ + lib_ra.resource_agent_error_to_report_item( + e, + reports.ReportItemSeverity.warning(), + ) + ] + ) + + utils.replace_cib_configuration( + resource_operation_add( + dom, + res_id, + argv, + allowed_operation_name_list=allowed_operation_name_list, + ) + ) def op_delete_cmd(lib: Any, argv: Argv, modifiers: InputModifiers) -> None: @@ -1046,10 +1071,13 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None: # noqa: PLR params = utils.convert_args_to_tuples(ra_values) + agent_name = _get_resource_agent_name_from_rsc_el(resource) + allowed_operation_name_list = None try: - agent_facade = _get_resource_agent_facade( - _get_resource_agent_name_from_rsc_el(resource) - ) + agent_facade = _get_resource_agent_facade(agent_name) + allowed_operation_name_list = [ + op.name for op in agent_facade.metadata.actions + ] report_list = primitive.validate_resource_instance_attributes_update( utils.cmd_runner(), agent_facade, @@ -1155,6 +1183,7 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None: # noqa: PLR op_argv, validate_strict=False, before_op=updating_op_before, + allowed_operation_name_list=allowed_operation_name_list, ) utils.replace_cib_configuration(dom) @@ -1238,7 +1267,12 @@ def transform_master_to_clone(master_element): def resource_operation_add( # noqa: PLR0912, PLR0915 - dom, res_id, argv, validate_strict=True, before_op=None + dom, + res_id, + argv, + validate_strict=True, + before_op=None, + allowed_operation_name_list=None, ): """ Commandline options: @@ -1259,36 +1293,6 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 if "=" in op_name: utils.err("%s does not appear to be a valid operation action" % op_name) - if "--force" not in utils.pcs_options: - valid_attrs = [ - "id", - "name", - "interval", - "description", - "start-delay", - "interval-origin", - "timeout", - "enabled", - "record-pending", - "role", - "on-fail", - OCF_CHECK_LEVEL_INSTANCE_ATTRIBUTE_NAME, - ] - for key, value in op_properties: - if key not in valid_attrs: - utils.err( - "%s is not a valid op option (use --force to override)" - % key - ) - if key == "role": - if value not in const.PCMK_ROLES: - utils.err( - "role must be: {} (use --force to override)".format( - format_list_custom_last_separator( - const.PCMK_ROLES, " or " - ) - ) - ) interval = None for key, val in op_properties: @@ -1299,8 +1303,31 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 interval = "60s" if op_name == "monitor" else "0s" op_properties.append(("interval", interval)) - op_properties.sort(key=lambda a: a[0]) - op_properties.insert(0, ("name", op_name)) + op_dict = dict(op_properties) + if "name" in op_dict: + deprecation_warning( + "Specifying an operation name with 'name=' syntax " + "is deprecated and might be removed in a future release. " + "Use the operation name as the first argument instead." + ) + op_dict.setdefault("name", op_name) + + normalized = operations.operations_to_normalized([op_dict]) + report_list = operations.validate_operation_list( + normalized, + allowed_operation_name_list, + allow_invalid="--force" in utils.pcs_options, + ) + if report_list: + process_library_reports(report_list) + + new_role_names_supported = utils.isCibVersionSatisfied( + dom, const.PCMK_NEW_ROLES_CIB_VERSION + ) + op_normalized = operations.normalized_to_operations( + normalized, new_role_names_supported + )[0] + op_properties = sorted(op_normalized.items()) generate_id = True for name, value in op_properties: @@ -1335,26 +1362,16 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 "id", utils.find_unique_id(dom, "-".join((op_id, key, val))) ) attrib_el.appendChild(nvpair_el) - elif key == "role" and "--force" not in utils.pcs_options: - op_el.setAttribute( - key, - pacemaker.role.get_value_for_cib( - val, - utils.isCibVersionSatisfied( - dom, const.PCMK_NEW_ROLES_CIB_VERSION - ), - ), - ) else: op_el.setAttribute(key, val) - operations = res_el.getElementsByTagName("operations") - if not operations: - operations = dom.createElement("operations") - res_el.appendChild(operations) + operations_el = res_el.getElementsByTagName("operations") + if not operations_el: + operations_el = dom.createElement("operations") + res_el.appendChild(operations_el) else: - operations = operations[0] - duplicate_op_list = utils.operation_exists(operations, op_el) + operations_el = operations_el[0] + duplicate_op_list = utils.operation_exists(operations_el, op_el) if duplicate_op_list: utils.err( "operation %s with interval %ss already specified for %s:\n%s" @@ -1369,7 +1386,7 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 ) if validate_strict and "--force" not in utils.pcs_options: duplicate_op_list = utils.operation_exists_by_name( - operations, op_el + operations_el, op_el ) if duplicate_op_list: msg = ( @@ -1389,7 +1406,7 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 ) ) - operations.insertBefore(op_el, before_op) + operations_el.insertBefore(op_el, before_op) return dom diff --git a/pcs_test/tier0/lib/cib/test_resource_operations.py b/pcs_test/tier0/lib/cib/test_resource_operations.py index dc6c4405a..b213fc17a 100644 --- a/pcs_test/tier0/lib/cib/test_resource_operations.py +++ b/pcs_test/tier0/lib/cib/test_resource_operations.py @@ -41,9 +41,9 @@ def _operation_fixture(name, interval="", role=None): @patch_operations("_get_remaining_defaults") @patch_operations("complete_operations_options") @patch_operations("validate_different_intervals") -@patch_operations("_validate_operation_list") -@patch_operations("_normalized_to_operations") -@patch_operations("_operations_to_normalized") +@patch_operations("validate_operation_list") +@patch_operations("normalized_to_operations") +@patch_operations("operations_to_normalized") class Prepare(TestCase): def test_prepare( self, @@ -299,9 +299,8 @@ def test_return_operation_with_normalized_values(self): class ValidateOperation(TestCase): def assert_operation_produces_report(self, operation, report_list): # pylint: disable=no-self-use - # pylint: disable=protected-access assert_report_item_list_equal( - operations._validate_operation_list( + operations.validate_operation_list( [operation], ["monitor"], ), diff --git a/pcs_test/tier1/cib_resource/test_operation_add.py b/pcs_test/tier1/cib_resource/test_operation_add.py index 907590431..0298d5b33 100644 --- a/pcs_test/tier1/cib_resource/test_operation_add.py +++ b/pcs_test/tier1/cib_resource/test_operation_add.py @@ -159,7 +159,9 @@ def test_unknown_option(self): self.assert_pcs_fail( "resource op add R start timeout=30 requires=quorum".split(), ( - "Error: requires is not a valid op option (use --force to " - "override)\n" + "Error: invalid resource operation option 'requires', allowed " + "options are: 'OCF_CHECK_LEVEL', 'description', 'enabled', " + "'id', 'interval', 'interval-origin', 'name', 'on-fail', " + "'record-pending', 'role', 'start-delay', 'timeout'\n" ), ) diff --git a/pcs_test/tier1/legacy/test_resource.py b/pcs_test/tier1/legacy/test_resource.py index 41375ce16..e1231189e 100644 --- a/pcs_test/tier1/legacy/test_resource.py +++ b/pcs_test/tier1/legacy/test_resource.py @@ -1,27 +1,18 @@ # pylint: disable=too-many-lines import json from textwrap import dedent -from unittest import ( - TestCase, - skip, -) +from unittest import TestCase, skip from lxml import etree -from pcs import ( - resource, - utils, -) +from pcs import resource, utils from pcs.common import const -from pcs.common.str_tools import format_list_custom_last_separator +from pcs.common.str_tools import format_list from pcs.constraint import LOCATION_NODE_VALIDATION_SKIP_MSG from pcs_test.tier1.cib_resource.common import ResourceTest from pcs_test.tier1.legacy.common import FIXTURE_UTILIZATION_WARNING -from pcs_test.tools.assertions import ( - AssertPcsMixin, - assert_pcs_status, -) +from pcs_test.tools.assertions import AssertPcsMixin, assert_pcs_status from pcs_test.tools.bin_mock import get_mock_settings from pcs_test.tools.cib import get_assert_pcs_effect_mixin from pcs_test.tools.fixture_cib import ( @@ -979,10 +970,6 @@ def test_add_operation(self): ), ) - self.assert_pcs_success( - "resource op add state monitor interval=15 role=Master --force".split() - ) - self.assert_pcs_success( "resource config state".split(), dedent( @@ -993,8 +980,6 @@ def test_add_operation(self): interval=10s timeout=20s role={const.PCMK_ROLE_PROMOTED_PRIMARY} monitor: state-monitor-interval-11s interval=11s timeout=20s role={const.PCMK_ROLE_UNPROMOTED_PRIMARY} - monitor: state-monitor-interval-15 - interval=15 role={const.PCMK_ROLE_PROMOTED_PRIMARY} """ ), ) @@ -1433,6 +1418,12 @@ def test_update_operation(self): self.assert_pcs_success( "resource update B op monitor interval=100 role=Master".split(), + stderr_full=( + "Deprecation Warning: Value 'Master' of option role is " + "deprecated and might be removed in a future release, " + "therefore it should not be used, use 'Promoted' value " + "instead\n" + ), ) self.assert_pcs_success( @@ -2791,7 +2782,12 @@ def test_op_option(self): self.assert_pcs_fail( "resource update B ocf:pcsmock:minimal op monitor interval=30s blah=blah".split(), - "Error: blah is not a valid op option (use --force to override)\n", + ( + "Error: invalid resource operation option 'blah', allowed " + "options are: 'OCF_CHECK_LEVEL', 'description', 'enabled', " + "'id', 'interval', 'interval-origin', 'name', 'on-fail', " + "'record-pending', 'role', 'start-delay', 'timeout'\n" + ), ) self.assert_pcs_success( @@ -2800,13 +2796,18 @@ def test_op_option(self): self.assert_pcs_fail( "resource op add C monitor interval=30s blah=blah".split(), - "Error: blah is not a valid op option (use --force to override)\n", + ( + "Error: invalid resource operation option 'blah', allowed " + "options are: 'OCF_CHECK_LEVEL', 'description', 'enabled', " + "'id', 'interval', 'interval-origin', 'name', 'on-fail', " + "'record-pending', 'role', 'start-delay', 'timeout'\n" + ), ) self.assert_pcs_fail( "resource op add C monitor interval=60 role=role".split(), - "Error: role must be: {} (use --force to override)\n".format( - format_list_custom_last_separator(const.PCMK_ROLES, " or ") + "Error: 'role' is not a valid role value, use {}\n".format( + format_list(const.PCMK_ROLES) ), ) @@ -2826,15 +2827,33 @@ def test_op_option(self): ), ) - self.assert_pcs_fail( + self.assert_pcs_success( "resource update B op monitor interval=30s monitor interval=31s role=master".split(), - "Error: role must be: {} (use --force to override)\n".format( - format_list_custom_last_separator(const.PCMK_ROLES, " or ") + stderr_full=( + "Deprecation Warning: Value 'master' of option role is " + "deprecated and might be removed in a future release, " + "therefore it should not be used, use 'Promoted' value " + "instead\n" + ), + ) + + self.assert_pcs_fail( + "resource update B op monitor interval=30s monitor interval=31s role=promoted".split(), + stderr_full=( + "Error: operation monitor with interval 31s already specified " + "for B:\n" + "monitor interval=31s role=Master (B-monitor-interval-31s)\n" ), ) self.assert_pcs_success( "resource update B op monitor interval=30s monitor interval=31s role=Master".split(), + stderr_full=( + "Deprecation Warning: Value 'Master' of option role is " + "deprecated and might be removed in a future release, " + "therefore it should not be used, use 'Promoted' value " + "instead\n" + ), ) self.assert_pcs_success( From 0192398e4175302386cca0d342f15ee3646fba88 Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Tue, 26 May 2026 15:35:28 +0200 Subject: [PATCH 2/6] extend test coverage for validate_operation_list --- .../tier0/lib/cib/test_resource_operations.py | 53 ++++++++++++++++++- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/pcs_test/tier0/lib/cib/test_resource_operations.py b/pcs_test/tier0/lib/cib/test_resource_operations.py index b213fc17a..0f1ab098b 100644 --- a/pcs_test/tier0/lib/cib/test_resource_operations.py +++ b/pcs_test/tier0/lib/cib/test_resource_operations.py @@ -297,12 +297,23 @@ def test_return_operation_with_normalized_values(self): class ValidateOperation(TestCase): - def assert_operation_produces_report(self, operation, report_list): + _DEFAULT = object() + + def assert_operation_produces_report( + self, + operation, + report_list, + allowed_operation_name_list=_DEFAULT, + allow_invalid=False, + ): # pylint: disable=no-self-use + if allowed_operation_name_list is self._DEFAULT: + allowed_operation_name_list = ["monitor"] assert_report_item_list_equal( operations.validate_operation_list( [operation], - ["monitor"], + allowed_operation_name_list, + allow_invalid=allow_invalid, ), report_list, ) @@ -520,6 +531,44 @@ def test_return_error_on_invalid_id(self): ], ) + def test_skip_name_validation_when_allowed_list_is_none(self): + self.assert_operation_produces_report( + {"name": "unknown_op"}, [], allowed_operation_name_list=None + ) + + def test_return_error_on_invalid_name_with_empty_allowed_list(self): + self.assert_operation_produces_report( + {"name": "monitor"}, + [ + fixture.error( + report_codes.INVALID_OPTION_VALUE, + force_code=report_codes.FORCE, + option_value="monitor", + option_name="operation name", + allowed_values=[], + cannot_be_empty=False, + forbidden_characters=None, + ), + ], + allowed_operation_name_list=[], + ) + + def test_invalid_name_forced(self): + self.assert_operation_produces_report( + {"name": "unknown_op"}, + [ + fixture.warn( + report_codes.INVALID_OPTION_VALUE, + option_value="unknown_op", + option_name="operation name", + allowed_values=["monitor"], + cannot_be_empty=False, + forbidden_characters=None, + ), + ], + allow_invalid=True, + ) + class GetRemainingDefaults(TestCase): def test_success(self): From 855b932359fd561eae3f5f8a1eed7e38f569442c Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Wed, 27 May 2026 10:17:18 +0200 Subject: [PATCH 3/6] add tier1 tests for `pcs resource update ... op` --- pcs_test/tier1/cib_resource/test_update.py | 417 +++++++++++++++++++++ 1 file changed, 417 insertions(+) diff --git a/pcs_test/tier1/cib_resource/test_update.py b/pcs_test/tier1/cib_resource/test_update.py index 7a836c5f6..898ff283c 100644 --- a/pcs_test/tier1/cib_resource/test_update.py +++ b/pcs_test/tier1/cib_resource/test_update.py @@ -8,6 +8,7 @@ from pcs_test.tools.misc import ( get_test_resource, get_tmp_file, + skip_unless_pacemaker_supports_op_onfail_demote, write_data_to_tmpfile, ) from pcs_test.tools.pcs_runner import PcsRunner @@ -202,3 +203,419 @@ class ResourceMetaClone(ResourceMetaPrimitive): class ResourceMetaBundle(ResourceMetaPrimitive): rsc_id = "B" resource_fixture = staticmethod(fixture_bundle) + + +def fixture_instance_attrs(parent_id, *nvpairs): + nvpairs_xml = "\n".join( + f'' + for name, value in nvpairs + ) + return dedent( + f"""\ + + {nvpairs_xml} + """ + ) + + +def fixture_op( # noqa: PLR0913 + op_id, + name, + interval, + description=None, + enabled=None, + inner_xml="", + interval_origin=None, + on_fail=None, + record_pending=None, + role=None, + start_delay=None, + timeout=None, +): + attrs = [ + f'id="{op_id}"', + f'interval="{interval}"', + f'name="{name}"', + ] + if description: + attrs.append(f'description="{description}"') + if enabled: + attrs.append(f'enabled="{enabled}"') + if interval_origin: + attrs.append(f'interval-origin="{interval_origin}"') + if on_fail: + attrs.append(f'on-fail="{on_fail}"') + if record_pending: + attrs.append(f'record-pending="{record_pending}"') + if role: + attrs.append(f'role="{role}"') + if start_delay: + attrs.append(f'start-delay="{start_delay}"') + if timeout: + attrs.append(f'timeout="{timeout}"') + if inner_xml: + return f"\n{inner_xml}\n" + return f"" + + +def fixture_operations(*ops_xml): + ops = "\n".join(ops_xml) + return f""" + + {ops} + + """ + + +FIXTURE_EXISTING_OP_MONITOR = fixture_op( + "R-monitor-interval-10s", + "monitor", + "10s", + timeout="20s", + on_fail="restart", + enabled="1", +) +FIXTURE_EXISTING_OP_RELOAD = fixture_op( + "R-restart-interval-10s", "reload", "10s", timeout="20s", role="Started" +) +FIXTURE_EXISTING_OP_CIB = fixture_resources( + fixture_primitive( + "R", + inner_xml=fixture_operations( + FIXTURE_EXISTING_OP_MONITOR, + FIXTURE_EXISTING_OP_RELOAD, + ), + ) +) + + +class ResourceUpdateOperations( + TestCase, get_assert_pcs_effect_mixin(get_cib_resources) +): + def setUp(self): + self.temp_cib = get_tmp_file("tier1_test_resource_update_operations") + self.pcs_runner = PcsRunner(self.temp_cib.name) + self.pcs_runner.mock_settings = get_mock_settings() + write_data_to_tmpfile( + modify_cib_file( + get_test_resource("cib-empty.xml"), + resources=FIXTURE_EXISTING_OP_CIB, + ), + self.temp_cib, + ) + + def tearDown(self): + self.temp_cib.close() + + def _fixture_primitive_with_ops(self, *ops_xml): + return fixture_resources( + fixture_primitive("R", inner_xml=fixture_operations(*ops_xml)) + ) + + def test_no_op_args(self): + self.assert_effect( + "resource update R op".split(), + FIXTURE_EXISTING_OP_CIB, + ) + + def test_op_name_only(self): + self.assert_effect( + "resource update R op monitRO".split(), + FIXTURE_EXISTING_OP_CIB, + ) + + def test_add_single_operation(self): + self.assert_effect( + "resource update R op start interval=20s".split(), + self._fixture_primitive_with_ops( + FIXTURE_EXISTING_OP_MONITOR, + FIXTURE_EXISTING_OP_RELOAD, + fixture_op("R-start-interval-20s", "start", "20s"), + ), + ) + + def test_update_existing_operation_not_specified_options_are_deleted(self): + self.assert_effect( + ( + "resource update R op monitor interval=10s timeout=30s " + "description=desc" + ).split(), + self._fixture_primitive_with_ops( + fixture_op( + "R-monitor-interval-10s", + "monitor", + "10s", + timeout="30s", + description="desc", + ), + FIXTURE_EXISTING_OP_RELOAD, + ), + ) + + def test_update_existing_operation_add_role_fails(self): + self.assert_pcs_fail( + ( + "resource update R op monitor interval=10s timeout=20s " + "role=Started" + ).split(), + ( + "Error: operation monitor with interval 10s already specified" + " for R:\n" + "monitor enabled=1 interval=10s on-fail=restart timeout=20s " + "(R-monitor-interval-10s)\n" + ), + ) + self.assert_resources_xml_in_cib(FIXTURE_EXISTING_OP_CIB) + + def test_update_existing_op_with_role_without_specifying_role_fails(self): + self.assert_pcs_fail( + ( + "resource update R op reload interval=10s timeout=30s " + "description=desc" + ).split(), + ( + "Error: operation reload with interval 10s already specified" + " for R:\n" + "reload interval=10s role=Started timeout=20s " + "(R-restart-interval-10s)\n" + ), + ) + self.assert_resources_xml_in_cib(FIXTURE_EXISTING_OP_CIB) + + def test_update_existing_operation_with_role_success(self): + self.assert_effect( + ( + "resource update R op reload interval=10s timeout=30s " + "role=Started description=desc" + ).split(), + self._fixture_primitive_with_ops( + FIXTURE_EXISTING_OP_MONITOR, + fixture_op( + "R-reload-interval-10s", + "reload", + "10s", + timeout="30s", + role="Started", + description="desc", + ), + ), + ) + + def test_update_existing_add_new(self): + self.assert_effect( + ( + "resource update R op monitor interval=5s start interval=0 " + "timeout=20 " + ).split(), + self._fixture_primitive_with_ops( + fixture_op("R-monitor-interval-5s", "monitor", "5s"), + FIXTURE_EXISTING_OP_RELOAD, + fixture_op("R-start-interval-0", "start", "0", timeout="20"), + ), + ) + + def test_same_operation_different_interval_same_role(self): + self.assert_effect( + ( + "resource update R op meta-data interval=10 timeout=20 " + "enabled=1 meta-data interval=20 timeout=30" + ).split(), + self._fixture_primitive_with_ops( + FIXTURE_EXISTING_OP_MONITOR, + FIXTURE_EXISTING_OP_RELOAD, + fixture_op( + "R-meta-data-interval-20", "meta-data", "20", timeout="30" + ), + ), + ) + + def test_same_operation_different_interval_different_role(self): + self.assert_effect( + ( + "resource update R op meta-data interval=10 timeout=20 " + "enabled=1 meta-data interval=20 timeout=30 role=Stopped" + ).split(), + self._fixture_primitive_with_ops( + FIXTURE_EXISTING_OP_MONITOR, + FIXTURE_EXISTING_OP_RELOAD, + fixture_op( + "R-meta-data-interval-10", + "meta-data", + "10", + timeout="20", + enabled="1", + ), + fixture_op( + "R-meta-data-interval-20", + "meta-data", + "20", + timeout="30", + role="Stopped", + ), + ), + ) + + def test_duplicate_op_id(self): + self.assert_pcs_fail_regardless_of_force( + "resource update R op monitor interval=30 id=R".split(), + "Error: id 'R' is already in use, please specify another one\n", + ) + self.assert_resources_xml_in_cib(FIXTURE_EXISTING_OP_CIB) + + def _invalid_operations(self, force=False): + forceable = "Warning" if force else "Error" + force_opt = "--force" if force else "" + use_force = ", use --force to override" if not force else "" + + self.assert_pcs_fail( + ( + "resource update R op monitor interval=5s " + "status id=ab#cd enabled=invalid-bool interval=invalid-number " + "interval-origin=value name=status on-fail=invalid-on-fail " + "record-pending=invalid-bool role=invalid-role " + f"start-delay=value timeout=invalid-timeout {force_opt}" + ).split(), + ( + "Deprecation Warning: Specifying an operation name with " + "'name=' syntax is deprecated and might be removed in a " + "future release. Use the operation name as the first argument " + "instead.\n" + f"{forceable}: 'status' is not a valid operation name value, " + "use 'meta-data', 'migrate_from', 'migrate_to', 'monitor', " + "'reload', 'reload-agent', 'start', 'stop', 'validate-all'" + f"{use_force}\n" + "Error: 'invalid-role' is not a valid role value, use " + "'Master', 'Promoted', 'Slave', 'Started', 'Stopped', " + "'Unpromoted'\n" + "Error: 'invalid-on-fail' is not a valid on-fail value, use " + "'block', 'demote', 'fence', 'ignore', 'restart', " + "'restart-container', 'standby', 'stop'\n" + "Error: 'invalid-bool' is not a valid record-pending value, " + "use a pacemaker boolean value: '0', '1', 'false', 'n', 'no', " + "'off', 'on', 'true', 'y', 'yes'\n" + "Error: 'invalid-bool' is not a valid enabled value, use a " + "pacemaker boolean value: '0', '1', 'false', 'n', 'no', 'off', " + "'on', 'true', 'y', 'yes'\n" + "Error: Only one of resource operation options " + "'interval-origin' and 'start-delay' can be used\n" + "Error: invalid operation id 'ab#cd', '#' is not a valid " + "character for a operation id\n" + "Error: 'invalid-number' is not a valid interval value, use " + "time interval (e.g. 1, 2s, 3m, 4h, ...)\n" + "Error: 'invalid-timeout' is not a valid timeout value, use " + "time interval (e.g. 1, 2s, 3m, 4h, ...)\n" + ), + ) + self.assert_resources_xml_in_cib(FIXTURE_EXISTING_OP_CIB) + + def test_invalid_operations(self): + self._invalid_operations(force=False) + + def test_invalid_operations_force(self): + self._invalid_operations(force=True) + + def test_add_operation_with_ocf_check_level(self): + self.assert_effect( + "resource update R op monitor OCF_CHECK_LEVEL=1".split(), + self._fixture_primitive_with_ops( + fixture_op( + "R-monitor-interval-60s", + "monitor", + "60s", + inner_xml=fixture_instance_attrs( + "R-monitor-interval-60s", + ("OCF_CHECK_LEVEL", "1"), + ), + ), + FIXTURE_EXISTING_OP_RELOAD, + ), + ) + + @skip_unless_pacemaker_supports_op_onfail_demote() + def test_add_operation_onfail_demote_upgrade_cib(self): + write_data_to_tmpfile( + modify_cib_file( + get_test_resource("cib-empty-3.3.xml"), + resources=FIXTURE_EXISTING_OP_CIB, + ), + self.temp_cib, + ) + self.assert_effect( + "resource update R op start on-fail=demote".split(), + self._fixture_primitive_with_ops( + FIXTURE_EXISTING_OP_MONITOR, + FIXTURE_EXISTING_OP_RELOAD, + fixture_op( + "R-start-interval-0s", + "start", + "0s", + on_fail="demote", + ), + ), + stderr_full="Cluster CIB has been upgraded to latest version\n", + ) + + def test_duplicate_operation_same_interval(self): + fixture_cib = fixture_resources( + fixture_primitive( + "R", + inner_xml=fixture_operations( + fixture_op( + "R-monitor-interval-10", + "monitor", + "10", + ), + fixture_op( + "R-monitor-interval-20", + "monitor", + "20", + ), + ), + ) + ) + write_data_to_tmpfile( + modify_cib_file( + get_test_resource("cib-empty.xml"), + resources=fixture_cib, + ), + self.temp_cib, + ) + self.assert_pcs_fail( + "resource update R op monitor interval=20".split(), + ( + "Error: operation monitor with interval 20s already specified" + " for R:\n" + "monitor interval=20 (R-monitor-interval-20)\n" + ), + ) + self.assert_resources_xml_in_cib(fixture_cib) + + +class ResourceUpdateCloneOperations( + TestCase, get_assert_pcs_effect_mixin(get_cib_resources) +): + def setUp(self): + self.temp_cib = get_tmp_file( + "tier1_test_resource_update_clone_operations" + ) + self.pcs_runner = PcsRunner(self.temp_cib.name) + self.pcs_runner.mock_settings = get_mock_settings() + write_data_to_tmpfile( + modify_cib_file( + get_test_resource("cib-empty.xml"), + resources=fixture_resources(fixture_clone("R-clone")), + ), + self.temp_cib, + ) + + def tearDown(self): + self.temp_cib.close() + + def test_clone_rejects_op_keyword(self): + self.assert_pcs_fail( + "resource update R-clone op monitor interval=20s".split(), + stderr_full=( + "Error: op settings must be changed on base resource," + " not the clone\n" + ), + ) From 206c12924f2b558c1e3fdeafd3a263996807f99c Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Thu, 28 May 2026 14:25:13 +0200 Subject: [PATCH 4/6] extend tier1 tests for `pcs resource op add` --- .../tier1/cib_resource/test_operation_add.py | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/pcs_test/tier1/cib_resource/test_operation_add.py b/pcs_test/tier1/cib_resource/test_operation_add.py index 0298d5b33..224964e60 100644 --- a/pcs_test/tier1/cib_resource/test_operation_add.py +++ b/pcs_test/tier1/cib_resource/test_operation_add.py @@ -165,3 +165,54 @@ def test_unknown_option(self): "'record-pending', 'role', 'start-delay', 'timeout'\n" ), ) + + def _invalid_operations(self, force=False): + forceable = "Warning" if force else "Error" + force_opt = "--force" if force else "" + use_force = ", use --force to override" if not force else "" + + self.assert_pcs_fail( + ( + "resource op add R status " + "id=ab#cd enabled=invalid-bool interval=invalid-number " + "interval-origin=value name=status on-fail=invalid-on-fail " + "record-pending=invalid-bool role=invalid-role " + f"start-delay=value timeout=invalid-timeout {force_opt}" + ).split(), + ( + "Deprecation Warning: Specifying an operation name with " + "'name=' syntax is deprecated and might be removed in a " + "future release. Use the operation name as the first argument " + "instead.\n" + f"{forceable}: 'status' is not a valid operation name value, " + "use 'meta-data', 'migrate_from', 'migrate_to', 'monitor', " + "'reload', 'reload-agent', 'start', 'stop', 'validate-all'" + f"{use_force}\n" + "Error: 'invalid-role' is not a valid role value, use " + "'Master', 'Promoted', 'Slave', 'Started', 'Stopped', " + "'Unpromoted'\n" + "Error: 'invalid-on-fail' is not a valid on-fail value, use " + "'block', 'demote', 'fence', 'ignore', 'restart', " + "'restart-container', 'standby', 'stop'\n" + "Error: 'invalid-bool' is not a valid record-pending value, " + "use a pacemaker boolean value: '0', '1', 'false', 'n', 'no', " + "'off', 'on', 'true', 'y', 'yes'\n" + "Error: 'invalid-bool' is not a valid enabled value, use a " + "pacemaker boolean value: '0', '1', 'false', 'n', 'no', 'off', " + "'on', 'true', 'y', 'yes'\n" + "Error: Only one of resource operation options " + "'interval-origin' and 'start-delay' can be used\n" + "Error: invalid operation id 'ab#cd', '#' is not a valid " + "character for a operation id\n" + "Error: 'invalid-number' is not a valid interval value, use " + "time interval (e.g. 1, 2s, 3m, 4h, ...)\n" + "Error: 'invalid-timeout' is not a valid timeout value, use " + "time interval (e.g. 1, 2s, 3m, 4h, ...)\n" + ), + ) + + def test_invalid_operations(self): + self._invalid_operations(force=False) + + def test_invalid_operations_force(self): + self._invalid_operations(force=True) From f1e39de291f6949764a9a767503c0c7b6faf3c06 Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Thu, 28 May 2026 22:07:52 +0200 Subject: [PATCH 5/6] update changelog and capabilities --- CHANGELOG.md | 5 +++++ pcsd/capabilities.xml.in | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7239ee33a..46237e721 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,14 @@ - `pcs constraint config` (and its variants for each constraint type) now list resources in sets in the order defined in the CIB, instead of sorting them alphabetically ([rhbz#2461143]) ([RHEL-176478]) +- Add validation for operations in `pcs resource update`, `pcs resource op add`, + `pcs stonith update`, and `pcs stonith op add commands`. Operation names are + now validated against agent metadata, and operation option values are also + verified ([RHEL-176268]) [RHEL-149172]: https://redhat.atlassian.net/browse/RHEL-149172 +[RHEL-176268]: https://redhat.atlassian.net/browse/RHEL-176268 [RHEL-176478]: https://redhat.atlassian.net/browse/RHEL-176478 [rhbz#2461143]: https://bugzilla.redhat.com/show_bug.cgi?id=2461143 diff --git a/pcsd/capabilities.xml.in b/pcsd/capabilities.xml.in index 6ef23ea91..e72facb24 100644 --- a/pcsd/capabilities.xml.in +++ b/pcsd/capabilities.xml.in @@ -1650,6 +1650,17 @@ pcs commands: resource op add + + + Validation of operation name based on agent metadata and options values. + + pcs commands: + resource update + resource op add + stonith update + stonith op add + + From 17d2eb6107cc0fc5bdfc5dcc337ca361847e4541 Mon Sep 17 00:00:00 2001 From: Miroslav Lisik Date: Fri, 29 May 2026 17:04:40 +0200 Subject: [PATCH 6/6] fixes based on review --- CHANGELOG.md | 2 +- pcs/lib/cib/resource/operations.py | 50 ++++++++++++-------------- pcs/resource.py | 34 +++++++++--------- pcs_test/tier1/legacy/test_resource.py | 14 ++++++++ pcsd/capabilities.xml.in | 11 ------ 5 files changed, 56 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46237e721..e02cb2163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ resources in sets in the order defined in the CIB, instead of sorting them alphabetically ([rhbz#2461143]) ([RHEL-176478]) - Add validation for operations in `pcs resource update`, `pcs resource op add`, - `pcs stonith update`, and `pcs stonith op add commands`. Operation names are + `pcs stonith update`, and `pcs stonith op add` commands. Operation names are now validated against agent metadata, and operation option values are also verified ([RHEL-176268]) diff --git a/pcs/lib/cib/resource/operations.py b/pcs/lib/cib/resource/operations.py index 1d80520fe..b849d703c 100644 --- a/pcs/lib/cib/resource/operations.py +++ b/pcs/lib/cib/resource/operations.py @@ -1,34 +1,18 @@ from collections import defaultdict from dataclasses import replace as dt_replace -from typing import ( - Iterable, - List, - Optional, - Tuple, - cast, -) +from typing import Iterable, List, Optional, Tuple, cast from lxml import etree from lxml.etree import _Element -from pcs.common import ( - const, - pacemaker, - reports, -) +from pcs.common import const, pacemaker, reports from pcs.common.pacemaker.resource.operations import CibResourceOperationDto -from pcs.common.reports import ( - ReportItemList, - ReportProcessor, -) +from pcs.common.reports import ReportItemList, ReportProcessor from pcs.common.reports.item import ReportItem from pcs.common.tools import timeout_to_seconds from pcs.common.types import StringCollection from pcs.lib import validate -from pcs.lib.cib import ( - nvpair_multi, - rule, -) +from pcs.lib.cib import nvpair_multi, rule from pcs.lib.cib.nvpair import append_new_instance_attributes from pcs.lib.cib.resource.agent import ( complete_operations_options, @@ -171,19 +155,31 @@ def _replace_role(op_dict): def validate_operation_list( - operation_list, allowed_operation_name_list, allow_invalid=False -): + operation_list: Iterable[validate.TypeOptionNormalizedMap], + allowed_operation_name_list: Optional[StringCollection], + allow_invalid: bool = False, +) -> ReportItemList: + """ + Validate given operation list and return a list of report items. + + operation_list -- operations with normalized option names and values + allowed_operation_name_list -- operation names defined by a resource agent, + or None if agent metadata failed to load (in pcs resource update or + pcs resource op add). When None, operation name validation is skipped + for backward compatibility - the user should still be able to configure + operations even when the agent metadata is unavailable. + In pcs resource create, a list is always passed (empty for a void agent + when an agent loading error is forced). + allow_invalid -- if True, downgrade unknown operation name errors to + warnings (i.e. allow forcing) + """ severity = reports.item.get_severity(reports.codes.FORCE, allow_invalid) option_type = "resource operation" - validators = [ + validators: list[validate.ValidatorInterface] = [ validate.NamesIn(ATTRIBUTES, option_type=option_type), validate.IsRequiredAll(["name"], option_type=option_type), ] - # None means agent metadata failed to load in pcs resource update or - # pcs resource op add - skip name validation for backward compatibility. - # pcs resource create always passes a list - empty for void agent when - # agent loading error is forced. if allowed_operation_name_list is not None: validators.append( validate.ValueIn( diff --git a/pcs/resource.py b/pcs/resource.py index 90c7f027c..e946efd11 100644 --- a/pcs/resource.py +++ b/pcs/resource.py @@ -1129,12 +1129,12 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None: # noqa: PLR resource, utils.convert_args_to_tuples(meta_values) ) - operations = resource.getElementsByTagName("operations") - if not operations: - operations = dom.createElement("operations") - resource.appendChild(operations) + operations_el = resource.getElementsByTagName("operations") + if not operations_el: + operations_el = dom.createElement("operations") + resource.appendChild(operations_el) else: - operations = operations[0] + operations_el = operations_el[0] get_role = partial( pacemaker.role.get_value_for_cib, @@ -1165,7 +1165,7 @@ def resource_update(args: Argv, modifiers: InputModifiers) -> None: # noqa: PLR updating_op = None updating_op_before = None - for existing_op in operations.getElementsByTagName("op"): + for existing_op in operations_el.getElementsByTagName("op"): if updating_op: updating_op_before = existing_op break @@ -1294,23 +1294,16 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 if "=" in op_name: utils.err("%s does not appear to be a valid operation action" % op_name) - interval = None - for key, val in op_properties: - if key == "interval": - interval = val - break - if not interval: - interval = "60s" if op_name == "monitor" else "0s" - op_properties.append(("interval", interval)) - op_dict = dict(op_properties) if "name" in op_dict: + # deprecated since pcs-0.12.3 deprecation_warning( "Specifying an operation name with 'name=' syntax " "is deprecated and might be removed in a future release. " "Use the operation name as the first argument instead." ) - op_dict.setdefault("name", op_name) + else: + op_dict["name"] = op_name normalized = operations.operations_to_normalized([op_dict]) report_list = operations.validate_operation_list( @@ -1329,6 +1322,15 @@ def resource_operation_add( # noqa: PLR0912, PLR0915 )[0] op_properties = sorted(op_normalized.items()) + interval = None + for key, val in op_properties: + if key == "interval": + interval = val + break + if not interval: + interval = "60s" if op_name == "monitor" else "0s" + op_properties.append(("interval", interval)) + generate_id = True for name, value in op_properties: if name == "id": diff --git a/pcs_test/tier1/legacy/test_resource.py b/pcs_test/tier1/legacy/test_resource.py index e1231189e..d24a36793 100644 --- a/pcs_test/tier1/legacy/test_resource.py +++ b/pcs_test/tier1/legacy/test_resource.py @@ -970,6 +970,18 @@ def test_add_operation(self): ), ) + self.assert_pcs_success( + ( + "resource op add state monitor interval=15 role=Master --force" + ).split(), + stderr_full=( + "Deprecation Warning: Value 'Master' of option role is " + "deprecated and might be removed in a future release, " + "therefore it should not be used, use 'Promoted' value " + "instead\n" + ), + ) + self.assert_pcs_success( "resource config state".split(), dedent( @@ -980,6 +992,8 @@ def test_add_operation(self): interval=10s timeout=20s role={const.PCMK_ROLE_PROMOTED_PRIMARY} monitor: state-monitor-interval-11s interval=11s timeout=20s role={const.PCMK_ROLE_UNPROMOTED_PRIMARY} + monitor: state-monitor-interval-15 + interval=15 role={const.PCMK_ROLE_PROMOTED_PRIMARY} """ ), ) diff --git a/pcsd/capabilities.xml.in b/pcsd/capabilities.xml.in index e72facb24..6ef23ea91 100644 --- a/pcsd/capabilities.xml.in +++ b/pcsd/capabilities.xml.in @@ -1650,17 +1650,6 @@ pcs commands: resource op add - - - Validation of operation name based on agent metadata and options values. - - pcs commands: - resource update - resource op add - stonith update - stonith op add - -