Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ideaship merged 8 commits into from
Jun 10, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
398baf6
Add unit tests for SONiC exporter module
berendt Jun 9, 2026
f13adf0
Add unit tests for SONiC sync orchestrator
berendt Jun 9, 2026
c50109d
Own only the sonic_config key in NetBox local context
berendt Jun 10, 2026
1204986
Create SONiC journal entry only after successful save
berendt Jun 10, 2026
d5fc20d
Write SONiC config exports atomically
berendt Jun 10, 2026
2d9cda9
Reconcile hostname symlink independently of config changes
berendt Jun 10, 2026
91d5f7f
Always clean up caches and finish task output in sync_sonic
berendt Jun 10, 2026
cbf7708
Raise on SONiC persistence failures instead of reporting no change
berendt Jun 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 85 additions & 42 deletions osism/tasks/conductor/sonic/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -26,20 +28,25 @@ 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
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(
Expand All @@ -50,10 +57,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,
Expand All @@ -65,10 +72,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}")

# Save diff to device journal log
# 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 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",
Expand All @@ -81,13 +97,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)
Expand All @@ -100,20 +110,26 @@ 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):
"""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
config: SONiC configuration dictionary

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
Expand Down Expand Up @@ -175,28 +191,55 @@ 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}")

# 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
Expand All @@ -210,17 +253,17 @@ 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

except Exception as e:
logger.error(f"Failed to export config for device {device.name}: {e}")
return False
raise
Loading