cleanup Version and cib_push

main changes:
* remove support for comparing to tuples from Version as it is not
  needed
* restructure methods for pushing the CIB to keep various levels of
  abstraction in their own methods

pcs/common/test/test_tools.py

@@ -32,13 +32,9 @@ def assert_asterisk(self, expected, major, minor=None, revision=None):
 
     def assert_eq_tuple(self, a, b):
         self.assert_eq(Version(*a), Version(*b))
-        self.assert_eq(a, Version(*b))
-        self.assert_eq(Version(*a), b)
 
     def assert_lt_tuple(self, a, b):
         self.assert_lt(Version(*a), Version(*b))
-        self.assert_lt(a, Version(*b))
-        self.assert_lt(Version(*a), b)
 
     def assert_eq(self, a, b):
         self.assertTrue(a == b)
@@ -65,6 +61,7 @@ def test_major(self):
         self.assertEqual(ver[1], None)
         self.assertEqual(ver.revision, None)
         self.assertEqual(ver[2], None)
+        self.assertEqual(ver.as_full_tuple, (2, 0, 0))
         self.assertEqual(str(ver), "2")
         self.assertEqual(str(ver.normalize()), "2.0.0")
 
@@ -77,6 +74,7 @@ def test_major_minor(self):
         self.assertEqual(ver[1], 3)
         self.assertEqual(ver.revision, None)
         self.assertEqual(ver[2], None)
+        self.assertEqual(ver.as_full_tuple, (2, 3, 0))
         self.assertEqual(str(ver), "2.3")
         self.assertEqual(str(ver.normalize()), "2.3.0")
 
@@ -89,6 +87,7 @@ def test_major_minor_revision(self):
         self.assertEqual(ver[1], 3)
         self.assertEqual(ver.revision, 4)
         self.assertEqual(ver[2], 4)
+        self.assertEqual(ver.as_full_tuple, (2, 3, 4))
         self.assertEqual(str(ver), "2.3.4")
         self.assertEqual(str(ver.normalize()), "2.3.4")
 
@@ -108,8 +107,12 @@ def test_compare(self):
 
 
         self.assert_eq_tuple((2, 0, 0), (2, 0, 0))
+        self.assert_lt_tuple((2, 0, 0), (2, 0, 1))
         self.assert_lt_tuple((2, 0, 0), (2, 5, 0))
         self.assert_lt_tuple((2, 0, 0), (2, 5, 1))
+        self.assert_lt_tuple((2, 0, 0), (3, 0, 0))
+        self.assert_lt_tuple((2, 0, 0), (3, 0, 1))
+        self.assert_lt_tuple((2, 0, 0), (3, 5, 0))
         self.assert_lt_tuple((2, 0, 0), (3, 5, 1))
 
         self.assert_eq_tuple((2, 0, 0), (2, 0))

pcs/common/tools.py

@@ -81,59 +81,37 @@ def xml_fromstring(xml):
     )
 
 class Version(namedtuple("Version", ["major", "minor", "revision"])):
-    @staticmethod
-    def _normalize(ver):
-        if isinstance(ver, Version):
-            return (
-                ver.major,
-                ver.minor if ver.minor is not None else 0,
-                ver.revision if ver.revision is not None else 0,
-            )
-        if len(ver) == 1:
-            return (
-                ver[0] if ver[0] is not None else 0,
-                0,
-                0,
-            )
-        if len(ver) == 2:
-            return (
-                ver[0] if ver[0] is not None else 0,
-                ver[1] if ver[1] is not None else 0,
-                0,
-            )
-        if len(ver) == 3:
-            return (
-                ver[0] if ver[0] is not None else 0,
-                ver[1] if ver[1] is not None else 0,
-                ver[2] if ver[2] is not None else 0,
-            )
-        raise AssertionError(
-            "Expected a version with 1 to 3 items, got {0}".format(ver)
-        )
-
     def __new__(cls, major, minor=None, revision=None):
         return super(Version, cls).__new__(cls, major, minor, revision)
 
+    @property
+    def as_full_tuple(self):
+        return (
+            self.major,
+            self.minor if self.minor is not None else 0,
+            self.revision if self.revision is not None else 0,
+        )
+
+    def normalize(self):
+        return self.__class__(*self.as_full_tuple)
+
     def __str__(self):
         return ".".join([str(x) for x in self if x is not None])
 
