Salt: 2016.3.0: modules.pkg.upgrade not liking not passing an argument.

[whytewolf]

Description of Issue/Question

since upgrading to 2016.3.0 having issues with upgrading. pkg.upgrade complains about NoneType when ran without an argument. which used to be how to upgrade an entire system.

Setup

Centos 7.2

cat /etc/redhat-release
CentOS Linux release 7.2.1511 (Core)

Steps to Reproduce Issue

salt-call pkg.upgrade

[INFO    ] Executing command ['yum', '--quiet', 'clean', 'expire-cache'] in directory '/root'
[DEBUG   ] output:
[INFO    ] Executing command ['yum', '--quiet', 'check-update'] in directory '/root'
[INFO    ] Executing command ['rpm', '-qa', '--queryformat', '%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)\n'] in directory '/root'
[ERROR   ] No package sources provided

Passed invalid arguments: 'NoneType' object is not iterable.

Usage:
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/salt/cli/caller.py", line 197, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/salt/modules/yumpkg.py", line 1336, in upgrade
    targets = [x for x in pkg_params]
TypeError: 'NoneType' object is not iterable

    Run a full system upgrade (a ``yum upgrade`` or ``dnf upgrade``), or
    upgrade specified packages. If the packages aren't installed, they will
    not be installed.

    .. versionchanged:: 2014.7.0

    Return a dict containing the new package names and versions::

        {'<package>': {'old': '<old-version>',
                       'new': '<new-version>'}}

    CLI Example:

    .. code-block:: bash

        salt '*' pkg.upgrade
        salt '*' pkg.upgrade name=openssl

    Repository Options:

    fromrepo
        Specify a package repository (or repositories) from which to install.
        (e.g., ``yum --disablerepo='*' --enablerepo='somerepo'``)

    enablerepo (ignored if ``fromrepo`` is specified)
        Specify a disabled package repository (or repositories) to enable.
        (e.g., ``yum --enablerepo='somerepo'``)

    disablerepo (ignored if ``fromrepo`` is specified)
        Specify an enabled package repository (or repositories) to disable.
        (e.g., ``yum --disablerepo='somerepo'``)

    disableexcludes
        Disable exclude from main, for a repo or for everything.
        (e.g., ``yum --disableexcludes='main'``)

        .. versionadded:: 2014.7
    name
        The name of the package to be upgraded. Note that this parameter is
        ignored if "pkgs" is passed.

        32-bit packages can be upgraded on 64-bit systems by appending the
        architecture designation (``.i686``, ``.i586``, etc.) to the end of the
        package name.

        Warning: if you forget 'name=' and run pkg.upgrade openssl, ALL packages
        are upgraded. This will be addressed in next releases.

        CLI Example:

        .. code-block:: bash

            salt '*' pkg.upgrade name=openssl

        .. versionadded:: 2016.3.0
    pkgs
        A list of packages to upgrade from a software repository. Must be
        passed as a python list. A specific version number can be specified
        by using a single-element dict representing the package and its
        version. If the package was not already installed on the system,
        it will not be installed.

        CLI Examples:

        .. code-block:: bash

            salt '*' pkg.upgrade pkgs='["foo", "bar"]'
            salt '*' pkg.upgrade pkgs='["foo", {"bar": "1.2.3-4.el5"}]'

        .. versionadded:: 2016.3.0

    normalize : True
        Normalize the package name by removing the architecture. This is useful
        for poorly created packages which might include the architecture as an
        actual part of the name such as kernel modules which match a specific
        kernel version.

        .. code-block:: bash

            salt -G role:nsd pkg.install gpfs.gplbin-2.6.32-279.31.1.el6.x86_64 normalize=False

        .. versionadded:: 2016.3.0

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

salt --versions-report
Salt Version:
           Salt: 2016.3.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: 3.2.2
       dateutil: 1.5
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.7
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.5 (default, Nov 20 2015, 02:00:19)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.7.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: centos 7.2.1511 Core
        machine: x86_64
        release: 3.10.0-327.10.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.2.1511 Core

[EugeneKay]

This also breaks the pkg.uptodate state, which calls pkg.upgrade without an argument.

[redmcg]

I'm seeing this too. Looks like commit b8b6ac77cec2f73bab321f9477bda7283c05a29a made a change to yumpkg.upgrade where it now makes a call to pkg_resource.parse_targets. parse_targets requires name or pkgs to be set (otherwise it logs an error and returns None - which causes the iteration on pkg_params to fail). This results in upgrade now failing when called with the intent to upgrade the entire system.

I'm currently working around it with this patch:

