Repository: saltstack/salt
Pull request: Fix #53513 - parse error in manage.versions
### What does this PR do?

This changes manage.versions naive approach to calling minion `test.version`,
which always assumed the response would be a version number. Now we only assume
it's a version number if `test.version` returns 0 retcode. Otherwise we will
say that the minion is offline.

### What issues does this PR fix or reference?
Fixes: #53513

### Previous Behavior

When calling `salt-run manage.versions` it would show a stack trace if there
was a failure for the minion to respond (e.g. the minion was in a blackout, or
some other reason). Now it will report that the minion is offline instead. We
*could* add a new "offline" status to the results, but this was the least
invasive change.

### New Behavior

Now `salt-run manage.versions` will report the minion offline when it returns a
non-zero return code.

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
- [X] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [X] Tests written/updated

### Commits signed with GPG?
Yes


Problem statement:
salt-run manage.versions produces ValueError exception
### Description of Issue
salt-run manage.versions produces ValueError exception

```python
# salt-run manage.versions
Exception occurred in runner manage.versions: Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/salt/client/mixins.py", line 387, in _low
    data['return'] = self.functions[fun](*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/salt/runners/manage.py", line 697, in versions
    minion_version = salt.version.SaltStackVersion.parse(minions[minion])
  File "/usr/lib/python2.7/site-packages/salt/version.py", line 269, in parse
    'Unable to parse version string: \'{0}\''.format(version_string)
ValueError: Unable to parse version string: 'ERROR executing 'test.version': Executor 'direct_call.get' is not available'
```

### Setup
Salt-master and 2 minions.

### Steps to Reproduce Issue
Not quite sure. I think one of the minions was in the middle of being updated when `salt-run manage.versions` was ran.

### Versions Report
```yaml
Salt Version:
           Salt: 2018.3.3

Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: 0.26.3
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: 0.26.4
         Python: 2.7.5 (default, Jul 13 2018, 13:06:57)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.5.1804 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-862.11.6.el7.x86_64
         system: Linux
        version: CentOS Linux 7.5.1804 Core
```

Hints from issue discussion:
I experienced the same error on the Salt master version **2019.2.1**

```shell
# salt-run manage.versions
Exception occurred in runner manage.versions: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/salt/client/mixins.py", line 377, in low
    data['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/runners/manage.py", line 730, in versions
    minion_version = salt.version.SaltStackVersion.parse(minions[minion])
  File "/usr/lib/python2.7/dist-packages/salt/version.py", line 275, in parse
    'Unable to parse version string: \'{0}\''.format(version_string)
ValueError: Unable to parse version string: 'ERROR executing 'test.version': Executor 'direct_call.get' is not available'
[INFO    ] Runner completed: 20191008034207419266

```
+1, seeing this on 2019.2.1 as well
Similar issue here. Difference is Error seems related to minion in blackout mode:
```
salt-run manage.versions
Exception occurred in runner manage.versions: Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/salt/client/mixins.py", line 382, in _low
    data['return'] = self.functions[fun](*args, **kwargs)
  File "/usr/lib/python2.7/site-packages/salt/runners/manage.py", line 694, in versions
    minion_version = salt.version.SaltStackVersion.parse(minions[minion])
  File "/usr/lib/python2.7/site-packages/salt/version.py", line 267, in parse
    'Unable to parse version string: \'{0}\''.format(version_string)
ValueError: Unable to parse version string: 'ERROR executing 'test.version': Minion in blackout mode. Set 'minion_blackout' to False in pillar to resume operations. Only saltutil.refresh_pillar allowed in blackout mode.'
```
Salt 2017.7.7
I would expect there to be no error, or at least only error out for the minions in blackout. Other minions should still return their version.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

Still an issue
Thank you for updating this issue. It is no longer marked as stale.

This also affects the latest `2019.2` release (`2019.2.3`):
```
$ salt-run manage.versions
Exception occurred in runner manage.versions: Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/client/mixins.py", line 377, in low
    data['return'] = func(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/runners/manage.py", line 730, in versions
    minion_version = salt.version.SaltStackVersion.parse(minions[minion])
  File "/usr/lib/python3/dist-packages/salt/version.py", line 275, in parse
    'Unable to parse version string: \'{0}\''.format(version_string)
ValueError: Unable to parse version string: '3000'
```

```
$ salt-call --versions
Salt Version:
           Salt: 2019.2.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.9 (default, Nov  7 2019, 10:44:02)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-74-generic
         system: Linux
        version: Ubuntu 18.04 bionic
```
running into this too. I found this issue though when I had a minion return this error:
```
b-gp2-dvrmq-1.imovetv.com:
    ERROR executing 'grains.item': Executor 'direct_call.get' is not available
```
I was running a `grains.item osfinger saltversion` targeting this minion among 80 others or so. I had 2 boxes return this. The OP's error mentions this direct_call.get when trying to run the manage.versions so I don't know if this is related.
### Minion:
```
root@b-gp2-rsdvrmq-3:~# salt-call --versions-report
Salt Version:
           Salt: 2019.2.0
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 0.9.1
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.2.3
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
   python-gnupg: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.0.1
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.4
 
System Versions:
           dist: Ubuntu 14.04 trusty
         locale: UTF-8
        machine: x86_64
        release: 3.19.0-25-generic
         system: Linux
        version: Ubuntu 14.04 trusty
```
### Master:
```
root@b-gp2-salt-1:~# salt --versions-report
Salt Version:
           Salt: 2019.2.3
 
Dependency Versions:
           cffi: 1.14.0
       cherrypy: unknown
       dateutil: 2.4.2
      docker-py: Not Installed
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.3.7
      pycparser: 2.20
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.5.2 (default, Oct  8 2019, 13:06:37)
   python-gnupg: 0.3.8
         PyYAML: 3.11
          PyZMQ: 15.2.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: Ubuntu 16.04 xenial
         locale: ISO-8859-1
        machine: x86_64
        release: 4.4.0-34-generic
         system: Linux
        version: Ubuntu 16.04 xenial
```

