Add unit tests for SONiC sync and exporter modules and fix issues found in review

[berendt]

Closes #2224. Part of the Tier 3 SONiC unit-test effort (#2199), follow-up
to #2192 / #2193.

Adds two test modules covering the SONiC sync pipeline. Both mirror the
existing tests/unit/tasks/conductor/sonic/ conventions: the shared
mock_nb / loguru_logs fixtures, SimpleNamespace device stubs, and a
per-module fixture that stubs collaborators at their import site (the
patch_orchestrator_helpers pattern).

The review surfaced seven real defects in exporter.py / sync.py that the
initial tests would otherwise have pinned as intended behavior. Since the
requested regression tests only pass against fixed sources, the fixes are
included here as separate commits (see below), so the PR is no longer
tests-only.

Change set (commit order)

1. Add unit tests for SONiC exporter module — test_exporter.py

   • save_config_to_netbox: first-time create, no-change (DeepDiff empty,
     no save), changed config (unified diff + journal entry + save), journal
     failure still saves. Both the tuple (return_diff=True) and bool return
     forms are asserted.
   • export_config_to_file: filename selection (hostname vs. serial-number,
     empty/missing serial falls back to hostname with a warning), diff
     handling (missing / identical / changed / unreadable file), symlink
     handling (absent target, existing regular file, dangling symlink,
     os.symlink failure, hostname mode / falsy serial skip), and the outer
     os.makedirs failure path.
   • File-export tests use a real tmp_path filesystem where that reads more
     clearly (the symlink end-state proves remove-then-symlink ordering), and
     patch os.* only to inject the symlink / makedirs failures.

