diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index b011893f..1ea2b46f 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -192,6 +192,17 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= owned, inherited, or image-consumed — so a newly handled table cannot silently fall into the unpoliced pass-through tier and reintroduce stale config. + + The four categories above are *table-level ownership*, which applies to + config_db.json: the generator owns whole tables and may drop-and-rebuild + them because the file has no external co-owner. SONiC config is also + written to the NetBox device local_context_data, a store co-owned with + other generators (frr_parameters, netplan_parameters). That store uses a + different regime — *partitioned key ownership*: own only the + sonic_config key — carry the existing context forward and overwrite + just that key, never writing back a context that drops the co-owners' + keys (see save_config_to_netbox in exporter.py). The deciding factor is + the external co-owner. """ # Get port configuration for the HWSKU port_config = get_port_config(hwsku) diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 8ed0cda8..f8995880 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -15,10 +15,28 @@ def save_config_to_netbox(device, config, return_diff=False): """Save SONiC configuration to NetBox device local context with diff checking. - 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. + Ownership regime -- partitioned key ownership: + The device ``local_context_data`` is co-owned with other generators + (``frr_parameters``, ``netplan_parameters``, ...), so this function + owns only its own ``sonic_config`` key: it carries the existing context + forward and overwrites just that key, so the value written back to + ``local_context_data`` always still holds the sibling keys. It must + never write back a context containing only ``sonic_config``, which + would drop them. Diff checking is likewise scoped to ``sonic_config``, + so a sibling change neither registers as a SONiC change nor triggers a + save. + + This differs deliberately from the ``config_db.json`` regime in + generate_sonic_config (table-level ownership), where the generator owns + whole tables and drops-and-rebuilds them: that store has no external + co-owner, so wholesale replacement of an owned table is safe. Here it is + not. The deciding factor is the external co-owner. Do not "simplify" + this into writing back a fresh ``{"sonic_config": ...}`` that omits the + existing context; the sibling-preservation tests in test_exporter.py + enforce that the written-back value keeps the siblings. + + Only saves if the SONiC configuration has changed; logs the diff when + changes are detected. Args: device: NetBox device object diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py index 44981699..5185a213 100644 --- a/tests/unit/tasks/conductor/sonic/test_exporter.py +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -35,6 +35,21 @@ def _has_log(records, level, substring): # --------------------------------------------------------------------------- +# Surfaced when a sibling-preservation assertion fails, so an author who +# reverts the exporter to wholesale replacement is told why the test fails +# rather than reading it as a stale test and "fixing" it. The full reasoning +# lives on the save_config_to_netbox docstring; this points there. +_PARTITIONED_KEY_OWNERSHIP = ( + "local_context_data uses partitioned key ownership: sibling keys such as " + "frr_parameters and netplan_parameters are co-owned by other generators. " + "save_config_to_netbox must carry the existing context forward and " + "overwrite only the sonic_config key; the value written back to " + "local_context_data must still hold the sibling keys. Writing back a " + "context that drops them is the regression this guards -- see the " + "save_config_to_netbox docstring." +) + + 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( @@ -139,7 +154,7 @@ def test_save_config_preserves_sibling_context_keys(mock_nb): assert device.local_context_data == { "frr_parameters": frr, "sonic_config": {"PORT": {"Ethernet1": {}}}, - } + }, _PARTITIONED_KEY_OWNERSHIP def test_save_config_sibling_only_context_is_first_time(mock_nb): @@ -155,7 +170,7 @@ def test_save_config_sibling_only_context_is_first_time(mock_nb): assert device.local_context_data == { "frr_parameters": frr, "sonic_config": {"PORT": {}}, - } + }, _PARTITIONED_KEY_OWNERSHIP # First-time configuration creates no journal entry. mock_nb.extras.journal_entries.create.assert_not_called() @@ -170,7 +185,11 @@ def test_save_config_sibling_keys_do_not_trigger_change(mock_nb): result = save_config_to_netbox(device, config) - assert result is False + assert result is False, ( + "Diffing must cover only the owned sonic_config key; an unchanged " + "SONiC config must not save just because sibling keys are present. " + + _PARTITIONED_KEY_OWNERSHIP + ) device.save.assert_not_called()