From 398baf64a842302e2aec9bab32be011215f73063 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Tue, 9 Jun 2026 13:08:40 +0200 Subject: [PATCH 1/8] Add unit tests for SONiC exporter module Cover save_config_to_netbox (NetBox local-context persistence with diff checking and journal logging) and export_config_to_file (on-disk export, filename selection, and the serial-number to hostname symlink). The file-export tests drive a real filesystem under tmp_path where that reads more clearly than asserting on mocked os calls, and patch os.* only to inject the symlink and makedirs failure paths. Part of #2199. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- .../tasks/conductor/sonic/test_exporter.py | 385 ++++++++++++++++++ 1 file changed, 385 insertions(+) create mode 100644 tests/unit/tasks/conductor/sonic/test_exporter.py diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py new file mode 100644 index 00000000..f40bf58e --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -0,0 +1,385 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for ``osism.tasks.conductor.sonic.exporter``. + +Covers ``save_config_to_netbox`` (NetBox local-context persistence with diff +checking + journal logging) and ``export_config_to_file`` (on-disk export with +diff checking, filename selection, and the serial-number→hostname symlink). + +``save_config_to_netbox`` reuses the shared ``mock_nb`` fixture (it patches +``osism.utils.nb``, which ``exporter.utils.nb`` resolves to). The file-export +tests drive a real filesystem under ``tmp_path`` where that reads more clearly +than asserting on mocked ``os`` calls, and fall back to patching ``os.*`` only +to inject the two error paths (symlink / makedirs failures). +""" + +import json +import os +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from osism.tasks.conductor.sonic.exporter import ( + export_config_to_file, + save_config_to_netbox, +) + + +def _has_log(records, level, substring): + return any(r["level"] == level and substring in r["message"] for r in records) + + +# --------------------------------------------------------------------------- +# save_config_to_netbox +# --------------------------------------------------------------------------- + + +def _make_save_device(local_context_data=None, device_id=1, name="sw-1"): + """Build a NetBox-shaped device whose ``save`` is observable.""" + return SimpleNamespace( + id=device_id, + name=name, + local_context_data=local_context_data, + save=MagicMock(), + ) + + +def test_save_config_first_time_saves_and_returns_true(mock_nb): + """No existing local context → first-time path saves and reports change.""" + device = _make_save_device(local_context_data=None) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet0": {}}}) + + assert result is True + device.save.assert_called_once_with() + assert device.local_context_data == {"sonic_config": {"PORT": {"Ethernet0": {}}}} + # First-time configuration creates no journal entry. + mock_nb.extras.journal_entries.create.assert_not_called() + + +def test_save_config_first_time_return_diff_tuple(mock_nb): + """``return_diff=True`` on the first-time path yields ``(True, None)``.""" + device = _make_save_device(local_context_data=None) + + result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert result == (True, None) + + +def test_save_config_no_change_skips_save(mock_nb): + """Identical existing context → ``DeepDiff`` empty, no save, returns False.""" + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device(local_context_data={"sonic_config": config}) + + result = save_config_to_netbox(device, config) + + assert result is False + device.save.assert_not_called() + + +def test_save_config_no_change_return_diff_tuple(mock_nb): + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device(local_context_data={"sonic_config": config}) + + result = save_config_to_netbox(device, config, return_diff=True) + + assert result == (False, None) + + +def test_save_config_change_writes_journal_and_saves(mock_nb): + """A changed config generates a diff, a journal entry, and a save.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + changed, diff_text = save_config_to_netbox( + device, {"PORT": {"Ethernet1": {}}}, return_diff=True + ) + + assert changed is True + # The unified diff names both the removed and the added port. + assert "Ethernet0" in diff_text + assert "Ethernet1" in diff_text + device.save.assert_called_once_with() + mock_nb.extras.journal_entries.create.assert_called_once() + kwargs = mock_nb.extras.journal_entries.create.call_args.kwargs + assert kwargs["assigned_object_type"] == "dcim.device" + assert kwargs["assigned_object_id"] == device.id + assert kwargs["kind"] == "info" + assert "```diff" in kwargs["comments"] + + +def test_save_config_change_bool_form(mock_nb): + """Without ``return_diff`` the changed path returns a bare ``True``.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}) + + assert result is True + + +def test_save_config_journal_failure_still_saves(mock_nb, loguru_logs): + """A failing journal create is logged but must not block the save.""" + mock_nb.extras.journal_entries.create.side_effect = RuntimeError("journal down") + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + changed, diff_text = save_config_to_netbox( + device, {"PORT": {"Ethernet1": {}}}, return_diff=True + ) + + assert changed is True + assert diff_text is not None + device.save.assert_called_once_with() + assert _has_log(loguru_logs, "ERROR", "Failed to save diff to journal") + + +def test_save_config_save_failure_returns_false(mock_nb, loguru_logs): + """A raising ``device.save()`` is caught, logged, and reported as no-change.""" + device = _make_save_device(local_context_data=None) + device.save.side_effect = RuntimeError("netbox write failed") + + result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert result == (False, None) + assert _has_log(loguru_logs, "ERROR", "Failed to save local context") + + +def test_save_config_save_failure_bool_form(mock_nb): + device = _make_save_device(local_context_data=None) + device.save.side_effect = RuntimeError("boom") + + assert save_config_to_netbox(device, {"PORT": {}}) is False + + +# --------------------------------------------------------------------------- +# export_config_to_file +# --------------------------------------------------------------------------- + + +@pytest.fixture +def export_settings(mocker): + """Patch the four ``SONIC_EXPORT_*`` settings exporter reads at call time.""" + + def _set( + export_dir, + identifier="serial-number", + prefix="osism_", + suffix="_config_db.json", + ): + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_DIR", + str(export_dir), + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_PREFIX", + prefix, + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_SUFFIX", + suffix, + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_IDENTIFIER", + identifier, + ) + + return _set + + +@pytest.fixture +def patch_hostname(mocker): + """Pin ``get_device_hostname`` so filenames are deterministic.""" + return mocker.patch( + "osism.tasks.conductor.sonic.exporter.get_device_hostname", + return_value="sw-1", + ) + + +# --- filename selection ----------------------------------------------------- + + +def test_export_hostname_identifier_uses_hostname( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, config) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert json.loads(target.read_text()) == config + + +def test_export_serial_identifier_uses_serial( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert (tmp_path / "osism_ABC123_config_db.json").exists() + + +@pytest.mark.parametrize("serial", ["", None]) +def test_export_serial_missing_falls_back_to_hostname( + tmp_path, export_settings, patch_hostname, loguru_logs, serial +): + """An empty or absent serial in serial-number mode warns and uses hostname, + and the symlink branch is skipped entirely (debug logged).""" + export_settings(tmp_path, identifier="serial-number") + device = ( + SimpleNamespace(name="sw-1", serial=serial) + if serial is not None + else SimpleNamespace(name="sw-1") + ) + + assert export_config_to_file(device, {"PORT": {}}) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + assert _has_log(loguru_logs, "WARNING", "Serial number not found") + assert _has_log(loguru_logs, "DEBUG", "Symlink conditions not met") + + +# --- diff handling ---------------------------------------------------------- + + +def test_export_no_change_returns_false(tmp_path, export_settings, patch_hostname): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_sw-1_config_db.json").write_text(json.dumps(config)) + + assert export_config_to_file(device, config) is False + + +def test_export_changed_content_rewrites_file( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + target.write_text(json.dumps({"PORT": {"Ethernet9": {}}})) + new_config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, new_config) is True + assert json.loads(target.read_text()) == new_config + + +def test_export_unreadable_file_is_overwritten( + tmp_path, export_settings, patch_hostname, loguru_logs +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + target.write_text("not valid json {") + new_config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, new_config) is True + assert json.loads(target.read_text()) == new_config + assert _has_log(loguru_logs, "WARNING", "Could not read existing config file") + + +# --- symlink handling ------------------------------------------------------- + + +def test_export_creates_symlink_when_hostname_absent( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + link = tmp_path / "osism_sw-1_config_db.json" + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_replaces_existing_regular_file_with_symlink( + tmp_path, export_settings, patch_hostname +): + """A regular file at the hostname path is removed before the symlink is + created — observable because the path ends up as a symlink, not a file.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + link = tmp_path / "osism_sw-1_config_db.json" + link.write_text("stale regular file") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_replaces_dangling_symlink(tmp_path, export_settings, patch_hostname): + """A dangling symlink (``exists`` False, ``islink`` True) is removed and + repointed at the serial-number file.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("does_not_exist_target") + assert not os.path.exists(link) and os.path.islink(link) + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_symlink_failure_still_returns_true( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + """A failing ``os.symlink`` is logged but the written config still counts + as a change (the file was exported successfully).""" + export_settings(tmp_path, identifier="serial-number") + mocker.patch( + "osism.tasks.conductor.sonic.exporter.os.symlink", + side_effect=OSError("permission denied"), + ) + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert (tmp_path / "osism_ABC123_config_db.json").exists() + assert _has_log(loguru_logs, "ERROR", "Failed to create hostname symlink") + + +def test_export_hostname_mode_makes_no_symlink( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + + +# --- error handling --------------------------------------------------------- + + +def test_export_makedirs_failure_returns_false( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + export_settings(tmp_path, identifier="hostname") + mocker.patch( + "osism.tasks.conductor.sonic.exporter.os.makedirs", + side_effect=OSError("read-only filesystem"), + ) + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is False + + assert _has_log(loguru_logs, "ERROR", "Failed to export config") From f13adf09b06ec4e618c2c3d41fd798a1bb9a56e9 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Tue, 9 Jun 2026 13:08:40 +0200 Subject: [PATCH 2/8] Add unit tests for SONiC sync orchestrator Cover the sync_sonic entry point: cache lifecycle ordering, single-device and NetBox-filtered device resolution, role/HWSKU filtering, spine and superspine AS-mapping propagation, diff streaming to task output, and cache-stats logging. Every heavy collaborator (generate_sonic_config, find_interconnected_devices, NetBox access) is stubbed at its sync import site so only the orchestration glue is exercised; those modules are covered separately under #2199. Part of #2199. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- tests/unit/tasks/conductor/sonic/test_sync.py | 474 ++++++++++++++++++ 1 file changed, 474 insertions(+) create mode 100644 tests/unit/tasks/conductor/sonic/test_sync.py diff --git a/tests/unit/tasks/conductor/sonic/test_sync.py b/tests/unit/tasks/conductor/sonic/test_sync.py new file mode 100644 index 00000000..d8701d06 --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_sync.py @@ -0,0 +1,474 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for ``osism.tasks.conductor.sonic.sync.sync_sonic``. + +``sync_sonic`` is the orchestration entry point: it manages the cache +lifecycle, resolves the device set (single-device or NetBox-filtered), +computes spine/superspine AS mappings, and drives config generation, NetBox +persistence, and file export per device. Every heavy collaborator is stubbed +at its import site in ``sync`` so only the orchestration glue is exercised +here — ``find_interconnected_devices`` and ``generate_sonic_config`` have their +own coverage (see #2199) and are intentionally mocked. + +``utils.nb`` is supplied by the shared ``mock_nb`` fixture (it patches +``osism.utils.nb``, which ``sync.utils.nb`` resolves to). +""" + +from types import SimpleNamespace +from unittest.mock import call + +import pytest + +from osism.tasks.conductor.sonic.sync import sync_sonic + + +def _has_log(records, level, substring): + return any(r["level"] == level and substring in r["message"] for r in records) + + +def make_device( + name="sw-1", + device_id=1, + role_slug="leaf", + hwsku="Accton-AS7326-56X", + config_version=None, +): + """Build a NetBox-shaped device the orchestrator can consume. + + ``role_slug=None`` yields ``role=None``; ``hwsku=None`` yields an empty + ``sonic_parameters`` so the HWSKU-missing branch is reachable. + """ + params = {} + if hwsku is not None: + params["hwsku"] = hwsku + if config_version is not None: + params["config_version"] = config_version + role = SimpleNamespace(slug=role_slug) if role_slug is not None else None + return SimpleNamespace( + id=device_id, + name=name, + serial="ABC", + role=role, + local_context_data=None, + custom_fields={"sonic_parameters": params}, + config_context={}, + ) + + +@pytest.fixture +def patch_sync_deps(mocker): + """Stub every heavy collaborator ``sync_sonic`` imports. + + Returns a ``SimpleNamespace`` of the mocks so tests can tune individual + return values / side effects. Each name is patched at its ``sync`` import + site, not at its source module, so the rebound reference is the one that + gets replaced. + """ + + def patch(name, **kw): + return mocker.patch(f"osism.tasks.conductor.sonic.sync.{name}", **kw) + + return SimpleNamespace( + get_nb_device_query_list_sonic=patch( + "get_nb_device_query_list_sonic", return_value=[] + ), + find_interconnected_devices=patch( + "find_interconnected_devices", return_value=[] + ), + calculate_minimum_as_for_group=patch( + "calculate_minimum_as_for_group", return_value=None + ), + generate_sonic_config=patch( + "generate_sonic_config", return_value={"PORT": {"Ethernet0": {}}} + ), + save_config_to_netbox=patch("save_config_to_netbox", return_value=(True, None)), + export_config_to_file=patch("export_config_to_file", return_value=False), + clear_interface_cache=patch("clear_interface_cache"), + clear_all_caches=patch("clear_all_caches"), + clear_vip_addresses_cache=patch("clear_vip_addresses_cache"), + _load_metalbox_devices_cache=patch("_load_metalbox_devices_cache"), + load_vip_addresses_cache=patch("load_vip_addresses_cache"), + get_interface_cache_stats=patch("get_interface_cache_stats", return_value={}), + push_task_output=patch("utils.push_task_output"), + finish_task_output=patch("utils.finish_task_output"), + ) + + +# --------------------------------------------------------------------------- +# Cache lifecycle +# --------------------------------------------------------------------------- + + +def test_cache_lifecycle_clears_in_order(mock_nb, patch_sync_deps, mocker): + """Caches are cleared at start (interface, all) and end (interface, all, + vip), with the two cache loads in between — pinned as an exact sequence.""" + deps = patch_sync_deps + manager = mocker.Mock() + manager.attach_mock(deps.clear_interface_cache, "clear_interface_cache") + manager.attach_mock(deps.clear_all_caches, "clear_all_caches") + manager.attach_mock(deps.clear_vip_addresses_cache, "clear_vip_addresses_cache") + manager.attach_mock(deps._load_metalbox_devices_cache, "load_metalbox") + manager.attach_mock(deps.load_vip_addresses_cache, "load_vip") + + sync_sonic() + + assert manager.mock_calls == [ + call.clear_interface_cache(), + call.clear_all_caches(), + call.load_metalbox(), + call.load_vip(), + call.clear_interface_cache(), + call.clear_all_caches(), + call.clear_vip_addresses_cache(), + ] + + +def test_caches_loaded_once_per_invocation(mock_nb, patch_sync_deps): + deps = patch_sync_deps + + sync_sonic() + + deps._load_metalbox_devices_cache.assert_called_once_with() + deps.load_vip_addresses_cache.assert_called_once_with() + + +# --------------------------------------------------------------------------- +# Single-device path +# --------------------------------------------------------------------------- + + +def test_single_device_allowed_role_is_processed(mock_nb, patch_sync_deps): + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {"sw-1": {"PORT": {"Ethernet0": {}}}} + + +def test_single_device_disallowed_role_returns_empty( + mock_nb, patch_sync_deps, loguru_logs +): + device = make_device(name="sw-1", role_slug="router") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + patch_sync_deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "WARNING", "not in allowed SONiC roles") + + +def test_single_device_role_none_returns_empty(mock_nb, patch_sync_deps, loguru_logs): + device = make_device(name="sw-1", role_slug=None) + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + assert _has_log(loguru_logs, "WARNING", "role 'None'") + + +def test_single_device_not_found_returns_empty(mock_nb, patch_sync_deps, loguru_logs): + mock_nb.dcim.devices.get.return_value = None + + result = sync_sonic(device_name="missing") + + assert result == {} + assert _has_log(loguru_logs, "ERROR", "not found in NetBox") + + +def test_single_device_lookup_raises_returns_empty( + mock_nb, patch_sync_deps, loguru_logs +): + mock_nb.dcim.devices.get.side_effect = RuntimeError("netbox down") + + result = sync_sonic(device_name="sw-1") + + assert result == {} + assert _has_log(loguru_logs, "ERROR", "Error fetching device") + + +def test_single_spine_fetches_all_spine_devices(mock_nb, patch_sync_deps): + """A single spine triggers a full spine/superspine fetch for group + detection, so ``nb.dcim.devices.filter`` runs via the query list.""" + deps = patch_sync_deps + device = make_device(name="spine-1", role_slug="spine") + mock_nb.dcim.devices.get.return_value = device + deps.get_nb_device_query_list_sonic.return_value = [{"status": "active"}] + mock_nb.dcim.devices.filter.return_value = [device] + + sync_sonic(device_name="spine-1") + + mock_nb.dcim.devices.filter.assert_called_once_with(status="active") + deps.find_interconnected_devices.assert_called_once_with( + [device], ["spine", "superspine"] + ) + + +def test_single_leaf_uses_device_list_without_extra_fetch(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="leaf-1") + + mock_nb.dcim.devices.filter.assert_not_called() + deps.find_interconnected_devices.assert_called_once_with( + [device], ["spine", "superspine"] + ) + + +# --------------------------------------------------------------------------- +# Multi-device path +# --------------------------------------------------------------------------- + + +def test_multi_device_keeps_only_allowed_roles(mock_nb, patch_sync_deps): + deps = patch_sync_deps + leaf = make_device(name="leaf-1", device_id=1, role_slug="leaf") + router = make_device(name="router-1", device_id=2, role_slug="router") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [leaf, router] + + result = sync_sonic() + + assert "leaf-1" in result + assert "router-1" not in result + + +def test_multi_device_skips_devices_without_role(mock_nb, patch_sync_deps): + deps = patch_sync_deps + leaf = make_device(name="leaf-1", device_id=1, role_slug="leaf") + norole = make_device(name="x-1", device_id=2, role_slug=None) + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [norole, leaf] + + result = sync_sonic() + + assert set(result) == {"leaf-1"} + + +# --------------------------------------------------------------------------- +# Per-device processing +# --------------------------------------------------------------------------- + + +def test_hwsku_missing_skips_device(mock_nb, patch_sync_deps, loguru_logs): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", hwsku=None) + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "DEBUG", "no HWSKU configured") + + +def test_hwsku_unsupported_skips_device(mock_nb, patch_sync_deps, loguru_logs): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", hwsku="Bogus-HWSKU") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "WARNING", "unsupported HWSKU") + + +def test_hwsku_valid_generates_and_stores_config(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + deps.generate_sonic_config.assert_called_once_with( + device, "Accton-AS7326-56X", {}, None + ) + assert result == {"sw-1": {"PORT": {"Ethernet0": {}}}} + + +def test_config_version_read_from_custom_fields(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", config_version="4_2_0") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1") + + assert deps.generate_sonic_config.call_args.args[3] == "4_2_0" + + +# --------------------------------------------------------------------------- +# AS mapping +# --------------------------------------------------------------------------- + + +def test_as_mapping_calculated_per_spine_group(mock_nb, patch_sync_deps): + deps = patch_sync_deps + g1 = make_device(name="spine-1", device_id=1, role_slug="spine") + g2 = make_device(name="spine-2", device_id=2, role_slug="spine") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [g1, g2] + deps.find_interconnected_devices.return_value = [[g1], [g2]] + deps.calculate_minimum_as_for_group.side_effect = [4200000001, 4200000002] + + sync_sonic() + + assert deps.calculate_minimum_as_for_group.call_count == 2 + # The full mapping is handed to every per-device generation call. + assert deps.generate_sonic_config.call_args_list[0].args[2] == { + 1: 4200000001, + 2: 4200000002, + } + + +def test_as_mapping_passed_into_generate(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", device_id=7, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [device] + deps.find_interconnected_devices.return_value = [[device]] + deps.calculate_minimum_as_for_group.return_value = 4200000001 + + sync_sonic() + + assert deps.generate_sonic_config.call_args.args == ( + device, + "Accton-AS7326-56X", + {7: 4200000001}, + None, + ) + + +def test_group_with_none_min_as_not_propagated(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", device_id=7, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [device] + deps.find_interconnected_devices.return_value = [[device]] + deps.calculate_minimum_as_for_group.return_value = None + + sync_sonic() + + assert deps.generate_sonic_config.call_args.args[2] == {} + + +# --------------------------------------------------------------------------- +# Diff handling +# --------------------------------------------------------------------------- + + +def test_diff_streamed_to_task_output(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (True, "the-diff") + + sync_sonic(device_name="sw-1", task_id="t") + + deps.save_config_to_netbox.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}}, return_diff=True + ) + assert call("t", "the-diff\n") in deps.push_task_output.call_args_list + + +def test_first_time_configuration_message(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (True, None) + + sync_sonic(device_name="sw-1", task_id="t") + + assert ( + call("t", "First-time configuration created for sw-1\n") + in deps.push_task_output.call_args_list + ) + + +def test_no_change_skips_diff_output_but_still_exports(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (False, None) + + sync_sonic(device_name="sw-1", task_id="t") + + deps.export_config_to_file.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}} + ) + # Only the "Processing device" line is pushed — no diff / first-time lines. + assert deps.push_task_output.call_args_list == [ + call("t", "Processing device: sw-1\n") + ] + + +def test_show_diff_false_calls_save_without_return_diff(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = True + + sync_sonic(device_name="sw-1", show_diff=False) + + deps.save_config_to_netbox.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}} + ) + + +# --------------------------------------------------------------------------- +# Task output +# --------------------------------------------------------------------------- + + +def test_task_id_pushes_per_device_and_finishes(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1", task_id="t") + + assert ( + call("t", "Processing device: sw-1\n") in deps.push_task_output.call_args_list + ) + deps.finish_task_output.assert_called_once_with("t", rc=0) + + +def test_no_task_id_suppresses_task_output(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1") + + deps.push_task_output.assert_not_called() + deps.finish_task_output.assert_not_called() + + +# --------------------------------------------------------------------------- +# Cache stats +# --------------------------------------------------------------------------- + + +def test_cache_stats_logged_when_present(mock_nb, patch_sync_deps, loguru_logs): + patch_sync_deps.get_interface_cache_stats.return_value = { + "cached_devices": 2, + "total_interfaces": 10, + } + + sync_sonic() + + assert _has_log( + loguru_logs, "DEBUG", "Interface cache stats: 2 devices, 10 interfaces" + ) + + +def test_cache_stats_not_logged_when_empty(mock_nb, patch_sync_deps, loguru_logs): + patch_sync_deps.get_interface_cache_stats.return_value = {} + + sync_sonic() + + assert not _has_log(loguru_logs, "DEBUG", "Interface cache stats") From c50109d6478fb5b8008560dc70f9f56cee09dd04 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:53:50 +0200 Subject: [PATCH 3/8] Own only the sonic_config key in NetBox local context save_config_to_netbox previously replaced the device's entire local_context_data dict and diffed the full existing context against {"sonic_config": ...}. Sibling keys such as frr_parameters or netplan_parameters (consumed by the inventory reconciler via config_context) were silently deleted on every SONiC config update, with the removal attributed to SONiC in the journal diff. Mirror the partitioned-ownership model from #2297: merge the new sonic_config key into the existing context instead of replacing it, and diff only the owned key so unrelated sibling changes neither trigger nor pollute SONiC updates. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/exporter.py | 21 ++++---- .../tasks/conductor/sonic/test_exporter.py | 53 +++++++++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 1fb0f7f6..8a99358d 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -15,8 +15,10 @@ def save_config_to_netbox(device, config, return_diff=False): """Save SONiC configuration to NetBox device local context with diff checking. - Checks for existing local context and only saves if configuration has changed. - Logs diff when changes are detected. + Owns only the ``sonic_config`` key of the device's local context: sibling + keys (e.g. ``frr_parameters``, ``netplan_parameters``) are preserved and + excluded from diff checking. Only saves if the SONiC configuration has + changed. Logs diff when changes are detected. Args: device: NetBox device object @@ -30,16 +32,17 @@ def save_config_to_netbox(device, config, return_diff=False): try: # Get existing local context data existing_local_context = device.local_context_data or {} + existing_sonic_config = existing_local_context.get("sonic_config") - # Prepare new local context data - new_config_data = {"sonic_config": config} + # Replace only the sonic_config key, preserving sibling keys + new_config_data = {**existing_local_context, "sonic_config": config} diff_output = None - if existing_local_context: - # Compare existing local context with new config + if existing_sonic_config is not None: + # Compare only the owned sonic_config key with the new config # Generate diff - diff = DeepDiff(existing_local_context, new_config_data, ignore_order=True) + diff = DeepDiff(existing_sonic_config, config, ignore_order=True) if not diff: logger.info( @@ -50,10 +53,10 @@ def save_config_to_netbox(device, config, return_diff=False): # Log the unified diff logger.info(f"Configuration changes detected for device {device.name}:") existing_json = json.dumps( - existing_local_context, indent=2, sort_keys=True + {"sonic_config": existing_sonic_config}, indent=2, sort_keys=True ).splitlines() new_json = json.dumps( - new_config_data, indent=2, sort_keys=True + {"sonic_config": config}, indent=2, sort_keys=True ).splitlines() unified_diff = difflib.unified_diff( existing_json, diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index f40bf58e..806c0f46 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -121,6 +121,59 @@ def test_save_config_change_bool_form(mock_nb): assert result is True +def test_save_config_preserves_sibling_context_keys(mock_nb): + """Only the ``sonic_config`` key is owned — sibling keys like + ``frr_parameters`` must survive a config update untouched.""" + frr = {"asn": 4200000001, "loopback": "10.0.0.1"} + device = _make_save_device( + local_context_data={ + "frr_parameters": frr, + "sonic_config": {"PORT": {"Ethernet0": {}}}, + } + ) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}) + + assert result is True + device.save.assert_called_once_with() + assert device.local_context_data == { + "frr_parameters": frr, + "sonic_config": {"PORT": {"Ethernet1": {}}}, + } + + +def test_save_config_sibling_only_context_is_first_time(mock_nb): + """A context holding only sibling keys has no ``sonic_config`` to diff: + the first-time path adds the key and keeps the siblings.""" + frr = {"asn": 4200000001} + device = _make_save_device(local_context_data={"frr_parameters": frr}) + + result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert result == (True, None) + device.save.assert_called_once_with() + assert device.local_context_data == { + "frr_parameters": frr, + "sonic_config": {"PORT": {}}, + } + # First-time configuration creates no journal entry. + mock_nb.extras.journal_entries.create.assert_not_called() + + +def test_save_config_sibling_keys_do_not_trigger_change(mock_nb): + """Diffing covers only ``sonic_config`` — sibling keys must not register + as removals and force a save when the SONiC config itself is unchanged.""" + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device( + local_context_data={"frr_parameters": {"asn": 1}, "sonic_config": config} + ) + + result = save_config_to_netbox(device, config) + + assert result is False + device.save.assert_not_called() + + def test_save_config_journal_failure_still_saves(mock_nb, loguru_logs): """A failing journal create is logged but must not block the save.""" mock_nb.extras.journal_entries.create.side_effect = RuntimeError("journal down") From 12049863276d5e02148fc20a8d0e71f85982f4e5 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:54:27 +0200 Subject: [PATCH 4/8] Create SONiC journal entry only after successful save The journal entry documenting a configuration update was created before device.save(). When the save subsequently failed, the function reported no change while an orphaned journal entry already claimed the update had happened. Move journal creation after the successful save. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/exporter.py | 19 +++++++++++-------- .../tasks/conductor/sonic/test_exporter.py | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 8a99358d..2349ceef 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -68,10 +68,19 @@ def save_config_to_netbox(device, config, return_diff=False): diff_output = "\n".join(unified_diff) if diff_output: logger.info(f"Diff:\n{diff_output}") + else: + logger.info(f"Diff: {diff}") + + # Update existing local context + device.local_context_data = new_config_data + device.save() + logger.info(f"Updated SONiC local context for device {device.name}") - # Save diff to device journal log + # Save diff to device journal log only after the save succeeded, + # so a failed save cannot leave a journal entry claiming success + if diff_output: try: - journal_entry = utils.nb.extras.journal_entries.create( + utils.nb.extras.journal_entries.create( assigned_object_type="dcim.device", assigned_object_id=device.id, kind="info", @@ -84,13 +93,7 @@ def save_config_to_netbox(device, config, return_diff=False): logger.error( f"Failed to save diff to journal for device {device.name}: {e}" ) - else: - logger.info(f"Diff: {diff}") - # Update existing local context - device.local_context_data = new_config_data - device.save() - logger.info(f"Updated SONiC local context for device {device.name}") return (True, diff_output) if return_diff else True else: # Create new local context (no existing context to compare) diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index 806c0f46..65302850 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -202,6 +202,25 @@ def test_save_config_save_failure_returns_false(mock_nb, loguru_logs): assert _has_log(loguru_logs, "ERROR", "Failed to save local context") +def test_save_config_changed_path_save_failure_creates_no_journal( + mock_nb, loguru_logs +): + """A failed save on the changed path must not leave an orphaned journal + entry claiming the update succeeded — journal creation follows the save.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + device.save.side_effect = RuntimeError("netbox write failed") + + result = save_config_to_netbox( + device, {"PORT": {"Ethernet1": {}}}, return_diff=True + ) + + assert result == (False, None) + mock_nb.extras.journal_entries.create.assert_not_called() + assert _has_log(loguru_logs, "ERROR", "Failed to save local context") + + def test_save_config_save_failure_bool_form(mock_nb): device = _make_save_device(local_context_data=None) device.save.side_effect = RuntimeError("boom") From d5fc20d0873ff7dabbb8e3d99544bc498f187a75 Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:54:55 +0200 Subject: [PATCH 5/8] Write SONiC config exports atomically export_config_to_file opened the target with "w", truncating the previous export before serialization. A mid-write failure (ENOSPC, killed process) left the prior config truncated on disk. Write to a temporary file and os.replace() it into place so the previous export survives a failed write. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/exporter.py | 16 +++++++++++--- .../tasks/conductor/sonic/test_exporter.py | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 2349ceef..33e327c3 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -181,9 +181,19 @@ def export_config_to_file(device, config): config_changed = True if config_changed: - # Export configuration to JSON file - with open(filepath, "w") as f: - json.dump(config, f, indent=2) + # Write to a temporary file and rename it into place so a failed + # write (ENOSPC, killed process) cannot truncate the previous export + tmp_filepath = f"{filepath}.tmp" + try: + with open(tmp_filepath, "w") as f: + json.dump(config, f, indent=2) + os.replace(tmp_filepath, filepath) + except Exception: + try: + os.remove(tmp_filepath) + except OSError: + pass + raise logger.info(f"Exported SONiC config for device {device.name} to {filepath}") diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index 65302850..8a22ca48 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -361,6 +361,28 @@ def test_export_unreadable_file_is_overwritten( assert _has_log(loguru_logs, "WARNING", "Could not read existing config file") +def test_export_write_failure_preserves_previous_export( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + """The export is written atomically: a mid-write failure must leave the + previous export intact instead of truncating it, and not leak a temp file.""" + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + old_config = {"PORT": {"Ethernet9": {}}} + target.write_text(json.dumps(old_config)) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.json.dump", + side_effect=OSError("no space left on device"), + ) + + assert export_config_to_file(device, {"PORT": {"Ethernet0": {}}}) is False + + assert json.loads(target.read_text()) == old_config + assert not (tmp_path / "osism_sw-1_config_db.json.tmp").exists() + assert _has_log(loguru_logs, "ERROR", "Failed to export config") + + # --- symlink handling ------------------------------------------------------- From 2d9cda9bb915a2750a3515815715a71f1c799eeb Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:56:05 +0200 Subject: [PATCH 6/8] Reconcile hostname symlink independently of config changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole symlink block was gated on config_changed, so an unchanged config with a missing or stale hostname link was never repaired — and after a symlink failure, later unchanged runs never retried. Move the reconciliation out of the gate and make it idempotent (a link already pointing at the config file is left untouched). Also guard against hostname == serial: the just-written config file is the hostname path in that case, and the old code removed it and symlinked it to itself, destroying the config while reporting success. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/exporter.py | 59 +++++++++------ .../tasks/conductor/sonic/test_exporter.py | 72 +++++++++++++++++++ 2 files changed, 111 insertions(+), 20 deletions(-) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 33e327c3..0eb82943 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -113,6 +113,8 @@ def export_config_to_file(device, config): """Export SONiC configuration to local file with diff checking. Only writes to file if configuration has changed compared to existing file. + The serial-number→hostname symlink is reconciled on every call, even when + the configuration itself is unchanged. Args: device: NetBox device object @@ -197,22 +199,39 @@ def export_config_to_file(device, config): logger.info(f"Exported SONiC config for device {device.name} to {filepath}") - # Create hostname symlink if using serial number identifier - if ( - identifier_type == "serial-number" - and hasattr(device, "serial") - and device.serial - ): - try: - hostname = get_device_hostname(device) - hostname_filename = f"{prefix}{hostname}{suffix}" - hostname_filepath = os.path.join(export_dir, hostname_filename) + # Reconcile the hostname symlink on every run, not only when the + # config changed, so a missing or stale link is repaired even when + # the exported content is unchanged + if ( + identifier_type == "serial-number" + and hasattr(device, "serial") + and device.serial + ): + try: + hostname = get_device_hostname(device) + hostname_filename = f"{prefix}{hostname}{suffix}" + hostname_filepath = os.path.join(export_dir, hostname_filename) + + logger.debug( + f"Attempting to create symlink: {hostname_filepath} -> {filename}" + ) + logger.debug(f"Hostname: {hostname}, Serial: {device.serial}") + if hostname_filepath == filepath: + # hostname equals the serial: the just-written config file + # already lives at the hostname path; a symlink would + # replace the config with a self-reference logger.debug( - f"Attempting to create symlink: {hostname_filepath} -> {filename}" + f"Skipping hostname symlink for device {device.name}: hostname path equals config path" ) - logger.debug(f"Hostname: {hostname}, Serial: {device.serial}") - + elif ( + os.path.islink(hostname_filepath) + and os.readlink(hostname_filepath) == filename + ): + logger.debug( + f"Hostname symlink {hostname_filepath} already points to {filename}" + ) + else: # Create symlink from hostname file to serial number file if os.path.exists(hostname_filepath) or os.path.islink( hostname_filepath @@ -226,14 +245,14 @@ def export_config_to_file(device, config): logger.info( f"Created hostname symlink {hostname_filepath} -> {filename}" ) - except Exception as symlink_error: - logger.error( - f"Failed to create hostname symlink for device {device.name}: {symlink_error}" - ) - else: - logger.debug( - f"Symlink conditions not met - identifier_type: {identifier_type}, has_serial: {hasattr(device, 'serial')}, serial_value: {getattr(device, 'serial', None)}" + except Exception as symlink_error: + logger.error( + f"Failed to create hostname symlink for device {device.name}: {symlink_error}" ) + else: + logger.debug( + f"Symlink conditions not met - identifier_type: {identifier_type}, has_serial: {hasattr(device, 'serial')}, serial_value: {getattr(device, 'serial', None)}" + ) return config_changed diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index 8a22ca48..cde146e5 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -430,6 +430,78 @@ def test_export_replaces_dangling_symlink(tmp_path, export_settings, patch_hostn assert os.readlink(link) == "osism_ABC123_config_db.json" +def test_export_unchanged_config_repairs_missing_symlink( + tmp_path, export_settings, patch_hostname +): + """Symlink reconciliation is independent of a content change: an unchanged + config with a missing hostname link still gets the link created.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + + assert export_config_to_file(device, config) is False + + link = tmp_path / "osism_sw-1_config_db.json" + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_unchanged_config_repoints_stale_symlink( + tmp_path, export_settings, patch_hostname +): + """A hostname link pointing at the wrong target is repointed even when + the exported content is unchanged.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("osism_OTHER_config_db.json") + + assert export_config_to_file(device, config) is False + + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_existing_correct_symlink_left_untouched( + tmp_path, export_settings, patch_hostname, mocker +): + """A link that already points at the config file is not removed and + recreated on every run.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("osism_ABC123_config_db.json") + remove_spy = mocker.spy(os, "remove") + symlink_spy = mocker.spy(os, "symlink") + + assert export_config_to_file(device, config) is False + + remove_spy.assert_not_called() + symlink_spy.assert_not_called() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_hostname_equals_serial_keeps_config_file( + tmp_path, export_settings, patch_hostname +): + """When the serial equals the hostname the config file already lives at + the hostname path — no self-referential symlink may replace it.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="sw-1") + config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, config) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + assert json.loads(target.read_text()) == config + + def test_export_symlink_failure_still_returns_true( tmp_path, export_settings, patch_hostname, mocker, loguru_logs ): From 91d5f7ff633d349e9c181d4973c4bda428c5428c Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:57:25 +0200 Subject: [PATCH 7/8] Always clean up caches and finish task output in sync_sonic The single-device early returns and any exception in the per-device loop returned after the module-level caches were loaded but before the cleanup and finish_task_output calls, leaking cache state into the next run and leaving the task unfinished. Wrap the sync body in try/finally so cleanup and task finalization run on every exit path. Early returns and per-device failures now finish the task with rc=1 instead of silently reporting success; a failing device no longer aborts the sync of the remaining devices. The per-device summary log no longer raises on configs without a PORT section. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/sync.py | 354 ++++++++++-------- tests/unit/tasks/conductor/sonic/test_sync.py | 75 ++++ 2 files changed, 266 insertions(+), 163 deletions(-) diff --git a/osism/tasks/conductor/sonic/sync.py b/osism/tasks/conductor/sonic/sync.py index 8f4046f0..8f41c1b7 100644 --- a/osism/tasks/conductor/sonic/sync.py +++ b/osism/tasks/conductor/sonic/sync.py @@ -25,6 +25,10 @@ def sync_sonic(device_name=None, task_id=None, show_diff=True): """Sync SONiC configurations for eligible devices. + Caches are always cleared and the task output is always finished, even + when the sync exits early or a device fails. Failures are reported to the + task layer via a non-zero rc. + Args: device_name (str, optional): Name of specific device to sync. If None, sync all eligible devices. task_id (str, optional): Task ID for output logging. @@ -54,194 +58,218 @@ def sync_sonic(device_name=None, task_id=None, show_diff=True): # Dictionary to store configurations for all devices device_configs = {} - logger.debug(f"Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}") - - devices = [] - - if device_name: - # When specific device is requested, fetch it directly - try: - device = utils.nb.dcim.devices.get(name=device_name) - if device: - # Check if device role matches allowed roles - if device.role and device.role.slug in DEFAULT_SONIC_ROLES: - devices.append(device) - logger.debug( - f"Found device: {device.name} with role: {device.role.slug}" - ) + rc = 0 + + try: + logger.debug(f"Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}") + + devices = [] + + if device_name: + # When specific device is requested, fetch it directly + try: + device = utils.nb.dcim.devices.get(name=device_name) + if device: + # Check if device role matches allowed roles + if device.role and device.role.slug in DEFAULT_SONIC_ROLES: + devices.append(device) + logger.debug( + f"Found device: {device.name} with role: {device.role.slug}" + ) + else: + logger.warning( + f"Device {device_name} has role '{device.role.slug if device.role else 'None'}' " + f"which is not in allowed SONiC roles: {', '.join(DEFAULT_SONIC_ROLES)}" + ) + rc = 1 + return device_configs else: - logger.warning( - f"Device {device_name} has role '{device.role.slug if device.role else 'None'}' " - f"which is not in allowed SONiC roles: {', '.join(DEFAULT_SONIC_ROLES)}" - ) + logger.error(f"Device {device_name} not found in NetBox") + rc = 1 return device_configs - else: - logger.error(f"Device {device_name} not found in NetBox") + except Exception as e: + logger.error(f"Error fetching device {device_name}: {e}") + rc = 1 return device_configs - except Exception as e: - logger.error(f"Error fetching device {device_name}: {e}") - return device_configs - else: - # Get device query list from NETBOX_FILTER_CONDUCTOR_SONIC - nb_device_query_list = get_nb_device_query_list_sonic() - - for nb_device_query in nb_device_query_list: - # Query devices with the NETBOX_FILTER_CONDUCTOR_SONIC criteria - for device in utils.nb.dcim.devices.filter(**nb_device_query): - # Check if device role matches allowed roles - if device.role and device.role.slug in DEFAULT_SONIC_ROLES: - devices.append(device) - logger.debug( - f"Found device: {device.name} with role: {device.role.slug}" - ) - - logger.info(f"Found {len(devices)} devices matching criteria") - - # Find interconnected spine/superspine groups for special AS calculation - # When processing a single device, we need to consider all spine/superspine devices - # to properly detect interconnected groups, not just the requested device - if device_name and devices: - # Check if the single device is a spine/superspine - target_device = devices[0] - if target_device.role and target_device.role.slug in ["spine", "superspine"]: - # Fetch ALL spine/superspine devices to properly detect groups - logger.debug( - "Single spine/superspine device detected, fetching all spine/superspine devices for group detection" - ) - all_spine_devices = [] + else: + # Get device query list from NETBOX_FILTER_CONDUCTOR_SONIC nb_device_query_list = get_nb_device_query_list_sonic() + for nb_device_query in nb_device_query_list: + # Query devices with the NETBOX_FILTER_CONDUCTOR_SONIC criteria for device in utils.nb.dcim.devices.filter(**nb_device_query): - if device.role and device.role.slug in ["spine", "superspine"]: - all_spine_devices.append(device) - spine_groups = find_interconnected_devices( - all_spine_devices, ["spine", "superspine"] - ) + # Check if device role matches allowed roles + if device.role and device.role.slug in DEFAULT_SONIC_ROLES: + devices.append(device) + logger.debug( + f"Found device: {device.name} with role: {device.role.slug}" + ) + + logger.info(f"Found {len(devices)} devices matching criteria") + + # Find interconnected spine/superspine groups for special AS calculation + # When processing a single device, we need to consider all spine/superspine devices + # to properly detect interconnected groups, not just the requested device + if device_name and devices: + # Check if the single device is a spine/superspine + target_device = devices[0] + if target_device.role and target_device.role.slug in [ + "spine", + "superspine", + ]: + # Fetch ALL spine/superspine devices to properly detect groups + logger.debug( + "Single spine/superspine device detected, fetching all spine/superspine devices for group detection" + ) + all_spine_devices = [] + nb_device_query_list = get_nb_device_query_list_sonic() + for nb_device_query in nb_device_query_list: + for device in utils.nb.dcim.devices.filter(**nb_device_query): + if device.role and device.role.slug in ["spine", "superspine"]: + all_spine_devices.append(device) + spine_groups = find_interconnected_devices( + all_spine_devices, ["spine", "superspine"] + ) + else: + # For non-spine devices, use the original logic + spine_groups = find_interconnected_devices( + devices, ["spine", "superspine"] + ) else: - # For non-spine devices, use the original logic + # For multi-device processing, use the original logic spine_groups = find_interconnected_devices(devices, ["spine", "superspine"]) - else: - # For multi-device processing, use the original logic - spine_groups = find_interconnected_devices(devices, ["spine", "superspine"]) - logger.info(f"Found {len(spine_groups)} interconnected spine/superspine groups") + logger.info(f"Found {len(spine_groups)} interconnected spine/superspine groups") - # Create mapping from device ID to its assigned AS number - device_as_mapping = {} - - # Calculate AS numbers for spine/superspine groups - for group in spine_groups: - min_as = calculate_minimum_as_for_group(group) - if min_as: - for device in group: - device_as_mapping[device.id] = min_as - logger.debug( - f"Assigned AS {min_as} to {len(group)} devices in spine/superspine group" - ) + # Create mapping from device ID to its assigned AS number + device_as_mapping = {} - # Generate SONIC configuration for each device - for device in devices: - # Get HWSKU from sonic_parameters custom field, default to None - hwsku = None - if ( - hasattr(device, "custom_fields") - and "sonic_parameters" in device.custom_fields - and device.custom_fields["sonic_parameters"] - and "hwsku" in device.custom_fields["sonic_parameters"] - ): - hwsku = device.custom_fields["sonic_parameters"]["hwsku"] - - # Get config_version from sonic_parameters custom field, default to None - config_version = None - if ( - hasattr(device, "custom_fields") - and "sonic_parameters" in device.custom_fields - and device.custom_fields["sonic_parameters"] - and "config_version" in device.custom_fields["sonic_parameters"] - ): - config_version = device.custom_fields["sonic_parameters"]["config_version"] - logger.debug( - f"Device {device.name} has custom config_version: {config_version}" - ) - - # Skip devices without HWSKU - if not hwsku: - logger.debug(f"Skipping device {device.name}: no HWSKU configured") - continue - - logger.debug(f"Processing device: {device.name} with HWSKU: {hwsku}") - - # Output current device being processed if task_id is available - if task_id: - utils.push_task_output(task_id, f"Processing device: {device.name}\n") + # Calculate AS numbers for spine/superspine groups + for group in spine_groups: + min_as = calculate_minimum_as_for_group(group) + if min_as: + for device in group: + device_as_mapping[device.id] = min_as + logger.debug( + f"Assigned AS {min_as} to {len(group)} devices in spine/superspine group" + ) - # Validate that HWSKU is supported - if hwsku not in SUPPORTED_HWSKUS: - logger.warning( - f"Device {device.name} has unsupported HWSKU: {hwsku}. Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}" - ) - continue + # Generate SONIC configuration for each device + for device in devices: + # Get HWSKU from sonic_parameters custom field, default to None + hwsku = None + if ( + hasattr(device, "custom_fields") + and "sonic_parameters" in device.custom_fields + and device.custom_fields["sonic_parameters"] + and "hwsku" in device.custom_fields["sonic_parameters"] + ): + hwsku = device.custom_fields["sonic_parameters"]["hwsku"] + + # Get config_version from sonic_parameters custom field, default to None + config_version = None + if ( + hasattr(device, "custom_fields") + and "sonic_parameters" in device.custom_fields + and device.custom_fields["sonic_parameters"] + and "config_version" in device.custom_fields["sonic_parameters"] + ): + config_version = device.custom_fields["sonic_parameters"][ + "config_version" + ] + logger.debug( + f"Device {device.name} has custom config_version: {config_version}" + ) - # Generate SONIC configuration based on device HWSKU - sonic_config = generate_sonic_config( - device, hwsku, device_as_mapping, config_version - ) + # Skip devices without HWSKU + if not hwsku: + logger.debug(f"Skipping device {device.name}: no HWSKU configured") + continue - # Store configuration in the dictionary - device_configs[device.name] = sonic_config + logger.debug(f"Processing device: {device.name} with HWSKU: {hwsku}") - # Save the generated configuration to NetBox config context (only if changed) - if show_diff: - netbox_changed, diff_output = save_config_to_netbox( - device, sonic_config, return_diff=True - ) + # Output current device being processed if task_id is available + if task_id: + utils.push_task_output(task_id, f"Processing device: {device.name}\n") - # Output diff to task if available and there are changes - if task_id and netbox_changed and diff_output: - utils.push_task_output(task_id, f"\n{'='*60}\n") - utils.push_task_output( - task_id, f"Configuration diff for {device.name}:\n" + # Validate that HWSKU is supported + if hwsku not in SUPPORTED_HWSKUS: + logger.warning( + f"Device {device.name} has unsupported HWSKU: {hwsku}. Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}" ) - utils.push_task_output(task_id, f"{'='*60}\n") - utils.push_task_output(task_id, f"{diff_output}\n") - utils.push_task_output(task_id, f"{'='*60}\n\n") - elif task_id and netbox_changed and not diff_output: - # First-time configuration (no diff available) - utils.push_task_output( - task_id, f"First-time configuration created for {device.name}\n" + continue + + # A failing device must not abort the sync of the remaining + # devices, but it has to surface in the task rc + try: + # Generate SONIC configuration based on device HWSKU + sonic_config = generate_sonic_config( + device, hwsku, device_as_mapping, config_version ) - else: - netbox_changed = save_config_to_netbox(device, sonic_config) - # Export the generated configuration to local file (only if changed) - file_changed = export_config_to_file(device, sonic_config) + # Store configuration in the dictionary + device_configs[device.name] = sonic_config - if netbox_changed or file_changed: - logger.info(f"Configuration updated for device {device.name}") - else: - logger.info(f"No configuration changes for device {device.name}") + # Save the generated configuration to NetBox config context (only if changed) + if show_diff: + netbox_changed, diff_output = save_config_to_netbox( + device, sonic_config, return_diff=True + ) - logger.info( - f"Generated SONiC config for device {device.name} with {len(sonic_config['PORT'])} ports" - ) + # Output diff to task if available and there are changes + if task_id and netbox_changed and diff_output: + utils.push_task_output(task_id, f"\n{'='*60}\n") + utils.push_task_output( + task_id, f"Configuration diff for {device.name}:\n" + ) + utils.push_task_output(task_id, f"{'='*60}\n") + utils.push_task_output(task_id, f"{diff_output}\n") + utils.push_task_output(task_id, f"{'='*60}\n\n") + elif task_id and netbox_changed and not diff_output: + # First-time configuration (no diff available) + utils.push_task_output( + task_id, + f"First-time configuration created for {device.name}\n", + ) + else: + netbox_changed = save_config_to_netbox(device, sonic_config) - logger.info(f"Generated SONiC configurations for {len(device_configs)} devices") + # Export the generated configuration to local file (only if changed) + file_changed = export_config_to_file(device, sonic_config) - # Log cache statistics and cleanup - cache_stats = get_interface_cache_stats() - if cache_stats: - logger.debug( - f"Interface cache stats: {cache_stats['cached_devices']} devices, {cache_stats['total_interfaces']} interfaces" - ) + if netbox_changed or file_changed: + logger.info(f"Configuration updated for device {device.name}") + else: + logger.info(f"No configuration changes for device {device.name}") - clear_interface_cache() - clear_all_caches() - clear_vip_addresses_cache() - logger.debug("Cleared all caches after sync_sonic task completion") + logger.info( + f"Generated SONiC config for device {device.name} with {len(sonic_config.get('PORT', {}))} ports" + ) + except Exception as e: + logger.error( + f"Failed to sync SONiC configuration for device {device.name}: {e}" + ) + rc = 1 + + logger.info(f"Generated SONiC configurations for {len(device_configs)} devices") - # Finish task output if task_id is available - if task_id: - utils.finish_task_output(task_id, rc=0) + # Log cache statistics + cache_stats = get_interface_cache_stats() + if cache_stats: + logger.debug( + f"Interface cache stats: {cache_stats['cached_devices']} devices, {cache_stats['total_interfaces']} interfaces" + ) + finally: + # Cleanup must run on every exit path — the caches are module-level + # and would otherwise leak into the next run + clear_interface_cache() + clear_all_caches() + clear_vip_addresses_cache() + logger.debug("Cleared all caches after sync_sonic task completion") + + # Finish task output if task_id is available + if task_id: + utils.finish_task_output(task_id, rc=rc) # Return the dictionary with all device configurations return device_configs diff --git a/tests/unit/tasks/conductor/sonic/test_sync.py b/tests/unit/tasks/conductor/sonic/test_sync.py index d8701d06..359b8ee7 100644 --- a/tests/unit/tasks/conductor/sonic/test_sync.py +++ b/tests/unit/tasks/conductor/sonic/test_sync.py @@ -448,6 +448,81 @@ def test_no_task_id_suppresses_task_output(mock_nb, patch_sync_deps): deps.finish_task_output.assert_not_called() +# --------------------------------------------------------------------------- +# Failure handling and cleanup +# --------------------------------------------------------------------------- + + +def _assert_caches_cleaned(deps): + """The end-of-run cleanup must have run: interface and generator caches + are cleared a second time (after the initial clear) and the VIP cache once.""" + assert deps.clear_interface_cache.call_count == 2 + assert deps.clear_all_caches.call_count == 2 + deps.clear_vip_addresses_cache.assert_called_once_with() + + +@pytest.mark.parametrize("path", ["disallowed_role", "not_found", "lookup_raises"]) +def test_early_return_cleans_caches_and_reports_failure( + mock_nb, patch_sync_deps, path +): + """Single-device early returns happen after the module-level caches are + loaded — cleanup and task finalization must still run, with rc=1.""" + deps = patch_sync_deps + if path == "disallowed_role": + mock_nb.dcim.devices.get.return_value = make_device( + name="sw-1", role_slug="router" + ) + elif path == "not_found": + mock_nb.dcim.devices.get.return_value = None + else: + mock_nb.dcim.devices.get.side_effect = RuntimeError("netbox down") + + result = sync_sonic(device_name="sw-1", task_id="t") + + assert result == {} + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + + +def test_mid_loop_exception_continues_and_reports_failure( + mock_nb, patch_sync_deps, loguru_logs +): + """A device failing mid-loop must not abort the remaining devices, must + not leak the module-level caches, and must surface as rc=1.""" + deps = patch_sync_deps + bad = make_device(name="bad-1", device_id=1, role_slug="leaf") + good = make_device(name="good-1", device_id=2, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [bad, good] + deps.generate_sonic_config.side_effect = [ + RuntimeError("generation failed"), + {"PORT": {"Ethernet0": {}}}, + ] + + result = sync_sonic(task_id="t") + + assert result == {"good-1": {"PORT": {"Ethernet0": {}}}} + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device bad-1" + ) + + +def test_config_without_port_section_is_handled(mock_nb, patch_sync_deps, loguru_logs): + """A config lacking the PORT section must not raise in the summary log.""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.generate_sonic_config.return_value = {"VLAN": {}} + + result = sync_sonic(device_name="sw-1", task_id="t") + + assert result == {"sw-1": {"VLAN": {}}} + deps.finish_task_output.assert_called_once_with("t", rc=0) + assert _has_log(loguru_logs, "INFO", "with 0 ports") + + # --------------------------------------------------------------------------- # Cache stats # --------------------------------------------------------------------------- From cbf7708d907344c119f6e7252d731cbd688c8d1b Mon Sep 17 00:00:00 2001 From: Christian Berendt Date: Wed, 10 Jun 2026 07:59:01 +0200 Subject: [PATCH 8/8] Raise on SONiC persistence failures instead of reporting no change save_config_to_netbox returned the same (False, None) for a failed save as for "no changes", and export_config_to_file returned the same False for a failed write as for an unchanged file. sync_sonic then treated failed writes as "no changes" and finished the task with rc=0, so failed syncs reported success. Log and re-raise in both exporter functions; the per-device handler in sync_sonic catches the failure, continues with the remaining devices, and finishes the task with rc=1. AI-assisted: Claude Code Signed-off-by: Christian Berendt --- osism/tasks/conductor/sonic/exporter.py | 12 +++++- .../tasks/conductor/sonic/test_exporter.py | 34 +++++++---------- tests/unit/tasks/conductor/sonic/test_sync.py | 38 +++++++++++++++++-- 3 files changed, 58 insertions(+), 26 deletions(-) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 0eb82943..8ed0cda8 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -28,6 +28,10 @@ def save_config_to_netbox(device, config, return_diff=False): Returns: bool or tuple: If return_diff is False, returns True if config was saved (changed), False if no changes. If return_diff is True, returns (changed, diff_output) tuple. + + Raises: + Exception: If persisting the local context to NetBox fails, so a + failed save is distinguishable from "no changes". """ try: # Get existing local context data @@ -106,7 +110,7 @@ def save_config_to_netbox(device, config, return_diff=False): except Exception as e: logger.error(f"Failed to save local context for device {device.name}: {e}") - return (False, None) if return_diff else False + raise def export_config_to_file(device, config): @@ -122,6 +126,10 @@ def export_config_to_file(device, config): Returns: bool: True if config was written (changed), False if no changes + + Raises: + Exception: If the export fails, so a failed write is distinguishable + from "no changes". """ try: # Get configuration from settings @@ -258,4 +266,4 @@ def export_config_to_file(device, config): except Exception as e: logger.error(f"Failed to export config for device {device.name}: {e}") - return False + raise diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index cde146e5..44981699 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -191,20 +191,19 @@ def test_save_config_journal_failure_still_saves(mock_nb, loguru_logs): assert _has_log(loguru_logs, "ERROR", "Failed to save diff to journal") -def test_save_config_save_failure_returns_false(mock_nb, loguru_logs): - """A raising ``device.save()`` is caught, logged, and reported as no-change.""" +def test_save_config_save_failure_raises(mock_nb, loguru_logs): + """A raising ``device.save()`` is logged and re-raised so a failed save + stays distinguishable from "no changes" at the task layer.""" device = _make_save_device(local_context_data=None) device.save.side_effect = RuntimeError("netbox write failed") - result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + with pytest.raises(RuntimeError, match="netbox write failed"): + save_config_to_netbox(device, {"PORT": {}}, return_diff=True) - assert result == (False, None) assert _has_log(loguru_logs, "ERROR", "Failed to save local context") -def test_save_config_changed_path_save_failure_creates_no_journal( - mock_nb, loguru_logs -): +def test_save_config_changed_path_save_failure_creates_no_journal(mock_nb, loguru_logs): """A failed save on the changed path must not leave an orphaned journal entry claiming the update succeeded — journal creation follows the save.""" device = _make_save_device( @@ -212,22 +211,13 @@ def test_save_config_changed_path_save_failure_creates_no_journal( ) device.save.side_effect = RuntimeError("netbox write failed") - result = save_config_to_netbox( - device, {"PORT": {"Ethernet1": {}}}, return_diff=True - ) + with pytest.raises(RuntimeError, match="netbox write failed"): + save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}, return_diff=True) - assert result == (False, None) mock_nb.extras.journal_entries.create.assert_not_called() assert _has_log(loguru_logs, "ERROR", "Failed to save local context") -def test_save_config_save_failure_bool_form(mock_nb): - device = _make_save_device(local_context_data=None) - device.save.side_effect = RuntimeError("boom") - - assert save_config_to_netbox(device, {"PORT": {}}) is False - - # --------------------------------------------------------------------------- # export_config_to_file # --------------------------------------------------------------------------- @@ -376,7 +366,8 @@ def test_export_write_failure_preserves_previous_export( side_effect=OSError("no space left on device"), ) - assert export_config_to_file(device, {"PORT": {"Ethernet0": {}}}) is False + with pytest.raises(OSError, match="no space left on device"): + export_config_to_file(device, {"PORT": {"Ethernet0": {}}}) assert json.loads(target.read_text()) == old_config assert not (tmp_path / "osism_sw-1_config_db.json.tmp").exists() @@ -536,7 +527,7 @@ def test_export_hostname_mode_makes_no_symlink( # --- error handling --------------------------------------------------------- -def test_export_makedirs_failure_returns_false( +def test_export_makedirs_failure_raises( tmp_path, export_settings, patch_hostname, mocker, loguru_logs ): export_settings(tmp_path, identifier="hostname") @@ -546,6 +537,7 @@ def test_export_makedirs_failure_returns_false( ) device = SimpleNamespace(name="sw-1", serial="ABC123") - assert export_config_to_file(device, {"PORT": {}}) is False + with pytest.raises(OSError, match="read-only filesystem"): + export_config_to_file(device, {"PORT": {}}) assert _has_log(loguru_logs, "ERROR", "Failed to export config") diff --git a/tests/unit/tasks/conductor/sonic/test_sync.py b/tests/unit/tasks/conductor/sonic/test_sync.py index 359b8ee7..1b13f626 100644 --- a/tests/unit/tasks/conductor/sonic/test_sync.py +++ b/tests/unit/tasks/conductor/sonic/test_sync.py @@ -462,9 +462,7 @@ def _assert_caches_cleaned(deps): @pytest.mark.parametrize("path", ["disallowed_role", "not_found", "lookup_raises"]) -def test_early_return_cleans_caches_and_reports_failure( - mock_nb, patch_sync_deps, path -): +def test_early_return_cleans_caches_and_reports_failure(mock_nb, patch_sync_deps, path): """Single-device early returns happen after the module-level caches are loaded — cleanup and task finalization must still run, with rc=1.""" deps = patch_sync_deps @@ -509,6 +507,40 @@ def test_mid_loop_exception_continues_and_reports_failure( ) +def test_netbox_save_failure_reports_nonzero_rc(mock_nb, patch_sync_deps, loguru_logs): + """A raising ``save_config_to_netbox`` must not finish the task as rc=0 + — a failed write is not "no changes".""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.side_effect = RuntimeError("netbox write failed") + + sync_sonic(device_name="sw-1", task_id="t") + + deps.export_config_to_file.assert_not_called() + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device sw-1" + ) + + +def test_file_export_failure_reports_nonzero_rc(mock_nb, patch_sync_deps, loguru_logs): + """A raising ``export_config_to_file`` surfaces as rc=1 as well.""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.export_config_to_file.side_effect = OSError("disk full") + + sync_sonic(device_name="sw-1", task_id="t") + + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device sw-1" + ) + + def test_config_without_port_section_is_handled(mock_nb, patch_sync_deps, loguru_logs): """A config lacking the PORT section must not raise in the summary log.""" deps = patch_sync_deps