From 5eeeab0844a53db1950b443483814913f42e746c Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Fri, 24 Jul 2026 22:50:54 +0200 Subject: [PATCH 1/3] sonic: document base config as repo-shipped The config ownership model described the file the generator builds on as "the device's image-provided base config_db.json". That is wrong, and the error is not cosmetic: it is the premise the model reasons from. The base config is this repo's files/sonic/config_db.json, which the Containerfile installs into the conductor image at /etc/sonic/config_db.json. generate_sonic_config() reads that path from its own container filesystem. Nothing from a switch's own config_db.json ever reaches the generator, and no code writes that path at runtime. Everything device-specific comes from NetBox and the policy in this module, while the base config supplies the shared content of the inherited, read-only and pass-through tables. The file is therefore an input this repo controls, not foreign vendor or operator content. Two consequences follow, and the model captured neither. The "operator hand-edits are unsupported" rationale that justified drop-and-rebuild aimed at a surface that does not exist on this path: a file baked into a container image is not operator-editable to begin with. Meanwhile the role that file actually had -- carrying shipped defaults -- went unrecognised, so classifying those tables as owned silently deleted them. That is how the default-VRF regression came about: the BGP entries shipped in the base config, the owned-table drop removed them on every regen, nothing regenerated them, and the default VRF stopped advertising its routes into EVPN. https://github.com/osism/python-osism/issues/2515 Correct the provenance wherever it is stated -- the ownership model on generate_sonic_config, the INHERITED/READ_ONLY constant comments, the DEFAULT_VRF_* comment, _get_gnmi_port, and the test docstrings that restate the taxonomy. Add a "Where defaults belong" paragraph to the model, because the provenance alone does not tell a reader what to do: owned tables are dropped before any helper runs, so a populated owned table in the base config is dead content, and defaults the generator needs must live in this module as policy constants. The rule is about owned tables specifically: a read-only table such as TELEMETRY legitimately lives only in the base config, because the generator reads it rather than emitting it. Note separately that files/ is not Python package data, so a generator running outside the conductor image finds no base config at all. Rename IMAGE_CONSUMED_TABLE_KEYS to READ_ONLY_TABLE_KEYS. Fixing the prose while the constant still says "image" would leave the documentation contradicting the name, and "image" is precisely the word that invited the confusion: the base config does come from an image, but the conductor's, not the switch's. The replacement names the behaviour rather than the source, because the source no longer discriminates -- inherited and pass-through tables come from the base config too -- while "read but never written" is what sets this category apart, and is how the docstring already defines it. Documentation, comments and test docstrings only; no behaviour change; the SONiC unit tests pass unchanged. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../tasks/conductor/sonic/config_generator.py | 112 +++++++++++------- .../test_config_generator_orchestrator.py | 4 +- .../sonic/test_config_generator_ownership.py | 44 +++---- 3 files changed, 94 insertions(+), 66 deletions(-) diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index c66b2fb61..2d54b2b4b 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -95,26 +95,24 @@ "VERSIONS", ) -# Tables inherited from the device's image-provided base config_db.json: not -# dropped on regen, so their base content is preserved, while the generator -# updates only selected fields in place (DEVICE_METADATA localhost attributes, -# VERSIONS.DATABASE.VERSION) rather than rebuilding them wholesale (see the -# ownership model on generate_sonic_config). +# Tables inherited from the base config_db.json: not dropped on regen, so their +# base content is preserved, while the generator updates only selected fields in +# place (DEVICE_METADATA localhost attributes, VERSIONS.DATABASE.VERSION) rather +# than rebuilding them wholesale (see the ownership model on +# generate_sonic_config). INHERITED_TABLE_KEYS = ("DEVICE_METADATA", "VERSIONS") -# Tables read from the device's image-provided base config_db.json but never -# modified by the generator: their values are consumed (via a defensive -# config.get(...), so a missing table is tolerated). They are neither dropped -# nor written, so an image-provided table passes through to the output -# unchanged -- the same output behaviour as a pass-through table, the -# difference being only that the generator depends on this one as an input. -# Distinct from inherited tables, which the generator also writes selected +# Tables read from the base config_db.json but never modified by the generator: +# their values are consumed (via a defensive config.get(...), so a missing table +# is tolerated). They are neither dropped nor written, so such a table passes +# through to the output unchanged -- the same output behaviour as a pass-through +# table, the difference being only that the generator depends on this one as an +# input. Distinct from inherited tables, which the generator also writes selected # fields into; the distinction is read-only vs. read-and-update, not the access -# syntax (inherited tables are read via config.get() too). For example, the -# gNMI listen port is read from TELEMETRY. Must stay disjoint from -# OWNED_TABLE_KEYS -- an image-consumed table dropped up front would always -# read back empty. -IMAGE_CONSUMED_TABLE_KEYS = ("TELEMETRY",) +# syntax (inherited tables are read via config.get() too). For example, the gNMI +# listen port is read from TELEMETRY. Must stay disjoint from OWNED_TABLE_KEYS -- +# a read-only table dropped up front would always read back empty. +READ_ONLY_TABLE_KEYS = ("TELEMETRY",) # Owned tables that are also scaffolded: every scaffold key except the # inherited ones. The orchestrator setdefault-creates these up front, so @@ -175,12 +173,15 @@ # Configuration of the default VRF, emitted by _add_default_vrf_configuration # on every regen. # -# These entries used to ship in the image-provided base config_db.json, with -# the generator adding only the VRF-specific ones. The tables holding them are -# owned, so their base content is dropped up front -- which silently removed -# the default-VRF entries on every regen and, with them, the default VRF's -# EVPN route advertisement. The generator therefore owns these entries too; -# the values are the ones the base config used to carry. +# These entries used to ship in the base config_db.json (files/sonic/config_db.json, +# installed into the conductor image at /etc/sonic/config_db.json), with the +# generator adding only the VRF-specific ones. The tables holding them are owned, +# so their base content is dropped up front -- which silently removed the +# default-VRF entries on every regen and, with them, the default VRF's EVPN route +# advertisement (#2515). The generator therefore owns these entries too; the +# values are the ones the base config used to carry. Policy constants are where +# such defaults belong -- see "Where defaults belong" in the ownership model on +# generate_sonic_config. # # The mappings are read-only views: they are module-level state shared by every # device, so an in-place edit would rewrite the policy for the whole process. @@ -271,35 +272,62 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= dict: Minimal SONiC configuration dictionary Config ownership model: - The generator builds on the device's image-provided base - config_db.json and classifies every table it touches into one of four - categories. Operator hand-edits to config_db.json are unsupported in - all of them: customizations must be modeled in NetBox or expressed as - generator policy, never applied directly to the file. + The generator builds on a base config_db.json and classifies every + table it touches into one of four categories. + + What the base config is: this repo's files/sonic/config_db.json, which + the Containerfile installs into the conductor image at + /etc/sonic/config_db.json. The generator reads that path from its own + container filesystem. It is *not* the switch's SONiC image config and + not a per-device file — nothing from a device's own config_db.json ever + reaches the generator. Everything device-specific comes from NetBox and + the policy in this module; the base config supplies the shared content + of the inherited, read-only and pass-through tables below, which + nothing else provides. Because the base config is baked into a + container image, it is not an operator-editable surface either: + customizations must be modeled in NetBox or expressed as generator + policy, never applied directly to the file. - Owned (OWNED_TABLE_KEYS): fully owned by the generator and rebuilt from scratch every regen from NetBox data and hardcoded SONiC policy. Their base content is dropped up front, so neither pre-existing values nor entries removed from NetBox survive. - - Inherited (INHERITED_TABLE_KEYS): not dropped on regen, so the - image base content is preserved, while the generator updates - selected fields in place — currently DEVICE_METADATA localhost - attributes (hostname, hwsku, platform, mac) and the - VERSIONS.DATABASE.VERSION. Scaffold-created when absent, so they - always exist at access time even on a fresh base config. - - Image-consumed (IMAGE_CONSUMED_TABLE_KEYS): read from the image - base config but never modified by the generator. The defining - property is behavioural (read-only), not the access syntax: - inherited tables are also read via config.get(), so .get() alone - does not distinguish the two. IMAGE_CONSUMED_TABLE_KEYS lists the - current members. + - Inherited (INHERITED_TABLE_KEYS): not dropped on regen, so the base + content is preserved, while the generator updates selected fields in + place — currently DEVICE_METADATA localhost attributes (hostname, + hwsku, platform, mac) and the VERSIONS.DATABASE.VERSION. + Scaffold-created when absent, so they always exist at access time + even on a fresh base config. + - Read-only (READ_ONLY_TABLE_KEYS): read from the base config but + never modified by the generator. The defining property is + behavioural, not the access syntax: inherited tables are also read + via config.get(), so .get() alone does not distinguish the two. + READ_ONLY_TABLE_KEYS lists the current members. - Pass-through: every table the generator never references. Left untouched and unmanaged; not a supported place for operator customizations either. + Where defaults belong: owned tables are dropped before any helper + runs, so a populated *owned* table in files/sonic/config_db.json is + dead content — silently discarded on every regen. Defaults the + generator needs must live in this module as policy constants (see the + DEFAULT_VRF_* mappings), never in the base config. #2515 was exactly + this mistake: the default-VRF BGP entries shipped in the base config, + the ownership drop removed them, and nothing regenerated them, so the + default VRF stopped advertising its routes into EVPN. Only inherited, + read-only and pass-through tables carry meaningful base content. + The rule is about owned tables specifically: a read-only table such + as TELEMETRY is a dependency that legitimately lives only in the base + config, because the generator reads it rather than emitting it. + + The base config ships *only* in the container image — files/ is not + Python package data — so a generator running outside that image (pip + install, test harness) finds nothing and falls back to the hardcoded + defaults. + A static guard test (see test_config_generator_ownership.py) parses this module and fails the build if it references a table that is not - owned, inherited, or image-consumed — so a newly handled table cannot + owned, inherited, or read-only — so a newly handled table cannot silently fall into the unpoliced pass-through tier and reintroduce stale config. @@ -2467,7 +2495,7 @@ def _get_gnmi_port(config): The telemetry/gNMI container reads its listen port from TELEMETRY|gnmi|port and falls back to 8080 when unset, so the ACL rule - follows the same lookup against the (image-consumed) TELEMETRY table, + follows the same lookup against the (read-only) TELEMETRY table, treating a present-but-empty value (null, "") as unset like the container does. A malformed value (non-numeric or outside 1-65535) also falls back to the default, with a warning, rather than diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py index c292fe288..eb83fa83a 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py @@ -428,7 +428,7 @@ def test_generate_sonic_config_gnmi_port_survives_owned_table_drop_end_to_end( ): """End-to-end with the real ACL helper: a TELEMETRY|gnmi|port from the base config must reach the generated GNMI_ONLY rule. TELEMETRY is - image-consumed while ACL_TABLE/ACL_RULE are owned and dropped up front, + read-only while ACL_TABLE/ACL_RULE are owned and dropped up front, so this pins the whole flow -- the port is read after the drop and emitted into the final output -- which the wiring tests above (helper mocked) and the helper unit tests (no orchestrator) only cover @@ -555,7 +555,7 @@ def test_generate_sonic_config_emits_default_vrf_entries_without_vni_vrfs( """A device without VNI VRFs still gets the full default-VRF config. VRF, the BGP_GLOBALS* tables and ROUTE_REDISTRIBUTE are owned, so whatever - the image base config_db.json carries for the default VRF is dropped on + the base config_db.json carries for the default VRF is dropped on regen, and ``_add_vrf_configuration`` only writes entries for the VRFs NetBox carries. ``_add_default_vrf_configuration`` therefore emits the default-VRF entries — without them the default VRF stops advertising its diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py index b8b66f11b..154c7115d 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py @@ -21,7 +21,7 @@ from osism.tasks.conductor.sonic import config_generator from osism.tasks.conductor.sonic.config_generator import ( - IMAGE_CONSUMED_TABLE_KEYS, + READ_ONLY_TABLE_KEYS, INHERITED_TABLE_KEYS, MULTI_OWNER_OWNED_TABLE_KEYS, ON_DEMAND_OWNED_TABLE_KEYS, @@ -256,28 +256,28 @@ def test_scaffolded_and_on_demand_owned_are_disjoint(self): """ assert set(SCAFFOLDED_OWNED_TABLE_KEYS).isdisjoint(ON_DEMAND_OWNED_TABLE_KEYS) - def test_image_consumed_and_owned_are_disjoint(self): - """No table is both image-consumed and owned. + def test_read_only_and_owned_are_disjoint(self): + """No table is both read-only and owned. - Image-consumed tables are read defensively from the image base config - (via config.get) and never dropped. Owned tables are dropped up front - and rebuilt. A table in both would be dropped before it is read, so the + Read-only tables are read defensively from the base config (via + config.get) and never dropped. Owned tables are dropped up front and + rebuilt. A table in both would be dropped before it is read, so the consuming helper would always see an empty table -- the dependency the - image-consumed category exists to document would silently break. + read-only category exists to document would silently break. """ - assert set(IMAGE_CONSUMED_TABLE_KEYS).isdisjoint(OWNED_TABLE_KEYS) + assert set(READ_ONLY_TABLE_KEYS).isdisjoint(OWNED_TABLE_KEYS) - def test_image_consumed_and_inherited_are_disjoint(self): - """No table is both image-consumed and inherited. + def test_read_only_and_inherited_are_disjoint(self): + """No table is both read-only and inherited. - Image-consumed tables are read but never modified; inherited tables + Read-only tables are read but never modified; inherited tables are read and have selected fields updated in place. The two are mutually exclusive by definition (modified vs. not), so a table in both is a contradictory classification. With the owned/inherited and - owned/image-consumed invariants above, this completes pairwise + owned/read-only invariants above, this completes pairwise disjointness across all three classified categories. """ - assert set(IMAGE_CONSUMED_TABLE_KEYS).isdisjoint(INHERITED_TABLE_KEYS) + assert set(READ_ONLY_TABLE_KEYS).isdisjoint(INHERITED_TABLE_KEYS) # --------------------------------------------------------------------------- @@ -321,7 +321,7 @@ def _config_table_keys_referenced_in_source(): - subscripts (``config["X"]``), - the defensive accessors (``config.get/setdefault/pop("X", ...)``) -- a read is a real dependency on a table, so reads are collected the same - as writes; that is how an image-consumed table such as TELEMETRY is + as writes; that is how a read-only table such as TELEMETRY is caught, - update() in every literal form: ``config.update({"X": ...})``, ``config.update(X=...)`` (keyword), ``config.update(**{"X": ...})``, @@ -443,7 +443,7 @@ class TestStaticTableReferenceGuard: The taxonomy constants above are a hand-maintained allowlist; nothing forces a newly handled table into one of them. This guard parses the generator source and fails when it references a table that is neither - owned, inherited, nor image-consumed -- the omission that lets a new table + owned, inherited, nor read-only -- the omission that lets a new table fall into the unpoliced pass-through tier and accumulate stale config. It is static rather than runtime so it catches tables that emit only when NetBox carries their data (a generate-and-inspect test would miss them @@ -454,7 +454,7 @@ def test_every_referenced_table_is_classified(self): classified = ( set(OWNED_TABLE_KEYS) | set(INHERITED_TABLE_KEYS) - | set(IMAGE_CONSUMED_TABLE_KEYS) + | set(READ_ONLY_TABLE_KEYS) ) referenced = _config_table_keys_referenced_in_source() unclassified = referenced - classified @@ -469,11 +469,11 @@ def test_every_referenced_table_is_classified(self): " - owned, rebuilt from NetBox/policy every regen -> add to " "ON_DEMAND_OWNED_TABLE_KEYS (or TOP_LEVEL_SCAFFOLD_KEYS if it is " "created up front by the orchestrator)\n" - " - image base content preserved, with selected fields updated " + " - base content preserved, with selected fields updated " "in place -> add to INHERITED_TABLE_KEYS\n" - " - read from the image but never modified -> add to " - "IMAGE_CONSUMED_TABLE_KEYS\n" - "Leaving a table unclassified lets pre-existing operator or image " + " - read from the base config but never modified -> add to " + "READ_ONLY_TABLE_KEYS\n" + "Leaving a table unclassified lets pre-existing base-config " "content survive a regen, the stale-config bug the ownership model " "exists to prevent." ) @@ -693,11 +693,11 @@ def test_referenced_multi_owner_tables_are_owned(self): def test_multi_owner_tables_are_not_inherited_or_consumed(self): """Multi-owner tables belong to the owned/dropped regime only. - Inherited and image-consumed tables are preserved, not dropped; a + Inherited and read-only tables are preserved, not dropped; a multi-owner table in either category would be merged into without the up-front clear the per-key pattern relies on. Holds before and after the ACL helpers land. """ multi_owner = set(MULTI_OWNER_OWNED_TABLE_KEYS) assert multi_owner.isdisjoint(INHERITED_TABLE_KEYS) - assert multi_owner.isdisjoint(IMAGE_CONSUMED_TABLE_KEYS) + assert multi_owner.isdisjoint(READ_ONLY_TABLE_KEYS) From 9e617ef4384c72f79830ffa6ec0ca71edda7b9a5 Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Fri, 24 Jul 2026 23:13:19 +0200 Subject: [PATCH 2/3] sonic: guard base config against owned content Owned tables are dropped before any helper runs, so content the shipped base config_db.json puts in one is discarded on every regen: it reads as configuration but never reaches a switch. Nothing enforced that. The default-VRF regression is exactly this failure. The BGP entries shipped in files/sonic/config_db.json, the owned-table drop removed them, nothing regenerated them, and the default VRF stopped advertising its routes into EVPN: https://github.com/osism/python-osism/issues/2515 No existing test could catch it, and adding more of the same would not help. Every base-config fixture is built from TOP_LEVEL_SCAFFOLD_KEYS with all tables empty (make_base_config), so dropping populated content is indistinguishable from dropping nothing -- the orchestrator test asserting BGP_GLOBALS equals exactly router_id/local_asn passes either way. The exhaustive stale-entry sweep seeds a sentinel into every owned table and asserts it is gone, which verifies the drop works; the drop is the mechanism at fault, so a test that asserts deletion happens cannot detect over-deletion. Add a static guard that reads the real shipped file and fails when it populates a generator-owned table. It needs no fixture, so it cannot drift from production the way the scaffolds did, and it generalises: it fires for the next table that gets orphaned, not just for the five that were. Verified against the pre-fix base config, where it names BGP_GLOBALS, BGP_GLOBALS_AF, BGP_GLOBALS_ROUTE_ADVERTISE, ROUTE_REDISTRIBUTE and VRF. The failure message points at the fix -- move the values into policy constants and emit them from a helper -- and warns against the easy way out, adding the table to the allowlist. SNMP_SERVER is the one allowlisted table: _add_snmp_configuration emits SNMP_SERVER["SYSTEM"] on every run with hardcoded defaults regardless of NetBox data, so the shipped entry is redundant rather than load-bearing. It could be dropped from the base config, which would let the allowlist go away; that is left out here to keep this change test-only. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- .../sonic/test_config_generator_ownership.py | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py index 154c7115d..2073656a1 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_ownership.py @@ -16,6 +16,7 @@ """ import ast +import json from pathlib import Path from types import SimpleNamespace @@ -701,3 +702,70 @@ def test_multi_owner_tables_are_not_inherited_or_consumed(self): multi_owner = set(MULTI_OWNER_OWNED_TABLE_KEYS) assert multi_owner.isdisjoint(INHERITED_TABLE_KEYS) assert multi_owner.isdisjoint(READ_ONLY_TABLE_KEYS) + + +# --------------------------------------------------------------------------- +# Static guard: the shipped base config carries no owned-table content +# --------------------------------------------------------------------------- + +# The base config_db.json shipped in this repo. The Containerfile installs it +# into the conductor image at /etc/sonic/config_db.json, where +# generate_sonic_config loads it as the starting point for every device. +# Resolved from this test file so the path holds in any checkout. +SHIPPED_BASE_CONFIG_PATH = ( + Path(__file__).parents[5] / "files" / "sonic" / "config_db.json" +) + +# Owned tables the shipped base config may populate. Adding a table here is +# almost always the wrong fix -- see the guard's failure message below. +# +# SNMP_SERVER: _add_snmp_configuration emits SNMP_SERVER["SYSTEM"] on every run +# with hardcoded defaults, whether or not NetBox carries SNMP data, so the +# shipped entry is always regenerated. It is redundant rather than +# load-bearing, and could be dropped from the base config entirely. +BASE_CONFIG_OWNED_TABLE_ALLOWLIST = frozenset({"SNMP_SERVER"}) + + +class TestShippedBaseConfigCarriesNoOwnedContent: + """The shipped base config must not populate generator-owned tables. + + Owned tables are dropped before any helper runs, so whatever the base + config puts in one is discarded on every regen: it reads as configuration + but never reaches a switch. That is not hypothetical, it is #2515 -- the + default-VRF BGP entries shipped in files/sonic/config_db.json, the + owned-table drop removed them, nothing regenerated them, and the default + VRF stopped advertising its routes into EVPN. + + No existing test could catch that. Every base-config fixture is built from + TOP_LEVEL_SCAFFOLD_KEYS with all tables empty (see make_base_config), so + dropping populated content is indistinguishable from dropping nothing, and + the exhaustive stale-entry sweep asserts that a seeded entry *is* dropped + -- it verifies the drop works, which is the mechanism at fault, so it + cannot detect over-deletion. This guard reads the real shipped file + instead, and needs no fixture. + """ + + def test_no_owned_table_is_populated_in_shipped_base_config(self): + base_config = json.loads(SHIPPED_BASE_CONFIG_PATH.read_text()) + populated_owned = { + table + for table, content in base_config.items() + if content + and table in OWNED_TABLE_KEYS + and table not in BASE_CONFIG_OWNED_TABLE_ALLOWLIST + } + + assert not populated_owned, ( + "files/sonic/config_db.json populates these generator-owned " + "tables, so their content is dropped on every regen and never " + "reaches a switch: " + ", ".join(sorted(populated_owned)) + ".\n\n" + "Move the values into config_generator.py as policy constants and " + "emit them from a helper (see the DEFAULT_VRF_* mappings and " + "_add_default_vrf_configuration), then remove them from the base " + "config -- that is what 'Where defaults belong' in the " + "generate_sonic_config ownership model requires.\n\n" + "Do not add the table to BASE_CONFIG_OWNED_TABLE_ALLOWLIST unless " + "the generator already regenerates it unconditionally: the " + "allowlist is for entries that are redundant, not for entries " + "that are needed." + ) From a8f25b1439dc2bc79e54cfd004c002238826919b Mon Sep 17 00:00:00 2001 From: Roger Luethi Date: Fri, 24 Jul 2026 23:18:28 +0200 Subject: [PATCH 3/3] sonic: make the base config path configurable The base config_db.json path was hardcoded to /etc/sonic/config_db.json. That is correct inside the conductor image, where the Containerfile installs files/sonic/config_db.json at exactly that path, and wrong everywhere else: a generator running outside the image finds nothing there and silently proceeds with an empty base config. Silently, because the miss was logged at debug level. Nothing in the output says so either -- the config still generates, it just omits the DEVICE_METADATA localhost fields the generator does not write itself, every pass-through table, and the TELEMETRY gNMI port the control-plane ACL rule reads, which falls back to a default. A golden test harness generating in a venv rather than the conductor image hits exactly this: its goldens carry the 38 tables the generator emits rather than the 58 a real conductor starts from. Add SONIC_BASE_CONFIG_PATH, defaulting to the previous hardcoded value so production behaviour is unchanged, and read it through settings. A harness can then point at the in-repo files/sonic/config_db.json and generate what the conductor generates. Raise the not-found log from debug to warning. Generating without a base config is not a routine condition to be discovered by turning debug logging on; it means the output is incomplete, and the operator or harness should see that. ERROR is too strong: the fallback is deliberate and the run still produces usable config for the owned tables, which is what a from-scratch device needs. Tests cover the setting's default and override, that the generator reads the configured path rather than a hardcoded one, and that the missing-base case warns. patch_base_config now returns the open mock so a test can assert on the path it was called with. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi --- osism/settings.py | 10 ++++ .../tasks/conductor/sonic/config_generator.py | 22 +++++++-- .../sonic/_config_generator_helpers.py | 10 ++-- .../test_config_generator_orchestrator.py | 48 +++++++++++++++++++ tests/unit/test_settings.py | 14 ++++++ 5 files changed, 95 insertions(+), 9 deletions(-) diff --git a/osism/settings.py b/osism/settings.py index effb911d7..7054882a0 100644 --- a/osism/settings.py +++ b/osism/settings.py @@ -72,6 +72,16 @@ def read_secret(secret_name): DEFAULT_NETBOX_FILTER_CONDUCTOR_SONIC, ) +# Base config_db.json the SONiC config generator builds every device config on. +# Shipped in this repo as files/sonic/config_db.json and installed at this path +# by the Containerfile, so the default is correct inside the conductor image. +# Overridable so a generator running outside that image -- a test harness, a +# pip install -- can point at the in-repo copy instead of silently generating +# without a base config (see the ownership model on generate_sonic_config). +SONIC_BASE_CONFIG_PATH = os.getenv( + "SONIC_BASE_CONFIG_PATH", "/etc/sonic/config_db.json" +) + # SONiC export configuration SONIC_EXPORT_DIR = os.getenv("SONIC_EXPORT_DIR", "/etc/sonic/export") SONIC_EXPORT_PREFIX = os.getenv("SONIC_EXPORT_PREFIX", "osism_") diff --git a/osism/tasks/conductor/sonic/config_generator.py b/osism/tasks/conductor/sonic/config_generator.py index 2d54b2b4b..f83456270 100644 --- a/osism/tasks/conductor/sonic/config_generator.py +++ b/osism/tasks/conductor/sonic/config_generator.py @@ -11,7 +11,7 @@ from typing import Optional from loguru import logger -from osism import utils +from osism import settings, utils from osism.tasks.conductor.netbox import ( get_device_interface_ips, get_device_loopbacks, @@ -323,7 +323,7 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= The base config ships *only* in the container image — files/ is not Python package data — so a generator running outside that image (pip install, test harness) finds nothing and falls back to the hardcoded - defaults. + defaults. Point SONIC_BASE_CONFIG_PATH at the in-repo copy there. A static guard test (see test_config_generator_ownership.py) parses this module and fails the build if it references a table that is not @@ -410,9 +410,9 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= hostname = get_device_hostname(device) mac_address = get_device_mac_address(device) - # Try to load base configuration from /etc/sonic/config_db.json + # Try to load the base configuration (see the ownership model above) # Always start with a fresh, empty configuration for each device - base_config_path = "/etc/sonic/config_db.json" + base_config_path = settings.SONIC_BASE_CONFIG_PATH config = {} try: @@ -425,7 +425,19 @@ def generate_sonic_config(device, hwsku, device_as_mapping=None, config_version= f"Loaded fresh base configuration from {base_config_path} for device {device.name}" ) else: - logger.debug( + # Warn rather than debug: the base config supplies content nothing + # else does, so the result is incomplete in ways that are easy to + # miss. Absent are the DEVICE_METADATA localhost fields the + # generator does not itself write (type, default_config_profile, + # frr_mgmt_framework_config, intf_naming_mode) and every + # pass-through table (FEATURE, CLASSIFIER_TABLE, the POLICY_* set, + # ...); the read-only TELEMETRY gNMI port falls back to + # DEFAULT_GNMI_PORT. What the generator writes itself is unaffected + # -- the localhost attributes it owns (hostname, hwsku, platform, + # mac) and the DATABASE VERSION default -- which is precisely why + # the gap is easy to overlook downstream. Set + # SONIC_BASE_CONFIG_PATH when running outside the conductor image. + logger.warning( f"Base config file {base_config_path} not found, starting with empty config for device {device.name}" ) except Exception as e: diff --git a/tests/unit/tasks/conductor/sonic/_config_generator_helpers.py b/tests/unit/tasks/conductor/sonic/_config_generator_helpers.py index b7139882b..08c6c0879 100644 --- a/tests/unit/tasks/conductor/sonic/_config_generator_helpers.py +++ b/tests/unit/tasks/conductor/sonic/_config_generator_helpers.py @@ -37,16 +37,18 @@ def patch_base_config(mocker, *, exists=True, base_config=None, raise_on_open=No - ``exists=False`` → ``open`` is not patched (the orchestrator never reaches the ``with open`` path). - ``raise_on_open`` → ``open`` raises this exception (e.g. ``OSError``). + + Returns the patched ``open`` mock so a caller can assert on the path it was + called with, or ``None`` when ``exists=False`` and ``open`` is left alone. """ mocker.patch.object(config_generator.os.path, "exists", return_value=exists) if not exists: - return + return None if raise_on_open is not None: - mocker.patch("builtins.open", side_effect=raise_on_open) - return + return mocker.patch("builtins.open", side_effect=raise_on_open) cfg = base_config if base_config is not None else make_base_config() - mocker.patch("builtins.open", mock_open(read_data=json.dumps(cfg))) + return mocker.patch("builtins.open", mock_open(read_data=json.dumps(cfg))) def make_iface(name, *, mgmt_only=False, type_value=None, iface_id=None): diff --git a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py index eb83fa83a..53bbff472 100644 --- a/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py +++ b/tests/unit/tasks/conductor/sonic/test_config_generator_orchestrator.py @@ -223,6 +223,54 @@ def test_generate_sonic_config_preserves_existing_localhost_keys( # --------------------------------------------------------------------------- +def test_generate_sonic_config_loads_base_from_configured_path( + mocker, patch_orchestrator_helpers, make_orchestrator_device +): + """The base config is read from settings.SONIC_BASE_CONFIG_PATH. + + The path is overridable so a generator running outside the conductor image + (test harness, pip install) can point at the in-repo copy instead of + silently generating without a base config. + """ + mocker.patch.object( + config_generator.settings, + "SONIC_BASE_CONFIG_PATH", + "/custom/base/config_db.json", + ) + opener = patch_base_config( + mocker, base_config=make_base_config(version="version_4_2_0") + ) + device = make_orchestrator_device() + + config = generate_sonic_config(device, "Test-HWSKU", config_version=None) + + assert config["VERSIONS"]["DATABASE"]["VERSION"] == "version_4_2_0" + assert opener.call_args_list[0].args[0] == "/custom/base/config_db.json" + + +def test_generate_sonic_config_warns_when_base_absent( + mocker, patch_orchestrator_helpers, make_orchestrator_device +): + """A missing base config is a warning, not a debug line. + + Without it the config is missing the DEVICE_METADATA localhost fields the + generator does not write itself (type, default_config_profile, …) and every + pass-through table, while TELEMETRY falls back to DEFAULT_GNMI_PORT. What + the generator writes itself still lands, so the gap is easy to miss + downstream — the E2E harness generated against no base config for months. + """ + patch_base_config(mocker, exists=False) + warning = mocker.patch.object(config_generator.logger, "warning") + device = make_orchestrator_device() + + generate_sonic_config(device, "Test-HWSKU") + + assert any( + "not found, starting with empty config" in str(call) + for call in warning.call_args_list + ) + + def test_generate_sonic_config_starts_from_scaffold_when_base_absent( mocker, patch_orchestrator_helpers, make_orchestrator_device ): diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index 4f1df8caa..6696dc195 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -655,3 +655,17 @@ def test_netbox_max_connections_override(reload_settings, monkeypatch): reload_settings() assert settings_module.NETBOX_MAX_CONNECTIONS == 25 + + +def test_sonic_base_config_path_default(reload_settings, monkeypatch): + monkeypatch.delenv("SONIC_BASE_CONFIG_PATH", raising=False) + reload_settings() + + assert settings_module.SONIC_BASE_CONFIG_PATH == "/etc/sonic/config_db.json" + + +def test_sonic_base_config_path_override(reload_settings, monkeypatch): + monkeypatch.setenv("SONIC_BASE_CONFIG_PATH", "/repo/files/sonic/config_db.json") + reload_settings() + + assert settings_module.SONIC_BASE_CONFIG_PATH == "/repo/files/sonic/config_db.json"