Fix beacon delete not closing beacon modules

changelog/66449.fixed.md

@@ -0,0 +1,4 @@
+Fixed beacon delete not calling the beacon's close function, causing resource
+leaks (e.g. inotify file descriptors) and CPU spin after deleting beacons at
+runtime via ``beacons.delete``. Also fixed inotify file descriptor leak during
+beacon refresh when the Beacon instance is replaced.

salt/beacons/__init__.py

@@ -26,6 +26,55 @@ def __init__(self, opts, functions, interval_map=None):
         self.beacons = salt.loader.beacons(opts, functions)
         self.interval_map = interval_map or dict()
 
+    def _close_beacon(self, name):
+        """
+        Close a single beacon module if it has a close function.
+        This releases resources like inotify file descriptors.
+        """
+        beacon_config = self.opts["beacons"].get(name)
+        if beacon_config is None:
+            return
+
+        current_beacon_config = None
+        if isinstance(beacon_config, list):
+            current_beacon_config = {}
+            list(map(current_beacon_config.update, beacon_config))
+        elif isinstance(beacon_config, dict):
+            current_beacon_config = beacon_config
+
+        if current_beacon_config is None:
+            return
+
+        beacon_name = name
+        if self._determine_beacon_config(current_beacon_config, "beacon_module"):
+            beacon_name = current_beacon_config["beacon_module"]
+
+        close_str = f"{beacon_name}.close"
+        if close_str in self.beacons:
+            try:
+                config = copy.deepcopy(beacon_config)
+                if isinstance(config, list):
+                    config.append({"_beacon_name": name})
+                log.debug("Closing beacon %s", name)
+                self.beacons[close_str](config)
+            except Exception:  # pylint: disable=broad-except
+                log.debug("Failed to close beacon %s", name, exc_info=True)
+
+    def close_beacons(self):
+        """
+        Close all beacon modules that have a close function.
+        This ensures resources like inotify file descriptors are properly
+        released when beacons are refreshed or the Beacon instance is replaced.
+
+        See: https://github.com/saltstack/salt/issues/66449
+        See: https://github.com/saltstack/salt/issues/58907
+        """
+        beacons = self._get_beacons()
+        for mod in beacons:
+            if mod == "enabled":
+                continue
+            self._close_beacon(mod)
+
     def process(self, config, grains):
         """
         Process the configured beacons
@@ -405,6 +454,9 @@ def delete_beacon(self, name):
             complete = False
         else:
             if name in self.opts["beacons"]:
+                # Close the beacon module to release resources (e.g. inotify fds)
+                # before removing it from the configuration.
+                self._close_beacon(name)
                 del self.opts["beacons"][name]
                 comment = f"Deleting beacon item: {name}"
             else:

salt/minion.py

@@ -3203,6 +3203,10 @@ def beacons_refresh(self):
         prev_interval_map = {}
         if hasattr(self, "beacons") and hasattr(self.beacons, "interval_map"):
             prev_interval_map = self.beacons.interval_map
+        # Close existing beacon modules to release resources (e.g. inotify fds)
+        # before replacing the Beacon instance.
+        if hasattr(self, "beacons"):
+            self.beacons.close_beacons()
         self.beacons = salt.beacons.Beacon(
             self.opts, self.functions, interval_map=prev_interval_map
         )

tests/pytests/unit/test_beacons.py

@@ -121,3 +121,151 @@ def test_beacon_module(minion_opts):
     with patch.object(beacon, "beacons", mocked) as patched:
         beacon.process(minion_opts["beacons"], minion_opts["grains"])
         patched[name].assert_has_calls(calls)
+
+
+def test_close_beacons_calls_close_on_modules(minion_opts):
+    """
+    Test that close_beacons() calls the close function on each beacon
+    module that provides one, releasing resources like inotify fds.
+
+    See: https://github.com/saltstack/salt/issues/66449
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    close_mock = MagicMock()
+    beacon.beacons["inotify.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert isinstance(call_args, list)
+    assert {"_beacon_name": "inotify"} in call_args
+
+
+def test_close_beacons_with_beacon_module_override(minion_opts):
+    """
+    Test that close_beacons() respects beacon_module and calls close
+    on the correct underlying module name.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    close_mock = MagicMock()
+    beacon.beacons["ps.close"] = close_mock
+
+    beacon.close_beacons()
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert {"_beacon_name": "watch_apache"} in call_args
+
+
+def test_close_beacons_skips_modules_without_close(minion_opts):
+    """
+    Test that close_beacons() gracefully skips beacons that don't
+    have a close function.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "status": [
+            {"time": ["all"]},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+
+    assert "status.close" not in beacon.beacons
+    beacon.close_beacons()
+
+
+def test_delete_beacon_calls_close(minion_opts):
+    """
+    Test that delete_beacon() calls the beacon's close function before
+    removing it, so resources like inotify file descriptors are released.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "inotify": [
+            {"files": {"/etc/fstab": {}}},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    close_mock = MagicMock()
+    beacon.beacons["inotify.close"] = close_mock
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("inotify")
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert isinstance(call_args, list)
+    assert {"_beacon_name": "inotify"} in call_args
+    assert "inotify" not in minion_opts["beacons"]
+
+
+def test_delete_beacon_calls_close_with_beacon_module(minion_opts):
+    """
+    Test that delete_beacon() respects beacon_module and calls close
+    on the correct underlying module.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "watch_apache": [
+            {"processes": {"apache2": "stopped"}},
+            {"beacon_module": "ps"},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    close_mock = MagicMock()
+    beacon.beacons["ps.close"] = close_mock
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("watch_apache")
+
+    close_mock.assert_called_once()
+    call_args = close_mock.call_args[0][0]
+    assert {"_beacon_name": "watch_apache"} in call_args
+    assert "watch_apache" not in minion_opts["beacons"]
+
+
+def test_delete_beacon_without_close(minion_opts):
+    """
+    Test that delete_beacon() works when the beacon module has no close function.
+    """
+    minion_opts["id"] = "minion"
+    minion_opts["__role"] = "minion"
+    minion_opts["beacons"] = {
+        "status": [
+            {"time": ["all"]},
+        ],
+    }
+
+    beacon = salt.beacons.Beacon(minion_opts, [])
+    assert "status.close" not in beacon.beacons
+
+    with patch("salt.utils.event.get_event"):
+        beacon.delete_beacon("status")
+
+    assert "status" not in minion_opts["beacons"]