From e292dd4de2504da09901133fdab7ace5a97f9d73 Mon Sep 17 00:00:00 2001 From: Ondrej Mular Date: Wed, 7 Dec 2022 11:33:25 +0100 Subject: [PATCH 2/3] add warning when updating a misconfigured resource --- CHANGELOG.md | 8 ++ pcs/common/reports/codes.py | 3 + pcs/common/reports/messages.py | 19 +++++ pcs/lib/cib/resource/primitive.py | 84 ++++++++++++++----- pcs/lib/pacemaker/live.py | 38 ++------- .../tier0/common/reports/test_messages.py | 16 ++++ .../cib/resource/test_primitive_validate.py | 56 +++++++------ pcs_test/tier0/lib/pacemaker/test_live.py | 78 +++++------------ pcs_test/tier1/legacy/test_stonith.py | 5 +- 9 files changed, 169 insertions(+), 138 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7927eae6..ed2083af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,17 @@ ## [Unreleased] +### Added +- Warning to `pcs resource|stonith update` commands about not using agent + self-validation feature when the resource is already misconfigured + ([rhbz#2151524]) + ### Fixed - Graceful stopping pcsd service using `systemctl stop pcsd` command +[rhbz#2151524]: https://bugzilla.redhat.com/show_bug.cgi?id=2151524 + + ## [0.11.4] - 2022-11-21 ### Security diff --git a/pcs/common/reports/codes.py b/pcs/common/reports/codes.py index 76963733..90609f47 100644 --- a/pcs/common/reports/codes.py +++ b/pcs/common/reports/codes.py @@ -44,6 +44,9 @@ AGENT_NAME_GUESS_FOUND_MORE_THAN_ONE = M("AGENT_NAME_GUESS_FOUND_MORE_THAN_ONE") AGENT_NAME_GUESS_FOUND_NONE = M("AGENT_NAME_GUESS_FOUND_NONE") AGENT_NAME_GUESSED = M("AGENT_NAME_GUESSED") AGENT_SELF_VALIDATION_INVALID_DATA = M("AGENT_SELF_VALIDATION_INVALID_DATA") +AGENT_SELF_VALIDATION_SKIPPED_UPDATED_RESOURCE_MISCONFIGURED = M( + "AGENT_SELF_VALIDATION_SKIPPED_UPDATED_RESOURCE_MISCONFIGURED" +) AGENT_SELF_VALIDATION_RESULT = M("AGENT_SELF_VALIDATION_RESULT") BAD_CLUSTER_STATE_FORMAT = M("BAD_CLUSTER_STATE_FORMAT") BOOTH_ADDRESS_DUPLICATION = M("BOOTH_ADDRESS_DUPLICATION") diff --git a/pcs/common/reports/messages.py b/pcs/common/reports/messages.py index ba748eb2..fbc4de62 100644 --- a/pcs/common/reports/messages.py +++ b/pcs/common/reports/messages.py @@ -7494,6 +7494,25 @@ class AgentSelfValidationInvalidData(ReportItemMessage): return f"Invalid validation data from agent: {self.reason}" +@dataclass(frozen=True) +class AgentSelfValidationSkippedUpdatedResourceMisconfigured(ReportItemMessage): + """ + Agent self validation is skipped when updating a resource as it is + misconfigured in its current state. + """ + + result: str + _code = codes.AGENT_SELF_VALIDATION_SKIPPED_UPDATED_RESOURCE_MISCONFIGURED + + @property + def message(self) -> str: + return ( + "The resource was misconfigured before the update, therefore agent " + "self-validation will not be run for the updated configuration. " + "Validation output of the original configuration:\n{result}" + ).format(result="\n".join(indent(self.result.splitlines()))) + + @dataclass(frozen=True) class ResourceCloneIncompatibleMetaAttributes(ReportItemMessage): """ diff --git a/pcs/lib/cib/resource/primitive.py b/pcs/lib/cib/resource/primitive.py index d9940e9d..f9d52b9b 100644 --- a/pcs/lib/cib/resource/primitive.py +++ b/pcs/lib/cib/resource/primitive.py @@ -357,6 +357,31 @@ def _is_ocf_or_stonith_agent(resource_agent_name: ResourceAgentName) -> bool: return resource_agent_name.standard in ("stonith", "ocf") +def _get_report_from_agent_self_validation( + is_valid: Optional[bool], + reason: str, + report_severity: reports.ReportItemSeverity, +) -> reports.ReportItemList: + report_items = [] + if is_valid is None: + report_items.append( + reports.ReportItem( + report_severity, + reports.messages.AgentSelfValidationInvalidData(reason), + ) + ) + elif not is_valid or reason: + if is_valid: + report_severity = reports.ReportItemSeverity.warning() + report_items.append( + reports.ReportItem( + report_severity, + reports.messages.AgentSelfValidationResult(reason), + ) + ) + return report_items + + def validate_resource_instance_attributes_create( cmd_runner: CommandRunner, resource_agent: ResourceAgentFacade, @@ -405,16 +430,16 @@ def validate_resource_instance_attributes_create( for report_item in report_items ) ): - ( - dummy_is_valid, - agent_validation_reports, - ) = validate_resource_instance_attributes_via_pcmk( - cmd_runner, - agent_name, - instance_attributes, - reports.get_severity(reports.codes.FORCE, force), + report_items.extend( + _get_report_from_agent_self_validation( + *validate_resource_instance_attributes_via_pcmk( + cmd_runner, + agent_name, + instance_attributes, + ), + reports.get_severity(reports.codes.FORCE, force), + ) ) - report_items.extend(agent_validation_reports) return report_items @@ -508,25 +533,40 @@ def validate_resource_instance_attributes_update( ) ): ( - is_valid, - dummy_reports, + original_is_valid, + original_reason, ) = validate_resource_instance_attributes_via_pcmk( cmd_runner, agent_name, current_instance_attrs, - reports.ReportItemSeverity.error(), ) - if is_valid: - ( - dummy_is_valid, - agent_validation_reports, - ) = validate_resource_instance_attributes_via_pcmk( - cmd_runner, - resource_agent.metadata.name, - final_attrs, - reports.get_severity(reports.codes.FORCE, force), + if original_is_valid: + report_items.extend( + _get_report_from_agent_self_validation( + *validate_resource_instance_attributes_via_pcmk( + cmd_runner, + resource_agent.metadata.name, + final_attrs, + ), + reports.get_severity(reports.codes.FORCE, force), + ) + ) + elif original_is_valid is None: + report_items.append( + reports.ReportItem.warning( + reports.messages.AgentSelfValidationInvalidData( + original_reason + ) + ) + ) + else: + report_items.append( + reports.ReportItem.warning( + reports.messages.AgentSelfValidationSkippedUpdatedResourceMisconfigured( + original_reason + ) + ) ) - report_items.extend(agent_validation_reports) return report_items diff --git a/pcs/lib/pacemaker/live.py b/pcs/lib/pacemaker/live.py index 6dab613e..fb1e0a4a 100644 --- a/pcs/lib/pacemaker/live.py +++ b/pcs/lib/pacemaker/live.py @@ -884,8 +884,7 @@ def _validate_stonith_instance_attributes_via_pcmk( cmd_runner: CommandRunner, agent_name: ResourceAgentName, instance_attributes: Mapping[str, str], - not_valid_severity: reports.ReportItemSeverity, -) -> tuple[Optional[bool], reports.ReportItemList]: +) -> tuple[Optional[bool], str]: cmd = [ settings.stonith_admin, "--validate", @@ -899,7 +898,6 @@ def _validate_stonith_instance_attributes_via_pcmk( cmd, "./validate/command/output", instance_attributes, - not_valid_severity, ) @@ -907,8 +905,7 @@ def _validate_resource_instance_attributes_via_pcmk( cmd_runner: CommandRunner, agent_name: ResourceAgentName, instance_attributes: Mapping[str, str], - not_valid_severity: reports.ReportItemSeverity, -) -> tuple[Optional[bool], reports.ReportItemList]: +) -> tuple[Optional[bool], str]: cmd = [ settings.crm_resource_binary, "--validate", @@ -926,7 +923,6 @@ def _validate_resource_instance_attributes_via_pcmk( cmd, "./resource-agent-action/command/output", instance_attributes, - not_valid_severity, ) @@ -935,8 +931,7 @@ def _handle_instance_attributes_validation_via_pcmk( cmd: StringSequence, data_xpath: str, instance_attributes: Mapping[str, str], - not_valid_severity: reports.ReportItemSeverity, -) -> tuple[Optional[bool], reports.ReportItemList]: +) -> tuple[Optional[bool], str]: full_cmd = list(cmd) for key, value in sorted(instance_attributes.items()): full_cmd.extend(["--option", f"{key}={value}"]) @@ -945,12 +940,7 @@ def _handle_instance_attributes_validation_via_pcmk( # dom = _get_api_result_dom(stdout) dom = xml_fromstring(stdout) except (etree.XMLSyntaxError, etree.DocumentInvalid) as e: - return None, [ - reports.ReportItem( - not_valid_severity, - reports.messages.AgentSelfValidationInvalidData(str(e)), - ) - ] + return None, str(e) result = "\n".join( "\n".join( line.strip() for line in item.text.split("\n") if line.strip() @@ -958,38 +948,22 @@ def _handle_instance_attributes_validation_via_pcmk( for item in dom.iterfind(data_xpath) if item.get("source") == "stderr" and item.text ).strip() - if return_value == 0: - if result: - return True, [ - reports.ReportItem.warning( - reports.messages.AgentSelfValidationResult(result) - ) - ] - return True, [] - return False, [ - reports.ReportItem( - not_valid_severity, - reports.messages.AgentSelfValidationResult(result), - ) - ] + return return_value == 0, result def validate_resource_instance_attributes_via_pcmk( cmd_runner: CommandRunner, resource_agent_name: ResourceAgentName, instance_attributes: Mapping[str, str], - not_valid_severity: reports.ReportItemSeverity, -) -> tuple[Optional[bool], reports.ReportItemList]: +) -> tuple[Optional[bool], str]: if resource_agent_name.is_stonith: return _validate_stonith_instance_attributes_via_pcmk( cmd_runner, resource_agent_name, instance_attributes, - not_valid_severity, ) return _validate_resource_instance_attributes_via_pcmk( cmd_runner, resource_agent_name, instance_attributes, - not_valid_severity, ) diff --git a/pcs_test/tier0/common/reports/test_messages.py b/pcs_test/tier0/common/reports/test_messages.py index 64e74daa..b1e009ce 100644 --- a/pcs_test/tier0/common/reports/test_messages.py +++ b/pcs_test/tier0/common/reports/test_messages.py @@ -5525,6 +5525,22 @@ class AgentSelfValidationInvalidData(NameBuildTest): ) +class AgentSelfValidationSkippedUpdatedResourceMisconfigured(NameBuildTest): + def test_message(self): + lines = list(f"line #{i}" for i in range(3)) + self.assert_message_from_report( + ( + "The resource was misconfigured before the update, therefore " + "agent self-validation will not be run for the updated " + "configuration. Validation output of the original " + "configuration:\n {}" + ).format("\n ".join(lines)), + reports.AgentSelfValidationSkippedUpdatedResourceMisconfigured( + "\n".join(lines) + ), + ) + + class ResourceCloneIncompatibleMetaAttributes(NameBuildTest): def test_with_provider(self): attr = "attr_name" diff --git a/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py b/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py index 8b52314f..7a4e5c8f 100644 --- a/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py +++ b/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py @@ -660,7 +660,6 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ) def test_force(self): @@ -680,15 +679,14 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.warning(), ) def test_failure(self): attributes = {"required": "value"} facade = _fixture_ocf_agent() - failure_reports = ["report1", "report2"] - self.agent_self_validation_mock.return_value = False, failure_reports - self.assertEqual( + failure_reason = "failure reason" + self.agent_self_validation_mock.return_value = False, failure_reason + assert_report_item_list_equal( primitive.validate_resource_instance_attributes_create( self.cmd_runner, facade, @@ -696,13 +694,18 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): etree.Element("resources"), force=False, ), - failure_reports, + [ + fixture.error( + reports.codes.AGENT_SELF_VALIDATION_RESULT, + result=failure_reason, + force_code=reports.codes.FORCE, + ) + ], ) self.agent_self_validation_mock.assert_called_once_with( self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ) def test_stonith_check(self): @@ -722,7 +725,6 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ) def test_nonexisting_agent(self): @@ -1346,13 +1348,11 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, old_attributes, - reports.ReportItemSeverity.error(), ), mock.call( self.cmd_runner, facade.metadata.name, new_attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ), ], ) @@ -1379,13 +1379,11 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, old_attributes, - reports.ReportItemSeverity.error(), ), mock.call( self.cmd_runner, facade.metadata.name, new_attributes, - reports.ReportItemSeverity.warning(), ), ], ) @@ -1393,13 +1391,13 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): def test_failure(self): old_attributes = {"required": "old_value"} new_attributes = {"required": "new_value"} - failure_reports = ["report1", "report2"] + failure_reason = "failure reason" facade = _fixture_ocf_agent() self.agent_self_validation_mock.side_effect = ( - (True, []), - (False, failure_reports), + (True, ""), + (False, failure_reason), ) - self.assertEqual( + assert_report_item_list_equal( primitive.validate_resource_instance_attributes_update( self.cmd_runner, facade, @@ -1408,7 +1406,13 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self._fixture_resources(old_attributes), force=False, ), - failure_reports, + [ + fixture.error( + reports.codes.AGENT_SELF_VALIDATION_RESULT, + result=failure_reason, + force_code=reports.codes.FORCE, + ) + ], ) self.assertEqual( self.agent_self_validation_mock.mock_calls, @@ -1417,13 +1421,11 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, old_attributes, - reports.ReportItemSeverity.error(), ), mock.call( self.cmd_runner, facade.metadata.name, new_attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ), ], ) @@ -1450,13 +1452,11 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, old_attributes, - reports.ReportItemSeverity.error(), ), mock.call( self.cmd_runner, facade.metadata.name, new_attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ), ], ) @@ -1522,10 +1522,10 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): def test_current_attributes_failure(self): old_attributes = {"required": "old_value"} new_attributes = {"required": "new_value"} - failure_reports = ["report1", "report2"] + failure_reason = "failure reason" facade = _fixture_ocf_agent() - self.agent_self_validation_mock.return_value = False, failure_reports - self.assertEqual( + self.agent_self_validation_mock.return_value = False, failure_reason + assert_report_item_list_equal( primitive.validate_resource_instance_attributes_update( self.cmd_runner, facade, @@ -1534,7 +1534,12 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self._fixture_resources(old_attributes), force=False, ), - [], + [ + fixture.warn( + reports.codes.AGENT_SELF_VALIDATION_SKIPPED_UPDATED_RESOURCE_MISCONFIGURED, + result=failure_reason, + ) + ], ) self.assertEqual( self.agent_self_validation_mock.mock_calls, @@ -1543,7 +1548,6 @@ class ValidateResourceInstanceAttributesUpdateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, old_attributes, - reports.ReportItemSeverity.error(), ), ], ) diff --git a/pcs_test/tier0/lib/pacemaker/test_live.py b/pcs_test/tier0/lib/pacemaker/test_live.py index 1f37d759..c1363a65 100644 --- a/pcs_test/tier0/lib/pacemaker/test_live.py +++ b/pcs_test/tier0/lib/pacemaker/test_live.py @@ -1706,16 +1706,15 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertTrue(is_valid) - self.assertEqual(report_list, []) + self.assertEqual(reason, "") runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] ) @@ -1725,23 +1724,17 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertIsNone(is_valid) - assert_report_item_list_equal( - report_list, - [ - fixture.info( - report_codes.AGENT_SELF_VALIDATION_INVALID_DATA, - reason="Start tag expected, '<' not found, line 1, column 1 (, line 1)", - ) - ], + self.assertEqual( + reason, + "Start tag expected, '<' not found, line 1, column 1 (, line 1)", ) runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] @@ -1760,19 +1753,15 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertTrue(is_valid) - assert_report_item_list_equal( - report_list, - [], - ) + self.assertEqual(reason, "") runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] ) @@ -1791,23 +1780,15 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertFalse(is_valid) - assert_report_item_list_equal( - report_list, - [ - fixture.info( - report_codes.AGENT_SELF_VALIDATION_RESULT, result="" - ) - ], - ) + self.assertEqual(reason, "") runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] ) @@ -1835,23 +1816,17 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertFalse(is_valid) - assert_report_item_list_equal( - report_list, - [ - fixture.info( - report_codes.AGENT_SELF_VALIDATION_RESULT, - result="first line\nImportant output\nand another line", - ) - ], + self.assertEqual( + reason, + "first line\nImportant output\nand another line", ) runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] @@ -1879,23 +1854,17 @@ class HandleInstanceAttributesValidateViaPcmkTest(TestCase): base_cmd = ["some", "command"] ( is_valid, - report_list, + reason, ) = lib._handle_instance_attributes_validation_via_pcmk( runner, base_cmd, "result/output", {"attr1": "val1", "attr2": "val2"}, - not_valid_severity=Severity.info(), ) self.assertTrue(is_valid) - assert_report_item_list_equal( - report_list, - [ - fixture.warn( - report_codes.AGENT_SELF_VALIDATION_RESULT, - result="first line\nImportant output\nand another line", - ) - ], + self.assertEqual( + reason, + "first line\nImportant output\nand another line", ) runner.run.assert_called_once_with( base_cmd + ["--option", "attr1=val1", "--option", "attr2=val2"] @@ -1907,7 +1876,6 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): def setUp(self): self.runner = mock.Mock() self.attrs = dict(attra="val1", attrb="val2") - self.severity = Severity.info() patcher = mock.patch( "pcs.lib.pacemaker.live._handle_instance_attributes_validation_via_pcmk" ) @@ -1921,7 +1889,7 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ) self.assertEqual( lib._validate_resource_instance_attributes_via_pcmk( - self.runner, agent, self.attrs, self.severity + self.runner, agent, self.attrs ), self.ret_val, ) @@ -1941,7 +1909,6 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ], "./resource-agent-action/command/output", self.attrs, - self.severity, ) def test_without_provider(self): @@ -1950,7 +1917,7 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ) self.assertEqual( lib._validate_resource_instance_attributes_via_pcmk( - self.runner, agent, self.attrs, self.severity + self.runner, agent, self.attrs ), self.ret_val, ) @@ -1968,7 +1935,6 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ], "./resource-agent-action/command/output", self.attrs, - self.severity, ) @@ -1978,7 +1944,6 @@ class ValidateStonithInstanceAttributesViaPcmkTest(TestCase): def setUp(self): self.runner = mock.Mock() self.attrs = dict(attra="val1", attrb="val2") - self.severity = Severity.info() patcher = mock.patch( "pcs.lib.pacemaker.live._handle_instance_attributes_validation_via_pcmk" ) @@ -1992,7 +1957,7 @@ class ValidateStonithInstanceAttributesViaPcmkTest(TestCase): ) self.assertEqual( lib._validate_stonith_instance_attributes_via_pcmk( - self.runner, agent, self.attrs, self.severity + self.runner, agent, self.attrs ), self.ret_val, ) @@ -2008,5 +1973,4 @@ class ValidateStonithInstanceAttributesViaPcmkTest(TestCase): ], "./validate/command/output", self.attrs, - self.severity, ) diff --git a/pcs_test/tier1/legacy/test_stonith.py b/pcs_test/tier1/legacy/test_stonith.py index 8b31094b..7e7ec030 100644 --- a/pcs_test/tier1/legacy/test_stonith.py +++ b/pcs_test/tier1/legacy/test_stonith.py @@ -1291,7 +1291,10 @@ class StonithTest(TestCase, AssertPcsMixin): ), ) - self.assert_pcs_success("stonith update test3 username=testA".split()) + self.assert_pcs_success( + "stonith update test3 username=testA".split(), + stdout_start="Warning: ", + ) self.assert_pcs_success( "stonith config test2".split(), -- 2.38.1