Commit message: Merge branch 'master' into tc/fix-53513-salt-version-error

Patch to review:
diff --git a/changelog/53513.fixed b/changelog/53513.fixed
new file mode 100644
index 000000000000..9a653c8476a0
--- /dev/null
+++ b/changelog/53513.fixed
@@ -0,0 +1 @@
+Changed manage.versions to report minions offline if minion call fails.
diff --git a/salt/runners/manage.py b/salt/runners/manage.py
index a47861065942..b9bf923e990d 100644
--- a/salt/runners/manage.py
+++ b/salt/runners/manage.py
@@ -578,7 +578,9 @@ def versions():
     ret = {}
     client = salt.client.get_local_client(__opts__["conf_file"])
     try:
-        minions = client.cmd("*", "test.version", timeout=__opts__["timeout"])
+        minions = client.cmd(
+            "*", "test.version", full_return=True, timeout=__opts__["timeout"]
+        )
     except SaltClientError as client_error:
         print(client_error)
         return ret
@@ -596,11 +598,11 @@ def versions():
     master_version = salt.version.__saltstack_version__
 
     for minion in minions:
-        if not minions[minion]:
+        if not minions[minion] or minions[minion]["retcode"]:
             minion_version = False
             ver_diff = -2
         else:
-            minion_version = salt.version.SaltStackVersion.parse(minions[minion])
+            minion_version = salt.version.SaltStackVersion.parse(minions[minion]["ret"])
             ver_diff = salt.utils.compat.cmp(minion_version, master_version)
 
         if ver_diff not in version_status:
diff --git a/tests/pytests/integration/runners/test_manage.py b/tests/pytests/integration/runners/test_manage.py
new file mode 100644
index 000000000000..1ff83b3d57ce
--- /dev/null
+++ b/tests/pytests/integration/runners/test_manage.py
@@ -0,0 +1,45 @@
+import pytest
+
+
+@pytest.fixture
+def blackout_minion(salt_minion, salt_master, salt_run_cli):
+    with pytest.helpers.temp_file(
+        "top.sls",
+        """base:\n  '*':\n    - fnord""",
+        salt_master.pillar_tree.base.paths[0],
+    ), pytest.helpers.temp_file(
+        "fnord.sls", """minion_blackout: True""", salt_master.pillar_tree.base.paths[0]
+    ):
+        op = salt_run_cli.run("saltutil.sync_all")
+        yield op
+
+
+@pytest.fixture
+def non_blackout_minion(salt_minion, salt_master, salt_call_cli):
+    with pytest.helpers.temp_file(
+        "top.sls",
+        """base:\n  '*':\n    - fnord""",
+        salt_master.pillar_tree.base.paths[0],
+    ), pytest.helpers.temp_file(
+        "fnord.sls", """minion_blackout: False""", salt_master.pillar_tree.base.paths[0]
+    ):
+        op = salt_call_cli.run("saltutil.refresh_pillar")
+        # Refresh pillar appears to return too quickly. This call ensures that
+        # we wait until the pillar refresh gets the correct value in there.
+        op = salt_call_cli.run("pillar.get", "minion_blackout")
+        yield op
+
+
+def test_when_manage_versions_runs_and_the_minion_errors_out_it_should_not_(
+    salt_run_cli, blackout_minion
+):
+    op = salt_run_cli.run("manage.versions")
+    assert "Unable to parse version string" not in op.stdout
+
+
+def test_when_manage_versions_runs_and_the_minion_does_not_error_it_should_have_version_info(
+    salt_run_cli, non_blackout_minion
+):
+    op = salt_run_cli.run("manage.versions")
+    result = op.stdout
+    assert "Up to date" in result


Code review comments:
diff_hunk: @@ -0,0 +1,45 @@
+import pytest
+
+
+@pytest.fixture
+def blackout_minion(salt_minion, salt_master, salt_run_cli):
+    with pytest.helpers.temp_file(
+        "top.sls",
+        """base:\n  '*':\n    - fnord""",
+        salt_master.pillar_tree.base.paths[0],
path: tests/pytests/integration/runners/test_manage.py
text: @user1:
```suggestion
        salt_master.pillar_tree.base.paths.write_path,
```
Should do the same.

@user1:
Actually, use `salt_master.pillar_tree.base.temp_file`

@author:
Done. Hopefully tests pass tonight :joy: