host_helpers/packaging: support more operators in DPKGVersionCompare

this patch adds the complete set of operators that "dpkg --compare-versions"
support to the DPKGVersionCompare. also, DPKGVersionCompare will now throw
DPKGBadVersionSyntax exception when an invalid version syntax is reported
by the "dpkg --compare-versions".

the existing "min" and "max" are now aliases for "ge" and "le", respectively.

Resolves #766

Signed-off-by: Mustafa Kemal Gilor <[email protected]>

doc/source/contrib/language_ref/property_ref/requirement_types.rst

@@ -72,6 +72,27 @@ Or with version ranges as follows:
 In the above example *mypackage* must have a version between 0.0 and 1.0 or
 4.0 and 5.0 inclusive.
 
+Another example:
+
+.. code-block:: yaml
+
+    apt:
+      mypackage:
+        - gt: 1.0
+          lt: 3.0
+        - eq: 5.0.3
+
+In the above example *mypackage* must have a version between 1.0 and 3.0
+(non-inclusive), or specifically 5.0.3.
+
+Supported operators:
+
+  * eq - equality comparison
+  * lt - less than (<)
+  * gt - greater than (>)
+  * le/max - less than or equal (<=)
+  * ge/min - greater than or equal (>=)
+
 Cache keys:
 
   * package - name of each installed package

hotsos/core/host_helpers/__init__.py

