From 4c2984d78d640dee6f70fbc51f2993dfe09a71c6 Mon Sep 17 00:00:00 2001 From: "Ryan Addessi (raddessi)" Date: Mon, 19 Jun 2023 22:51:40 -0600 Subject: [PATCH 1/8] feat: Cache presence data between process restarts --- salt/master.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/salt/master.py b/salt/master.py index 9d2239bffbe2..4f11aa51d155 100644 --- a/salt/master.py +++ b/salt/master.py @@ -15,6 +15,7 @@ import sys import threading import time +import json import salt.acl import salt.auth @@ -258,7 +259,13 @@ def run(self): now = int(time.time()) git_pillar_update_interval = self.opts.get("git_pillar_update_interval", 0) - old_present = set() + # load the presence data from the cache on disk + presence_fpath = os.path.join(self.opts["cachedir"], "presence-data") + try: + with salt.utils.files.fopen(presence_fpath, "rb") as fpath: + old_present = set(json.loads(fpath.read())) + except (FileNotFoundError, TypeError, json.JSONDecodeError): + old_present = set() while time.time() - start < self.restart_interval: log.trace("Running maintenance routines") if not last or (now - last) >= self.loop_interval: @@ -375,6 +382,13 @@ def handle_presence(self, old_present): old_present.clear() old_present.update(present) + # update the cache on disk + presence_fpath = os.path.join(self.opts["cachedir"], "presence-data") + with salt.utils.files.fopen(presence_fpath, "wb") as fpath: + fpath.write( + salt.utils.stringutils.to_bytes(json.dumps(list(set(present)))) + ) + class FileserverUpdate(salt.utils.process.SignalHandlingProcess): """ @@ -698,7 +712,6 @@ def start(self): # manager. We don't want the processes being started to inherit those # signal handlers with salt.utils.process.default_signals(signal.SIGINT, signal.SIGTERM): - # Setup the secrets here because the PubServerChannel may need # them as well. SMaster.secrets["aes"] = { From c8a476cb97b92fecd0290c7f2a24c4a406d835bb Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Tue, 26 Sep 2023 15:22:38 -0600 Subject: [PATCH 2/8] change json storage to salt.utils.cache.CacheFactory --- salt/master.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/salt/master.py b/salt/master.py index 4f11aa51d155..73694e816ac6 100644 --- a/salt/master.py +++ b/salt/master.py @@ -15,7 +15,6 @@ import sys import threading import time -import json import salt.acl import salt.auth @@ -260,12 +259,15 @@ def run(self): git_pillar_update_interval = self.opts.get("git_pillar_update_interval", 0) # load the presence data from the cache on disk - presence_fpath = os.path.join(self.opts["cachedir"], "presence-data") + presence_cache = salt.utils.cache.CacheFactory.factory( + "disk", + 3600, + minion_cache_path=os.path.join(self.opts["cachedir"], "presence-data"), + ) try: - with salt.utils.files.fopen(presence_fpath, "rb") as fpath: - old_present = set(json.loads(fpath.read())) - except (FileNotFoundError, TypeError, json.JSONDecodeError): - old_present = set() + last_present = set(presence_cache["present"]) + except KeyError: + last_present = set() while time.time() - start < self.restart_interval: log.trace("Running maintenance routines") if not last or (now - last) >= self.loop_interval: @@ -277,7 +279,7 @@ def run(self): self.handle_git_pillar() self.handle_schedule() self.handle_key_cache() - self.handle_presence(old_present) + self.handle_presence(last_present) self.handle_key_rotate(now) salt.utils.verify.check_max_open_files(self.opts) last = now @@ -383,12 +385,12 @@ def handle_presence(self, old_present): old_present.update(present) # update the cache on disk - presence_fpath = os.path.join(self.opts["cachedir"], "presence-data") - with salt.utils.files.fopen(presence_fpath, "wb") as fpath: - fpath.write( - salt.utils.stringutils.to_bytes(json.dumps(list(set(present)))) - ) - + presence_cache = salt.utils.cache.CacheFactory.factory( + "disk", + 3600, + minion_cache_path=os.path.join(self.opts["cachedir"], "presence-data"), + ) + presence_cache["present"] = list(set(present)) class FileserverUpdate(salt.utils.process.SignalHandlingProcess): """ From b39daf443d3acfa15167a43bf8b458b6a1495a6b Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 27 Sep 2023 09:55:47 -0600 Subject: [PATCH 3/8] black --- salt/master.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/master.py b/salt/master.py index 73694e816ac6..c0e08b3dfb62 100644 --- a/salt/master.py +++ b/salt/master.py @@ -392,6 +392,7 @@ def handle_presence(self, old_present): ) presence_cache["present"] = list(set(present)) + class FileserverUpdate(salt.utils.process.SignalHandlingProcess): """ A process from which to update any dynamic fileserver backends From 5718ee7855752fc007941289a3b1711d1763cd1d Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 22 Nov 2023 17:18:43 -0700 Subject: [PATCH 4/8] add tests --- tests/pytests/unit/test_master.py | 92 ++++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-) diff --git a/tests/pytests/unit/test_master.py b/tests/pytests/unit/test_master.py index 502767d3e343..7f90c80d6e5f 100644 --- a/tests/pytests/unit/test_master.py +++ b/tests/pytests/unit/test_master.py @@ -1,10 +1,29 @@ import time +import os import pytest import salt.master import salt.utils.platform -from tests.support.mock import patch +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def maintenance_opts(master_opts): + """ + Options needed for master's Maintenence class + """ + opts = master_opts.copy() + opts.update(git_pillar_update_interval=180, maintenance_interval=181) + yield opts + + +@pytest.fixture +def maintenance(maintenance_opts): + """ + The master's Maintenence class + """ + return salt.master.Maintenance(maintenance_opts) @pytest.fixture @@ -160,3 +179,74 @@ def test_when_syndic_return_processes_load_then_correct_values_should_be_returne with patch.object(encrypted_requests, "_return", autospec=True) as fake_return: encrypted_requests._syndic_return(payload) fake_return.assert_called_with(expected_return) + + +@pytest.mark.parametrize( + "presence_cache_data,connected_ids", + [ + ( + ["minion1", "minion2"], + set(["minion1", "minion2"]), + ), + ( + ["minion1"], + set(["minion1", "minion2"]), + ), + ( + ["minion1", "minion2"], + set(["minion1"]), + ), + ] +) +def test_handle_presence(maintenance, presence_cache_data, connected_ids): + """ + Test the handle_presence function inside Maintenance class. + """ + fire_event = MagicMock() + + # populate the presence cache with old data + presence_cache = salt.utils.cache.CacheFactory.factory( + "disk", + 3600, + minion_cache_path=os.path.join(maintenance.opts["cachedir"], "presence-data"), + ) + presence_cache.clear() + presence_cache["present"] = presence_cache_data + + with patch( + "salt.master.Maintenance.run", MagicMock() + ), patch( + "salt.master.Maintenance.presence_events", True, create=True + ), patch( + "salt.master.Maintenance.event", + MagicMock( + connect_pull=MagicMock(return_value=True), + fire_event=fire_event, + ), + create=True, + ), patch( + "salt.master.Maintenance.ckminions", + MagicMock( + connected_ids=MagicMock(return_value=connected_ids) + ), + create=True, + ): + maintenance.handle_presence(set(presence_cache["present"])) + + assert fire_event.called + + args, _ = fire_event.call_args + assert set(args[0]["present"]) == connected_ids, ( + "The presence event sent does not contain the expected minions" + ) + + # reload the presence data from disk + new_presence_cache = salt.utils.cache.CacheFactory.factory( + "disk", + 3600, + minion_cache_path=os.path.join(maintenance.opts["cachedir"], "presence-data"), + ) + new_present = set(new_presence_cache["present"]) + assert new_present == connected_ids, ( + "The presence cache on disk does not contain the expected minions" + ) From 3b055dc1764624174519660f524ebbf885dc2632 Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 22 Nov 2023 17:23:11 -0700 Subject: [PATCH 5/8] added changelog --- changelog/64508.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/64508.fixed.md diff --git a/changelog/64508.fixed.md b/changelog/64508.fixed.md new file mode 100644 index 000000000000..d0b0d773b7d3 --- /dev/null +++ b/changelog/64508.fixed.md @@ -0,0 +1 @@ +Fixed incorrect minion presence events being sent out on hourly `Maintenance` process restarts \ No newline at end of file From d7fe712cabf1bc5930c247a05c7a49accb8154ae Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 22 Nov 2023 17:27:01 -0700 Subject: [PATCH 6/8] rename changelod --- changelog/{64508.fixed.md => 64505.fixed.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{64508.fixed.md => 64505.fixed.md} (100%) diff --git a/changelog/64508.fixed.md b/changelog/64505.fixed.md similarity index 100% rename from changelog/64508.fixed.md rename to changelog/64505.fixed.md From fd91dde0f96faeebd6ec5409ce2a823e99d50575 Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 22 Nov 2023 17:41:41 -0700 Subject: [PATCH 7/8] precommit --- tests/pytests/unit/test_master.py | 34 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/tests/pytests/unit/test_master.py b/tests/pytests/unit/test_master.py index 7f90c80d6e5f..ae3476cdd9c9 100644 --- a/tests/pytests/unit/test_master.py +++ b/tests/pytests/unit/test_master.py @@ -1,5 +1,5 @@ -import time import os +import time import pytest @@ -186,17 +186,17 @@ def test_when_syndic_return_processes_load_then_correct_values_should_be_returne [ ( ["minion1", "minion2"], - set(["minion1", "minion2"]), + {"minion1", "minion2"}, ), ( ["minion1"], - set(["minion1", "minion2"]), + {"minion1", "minion2"}, ), ( ["minion1", "minion2"], - set(["minion1"]), + {"minion1"}, ), - ] + ], ) def test_handle_presence(maintenance, presence_cache_data, connected_ids): """ @@ -213,9 +213,7 @@ def test_handle_presence(maintenance, presence_cache_data, connected_ids): presence_cache.clear() presence_cache["present"] = presence_cache_data - with patch( - "salt.master.Maintenance.run", MagicMock() - ), patch( + with patch("salt.master.Maintenance.run", MagicMock()), patch( "salt.master.Maintenance.presence_events", True, create=True ), patch( "salt.master.Maintenance.event", @@ -226,9 +224,7 @@ def test_handle_presence(maintenance, presence_cache_data, connected_ids): create=True, ), patch( "salt.master.Maintenance.ckminions", - MagicMock( - connected_ids=MagicMock(return_value=connected_ids) - ), + MagicMock(connected_ids=MagicMock(return_value=connected_ids)), create=True, ): maintenance.handle_presence(set(presence_cache["present"])) @@ -236,17 +232,19 @@ def test_handle_presence(maintenance, presence_cache_data, connected_ids): assert fire_event.called args, _ = fire_event.call_args - assert set(args[0]["present"]) == connected_ids, ( - "The presence event sent does not contain the expected minions" - ) + assert ( + set(args[0]["present"]) == connected_ids + ), "The presence event sent does not contain the expected minions" # reload the presence data from disk new_presence_cache = salt.utils.cache.CacheFactory.factory( "disk", 3600, - minion_cache_path=os.path.join(maintenance.opts["cachedir"], "presence-data"), + minion_cache_path=os.path.join( + maintenance.opts["cachedir"], "presence-data" + ), ) new_present = set(new_presence_cache["present"]) - assert new_present == connected_ids, ( - "The presence cache on disk does not contain the expected minions" - ) + assert ( + new_present == connected_ids + ), "The presence cache on disk does not contain the expected minions" From 2e4570bbd3f69bdffd85d5fcddef20aab2287787 Mon Sep 17 00:00:00 2001 From: Ryan Addessi Date: Wed, 22 Nov 2023 18:22:22 -0700 Subject: [PATCH 8/8] fix the test --- tests/pytests/unit/test_master.py | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/tests/pytests/unit/test_master.py b/tests/pytests/unit/test_master.py index ae3476cdd9c9..f86a63247773 100644 --- a/tests/pytests/unit/test_master.py +++ b/tests/pytests/unit/test_master.py @@ -5,7 +5,7 @@ import salt.master import salt.utils.platform -from tests.support.mock import MagicMock, patch +from tests.support.mock import MagicMock, call, patch @pytest.fixture @@ -182,23 +182,28 @@ def test_when_syndic_return_processes_load_then_correct_values_should_be_returne @pytest.mark.parametrize( - "presence_cache_data,connected_ids", + "presence_cache_data,connected_ids,change_expected", [ ( ["minion1", "minion2"], {"minion1", "minion2"}, + False, ), ( ["minion1"], {"minion1", "minion2"}, + True, ), ( ["minion1", "minion2"], {"minion1"}, + True, ), ], ) -def test_handle_presence(maintenance, presence_cache_data, connected_ids): +def test_handle_presence( + maintenance, presence_cache_data, connected_ids, change_expected +): """ Test the handle_presence function inside Maintenance class. """ @@ -231,9 +236,22 @@ def test_handle_presence(maintenance, presence_cache_data, connected_ids): assert fire_event.called - args, _ = fire_event.call_args + if change_expected: + assert fire_event.call_count == 2 + change_events = [ + call[0][0] + for call in fire_event.call_args_list + if "/change" in call[0][1] + ] + assert change_events + else: + assert fire_event.call_count == 1 + + present_event = [ + call[0][0] for call in fire_event.call_args_list if "/present" in call[0][1] + ][0] assert ( - set(args[0]["present"]) == connected_ids + set(present_event["present"]) == connected_ids ), "The presence event sent does not contain the expected minions" # reload the presence data from disk