diff --git a/changelog/66449.fixed.md b/changelog/66449.fixed.md new file mode 100644 index 00000000000..99307c1a2d4 --- /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 c5115104630..a9aa0bfa16a 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 d56f9110c88..295928e40d8 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 217cd5c6a4d..c9e5a3758d0 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 2a8109301be..ea8dadc4eef 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,