From e56f42bf31ae0a52618fe8754fd0b2ae623e6a7a Mon Sep 17 00:00:00 2001 From: Tomas Jelinek Date: Thu, 12 Dec 2019 14:46:44 +0100 Subject: [PATCH 1/7] squash bz1781303 fix safe-disabling clones, groups, bundles fix simulate_cib_error report Putting only one CIB in the report is not enough info. Both original and changed CIB as well as crm_simulate output would be needed. All that info can be seen in debug messages. So there is no need to put it in the report. --- pcs/cli/common/console_report.py | 7 +- pcs/lib/cib/resource/common.py | 21 +- pcs/lib/commands/resource.py | 27 +- pcs/lib/pacemaker/live.py | 8 +- pcs/lib/reports.py | 4 +- .../tier0/cli/common/test_console_report.py | 10 +- .../tier0/lib/cib/test_resource_common.py | 60 ++++- .../resource/test_resource_enable_disable.py | 242 +++++++++++++++++- pcs_test/tier0/lib/pacemaker/test_live.py | 7 - 9 files changed, 350 insertions(+), 36 deletions(-) diff --git a/pcs/cli/common/console_report.py b/pcs/cli/common/console_report.py index d349c823..60dbb2a0 100644 --- a/pcs/cli/common/console_report.py +++ b/pcs/cli/common/console_report.py @@ -1269,8 +1269,11 @@ CODE_TO_MESSAGE_BUILDER_MAP = { , codes.CIB_SIMULATE_ERROR: lambda info: - "Unable to simulate changes in CIB: {reason}\n{cib}" - .format(**info) + "Unable to simulate changes in CIB{_reason}" + .format( + _reason=format_optional(info["reason"], ": {0}"), + **info + ) , codes.CIB_PUSH_FORCED_FULL_DUE_TO_CRM_FEATURE_SET: lambda info: diff --git a/pcs/lib/cib/resource/common.py b/pcs/lib/cib/resource/common.py index f1891003..e30c5e69 100644 --- a/pcs/lib/cib/resource/common.py +++ b/pcs/lib/cib/resource/common.py @@ -1,8 +1,9 @@ from collections import namedtuple from typing import ( cast, + List, Optional, - Sequence, + Set, ) from xml.etree.ElementTree import Element @@ -114,7 +115,23 @@ def find_primitives(resource_el): return [resource_el] return [] -def get_inner_resources(resource_el: Element) -> Sequence[Element]: +def get_all_inner_resources(resource_el: Element) -> Set[Element]: + """ + Return all inner resources (both direct and indirect) of a resource + Example: for a clone containing a group, this function will return both + the group and the resources inside the group + + resource_el -- resource element to get its inner resources + """ + all_inner: Set[Element] = set() + to_process = set([resource_el]) + while to_process: + new_inner = get_inner_resources(to_process.pop()) + to_process.update(set(new_inner) - all_inner) + all_inner.update(new_inner) + return all_inner + +def get_inner_resources(resource_el: Element) -> List[Element]: """ Return list of inner resources (direct descendants) of a resource specified as resource_el. diff --git a/pcs/lib/commands/resource.py b/pcs/lib/commands/resource.py index 1b652ea4..4f975c7f 100644 --- a/pcs/lib/commands/resource.py +++ b/pcs/lib/commands/resource.py @@ -802,7 +802,28 @@ def disable_safe(env, resource_ids, strict, wait): with resource_environment( env, wait, resource_ids, _ensure_disabled_after_wait(True) ) as resources_section: - _disable_validate_and_edit_cib(env, resources_section, resource_ids) + id_provider = IdProvider(resources_section) + resource_el_list = _find_resources_or_raise( + resources_section, + resource_ids + ) + env.report_processor.process_list( + _resource_list_enable_disable( + resource_el_list, + resource.common.disable, + id_provider, + env.get_cluster_state() + ) + ) + + inner_resources_names_set = set() + for resource_el in resource_el_list: + inner_resources_names_set.update({ + inner_resource_el.get("id") + for inner_resource_el + in resource.common.get_all_inner_resources(resource_el) + }) + plaintext_status, transitions, dummy_cib = simulate_cib( env.cmd_runner(), get_root(resources_section) @@ -830,6 +851,10 @@ def disable_safe(env, resource_ids, strict, wait): exclude=resource_ids ) ) + + # Stopping a clone stops all its inner resources. That should not block + # stopping the clone. + other_affected = other_affected - inner_resources_names_set if other_affected: raise LibraryError( reports.resource_disable_affects_other_resources( diff --git a/pcs/lib/pacemaker/live.py b/pcs/lib/pacemaker/live.py index 83274af0..233f2e2d 100644 --- a/pcs/lib/pacemaker/live.py +++ b/pcs/lib/pacemaker/live.py @@ -271,7 +271,7 @@ def simulate_cib_xml(runner, cib_xml): transitions_file = write_tmpfile(None) except OSError as e: raise LibraryError( - reports.cib_simulate_error(format_os_error(e), cib_xml) + reports.cib_simulate_error(format_os_error(e)) ) cmd = [ @@ -284,7 +284,7 @@ def simulate_cib_xml(runner, cib_xml): stdout, stderr, retval = runner.run(cmd, stdin_string=cib_xml) if retval != 0: raise LibraryError( - reports.cib_simulate_error(stderr.strip(), cib_xml) + reports.cib_simulate_error(stderr.strip()) ) try: @@ -297,7 +297,7 @@ def simulate_cib_xml(runner, cib_xml): return stdout, transitions_xml, new_cib_xml except OSError as e: raise LibraryError( - reports.cib_simulate_error(format_os_error(e), cib_xml) + reports.cib_simulate_error(format_os_error(e)) ) def simulate_cib(runner, cib): @@ -319,7 +319,7 @@ def simulate_cib(runner, cib): ) except (etree.XMLSyntaxError, etree.DocumentInvalid) as e: raise LibraryError( - reports.cib_simulate_error(str(e), cib_xml) + reports.cib_simulate_error(str(e)) ) ### wait for idle diff --git a/pcs/lib/reports.py b/pcs/lib/reports.py index 1f081007..c9b4a25d 100644 --- a/pcs/lib/reports.py +++ b/pcs/lib/reports.py @@ -1935,18 +1935,16 @@ def cib_diff_error(reason, cib_old, cib_new): } ) -def cib_simulate_error(reason, cib): +def cib_simulate_error(reason): """ cannot simulate effects a CIB would have on a live cluster string reason -- error description - string cib -- the CIB whose effects were to be simulated """ return ReportItem.error( report_codes.CIB_SIMULATE_ERROR, info={ "reason": reason, - "cib": cib, } ) diff --git a/pcs_test/tier0/cli/common/test_console_report.py b/pcs_test/tier0/cli/common/test_console_report.py index 0d0c2457..29e9614d 100644 --- a/pcs_test/tier0/cli/common/test_console_report.py +++ b/pcs_test/tier0/cli/common/test_console_report.py @@ -2238,8 +2238,14 @@ class CibDiffError(NameBuildTest): class CibSimulateError(NameBuildTest): def test_success(self): self.assert_message_from_report( - "Unable to simulate changes in CIB: error message\n", - reports.cib_simulate_error("error message", "") + "Unable to simulate changes in CIB: error message", + reports.cib_simulate_error("error message") + ) + + def test_empty_reason(self): + self.assert_message_from_report( + "Unable to simulate changes in CIB", + reports.cib_simulate_error("") ) diff --git a/pcs_test/tier0/lib/cib/test_resource_common.py b/pcs_test/tier0/lib/cib/test_resource_common.py index ebba09da..cd716ba2 100644 --- a/pcs_test/tier0/lib/cib/test_resource_common.py +++ b/pcs_test/tier0/lib/cib/test_resource_common.py @@ -200,10 +200,12 @@ class FindOneOrMoreResources(TestCase): class FindResourcesMixin: + _iterable_type = list + def assert_find_resources(self, input_resource_id, output_resource_ids): self.assertEqual( - output_resource_ids, - [ + self._iterable_type(output_resource_ids), + self._iterable_type([ element.get("id", "") for element in self._tested_fn( @@ -211,7 +213,7 @@ class FindResourcesMixin: './/*[@id="{0}"]'.format(input_resource_id) ) ) - ] + ]) ) def test_group(self): @@ -235,6 +237,27 @@ class FindResourcesMixin: def test_bundle_with_primitive(self): self.assert_find_resources("H-bundle", ["H"]) + def test_primitive(self): + raise NotImplementedError() + + def test_primitive_in_clone(self): + raise NotImplementedError() + + def test_primitive_in_master(self): + raise NotImplementedError() + + def test_primitive_in_group(self): + raise NotImplementedError() + + def test_primitive_in_bundle(self): + raise NotImplementedError() + + def test_cloned_group(self): + raise NotImplementedError() + + def test_mastered_group(self): + raise NotImplementedError() + class FindPrimitives(TestCase, FindResourcesMixin): _tested_fn = staticmethod(common.find_primitives) @@ -266,6 +289,37 @@ class FindPrimitives(TestCase, FindResourcesMixin): self.assert_find_resources("F-master", ["F1", "F2"]) +class GetAllInnerResources(TestCase, FindResourcesMixin): + _iterable_type = set + _tested_fn = staticmethod(common.get_all_inner_resources) + + def test_primitive(self): + self.assert_find_resources("A", set()) + + def test_primitive_in_clone(self): + self.assert_find_resources("B", set()) + + def test_primitive_in_master(self): + self.assert_find_resources("C", set()) + + def test_primitive_in_group(self): + self.assert_find_resources("D1", set()) + self.assert_find_resources("D2", set()) + self.assert_find_resources("E1", set()) + self.assert_find_resources("E2", set()) + self.assert_find_resources("F1", set()) + self.assert_find_resources("F2", set()) + + def test_primitive_in_bundle(self): + self.assert_find_resources("H", set()) + + def test_cloned_group(self): + self.assert_find_resources("E-clone", {"E", "E1", "E2"}) + + def test_mastered_group(self): + self.assert_find_resources("F-master", {"F", "F1", "F2"}) + + class GetInnerResources(TestCase, FindResourcesMixin): _tested_fn = staticmethod(common.get_inner_resources) diff --git a/pcs_test/tier0/lib/commands/resource/test_resource_enable_disable.py b/pcs_test/tier0/lib/commands/resource/test_resource_enable_disable.py index 634f0f33..62899940 100644 --- a/pcs_test/tier0/lib/commands/resource/test_resource_enable_disable.py +++ b/pcs_test/tier0/lib/commands/resource/test_resource_enable_disable.py @@ -1729,12 +1729,6 @@ class DisableSimulate(TestCase): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some stderr", - # curently, there is no way to normalize xml with our lxml - # version 4.2.3, so this never passes equality tests - # cib=self.config.calls.get( - # "runner.pcmk.simulate_cib" - # ).check_stdin.expected_stdin - # , ), ], expected_in_processor=False @@ -1988,12 +1982,6 @@ class DisableSafeMixin(): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some stderr", - # curently, there is no way to normalize xml with our lxml - # version 4.2.3, so this never passes equality tests - # cib=self.config.calls.get( - # "runner.pcmk.simulate_cib" - # ).check_stdin.expected_stdin - # , ), ], expected_in_processor=False @@ -2118,6 +2106,236 @@ class DisableSafeMixin(): fixture.report_resource_not_running("B"), ]) + def test_inner_resources(self, mock_write_tmpfile): + cib_xml = """ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + """ + status_xml = """ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + """ + synapses = [] + index = 0 + for res_name, is_clone in [ + ("A", False), + ("B", True), + ("C", True), + ("D1", False), + ("D2", False), + ("E1", True), + ("E2", True), + ("F1", True), + ("F2", True), + ("H", False), + ]: + if is_clone: + synapses.append(f""" + + + + + + + + + + + + + + + """) + index += 2 + else: + synapses.append(f""" + + + + + + + + """) + index += 1 + transitions_xml = ( + "" + "\n".join(synapses) + "" + ) + + self.tmpfile_transitions.read.return_value = transitions_xml + mock_write_tmpfile.side_effect = [ + self.tmpfile_new_cib, self.tmpfile_transitions, + AssertionError("No other write_tmpfile call expected") + ] + (self.config + .runner.cib.load(resources=cib_xml) + .runner.pcmk.load_state(resources=status_xml) + ) + self.config.runner.pcmk.simulate_cib( + self.tmpfile_new_cib.name, + self.tmpfile_transitions.name, + stdout="simulate output", + resources=""" + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + """ + ) + self.env_assist.assert_raise_library_error( + lambda: resource.disable_safe( + self.env_assist.get_env(), + ["B-clone", "C-master", "D", "E-clone", "F-master", "H-bundle"], + self.strict, + False, + ), + [ + fixture.error( + report_codes.RESOURCE_DISABLE_AFFECTS_OTHER_RESOURCES, + disabled_resource_list=[ + "B-clone", "C-master", "D", "E-clone", "F-master", + "H-bundle" + ], + affected_resource_list=["A"], + crm_simulate_plaintext_output="simulate output", + ), + ], + expected_in_processor=False + ) + @mock.patch("pcs.lib.pacemaker.live.write_tmpfile") class DisableSafe(DisableSafeMixin, TestCase): strict = False diff --git a/pcs_test/tier0/lib/pacemaker/test_live.py b/pcs_test/tier0/lib/pacemaker/test_live.py index dfebcb17..1ea5454e 100644 --- a/pcs_test/tier0/lib/pacemaker/test_live.py +++ b/pcs_test/tier0/lib/pacemaker/test_live.py @@ -686,7 +686,6 @@ class SimulateCibXml(LibraryPacemakerTest): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some error", - cib="", ), ) mock_runner.run.assert_not_called() @@ -703,7 +702,6 @@ class SimulateCibXml(LibraryPacemakerTest): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some error", - cib="", ), ) mock_runner.run.assert_not_called() @@ -729,7 +727,6 @@ class SimulateCibXml(LibraryPacemakerTest): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some error", - cib="", ), ) @@ -755,7 +752,6 @@ class SimulateCibXml(LibraryPacemakerTest): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some error", - cib="", ), ) @@ -782,7 +778,6 @@ class SimulateCibXml(LibraryPacemakerTest): fixture.error( report_codes.CIB_SIMULATE_ERROR, reason="some error", - cib="", ), ) @@ -819,7 +814,6 @@ class SimulateCib(TestCase): "Start tag expected, '<' not found, line 1, column 1 " "(, line 1)" ), - cib=self.cib_xml, ), ) @@ -835,7 +829,6 @@ class SimulateCib(TestCase): "Start tag expected, '<' not found, line 1, column 1 " "(, line 1)" ), - cib=self.cib_xml, ), ) -- 2.21.1