@@ -11,7 +11,7 @@
     HostNetworkingHelper,
 )
 from .packaging import (  # noqa: F403,F401
-    DPKGVersionCompare,
+    DPKGVersion,
     APTPackageHelper,
     DockerImageHelper,
     SnapPackageHelper,

hotsos/core/host_helpers/packaging.py

@@ -8,15 +8,21 @@
 from hotsos.core.utils import sorted_dict
 
 
-class DPKGVersionCompare(object):
+class DPKGBadVersionSyntax(Exception):
+    pass
+
+
+class DPKGVersion(object):
 
     def __init__(self, a):
-        self.a = a
+        self.a = str(a)
 
-    def _exec(self, op, b):
+    def _compare_impl(self, op, b):
         try:
-            subprocess.check_call(['dpkg', '--compare-versions',
-                                   self.a, op, b])
+            output = subprocess.check_output(['dpkg', '--compare-versions',
+                                   self.a, op, b], stderr=subprocess.STDOUT)
+            if re.search(b"dpkg: warning: version.*has bad syntax:.*", output):
+                raise DPKGBadVersionSyntax(output)
         except subprocess.CalledProcessError as se:
             if se.returncode == 1:
                 return False
@@ -25,20 +31,23 @@ def _exec(self, op, b):
 
         return True
 
+    def __str__(self):
+        return self.a
+
     def __eq__(self, b):
-        return self._exec('eq', b)
+        return self._compare_impl('eq', str(b))
 
     def __lt__(self, b):
-        return not self._exec('ge', b)
+        return not self._compare_impl('ge', str(b))
 
     def __gt__(self, b):
-        return not self._exec('le', b)
+        return not self._compare_impl('le', str(b))
 
     def __le__(self, b):
-        return self._exec('le', b)
+        return self._compare_impl('le', str(b))
 
     def __ge__(self, b):
-        return self._exec('ge', b)
+        return self._compare_impl('ge', str(b))
 
 
 class PackageHelperBase(abc.ABC):

hotsos/core/plugins/openstack/common.py

@@ -8,7 +8,7 @@
 from hotsos.core.host_helpers import (
     APTPackageHelper,
     DockerImageHelper,
-    DPKGVersionCompare,
+    DPKGVersion,
     PebbleHelper,
     SystemdHelper,
     SSLCertificate,
@@ -63,13 +63,13 @@ def installed_pkg_release_names(self):
                 # version - 1 we use last known lt as current version.
                 v_lt = None
                 r_lt = None
-                pkg_ver = DPKGVersionCompare(self.apt.core[pkg])
+                pkg_ver = DPKGVersion(self.apt.core[pkg])
                 for rel, ver in OST_REL_INFO[pkg].items():
                     if pkg_ver > ver:
                         if v_lt is None:
                             v_lt = ver
                             r_lt = rel
-                        elif ver > DPKGVersionCompare(v_lt):
+                        elif ver > DPKGVersion(v_lt):
                             v_lt = ver
                             r_lt = rel
 

hotsos/core/plugins/storage/ceph.py

@@ -12,7 +12,7 @@
     APTPackageHelper,
     CLIHelper,
     CLIHelperFile,
-    DPKGVersionCompare,
+    DPKGVersion,
     HostNetworkingHelper,
     PebbleHelper,
     SystemdHelper,
@@ -949,7 +949,7 @@ def release_name(self):
         if pkg in self.apt.core:
             for rel, ver in sorted(CEPH_REL_INFO[pkg].items(),
                                    key=lambda i: i[1], reverse=True):
-                if self.apt.core[pkg] > DPKGVersionCompare(ver):
+                if self.apt.core[pkg] > DPKGVersion(ver):
                     relname = rel
                     break
 

hotsos/core/ycheck/engine/properties/requires/types/apt.py

@@ -3,7 +3,7 @@
 from hotsos.core.log import log
 from hotsos.core.host_helpers import (
     APTPackageHelper,
-    DPKGVersionCompare,
+    DPKGVersion,
 )
 from hotsos.core.ycheck.engine.properties.requires import (
     intercept_exception,
@@ -28,32 +28,148 @@ def installed_versions(self):
 
         return _versions
 
-    def package_version_within_ranges(self, pkg, versions):
-        result = False
-        versions = sorted(versions, key=lambda i: str(i['min']), reverse=True)
+    def normalize_version_criteria(self, version_criteria):
+        """Normalize all the criterions in a criteria.
+
+        Normalization does the following:
+        - removes empty criteria
+        - replaces old ops with the new ones
+        - sorts each criterion(ascending) and criteria(descending)
+        - adds upper/lower bounds to criteria, where needed
+
+        @param version_criteria: List of version ranges to normalize
+        @return: Normalized list of version ranges
+        """
+
+        # Step 0: Ensure that all version values are DPKGVersion type
+        for idx, version_criterion in enumerate(version_criteria):
+            for k, v in version_criterion.items():
+                version_criterion.update({k: DPKGVersion(v)})
+
+        # Step 1: Remove empty criteria
+        version_criteria = [x for x in version_criteria if len(x) > 0]
+
+        # Step 2: Replace legacy ops with the new ones
+        legacy_ops = {"min": "ge", "max": "le"}
+        for idx, version_criterion in enumerate(version_criteria):
+            for lop, nop in legacy_ops.items():
+                if lop in version_criterion:
+                    version_criterion[nop] = version_criterion[lop]
+                    del version_criterion[lop]
+
+        # Step 3: Sort each criterion in itself, so the smallest version
+        # appears first
+        for idx, version_criterion in enumerate(version_criteria):
+            version_criterion = dict(sorted(version_criterion.items(),
+                                     key=lambda a: a[1]))
+            version_criteria[idx] = version_criterion
+
+        # Step 4: Sort all criteria by the first element in the criterion
+        version_criteria = sorted(version_criteria,
+                                  key=lambda a: list(a.values())[0])
+
+        # Step 5: Add the implicit upper/lower bounds where needed
+        lower_bound_ops = ["gt", "ge", "eq"]  # ops that define a lower bound
+        upper_bound_ops = ["lt", "le", "eq"]  # ops that define an upper bound
+        equal_compr_ops = ["eq", "ge", "le"]  # ops that compare for equality
+        for idx, version_criterion in enumerate(version_criteria):
+            log.debug("\tchecking criterion %s", str(version_criterion))
+
+            has_lower_bound = any(x in lower_bound_ops
+                                  for x in version_criterion)
+            has_upper_bound = any(x in upper_bound_ops
+                                  for x in version_criterion)
+            is_the_last_item = idx == (len(version_criteria) - 1)
+            is_the_first_item = idx == 0
+
+            log.debug("\t\tcriterion %s has lower bound?"
+                      "%s has upper bound? %s", str(version_criterion),
+                      has_lower_bound, has_upper_bound)
+
+            if not has_upper_bound and not is_the_last_item:
+                op = "le"  # default
+                next_criterion = version_criteria[idx + 1]
+                next_op, next_val = list(next_criterion.items())[0]
+                # If the next criterion op compares for equality, then the
+                # implicit op added to this criterion should not compare for
+                # equality.
+                if next_op in equal_compr_ops:
+                    op = "lt"
+                log.debug("\t\tadding implicit upper bound %s:%s to %s", op,
+                          next_val, version_criterion)
+                version_criterion[op] = next_val
+            elif not has_lower_bound and not is_the_first_item:
+                op = "ge"  # default
+                prev_criterion = version_criteria[idx - 1]
+                prev_op, prev_val = list(prev_criterion.items())[-1]
+                # If the previous criterion op compares for equality, then the
+                # implicit op added to this criterion should not compare for
+                # equality.
+                if prev_op in equal_compr_ops:
+                    op = "gt"
+                log.debug("\t\tadding implicit lower bound %s:%s to %s", op,
+                          prev_val, version_criterion)
+                version_criterion[op] = prev_val
+
+            # Re-sort and overwrite the criterion
+            version_criteria[idx] = dict(
+                sorted(version_criterion.items(),
+                       key=lambda a: a[1]))
+
+        # Step 6: Sort by descending order so the largest version range
+        # appears first
+        version_criteria = sorted(version_criteria,
+                                  key=lambda a: list(a.values())[0],
+                                  reverse=True)
+
+        log.debug("final criteria: %s", str(version_criteria))
+        return version_criteria
+
+    def package_version_within_ranges(self, pkg, version_criteria):
+        """Check if pkg's version satisfies any criterion listed in
+        the version_criteria.
+
+        @param pkg: The name of the apt package
+        @param version_criteria: List of version ranges to normalize
+
+        @return: True if ver(pkg) satisfies any criterion, false otherwise.
+        """
+        result = True
         pkg_version = self.packaging_helper.get_version(pkg)
-        for item in versions:
-            v_min = str(item['min'])
-            if 'max' in item:
-                v_max = str(item['max'])
-                lte_max = pkg_version <= DPKGVersionCompare(v_max)
-            else:
-                lte_max = True
 
-            if v_min:
-                lt_broken = pkg_version < DPKGVersionCompare(v_min)
+        # Supported operations for defining version ranges
+        ops = {
+            "eq": lambda lhs, rhs: lhs == DPKGVersion(rhs),
+            "lt": lambda lhs, rhs: lhs < DPKGVersion(rhs),
+            "le": lambda lhs, rhs: lhs <= DPKGVersion(rhs),
+            "gt": lambda lhs, rhs: lhs > DPKGVersion(rhs),
+            "ge": lambda lhs, rhs: lhs >= DPKGVersion(rhs),
+            "min": lambda lhs, rhs: ops["ge"](lhs, rhs),
+            "max": lambda lhs, rhs: ops["le"](lhs, rhs),
+        }
+
+        version_criteria = self.normalize_version_criteria(version_criteria)
+
+        for version_criterion in version_criteria:
+            # Each criterion is evaluated on its own
+            # so if any of the criteria is true, then
+            # the check is also true.
+            for op_name, op_fn in ops.items():
+                if op_name in version_criterion:
+                    version = str(version_criterion[op_name])
+                    # Check if the criterion is satisfied or not
+                    if not op_fn(str(pkg_version), version):
+                        break
             else:
-                lt_broken = None
-
-            if lt_broken:
-                continue
-
-            result = lte_max
-
-            break
+                # Loop is not exited by a break which means
+                # all ops in the criterion are satisfied.
+                result = True
+                # Break the outer loop
+                break
+            result = False
 
         log.debug("package %s=%s within version ranges %s "
-                  "(result=%s)", pkg, pkg_version, versions, result)
+            "(result=%s)", pkg, pkg_version, version_criteria, result)
         return result
 
 
@@ -67,13 +183,13 @@ class YRequirementTypeAPT(YRequirementTypeBase):
     def _result(self):
         _result = True
         items = APTCheckItems(self.content)
+
         # bail on first fail i.e. if any not installed
         if not items.not_installed:
             for pkg, versions in items:
                 log.debug("package %s installed=%s", pkg, _result)
                 if not versions:
                     continue
-
                 _result = items.package_version_within_ranges(pkg, versions)
                 # bail at first failure
                 if not _result:
@@ -85,7 +201,8 @@ def _result(self):
             _result = False
 
         self.cache.set('package', ', '.join(items.installed))
-        self.cache.set('version', ', '.join(items.installed_versions))
+        self.cache.set('version', ', '.join([
+            str(x) for x in items.installed_versions]))
         log.debug('requirement check: apt %s (result=%s)',
                   ', '.join(items.packages_to_check), _result)
         return _result