Fix package manager listing of requires

The previous implementation had the following issues:

- a bug when listing from a repository containing multiple versions of a
  single package (can happen with dirty ISO tree during development),
  as it was not able to choose the 'good one'.
- bad naming of functions/variables and imperfect documentation, leading
  to misunderstandings w.r.t. the goal of this method

We fix the issues by:

- implement filtering based on the `repo:packages` pillar information,
  which is already used by the `pkg_installed` macro
- renaming and documenting these custom methods for easier maintenance

Fixes: #1455

salt/_modules/metalk8s_package_manager.py

@@ -2,7 +2,6 @@
 Describes our custom way to deal with yum packages
 so that we can support downgrade in metalk8s
 '''
-
 import logging
 
 log = logging.getLogger(__name__)
@@ -15,64 +14,133 @@ def __virtual__():
     return __virtualname__
 
 
-def list_pkg_deps(pkg_name, version=None, fromrepo=None):
-    '''
-    Check dependencies related to the packages installed so that we can pass
-    this information to pkg.installed
-
-    name
-        Name of the package installed
-
-    version
-        Version number of the package
+def _list_dependents(
+    name, version, fromrepo=None, allowed_versions=None
+):
+    '''List and filter all packages requiring package `{name}-{version}`.
 
-        Use : salt '*' metalk8s_package_manager.list_pkg_deps kubelet 1.11.9
+    Filter based on the `allowed_versions` provided, within the provided
+    `fromrepo` repositories.
     '''
     log.info(
-        'Listing deps for "%s" with version "%s"',
-        str(pkg_name),
+        'Listing packages depending on "%s" with version "%s"',
+        str(name),
         str(version)
     )
-    pkgs_dict = {pkg_name: version}
 
-    if not version:
-        return pkgs_dict
+    allowed_versions = allowed_versions or {}
 
