From 9bb74397bf3eb87f8a7378329e7c69c54fc46ca4 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Sat, 13 Jun 2026 13:23:40 +0200 Subject: [PATCH] sonic: document config ownership regimes SONiC config is written to two stores with deliberately different ownership disciplines, and only one was documented. config_db.json uses table-level ownership: the generator owns whole tables and rebuilds them every regen, which is safe because the file has no external co-owner. The NetBox device local_context_data is co-owned with other generators (frr_parameters, netplan_parameters), so save_config_to_netbox owns only its sonic_config key: it carries the existing context forward and overwrites just that key, so the value written back always still holds the siblings -- a distinct regime, partitioned key ownership. The contrast was implicit, inviting a future change to "simplify" the exporter into writing back a context that holds only sonic_config and drops the siblings. The sibling preservation is already enforced by tests in test_exporter.py, but that lock was not discoverable: the tests asserted a bare dict equality, so a regression failed with only a dict diff. An author who did not already know the contract could read the test as stale and weaken it instead of recognizing the broken behavior, exactly the regression the test guards against. Make the lock teach at the point of failure. Add the regime, its reason, and the deciding factor (an external co-owner) to the save_config_to_netbox docstring, and name both regimes in the generate_sonic_config ownership-model docstring so the two read as one model. Give the sibling-preservation assertions an explanatory message that states the partitioned-key-ownership contract and points to the docstring, so a regression surfaces the reasoning rather than a bare diff. Documentation and test messages only; no behavior change. Assisted-by: Claude:claude-fable-5 Signed-off-by: Roger Luethi --- .../tasks/conductor/sonic/config_generator.py | 11 ++++++++ osism/tasks/conductor/sonic/exporter.py | 26 ++++++++++++++++--- .../tasks/conductor/sonic/test_exporter.py | 25 +++++++++++++++--- 3 files changed, 55 insertions(+), 7 deletions(-) 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()