-    def __lt__(self, x):
-        return self.__class__._normalize(self) < self.__class__._normalize(x)
+    def __lt__(self, other):
+        return self.as_full_tuple < other.as_full_tuple
 
-    def __le__(self, x):
-        return self.__class__._normalize(self) <= self.__class__._normalize(x)
+    def __le__(self, other):
+        return self.as_full_tuple <= other.as_full_tuple
 
-    def __eq__(self, x):
-        return self.__class__._normalize(self) == self.__class__._normalize(x)
+    def __eq__(self, other):
+        return self.as_full_tuple == other.as_full_tuple
 
-    def __ne__(self, x):
-        return self.__class__._normalize(self) != self.__class__._normalize(x)
+    def __ne__(self, other):
+        return self.as_full_tuple != other.as_full_tuple
 
-    def __gt__(self, x):
-        return self.__class__._normalize(self) > self.__class__._normalize(x)
+    def __gt__(self, other):
+        return self.as_full_tuple > other.as_full_tuple
 
-    def __ge__(self, x):
-        return self.__class__._normalize(self) >= self.__class__._normalize(x)
-
-    def normalize(self):
-        return self.__class__(*self.__class__._normalize(self))
+    def __ge__(self, other):
+        return self.as_full_tuple >= other.as_full_tuple

pcs/lib/cib/test/test_tools.py

@@ -438,13 +438,7 @@ def test_invalid_version(self):
 
     def test_no_revision(self):
         self.assertEqual(
-            (1, 2),
-            lib.get_pacemaker_version_by_which_cib_was_validated(
-                etree.XML('<cib validate-with="pacemaker-1.2"/>')
-            )
-        )
-        self.assertEqual(
-            (1, 2, 0),
+            Version(1, 2),
             lib.get_pacemaker_version_by_which_cib_was_validated(
                 etree.XML('<cib validate-with="pacemaker-1.2"/>')
             )

pcs/lib/cib/tools.py

@@ -16,6 +16,8 @@
 )
 from pcs.lib.xml_tools import get_root
 