-    command_all = [
-        'repoquery', '--whatrequires', '--recursive', '--qf',
-        '%{NAME} %{VERSION}-%{RELEASE}',
-        '{}-{}'.format(str(pkg_name), str(version))
+    command = [
+        'repoquery', '--whatrequires', '--recursive',
+        '--qf', '%{NAME} %{VERSION}-%{RELEASE}',
+        '{}-{}'.format(name, version)
     ]
 
     if fromrepo:
-        command_all.extend(['--disablerepo', '*', '--enablerepo', fromrepo])
+        command.extend(['--disablerepo', '*', '--enablerepo', fromrepo])
 
-    deps_list = __salt__['cmd.run_all'](command_all)
+    ret = __salt__['cmd.run_all'](command)
 
-    if deps_list['retcode'] != 0:
+    if ret['retcode'] != 0:
         log.error(
-            'Failed to list package dependencies: %s',
-            deps_list['stderr'] or deps_list['stdout']
+            'Failed to list packages requiring "%s": %s',
+            '{}-{}'.format(name, version),
+            ret['stderr'] or ret['stdout']
         )
         return None
 
-    out = deps_list['stdout'].splitlines()
-    for line in out:
-        name, version = line.strip().split()
-        pkgs_dict[name] = version
+    dependents = {}
+    for line in ret['stdout'].splitlines():
+        req_name, req_version = line.strip().split()
+
+        # NOTE: The following test filters out unknown packages and versions
+        #       not referenced in `allowed_versions` (there can be only one)
+        if req_version == allowed_versions.get(req_name):
+            dependents[req_name] = req_version
+
+    return dependents
+
+
+def list_pkg_dependents(
+    name, version=None, fromrepo=None, pkgs_info=None
+):
+    '''
+    Check dependents of the package `name`-`version` to install, to add in a
+    later `pkg.installed` state along with the original package.
+
+
+
+    Ensure all selected versions are compliant with those listed in `pkgs_info`
+    if provided.
+
+    name
+        Name of the package installed
 
-    for key in pkgs_dict.keys():
-        package_query = __salt__['cmd.run_all'](
-            ['rpm', '-qa', key]
+    version
+        Version number of the package
+
+    pkgs_info
+        Value of pillar key `repo:packages` to consider for the requiring
+        packages to update (format {"<name>": {"version": "<version>"}, ...})
+
+    Usage :
+        salt '*' metalk8s_package_manager.list_pkg_dependents kubelet 1.11.10
+    '''
+    if pkgs_info:
+        versions_dict = {
+            p_name: p_info['version']
+            for p_name, p_info in pkgs_info.items()
+        }
+    else:
+        versions_dict = {}
+
+    if pkgs_info and name not in versions_dict:
+        log.error(
+            'Trying to list dependents for "%s", which is not referenced in '
+            'the packages information provided',
+            name
         )
+        return None
+
+    all_pkgs = {name: version}
+
+    if not version:
+        return all_pkgs
 
-        if package_query['retcode'] == 1:
-            pkgs_dict.pop(key)
-        elif package_query['retcode'] != 0:
+    if pkgs_info and versions_dict[name] != version:
+        log.error(
+            'Trying to list dependents for "%s" with version "%s", '
+            'while version configured is "%s"',
+            name,
+            version,
+            versions_dict[name]
+        )
+        return None
+
+    dependents = _list_dependents(
+        name,
+        version,
+        fromrepo=fromrepo,
+        allowed_versions=versions_dict,
+    )
+
+    all_pkgs.update(dependents)
+
+    for pkg_name, desired_version in all_pkgs.items():
+        ret = __salt__['cmd.run_all'](['rpm', '-qa', pkg_name])
+
+        if ret['retcode'] != 0:
             log.error(
-                'Failed to check if package is installed: %s',
-                deps_list['stderr'] or deps_list['stdout']
+                'Failed to check if package "%s" is installed: %s',
+                pkg_name,
+                ret['stderr'] or ret['stdout']
             )
             return None
 
-    return pkgs_dict
+        is_installed = bool(ret['stdout'].strip())
+        if not is_installed and pkg_name != name:
+            # Any package requiring the target `name` that is not yet installed
+            # should not be installed
+            del all_pkgs[pkg_name]
+
+    return all_pkgs

salt/_states/metalk8s_package_manager.py

@@ -17,16 +17,31 @@ def __virtualname__():
     return (False, "metalk8s_package_manager: no RPM-based system detected")
 
 
-def installed(name, version=None, fromrepo=None, **kwargs):
-    """Simple helper to manage package downgrade including dependencies."""
+def installed(name, version=None, fromrepo=None, pkgs_info=None, **kwargs):
+    """Custom implementation of `pkg.installed`.
+
+    Manage packages requiring the target package when upgrading/downgrading,
+    ensuring all versions are uniform with respect to what is declared
+    in `pkgs_info`.
+    """
     if version is None or kwargs.get('pkgs'):
         return __states__["pkg.installed"](
             name=name, version=version, fromrepo=fromrepo, **kwargs
         )
 
-    dep_list = __salt__['metalk8s_package_manager.list_pkg_deps'](
-        name, version, fromrepo
+    dep_list = __salt__['metalk8s_package_manager.list_pkg_dependents'](
+        name, version, fromrepo=fromrepo, pkgs_info=pkgs_info,
     )
+    if dep_list is None:
+        return {
+            'name': name,
+            'result': False,
+            'changes': {},
+            'comment': (
+                'Failed to update package "{}" and its dependents'.format(name)
+            ),
+        }
+
     pkgs = [{k: v} for k, v in dep_list.items()]
     return __states__["pkg.installed"](
         name=name, pkgs=pkgs, fromrepo=fromrepo, **kwargs

salt/metalk8s/macro.sls

@@ -6,6 +6,7 @@
   metalk8s_package_manager.installed:
     - name: {{ name }}
     - fromrepo: {{ repo.repositories.keys() | join(',') }}
+    - pkgs_info: {{ repo.packages }}
     {%- if package.version | default(None) %}
     - version: {{ package.version }}
     - hold: True