Repository: saltstack/salt
Pull request: Beacon system fixes
### What does this PR do?

Have the beacon call run through a try...except, catching any errors logging and firing an event that includes the error. 
Fixing the swapusage beacon to ensure value is a tring before we attempt to filter out the %. Adding additional tests.
Moving tests/unit/test_beacons.py to tests/pytests/unit/test_beacons.py

### What issues does this PR fix or reference?
Fixes: #60585 

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltstack.com/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [x] Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes

Please review [Salt's Contributing Guide](https://docs.saltstack.com/en/master/topics/development/contributing.html) for best practices.

See GitHub's [page on GPG signing](https://help.github.com/articles/signing-commits-using-gpg/) for more information about signing commits with GPG.


Problem statement:
(config) error in one beacon affects all beacons
**Description**
When there is a (configuration) error in one beacon, other beacons are not reported.

**Setup**
Master plus a minion.

**Steps to Reproduce the behavior**
Activate a set of beacons on the minion, e.g. `load`, `status` and `swapusage`.
But configure `swapusage` as `{"percent":0}` instead of `{"percent":"0%"}`.
This causes an error in the `swapusage` beacon, but the results for the other 2 beacons are also not reported.

After inspecting the code in `beacons/__init__.py` line 128, it is clear that first all beacon values are collected and then reported.
any exception in that process is not caught and no futher beacon values are collected and none are reported.

Th following error message is visible for the original error:
```
2021-07-20 22:50:09,296 [salt.minion      :2947][CRITICAL][814] The beacon errored: 
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/salt/minion.py", line 2945, in handle_beacons
    beacons = self.process_beacons(self.functions)
  File "/usr/lib/python3/dist-packages/salt/minion.py", line 513, in process_beacons
    b_conf, self.opts["grains"]
  File "/usr/lib/python3/dist-packages/salt/beacons/__init__.py", line 128, in process
    raw = self.beacons[fun_str](b_config[mod])
  File "/usr/lib/python3/dist-packages/salt/loader.py", line 1241, in __call__
    return self.loader.run(run_func, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader.py", line 2274, in run
    return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/loader.py", line 2289, in _run_as
    return _func_or_method(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/beacons/swapusage.py", line 69, in beacon
    if "%" in monitor_usage:
TypeError: argument of type 'int' is not iterable
```

**Expected behavior**
An exception on one beacon should only affect that beacon, and not others from the same minion

And preferably (but that may be considered a feature request):
An exception in a beacon should emit a beacon event with the error message instead, so that the error does not go unnoticed.

**Versions Report**
<details><summary>salt --versions-report</summary>
(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) 

```
Salt Version:
          Salt: 3003.1
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: unknown
      dateutil: 2.7.3
     docker-py: 3.4.1
         gitdb: 2.0.5
     gitpython: 2.1.11
        Jinja2: 2.10
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: 1.0.7
       msgpack: 0.5.6
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: 2.6.1
  pycryptodome: 3.6.1
        pygit2: Not Installed
        Python: 3.7.3 (default, Jan 22 2021, 20:04:44)
  python-gnupg: Not Installed
        PyYAML: 3.13
         PyZMQ: 17.1.2
         smmap: 2.0.5
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.1
 
System Versions:
          dist: debian 10 buster
        locale: utf-8
       machine: x86_64
       release: 4.19.0-17-amd64
        system: Linux
       version: Debian GNU/Linux 10 buster
```
</details>

Hints from issue discussion:

Commit message: Merge branch 'master' into 60585_beacon_fixes

Patch to review:
diff --git a/changelog/60585.fixed b/changelog/60585.fixed
new file mode 100644
index 000000000000..65314cc2c90c
--- /dev/null
+++ b/changelog/60585.fixed
@@ -0,0 +1,2 @@
+Have the beacon call run through a try...except, catching any errors, logging and firing an event that includes the error.
+Fixing the swapusage beacon to ensure value is a tring before we attempt to filter out the %.
diff --git a/salt/beacons/__init__.py b/salt/beacons/__init__.py
index cd00b1527535..a9745ff1aae3 100644
--- a/salt/beacons/__init__.py
+++ b/salt/beacons/__init__.py
@@ -5,6 +5,7 @@
 import copy
 import logging
 import re
+import sys
 
 import salt.loader
 import salt.utils.event
@@ -125,16 +126,35 @@ def process(self, config, grains):
                         continue
 
                 # Fire the beacon!
-                raw = self.beacons[fun_str](b_config[mod])
-                for data in raw:
+                error = None
+                try:
+                    log.debug(self.beacons[fun_str])
+                    raw = self.beacons[fun_str](b_config[mod])
+                except:  # pylint: disable=bare-except
+                    error = "{}".format(sys.exc_info()[1])
+                    log.error("Unable to start %s beacon, %s", mod, error)
+                    # send beacon error event
                     tag = "salt/beacon/{}/{}/".format(self.opts["id"], mod)
-                    if "tag" in data:
-                        tag += data.pop("tag")
-                    if "id" not in data:
-                        data["id"] = self.opts["id"]
-                    ret.append({"tag": tag, "data": data, "beacon_name": beacon_name})
-                if runonce:
-                    self.disable_beacon(mod)
+                    ret.append(
+                        {
+                            "tag": tag,
+                            "error": error,
+                            "data": {},
+                            "beacon_name": beacon_name,
+                        }
+                    )
+                if not error:
+                    for data in raw:
+                        tag = "salt/beacon/{}/{}/".format(self.opts["id"], mod)
+                        if "tag" in data:
+                            tag += data.pop("tag")
+                        if "id" not in data:
+                            data["id"] = self.opts["id"]
+                        ret.append(
+                            {"tag": tag, "data": data, "beacon_name": beacon_name}
+                        )
+                    if runonce:
+                        self.disable_beacon(mod)
             else:
                 log.warning("Unable to process beacon %s", mod)
         return ret
diff --git a/tests/pytests/unit/beacons/test_swapusage.py b/tests/pytests/unit/beacons/test_swapusage.py
index 35b5ce28cdca..f05347a496a8 100644
--- a/tests/pytests/unit/beacons/test_swapusage.py
+++ b/tests/pytests/unit/beacons/test_swapusage.py
@@ -48,6 +48,15 @@ def test_swapusage_match(stub_swap_usage):
         ret = swapusage.beacon(config)
         assert ret == [{"swapusage": 9.7}]
 
+        # Test without the percent
+        config = [{"percent": "9"}, {"interval": 30}]
+
+        ret = swapusage.validate(config)
+        assert ret == (True, "Valid beacon configuration")
+
+        ret = swapusage.beacon(config)
+        assert ret == [{"swapusage": 9.7}]
+
 
 def test_swapusage_nomatch(stub_swap_usage):
     with patch("psutil.swap_memory", MagicMock(return_value=stub_swap_usage)):
diff --git a/tests/pytests/unit/test_beacons.py b/tests/pytests/unit/test_beacons.py
new file mode 100644
index 000000000000..27940c6f65e5
--- /dev/null
+++ b/tests/pytests/unit/test_beacons.py
@@ -0,0 +1,88 @@
+"""
+unit tests for the beacon_module parameter
+"""
+
+import logging
+
+import salt.config
+from tests.support.mock import MagicMock, call, patch
+
+log = logging.getLogger(__name__)
+
+
+def test_beacon_process():
+    """
+    Test the process function in the beacon class
+    returns the correct information when an exception
+    occurs
+    """
+    mock_opts = salt.config.DEFAULT_MINION_OPTS.copy()
+    mock_opts["id"] = "minion"
+    mock_opts["__role"] = "minion"
+    mock_opts["beacons"] = {
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ]
+    }
+    beacon_mock = MagicMock(side_effect=Exception("Global Thermonuclear War"))
+    beacon_mock.__globals__ = {}
+
+    beacon = salt.beacons.Beacon(mock_opts, [])
+
+    found = "ps.beacon" in beacon.beacons
+    beacon.beacons["ps.beacon"] = beacon_mock
+    ret = beacon.process(mock_opts["beacons"], mock_opts["grains"])
+
+    _expected = [
+        {
+            "tag": "salt/beacon/minion/watch_apache/",
+            "error": "Global Thermonuclear War",
+            "data": {},
+            "beacon_name": "ps",
+        }
+    ]
+    assert ret == _expected
+
+
+def test_beacon_module():
+    """
+    Test that beacon_module parameter for beacon configuration
+    """
+    mock_opts = salt.config.DEFAULT_MINION_OPTS.copy()
+    mock_opts["id"] = "minion"
+    mock_opts["__role"] = "minion"
+    mock_opts["beacons"] = {
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ]
+    }
+    beacon = salt.beacons.Beacon(mock_opts, [])
+    ret = beacon.process(mock_opts["beacons"], mock_opts["grains"])
+
+    _expected = [
+        {
+            "tag": "salt/beacon/minion/watch_apache/",
+            "data": {"id": "minion", "apache2": "Stopped"},
+            "beacon_name": "ps",
+        }
+    ]
+    assert ret == _expected
+
+    # Ensure that "beacon_name" is available in the call to the beacon function
+    name = "ps.beacon"
+    mocked = {name: MagicMock(return_value=_expected)}
+    mocked[name].__globals__ = {}
+    calls = [
+        call(
+            [
+                {"processes": {"apache2": "stopped"}},
+                {"beacon_module": "ps"},
+                {"_beacon_name": "watch_apache"},
+            ]
+        )
+    ]
+    with patch.object(beacon, "beacons", mocked) as patched:
+        beacon.process(mock_opts["beacons"], mock_opts["grains"])
+        patched[name].assert_has_calls(calls)
diff --git a/tests/unit/test_beacons.py b/tests/unit/test_beacons.py
deleted file mode 100644
index b7a5127179f9..000000000000
--- a/tests/unit/test_beacons.py
+++ /dev/null
@@ -1,65 +0,0 @@
-"""
-unit tests for the beacon_module parameter
-"""
-
-import logging
-
-import salt.beacons as beacons
-import salt.config
-from tests.support.mixins import LoaderModuleMockMixin
-from tests.support.mock import MagicMock, call, patch
-from tests.support.unit import TestCase
-
-log = logging.getLogger(__name__)
-
-
-class BeaconsTestCase(TestCase, LoaderModuleMockMixin):
-    """
-    Test cases for salt beacon_module parameter
-    """
-
-    def setup_loader_modules(self):
-        return {beacons: {}}
-
-    def test_beacon_module(self):
-        """
-        Test that beacon_module parameter for beacon configuration
-        """
-        mock_opts = salt.config.DEFAULT_MINION_OPTS.copy()
-        mock_opts["id"] = "minion"
-        mock_opts["__role"] = "minion"
-        mock_opts["beacons"] = {
-            "watch_apache": [
-                {"processes": {"apache2": "stopped"}},
-                {"beacon_module": "ps"},
-            ]
-        }
-        with patch.dict(beacons.__opts__, mock_opts):
-            beacon = salt.beacons.Beacon(mock_opts, [])
-            ret = beacon.process(mock_opts["beacons"], mock_opts["grains"])
-
-            _expected = [
-                {
-                    "tag": "salt/beacon/minion/watch_apache/",
-                    "data": {"id": "minion", "apache2": "Stopped"},
-                    "beacon_name": "ps",
-                }
-            ]
-            self.assertEqual(ret, _expected)
-
-            # Ensure that "beacon_name" is available in the call to the beacon function
-            name = "ps.beacon"
-            mocked = {name: MagicMock(return_value=_expected)}
-            mocked[name].__globals__ = {}
-            calls = [
-                call(
-                    [
-                        {"processes": {"apache2": "stopped"}},
-                        {"beacon_module": "ps"},
-                        {"_beacon_name": "watch_apache"},
-                    ]
-                )
-            ]
-            with patch.object(beacon, "beacons", mocked) as patched:
-                beacon.process(mock_opts["beacons"], mock_opts["grains"])
-                patched[name].assert_has_calls(calls)


Code review comments:
diff_hunk: @@ -125,16 +126,35 @@ def process(self, config, grains):
                         continue
 
                 # Fire the beacon!
-                raw = self.beacons[fun_str](b_config[mod])
-                for data in raw:
+                error = None
+                try:
+                    log.debug(self.beacons[fun_str])
path: salt/beacons/__init__.py
text: @user1:
What does this print out? I ask, because my concern is it might not be useful to an end user. 

@author:
Left over from some debugging.  Removed.