+_VERSION_FORMAT = r"(?P<major>\d+)\.(?P<minor>\d+)(\.(?P<rev>\d+))?"
+
 class IdProvider(object):
     """
     Book ids for future use in the CIB and generate new ids accordingly
@@ -280,7 +282,7 @@ def get_pacemaker_version_by_which_cib_was_validated(cib):
     return _get_cib_version(
         cib,
         "validate-with",
-        re.compile(r"pacemaker-(?P<major>\d+)\.(?P<minor>\d+)(\.(?P<rev>\d+))?")
+        re.compile(r"pacemaker-{0}".format(_VERSION_FORMAT))
     )
 
 def get_cib_crm_feature_set(cib, none_if_missing=False):
@@ -294,6 +296,6 @@ def get_cib_crm_feature_set(cib, none_if_missing=False):
     return _get_cib_version(
         cib,
         "crm_feature_set",
-        re.compile(r"(?P<major>\d+)\.(?P<minor>\d+)(\.(?P<rev>\d+))?"),
+        re.compile(_VERSION_FORMAT),
         none_if_missing=none_if_missing
     )

pcs/lib/commands/test/test_acl.py

@@ -5,13 +5,14 @@
 )
 
 import pcs.lib.commands.acl as cmd_acl
+from pcs.common.tools import Version
 from pcs.lib.env import LibraryEnvironment
 from pcs.test.tools.assertions import ExtendedAssertionsMixin
 from pcs.test.tools.custom_mock import MockLibraryReportProcessor
 from pcs.test.tools.pcs_unittest import mock, TestCase
 
 
-REQUIRED_CIB_VERSION = (2, 0, 0)
+REQUIRED_CIB_VERSION = Version(2, 0, 0)
 
 
 class AclCommandsTest(TestCase, ExtendedAssertionsMixin):

pcs/lib/commands/test/test_fencing_topology.py

@@ -11,6 +11,7 @@
     TARGET_TYPE_REGEXP,
     TARGET_TYPE_ATTRIBUTE,
 )
+from pcs.common.tools import Version
 from pcs.lib.env import LibraryEnvironment
 from pcs.test.tools.misc import create_patcher
 from pcs.test.tools.pcs_unittest import mock, TestCase
@@ -119,7 +120,7 @@ def test_target_attribute_updates_cib(
             "force device",
             "force node"
         )
-        mock_get_cib.assert_called_once_with((2, 4, 0))
+        mock_get_cib.assert_called_once_with(Version(2, 4, 0))
         self.assert_mocks(
             mock_status_xml, mock_status, mock_get_topology, mock_get_resources,
             mock_push_cib
@@ -152,7 +153,7 @@ def test_target_regexp_updates_cib(
             "force device",
             "force node"
         )
-        mock_get_cib.assert_called_once_with((2, 3, 0))
+        mock_get_cib.assert_called_once_with(Version(2, 3, 0))
         self.assert_mocks(
             mock_status_xml, mock_status, mock_get_topology, mock_get_resources,
             mock_push_cib

pcs/lib/env.py

@@ -189,44 +189,47 @@ def ensure_wait_satisfiable(self, wait):
         self._get_wait_timeout(wait)
 
     def push_cib(self, custom_cib=None, wait=False):
+        """
+        Push previously loaded instance of CIB or a custom CIB
+
+        etree custom_cib -- push a custom CIB instead of a loaded instance
+            (allows to push an externally provided CIB and replace the one in
+            the cluster completely)
+        mixed wait -- how many seconds to wait for pacemaker to process new CIB
+            or False for not waiting at all
+        """
         if custom_cib is not None:
-            return self._push_cib_full(custom_cib, wait)
+            if self.__loaded_cib_diff_source is not None:
+                raise AssertionError(
+                    "CIB has been loaded, cannot push custom CIB"
+                )
+            return self.__push_cib_full(custom_cib, wait)
+        if self.__loaded_cib_diff_source is None:
+            raise AssertionError("CIB has not been loaded")
         # Push by diff works with crm_feature_set > 3.0.8, see
         # https://bugzilla.redhat.com/show_bug.cgi?id=1488044 for details. We
         # only check the version if a CIB has been loaded, otherwise the push
         # fails anyway. By my testing it seems that only the source CIB's
         # version matters.
-        if self.__loaded_cib_diff_source is not None:
-            if self.__loaded_cib_diff_source_feature_set < Version(3, 0, 9):
-                self.report_processor.process(
-                    reports.cib_push_forced_full_due_to_crm_feature_set(
-                        Version(3, 0, 9),
-                        self.__loaded_cib_diff_source_feature_set
-                    )
+        if self.__loaded_cib_diff_source_feature_set < Version(3, 0, 9):
+            self.report_processor.process(
+                reports.cib_push_forced_full_due_to_crm_feature_set(
+                    Version(3, 0, 9),
+                    self.__loaded_cib_diff_source_feature_set
                 )
-                return self._push_cib_full(wait=wait)
-        return self._push_cib_diff(wait=wait)
-
-    def _push_cib_full(self, custom_cib=None, wait=False):
-        if custom_cib is None and self.__loaded_cib_diff_source is None:
-            raise AssertionError("CIB has not been loaded")
-        if custom_cib is not None and self.__loaded_cib_diff_source is not None:
-            raise AssertionError("CIB has been loaded, cannot push custom CIB")
+            )
+            return self.__push_cib_full(self.__loaded_cib_to_modify, wait=wait)
+        return self.__push_cib_diff(wait=wait)
 
+    def __push_cib_full(self, cib_to_push, wait=False):
         cmd_runner = self.cmd_runner()
-        cib_to_push = (
-            self.__loaded_cib_to_modify if custom_cib is None else custom_cib
-        )
         self.__do_push_cib(
             cmd_runner,
             lambda: replace_cib_configuration(cmd_runner, cib_to_push),
             wait
         )
 
-    def _push_cib_diff(self, wait=False):
-        if self.__loaded_cib_diff_source is None:
-            raise AssertionError("CIB has not been loaded")
-
+    def __push_cib_diff(self, wait=False):
         cmd_runner = self.cmd_runner()
         self.__do_push_cib(
             cmd_runner,

pcs/lib/reports.py

@@ -1408,7 +1408,7 @@ def cib_diff_error(reason, cib_old, cib_new):
 
 def cib_push_forced_full_due_to_crm_feature_set(required_set, current_set):
     """
-    Pcs uses the old approach of pushing the CIB so race conditions may occur.
+    Pcs uses the "push full CIB" approach so race conditions may occur.
 
     pcs.common.tools.Version required_set -- crm_feature_set required for diff
     pcs.common.tools.Version current_set -- actual CIB crm_feature_set