From e56f42bf31ae0a52618fe8754fd0b2ae623e6a7a Mon Sep 17 00:00:00 2001

From: Tomas Jelinek <tojeline@redhat.com>

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<cib />",

-            reports.cib_simulate_error("error message", "<cib />")

+            "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 = """

+            <resources>

+                <primitive id="A" />

+                <clone id="B-clone">

+                    <primitive id="B" />

+                </clone>

+                <master id="C-master">

+                    <primitive id="C" />

+                </master>

+                <group id="D">

+                    <primitive id="D1" />

+                    <primitive id="D2" />

+                </group>

+                <clone id="E-clone">

+                    <group id="E">

+                        <primitive id="E1" />

+                        <primitive id="E2" />

+                    </group>

+                </clone>

+                <master id="F-master">

+                    <group id="F">

+                        <primitive id="F1" />

+                        <primitive id="F2" />

+                    </group>

+                </master>

+                <bundle id="G-bundle" />

+                <bundle id="H-bundle">

+                    <primitive id="H" />

+                </bundle>

+            </resources>

+        """

+        status_xml = """

+            <resources>

+                <resource id="A" managed="true" />

+                

+                    unique="false"

+                >

+                    <resource id="B" managed="true" />

+                    <resource id="B" managed="true" />

+                </clone>

+                

+                    unique="false"

+                >

+                    <resource id="C" managed="true" />

+                    <resource id="C" managed="true" />

+                </clone>

+                <group id="D" number_resources="2">

+                    <resource id="D1" managed="true" />

+                    <resource id="D2" managed="true" />

+                </group>

+                

+                    unique="false"

+                >

+                    <group id="E:0" number_resources="2">

+                        <resource id="E1" managed="true" />

+                        <resource id="E2" managed="true" />

+                    </group>

+                    <group id="E:1" number_resources="2">

+                        <resource id="E1" managed="true" />

+                        <resource id="E2" managed="true" />

+                    </group>

+                </clone>

+                

+                    unique="false"

+                >

+                    <group id="F:0" number_resources="2">

+                        <resource id="F1" managed="true" />

+                        <resource id="F2" managed="true" />

+                    </group>

+                    <group id="F:1" number_resources="2">

+                        <resource id="F1" managed="true" />

+                        <resource id="F2" managed="true" />

+                    </group>

+                </clone>

+                

+                    unique="false" managed="true" failed="false"

+                >

+                    <replica id="0">

+                        <resource id="H" />

+                    </replica>

+                    <replica id="1">

+                        <resource id="H" />

+                    </replica>

+                </bundle>

+            </resources>

+        """

+        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"""

+                  <synapse>

+                    <action_set>

+                      <rsc_op id="{index}" operation="stop" on_node="node1">

+                        <primitive id="{res_name}" long_id="{res_name}:0" />

+                      </rsc_op>

+                    </action_set>

+                  </synapse>

+                  <synapse>

+                    <action_set>

+                      <rsc_op id="{index + 1}" operation="stop" on_node="node2">

+                        <primitive id="{res_name}" long_id="{res_name}:1" />

+                      </rsc_op>

+                    </action_set>

+                  </synapse>

+                """)

+                index += 2

+            else:

+                synapses.append(f"""

+                  <synapse>

+                    <action_set>

+                      <rsc_op id="{index}" operation="stop" on_node="node1">

+                        <primitive id="{res_name}" />

+                      </rsc_op>

+                    </action_set>

+                  </synapse>

+                """)

+                index += 1

+        transitions_xml = (

+            "<transition_graph>" + "\n".join(synapses) + "</transition_graph>"

+        )

+

+        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="""

+                <resources>

+                    <primitive id="A" />

+                    <clone id="B-clone">

+                        <meta_attributes id="B-clone-meta_attributes">

+                            

+                                id="B-clone-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <primitive id="B" />

+                    </clone>

+                    <master id="C-master">

+                        <meta_attributes id="C-master-meta_attributes">

+                            

+                                id="C-master-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <primitive id="C" />

+                    </master>

+                    <group id="D">

+                        <meta_attributes id="D-meta_attributes">

+                            

+                                id="D-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <primitive id="D1" />

+                        <primitive id="D2" />

+                    </group>

+                    <clone id="E-clone">

+                        <meta_attributes id="E-clone-meta_attributes">

+                            

+                                id="E-clone-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <group id="E">

+                            <primitive id="E1" />

+                            <primitive id="E2" />

+                        </group>

+                    </clone>

+                    <master id="F-master">

+                        <meta_attributes id="F-master-meta_attributes">

+                            

+                                id="F-master-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <group id="F">

+                            <primitive id="F1" />

+                            <primitive id="F2" />

+                        </group>

+                    </master>

+                    <bundle id="G-bundle" />

+                    <bundle id="H-bundle">

+                        <meta_attributes id="H-bundle-meta_attributes">

+                            

+                                id="H-bundle-meta_attributes-target-role"

+                            />

+                        </meta_attributes>

+                        <primitive id="H" />

+                    </bundle>

+                </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="<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="<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="<cib />",

             ),

         )

@@ -755,7 +752,6 @@ class SimulateCibXml(LibraryPacemakerTest):

             fixture.error(

                 report_codes.CIB_SIMULATE_ERROR,

                 reason="some error",

-                cib="<cib />",

             ),

         )

@@ -782,7 +778,6 @@ class SimulateCibXml(LibraryPacemakerTest):

             fixture.error(

                 report_codes.CIB_SIMULATE_ERROR,

                 reason="some error",

-                cib="<cib />",

             ),

         )

@@ -819,7 +814,6 @@ class SimulateCib(TestCase):

                     "Start tag expected, '<' not found, line 1, column 1 "

                     "(<string>, line 1)"

                 ),

-                cib=self.cib_xml,

             ),

         )

@@ -835,7 +829,6 @@ class SimulateCib(TestCase):

                     "Start tag expected, '<' not found, line 1, column 1 "

                     "(<string>, line 1)"

                 ),

-                cib=self.cib_xml,

             ),

         )

-- 

2.21.1