diff --git a/salt/modules/yumpkg.py b/salt/modules/yumpkg.py
index 79e33a8..0bdf05e 100644
--- a/salt/modules/yumpkg.py
+++ b/salt/modules/yumpkg.py
@@ -1323,17 +1323,19 @@ def upgrade(refresh=True,
         refresh_db(**kwargs)

     old = list_pkgs()
-    try:
-        pkg_params = __salt__['pkg_resource.parse_targets'](
-            name=name,
-            pkgs=pkgs,
-            sources=None,
-            normalize=normalize,
-            **kwargs)[0]
-    except MinionError as exc:
-        raise CommandExecutionError(exc)
+    targets = None
+    if name or pkgs:
+        try:
+            pkg_params = __salt__['pkg_resource.parse_targets'](
+                name=name,
+                pkgs=pkgs,
+                sources=None,
+                normalize=normalize,
+                **kwargs)[0]
+        except MinionError as exc:
+            raise CommandExecutionError(exc)

-    targets = [x for x in pkg_params]
+        targets = [x for x in pkg_params]

     cmd = [_yum(), '--quiet', '-y']
     for args in (repo_arg, exclude_arg, branch_arg):
@@ -1342,7 +1344,8 @@ def upgrade(refresh=True,
     if skip_verify:
         cmd.append('--nogpgcheck')
     cmd.append('upgrade')
-    cmd.extend(targets)
+    if targets:
+        cmd.extend(targets)

     __salt__['cmd.run'](cmd, output_loglevel='trace', python_shell=False)
     __context__.pop('pkg.list_pkgs', None)

Versions Report

Salt Version:
           Salt: 2016.3.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 1.4.1
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: 0.20.0
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: 0.20.3
         Python: 2.6.6 (r266:84292, Jul 23 2015, 15:22:56)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.5.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: centos 6.7 Final
        machine: x86_64
        release: 2.6.32-573.22.1.el6.x86_64
         system: Linux
        version: CentOS 6.7 Final

[Ch3LL]

I am able to replicate this and my git bisect also shows the same commit that @redmcg as the first bad commit. Thanks

[damon-atkins]

Please add a Unit Test

[TheBigBear]

I am also seeing this, and it breaks all my linux updates. How much longer is a bad regression like this going to survive on a latest, stable and recommended production release? Any expected update/fix timeframe?

[Ch3LL]

Looks like this is actually a duplicate of #33530 and there is a fix that will be included with 2016.3.1. Can anybody confirm this fix works for them?

[redmcg]

@Ch3LL That patch worked for me. But pkg_resource.parse_targets does still raise an error - which I don't think it should. In my patch (above) I avoid the call to pkg_resource.parse_targets altogether (as to avoid that error).

The output for my test follows (you can see the reported ERROR in there):

$ salt-call pkg.upgrade
[INFO    ] Executing command ['yum', '--quiet', 'clean', 'expire-cache'] in directory '/root'
[INFO    ] Executing command ['yum', '--quiet', 'check-update'] in directory '/root'
[INFO    ] Executing command ['rpm', '-qa', '--queryformat', '%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)\n'] in directory '/root'
[ERROR   ] No package sources provided
[INFO    ] Executing command ['yum', '--quiet', '-y', 'upgrade'] in directory '/root'
[INFO    ] Executing command ['rpm', '-qa', '--queryformat', '%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)\n'] in directory '/root'
local:
    ----------

[danlsgiga]

Also facing this odd issue.

[terminalmage]

@danlsgiga you didn't provide any information about the version you are running, but the fix referenced in this issue is in 2016.3.1. Please attempt again on that version.

[TheBigBear]

@terminalmage the patch in 2016.3.1 is 'better' but there is still a regression in there.
It now does the pkg.upgrade, but it still reports an error that was not there before. This is on a 2016.3.1 minion connected against a 2016.3.1 master.

salt-call pkg.upgrade
[ERROR   ] No package sources provided

local:
    ----------
    tzdata:
        ----------
        new:
            2016f-1.el6
        old:
            2016e-1.el6

[danlsgiga]

@terminalmage I was at the same version at the time of my comment. Thanks for reporting back about the fix on 2016.3.1. I've tested and it works fine!

[terminalmage]

@TheBigBear I noticed that too and fixed it in https://github.com/saltstack/salt/pull/34524.

Just to be clear: the error that was logged did not represent a functional problem, it was logged because we were running pkg_resource.parse_targets when we didn't need to.

[TheBigBear]

@terminalmage I take your word for it.

I was NOT saying this reminding 'error' in the log was an actual problem, seeing that the upgrade actually happened, but merely saying that the 2016.3.1 output and log still suggest there is an "[error]".

Thanks for fixing this as well and getting rid of the extraneous (and wrong - I suppose?) log entry. Well done. Will test when it get's released and report back here if it's now gone.

[terminalmage]

Yeah the log entry occurs when you invoke parse_targets with invalid input. We shouldn't have been calling it in the first place. This was caused by a community member adding the ability to specify packages to upgrade (see b8b6ac7), which uses pkg_resource.parse_targets to convert either the name or pkgs arguments to a list of targets to append to the yum/dnf command. What this person neglected to do however was account for instances in which no arguments were passed to the function. However, it just so happens that pkg_resource.parse_targets returns None (in addition to logging the error message you were seeing) when invoked with both name and pkgs unspecified, so it technically worked as targets would be None, causing the line of code which appends the targets to the command to not attempt to do so. Hence it worked the same way, functionally, as it did before. Not calling pkg_resource.parse_targets when no name or pkgs is installed (as my PR does) keeps us from unnecessarily trying to parse input that wasn't provided, and also avoids the spurious log message you were seeing.