2. Add unit tests for SONiC sync orchestrator — test_sync.py

   • Cache lifecycle: exact start/end clear ordering via attach_mock, and
     the metalbox / VIP loads called once per invocation.
   • Single-device path: allowed role processed; disallowed role, role=None,
     not-found, and lookup-raises each return {}; single spine triggers the
     full spine fetch (nb.dcim.devices.filter), single leaf does not.
   • Multi-device path: only allowed roles kept; role-less devices skipped.
   • Per-device: missing / unsupported / valid HWSKU; config_version read
     from custom fields.
   • AS mapping: computed per spine group, propagated into
     generate_sonic_config, and a min_as=None group is not propagated.
   • Diff handling: diff streamed to task output, first-time message,
     no-change still exports, show_diff=False calls save without
     return_diff.
   • Task output gated on task_id; cache-stats logged only when non-empty.

   generate_sonic_config and find_interconnected_devices are stubbed (they
   have their own issues under Meta: Unit test coverage for osism/ #2199), per the issue's instructions.

Review feedback fixes (each with regression tests)

3. Own only the sonic_config key in NetBox local context (review finding 1)
   — save_config_to_netbox no longer replaces the device's entire
   local_context_data dict; sibling keys such as frr_parameters /
   netplan_parameters survive, and diffing covers only the owned
   sonic_config key (mirrors the sonic: enforce full config_db.json ownership #2297 partitioned-ownership model).

4. Create SONiC journal entry only after successful save (finding 7)
   — a failed device.save() can no longer leave an orphaned journal entry
   claiming the update succeeded.

5. Write SONiC config exports atomically (finding 3)
   — temp file + os.replace() so a mid-write failure (ENOSPC, killed
   process) cannot truncate the previous export.

6. Reconcile hostname symlink independently of config changes
   (findings 2 + 4) — a missing/stale link is repaired even when the config
   content is unchanged, the reconciliation is idempotent, and a
   hostname == serial device no longer destroys its own config via a
   self-referential symlink.

7. Always clean up caches and finish task output in sync_sonic (finding 5)
   — try/finally around the sync body; early returns and per-device
   failures no longer leak the module-level caches into the next run, finish
   the task with rc=1, and a failing device no longer aborts the remaining
   devices. The summary log no longer raises on PORT-less configs.

8. Raise on SONiC persistence failures instead of reporting no change
   (finding 6) — failed NetBox saves and file exports raise instead of
   returning the same value as "no changes"; sync_sonic reports them via
   rc=1 instead of finishing failed syncs with rc=0.

Local verification

• black --check — pass
• flake8 — pass
• pytest — not run locally; the unit-test suite runs in the
  python-osism-unit-tests Zuul job. The coverage gate
  (--cov=...sync/...exporter >= 90 %) is verified there.

[sourcery-ai]

Hey - I've left some high level feedback:

• Both test_sync.py and test_exporter.py define identical helpers like _has_log; consider moving these small utilities into a shared test helper/fixture module under tests/unit/tasks/conductor/sonic/ to avoid duplication and keep future changes in one place.
• Several tests assert on exact log message substrings and precise call sequences (e.g. full cache lifecycle order); if the behavior doesn’t require strict ordering or exact wording, consider loosening these expectations slightly to reduce brittleness against harmless refactors while still checking the important semantics.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Both `test_sync.py` and `test_exporter.py` define identical helpers like `_has_log`; consider moving these small utilities into a shared test helper/fixture module under `tests/unit/tasks/conductor/sonic/` to avoid duplication and keep future changes in one place.
- Several tests assert on exact log message substrings and precise call sequences (e.g. full cache lifecycle order); if the behavior doesn’t require strict ordering or exact wording, consider loosening these expectations slightly to reduce brittleness against harmless refactors while still checking the important semantics.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[ideaship]

Nothing major, just some minor issues.

[ideaship]

Finding 1 — local_context_data wholesale-replace clobbers sibling keys (data loss)

These changed-context save tests only build local_context_data={"sonic_config": ...}, which hides that save_config_to_netbox replaces the device's entire dict (exporter.py:88/:94) and diffs the full existing context against {"sonic_config": ...} (exporter.py:42). Please add a case with a sibling key, e.g. local_context_data={"frr_parameters": {...}, "sonic_config": old}, and assert the sibling survives.

It won't today — verified by running the real exporter against a leaf-role device:

BEFORE: ['frr_parameters', 'sonic_config']
AFTER : ['sonic_config']

The exporter deletes frr_parameters, reports it as a removal in its diff log, and writes a journal entry attributing the deletion to the SONiC update. frr_parameters is not hypothetical — the inventory-reconciler reads it (and netplan_parameters) out of local_context_data via config_context.

Tests-only PR, so the source fix is a separate change: mirror the #2297 partitioned-ownership model — own and replace only the sonic_config key (device.local_context_data = {**(device.local_context_data or {}), "sonic_config": config}) and diff only that key. This test should pin sibling-key survival once that lands.

[ideaship]

Finding 6 — failed writes reported to the task layer as rc=0

assert result == (False, None) pins an ambiguity: save_config_to_netbox returns the same (False, None) for a failed save (exporter.py:103) as for no change (:48), and export_config_to_file does the same (:226 vs :222). In sync.py (not in this diff), :219 then treats a failed write as "no changes", logs accordingly, and still finishes rc=0 (:244) — failed syncs report success. Source fix: failures should raise or return a distinct sentinel so task status reflects them; this test should assert the distinction once that lands.

[ideaship]

Finding 7 — journal entry created before device.save()

Both save-failure tests pass local_context_data=None, hitting only the first-time branch. Please add a changed-path save failure (existing sonic_config + raising device.save()). In exporter.py (not in this diff) the journal entry is created at :71 before save() at :89, so a failed save returns (False, None) while an orphaned journal entry already claims the update succeeded (and the in-memory device stays mutated). Source fix: move journal creation after a successful save.

[ideaship]

Finding 4 — symlink repair gated on config_changed

Symlink reconciliation should be tested independently of a content change. In exporter.py (not in this diff) the whole symlink block is gated by if config_changed: (:177), so an unchanged serial-mode config with a missing/stale hostname link is never repaired — and after a symlink failure, a later unchanged run never retries. Please add a case: existing identical config + missing hostname link, and assert the link gets created. It currently won't.

[ideaship]

Finding 2 — hostname == serial → self-referential symlink (low priority)

Low-priority defensive case. Every symlink test uses distinct hostname (sw-1) and serial (ABC123); add one where they're equal. In exporter.py (not in this diff), when serial == hostname the just-written config file is the hostname path, so :207 os.removes it and :209 symlinks it to itself — config destroyed, yet the function returns True. One-line source fix: skip the symlink when filepath == hostname_filepath.

Unlikely in practice — needs the serial set equal to the device name/inventory_hostname, which doesn't happen with real manufacturer serials or on the seeded testbed (serials unset → safe fallback). But the guard is trivial against silent data loss that reports success.

[ideaship]

Finding 3 — non-atomic write truncates the previous export (cheap hygiene)

Cheap-hygiene note, not a blocker. Error coverage only injects a makedirs failure; a write/serialize-failure case (patch json.dump to raise, assert the previous export is still intact) would be a nice addition. In exporter.py (not in this diff) the file is opened "w" at :179, truncating before serialization, so a mid-write crash (ENOSPC / killed process) leaves the prior config truncated and returns False. Source fix: temp file + os.replace().

Low priority here — the export self-heals on the next sync and the in-repo reader rejects bad JSON — but it would rank higher if we expected attackers or external consumers of the export dir.

[ideaship]

Finding 5 — cleanup / finish_task_output skipped on early returns and mid-loop exceptions (would block)

These early-return tests check the return value and log message but not that caches were cleared or finish_task_output was called. In sync.py (not in this diff), the early returns (:77/:80/:83) and any exception in the per-device loop — e.g. the unguarded len(sonic_config['PORT']) at :225 on a PORT-less config — return after caches load (:42–52) but before cleanup (:237–239) and finish_task_output (:244). Caches are module-level, so the leak corrupts the next run. Please assert cleanup + finalization on these paths and add a mid-loop raising-collaborator case; the source fix is a try/finally.