From b8c71d263b0ecf5571bfaa988d213c8608be8c3b Mon Sep 17 00:00:00 2001 From: sb002465 Date: Fri, 20 Mar 2026 13:12:53 -0700 Subject: [PATCH] Fix inotify file descriptor leak during beacon refresh When beacons_refresh() creates a new Beacon instance, the old instance's beacon modules (e.g. inotify) held open file descriptors that were never closed. Each new Beacon gets a fresh empty __context__ dict via LazyLoader, so inotify's _get_notifier() creates a new pyinotify.Notifier while the old one is orphaned with its fds still open. Over repeated refreshes, this exhausts the inotify instance limit (default 128 on RHEL8), causing "Too many open files (EMFILE)" errors. Add close_beacons() to the Beacon class that calls close() on each beacon module before the instance is replaced. Also add __del__() as a safety net for garbage collection, and call close_beacons() explicitly from Minion.beacons_refresh() before creating a new Beacon instance. Fixes: https://github.com/saltstack/salt/issues/66449 Fixes: https://github.com/saltstack/salt/issues/58907 Made-with: Cursor --- changelog/66449.fixed.md | 4 + salt/beacons/__init__.py | 41 +++++++ salt/minion.py | 4 + tests/pytests/unit/test_beacons.py | 166 +++++++++++++++++++++++++++++ tests/pytests/unit/test_minion.py | 40 +++++++ 5 files changed, 255 insertions(+) create mode 100644 changelog/66449.fixed.md diff --git a/changelog/66449.fixed.md b/changelog/66449.fixed.md new file mode 100644 index 000000000000..99307c1a2d4c --- /dev/null +++ b/changelog/66449.fixed.md @@ -0,0 +1,4 @@ +Fixed inotify file descriptor leak in beacons. When beacons are refreshed +(e.g. during module refresh or pillar refresh), the old beacon modules are now +properly closed before creating new ones, preventing exhaustion of the inotify +instance limit. diff --git a/salt/beacons/__init__.py b/salt/beacons/__init__.py index c51151046308..a9aa0bfa16a7 100644 --- a/salt/beacons/__init__.py +++ b/salt/beacons/__init__.py @@ -26,6 +26,47 @@ def __init__(self, opts, functions, interval_map=None): self.beacons = salt.loader.beacons(opts, functions) self.interval_map = interval_map or dict() + 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 + + current_beacon_config = None + if isinstance(beacons[mod], list): + current_beacon_config = {} + list(map(current_beacon_config.update, beacons[mod])) + elif isinstance(beacons[mod], dict): + current_beacon_config = beacons[mod] + + if current_beacon_config is None: + continue + + beacon_name = None + if self._determine_beacon_config(current_beacon_config, "beacon_module"): + beacon_name = current_beacon_config["beacon_module"] + else: + beacon_name = mod + + close_str = f"{beacon_name}.close" + if close_str in self.beacons: + try: + config = copy.deepcopy(beacons[mod]) + if isinstance(config, list): + config.append({"_beacon_name": mod}) + log.debug("Closing beacon %s", mod) + self.beacons[close_str](config) + except Exception: # pylint: disable=broad-except + log.debug("Failed to close beacon %s", mod, exc_info=True) + def process(self, config, grains): """ Process the configured beacons diff --git a/salt/minion.py b/salt/minion.py index d56f9110c884..295928e40d8b 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -3202,6 +3202,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..c9e5a3758d02 100644 --- a/tests/pytests/unit/test_beacons.py +++ b/tests/pytests/unit/test_beacons.py @@ -121,3 +121,169 @@ 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_close_beacons_handles_close_exception(minion_opts): + """ + Test that close_beacons() does not propagate exceptions raised + by a beacon's close function. + """ + minion_opts["id"] = "minion" + minion_opts["__role"] = "minion" + minion_opts["beacons"] = { + "inotify": [ + {"files": {"/etc/fstab": {}}}, + ], + } + + beacon = salt.beacons.Beacon(minion_opts, []) + beacon.beacons["inotify.close"] = MagicMock(side_effect=Exception("close failed")) + + beacon.close_beacons() + + +def test_close_beacons_multiple_beacons(minion_opts): + """ + Test that close_beacons() calls close on all beacons that have + a close function. + """ + minion_opts["id"] = "minion" + minion_opts["__role"] = "minion" + minion_opts["beacons"] = { + "inotify": [ + {"files": {"/etc/fstab": {}}}, + ], + "watch_apache": [ + {"processes": {"apache2": "stopped"}}, + {"beacon_module": "ps"}, + ], + } + + beacon = salt.beacons.Beacon(minion_opts, []) + + inotify_close = MagicMock() + ps_close = MagicMock() + beacon.beacons["inotify.close"] = inotify_close + beacon.beacons["ps.close"] = ps_close + + beacon.close_beacons() + + inotify_close.assert_called_once() + ps_close.assert_called_once() + + +def test_close_beacons_skips_enabled_key(minion_opts): + """ + Test that close_beacons() skips the 'enabled' key in the beacons config. + """ + minion_opts["id"] = "minion" + minion_opts["__role"] = "minion" + minion_opts["beacons"] = { + "enabled": True, + "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() + + +def test_close_beacons_dict_config(minion_opts): + """ + Test that close_beacons() handles dict-style beacon configuration + (backwards-compatible format). + """ + minion_opts["id"] = "minion" + minion_opts["__role"] = "minion" + minion_opts["beacons"] = { + "status": {"time": ["all"]}, + } + + beacon = salt.beacons.Beacon(minion_opts, []) + close_mock = MagicMock() + beacon.beacons["status.close"] = close_mock + + beacon.close_beacons() + + close_mock.assert_called_once() + call_args = close_mock.call_args[0][0] + assert isinstance(call_args, dict) diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index 2a8109301be9..ea8dadc4eef1 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -702,6 +702,46 @@ def test_beacons_refresh_preserves_interval_map(minion_opts): minion.destroy() +def test_beacons_refresh_closes_old_beacons(minion_opts): + """ + Tests that 'beacons_refresh' calls close_beacons() on the old Beacon + instance before replacing it, preventing inotify fd leaks. + + See: https://github.com/saltstack/salt/issues/66449 + See: https://github.com/saltstack/salt/issues/58907 + """ + with patch("salt.minion.Minion.ctx", MagicMock(return_value={})), patch( + "salt.utils.process.SignalHandlingProcess.start", + MagicMock(return_value=True), + ), patch( + "salt.utils.process.SignalHandlingProcess.join", + MagicMock(return_value=True), + ): + minion = None + try: + minion = salt.minion.Minion( + minion_opts, + io_loop=salt.ext.tornado.ioloop.IOLoop.current(), + ) + minion.schedule = salt.utils.schedule.Schedule( + minion_opts, {}, returners={} + ) + + minion.module_refresh() + assert hasattr(minion, "beacons") + + old_beacons = minion.beacons + with patch.object(old_beacons, "close_beacons") as close_mock: + minion.beacons_refresh() + close_mock.assert_called_once() + + assert minion.beacons is not old_beacons + + finally: + if minion is not None: + minion.destroy() + + @pytest.mark.slow_test async def test_when_ping_interval_is_set_the_callback_should_be_added_to_periodic_callbacks( minion_opts,