From 58589e47f2913276ea1c2164a3ce8ee694fb2b78 Mon Sep 17 00:00:00 2001 From: Ondrej Mular Date: Wed, 7 Dec 2022 11:33:25 +0100 Subject: [PATCH 1/5] add warning when updating a misconfigured resource --- 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 +- 8 files changed, 161 insertions(+), 138 deletions(-) diff --git a/pcs/common/reports/codes.py b/pcs/common/reports/codes.py index deecc626..48048af7 100644 --- a/pcs/common/reports/codes.py +++ b/pcs/common/reports/codes.py @@ -40,6 +40,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 d27c1dee..24bb222f 100644 --- a/pcs/common/reports/messages.py +++ b/pcs/common/reports/messages.py @@ -7584,6 +7584,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 BoothAuthfileNotUsed(ReportItemMessage): """ diff --git a/pcs/lib/cib/resource/primitive.py b/pcs/lib/cib/resource/primitive.py index 3ebd01c6..c5df8e58 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, @@ -402,16 +427,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 @@ -505,25 +530,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 fd26dabb..726f6b67 100644 --- a/pcs/lib/pacemaker/live.py +++ b/pcs/lib/pacemaker/live.py @@ -902,8 +902,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", @@ -917,7 +916,6 @@ def _validate_stonith_instance_attributes_via_pcmk( cmd, "./validate/command/output", instance_attributes, - not_valid_severity, ) @@ -925,8 +923,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", @@ -944,7 +941,6 @@ def _validate_resource_instance_attributes_via_pcmk( cmd, "./resource-agent-action/command/output", instance_attributes, - not_valid_severity, ) @@ -953,8 +949,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}"]) @@ -963,12 +958,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() @@ -976,38 +966,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 17627b80..5fcc62fc 100644 --- a/pcs_test/tier0/common/reports/test_messages.py +++ b/pcs_test/tier0/common/reports/test_messages.py @@ -5562,6 +5562,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 BoothAuthfileNotUsed(NameBuildTest): def test_message(self): self.assert_message_from_report( 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 2cba7086..1bc3a5a6 100644 --- a/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py +++ b/pcs_test/tier0/lib/cib/resource/test_primitive_validate.py @@ -609,7 +609,6 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ) def test_force(self): @@ -629,15 +628,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, @@ -645,13 +643,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): @@ -671,7 +674,6 @@ class ValidateResourceInstanceAttributesCreateSelfValidation(TestCase): self.cmd_runner, facade.metadata.name, attributes, - reports.ReportItemSeverity.error(reports.codes.FORCE), ) def test_nonexisting_agent(self): @@ -1295,13 +1297,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), ), ], ) @@ -1328,13 +1328,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(), ), ], ) @@ -1342,13 +1340,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, @@ -1357,7 +1355,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, @@ -1366,13 +1370,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), ), ], ) @@ -1399,13 +1401,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), ), ], ) @@ -1471,10 +1471,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, @@ -1483,7 +1483,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, @@ -1492,7 +1497,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 5c8000cd..239a72b1 100644 --- a/pcs_test/tier0/lib/pacemaker/test_live.py +++ b/pcs_test/tier0/lib/pacemaker/test_live.py @@ -1752,16 +1752,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"] ) @@ -1771,23 +1770,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"] @@ -1806,19 +1799,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"] ) @@ -1837,23 +1826,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"] ) @@ -1881,23 +1862,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"] @@ -1925,23 +1900,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"] @@ -1953,7 +1922,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" ) @@ -1967,7 +1935,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, ) @@ -1987,7 +1955,6 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ], "./resource-agent-action/command/output", self.attrs, - self.severity, ) def test_without_provider(self): @@ -1996,7 +1963,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, ) @@ -2014,7 +1981,6 @@ class ValidateResourceInstanceAttributesViaPcmkTest(TestCase): ], "./resource-agent-action/command/output", self.attrs, - self.severity, ) @@ -2024,7 +1990,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" ) @@ -2038,7 +2003,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, ) @@ -2054,5 +2019,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 9911d604..cf430d75 100644 --- a/pcs_test/tier1/legacy/test_stonith.py +++ b/pcs_test/tier1/legacy/test_stonith.py @@ -1294,7 +1294,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.39.0