diff --git a/changelog/66449.fixed.md b/changelog/66449.fixed.md new file mode 100644 index 000000000000..0b6f103460ba --- /dev/null +++ b/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. diff --git a/salt/beacons/__init__.py b/salt/beacons/__init__.py index 75b442ac7d93..625511d83ae1 100644 --- a/salt/beacons/__init__.py +++ b/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: diff --git a/salt/minion.py b/salt/minion.py index d6bb8870a831..2230c6869cbe 100644 --- a/salt/minion.py +++ b/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 ) diff --git a/tests/pytests/unit/test_beacons.py b/tests/pytests/unit/test_beacons.py index 217cd5c6a4da..82a91086b181 100644 --- a/tests/pytests/unit/test_beacons.py +++ b/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"]