diff --git a/osism/tasks/conductor/sonic/exporter.py b/osism/tasks/conductor/sonic/exporter.py index 1fb0f7f6..8ed0cda8 100644 --- a/osism/tasks/conductor/sonic/exporter.py +++ b/osism/tasks/conductor/sonic/exporter.py @@ -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 @@ -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( @@ -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, @@ -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", @@ -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) @@ -100,13 +110,15 @@ 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 @@ -114,6 +126,10 @@ def export_config_to_file(device, config): 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 @@ -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 @@ -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 diff --git a/osism/tasks/conductor/sonic/sync.py b/osism/tasks/conductor/sonic/sync.py index 8f4046f0..8f41c1b7 100644 --- a/osism/tasks/conductor/sonic/sync.py +++ b/osism/tasks/conductor/sonic/sync.py @@ -25,6 +25,10 @@ def sync_sonic(device_name=None, task_id=None, show_diff=True): """Sync SONiC configurations for eligible devices. + Caches are always cleared and the task output is always finished, even + when the sync exits early or a device fails. Failures are reported to the + task layer via a non-zero rc. + Args: device_name (str, optional): Name of specific device to sync. If None, sync all eligible devices. task_id (str, optional): Task ID for output logging. @@ -54,194 +58,218 @@ def sync_sonic(device_name=None, task_id=None, show_diff=True): # Dictionary to store configurations for all devices device_configs = {} - logger.debug(f"Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}") - - devices = [] - - if device_name: - # When specific device is requested, fetch it directly - try: - device = utils.nb.dcim.devices.get(name=device_name) - if device: - # Check if device role matches allowed roles - if device.role and device.role.slug in DEFAULT_SONIC_ROLES: - devices.append(device) - logger.debug( - f"Found device: {device.name} with role: {device.role.slug}" - ) + rc = 0 + + try: + logger.debug(f"Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}") + + devices = [] + + if device_name: + # When specific device is requested, fetch it directly + try: + device = utils.nb.dcim.devices.get(name=device_name) + if device: + # Check if device role matches allowed roles + if device.role and device.role.slug in DEFAULT_SONIC_ROLES: + devices.append(device) + logger.debug( + f"Found device: {device.name} with role: {device.role.slug}" + ) + else: + logger.warning( + f"Device {device_name} has role '{device.role.slug if device.role else 'None'}' " + f"which is not in allowed SONiC roles: {', '.join(DEFAULT_SONIC_ROLES)}" + ) + rc = 1 + return device_configs else: - logger.warning( - f"Device {device_name} has role '{device.role.slug if device.role else 'None'}' " - f"which is not in allowed SONiC roles: {', '.join(DEFAULT_SONIC_ROLES)}" - ) + logger.error(f"Device {device_name} not found in NetBox") + rc = 1 return device_configs - else: - logger.error(f"Device {device_name} not found in NetBox") + except Exception as e: + logger.error(f"Error fetching device {device_name}: {e}") + rc = 1 return device_configs - except Exception as e: - logger.error(f"Error fetching device {device_name}: {e}") - return device_configs - else: - # Get device query list from NETBOX_FILTER_CONDUCTOR_SONIC - nb_device_query_list = get_nb_device_query_list_sonic() - - for nb_device_query in nb_device_query_list: - # Query devices with the NETBOX_FILTER_CONDUCTOR_SONIC criteria - for device in utils.nb.dcim.devices.filter(**nb_device_query): - # Check if device role matches allowed roles - if device.role and device.role.slug in DEFAULT_SONIC_ROLES: - devices.append(device) - logger.debug( - f"Found device: {device.name} with role: {device.role.slug}" - ) - - logger.info(f"Found {len(devices)} devices matching criteria") - - # Find interconnected spine/superspine groups for special AS calculation - # When processing a single device, we need to consider all spine/superspine devices - # to properly detect interconnected groups, not just the requested device - if device_name and devices: - # Check if the single device is a spine/superspine - target_device = devices[0] - if target_device.role and target_device.role.slug in ["spine", "superspine"]: - # Fetch ALL spine/superspine devices to properly detect groups - logger.debug( - "Single spine/superspine device detected, fetching all spine/superspine devices for group detection" - ) - all_spine_devices = [] + else: + # Get device query list from NETBOX_FILTER_CONDUCTOR_SONIC nb_device_query_list = get_nb_device_query_list_sonic() + for nb_device_query in nb_device_query_list: + # Query devices with the NETBOX_FILTER_CONDUCTOR_SONIC criteria for device in utils.nb.dcim.devices.filter(**nb_device_query): - if device.role and device.role.slug in ["spine", "superspine"]: - all_spine_devices.append(device) - spine_groups = find_interconnected_devices( - all_spine_devices, ["spine", "superspine"] - ) + # Check if device role matches allowed roles + if device.role and device.role.slug in DEFAULT_SONIC_ROLES: + devices.append(device) + logger.debug( + f"Found device: {device.name} with role: {device.role.slug}" + ) + + logger.info(f"Found {len(devices)} devices matching criteria") + + # Find interconnected spine/superspine groups for special AS calculation + # When processing a single device, we need to consider all spine/superspine devices + # to properly detect interconnected groups, not just the requested device + if device_name and devices: + # Check if the single device is a spine/superspine + target_device = devices[0] + if target_device.role and target_device.role.slug in [ + "spine", + "superspine", + ]: + # Fetch ALL spine/superspine devices to properly detect groups + logger.debug( + "Single spine/superspine device detected, fetching all spine/superspine devices for group detection" + ) + all_spine_devices = [] + nb_device_query_list = get_nb_device_query_list_sonic() + for nb_device_query in nb_device_query_list: + for device in utils.nb.dcim.devices.filter(**nb_device_query): + if device.role and device.role.slug in ["spine", "superspine"]: + all_spine_devices.append(device) + spine_groups = find_interconnected_devices( + all_spine_devices, ["spine", "superspine"] + ) + else: + # For non-spine devices, use the original logic + spine_groups = find_interconnected_devices( + devices, ["spine", "superspine"] + ) else: - # For non-spine devices, use the original logic + # For multi-device processing, use the original logic spine_groups = find_interconnected_devices(devices, ["spine", "superspine"]) - else: - # For multi-device processing, use the original logic - spine_groups = find_interconnected_devices(devices, ["spine", "superspine"]) - logger.info(f"Found {len(spine_groups)} interconnected spine/superspine groups") + logger.info(f"Found {len(spine_groups)} interconnected spine/superspine groups") - # Create mapping from device ID to its assigned AS number - device_as_mapping = {} - - # Calculate AS numbers for spine/superspine groups - for group in spine_groups: - min_as = calculate_minimum_as_for_group(group) - if min_as: - for device in group: - device_as_mapping[device.id] = min_as - logger.debug( - f"Assigned AS {min_as} to {len(group)} devices in spine/superspine group" - ) + # Create mapping from device ID to its assigned AS number + device_as_mapping = {} - # Generate SONIC configuration for each device - for device in devices: - # Get HWSKU from sonic_parameters custom field, default to None - hwsku = None - if ( - hasattr(device, "custom_fields") - and "sonic_parameters" in device.custom_fields - and device.custom_fields["sonic_parameters"] - and "hwsku" in device.custom_fields["sonic_parameters"] - ): - hwsku = device.custom_fields["sonic_parameters"]["hwsku"] - - # Get config_version from sonic_parameters custom field, default to None - config_version = None - if ( - hasattr(device, "custom_fields") - and "sonic_parameters" in device.custom_fields - and device.custom_fields["sonic_parameters"] - and "config_version" in device.custom_fields["sonic_parameters"] - ): - config_version = device.custom_fields["sonic_parameters"]["config_version"] - logger.debug( - f"Device {device.name} has custom config_version: {config_version}" - ) - - # Skip devices without HWSKU - if not hwsku: - logger.debug(f"Skipping device {device.name}: no HWSKU configured") - continue - - logger.debug(f"Processing device: {device.name} with HWSKU: {hwsku}") - - # Output current device being processed if task_id is available - if task_id: - utils.push_task_output(task_id, f"Processing device: {device.name}\n") + # Calculate AS numbers for spine/superspine groups + for group in spine_groups: + min_as = calculate_minimum_as_for_group(group) + if min_as: + for device in group: + device_as_mapping[device.id] = min_as + logger.debug( + f"Assigned AS {min_as} to {len(group)} devices in spine/superspine group" + ) - # Validate that HWSKU is supported - if hwsku not in SUPPORTED_HWSKUS: - logger.warning( - f"Device {device.name} has unsupported HWSKU: {hwsku}. Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}" - ) - continue + # Generate SONIC configuration for each device + for device in devices: + # Get HWSKU from sonic_parameters custom field, default to None + hwsku = None + if ( + hasattr(device, "custom_fields") + and "sonic_parameters" in device.custom_fields + and device.custom_fields["sonic_parameters"] + and "hwsku" in device.custom_fields["sonic_parameters"] + ): + hwsku = device.custom_fields["sonic_parameters"]["hwsku"] + + # Get config_version from sonic_parameters custom field, default to None + config_version = None + if ( + hasattr(device, "custom_fields") + and "sonic_parameters" in device.custom_fields + and device.custom_fields["sonic_parameters"] + and "config_version" in device.custom_fields["sonic_parameters"] + ): + config_version = device.custom_fields["sonic_parameters"][ + "config_version" + ] + logger.debug( + f"Device {device.name} has custom config_version: {config_version}" + ) - # Generate SONIC configuration based on device HWSKU - sonic_config = generate_sonic_config( - device, hwsku, device_as_mapping, config_version - ) + # Skip devices without HWSKU + if not hwsku: + logger.debug(f"Skipping device {device.name}: no HWSKU configured") + continue - # Store configuration in the dictionary - device_configs[device.name] = sonic_config + logger.debug(f"Processing device: {device.name} with HWSKU: {hwsku}") - # Save the generated configuration to NetBox config context (only if changed) - if show_diff: - netbox_changed, diff_output = save_config_to_netbox( - device, sonic_config, return_diff=True - ) + # Output current device being processed if task_id is available + if task_id: + utils.push_task_output(task_id, f"Processing device: {device.name}\n") - # Output diff to task if available and there are changes - if task_id and netbox_changed and diff_output: - utils.push_task_output(task_id, f"\n{'='*60}\n") - utils.push_task_output( - task_id, f"Configuration diff for {device.name}:\n" + # Validate that HWSKU is supported + if hwsku not in SUPPORTED_HWSKUS: + logger.warning( + f"Device {device.name} has unsupported HWSKU: {hwsku}. Supported HWSKUs: {', '.join(SUPPORTED_HWSKUS)}" ) - utils.push_task_output(task_id, f"{'='*60}\n") - utils.push_task_output(task_id, f"{diff_output}\n") - utils.push_task_output(task_id, f"{'='*60}\n\n") - elif task_id and netbox_changed and not diff_output: - # First-time configuration (no diff available) - utils.push_task_output( - task_id, f"First-time configuration created for {device.name}\n" + continue + + # A failing device must not abort the sync of the remaining + # devices, but it has to surface in the task rc + try: + # Generate SONIC configuration based on device HWSKU + sonic_config = generate_sonic_config( + device, hwsku, device_as_mapping, config_version ) - else: - netbox_changed = save_config_to_netbox(device, sonic_config) - # Export the generated configuration to local file (only if changed) - file_changed = export_config_to_file(device, sonic_config) + # Store configuration in the dictionary + device_configs[device.name] = sonic_config - if netbox_changed or file_changed: - logger.info(f"Configuration updated for device {device.name}") - else: - logger.info(f"No configuration changes for device {device.name}") + # Save the generated configuration to NetBox config context (only if changed) + if show_diff: + netbox_changed, diff_output = save_config_to_netbox( + device, sonic_config, return_diff=True + ) - logger.info( - f"Generated SONiC config for device {device.name} with {len(sonic_config['PORT'])} ports" - ) + # Output diff to task if available and there are changes + if task_id and netbox_changed and diff_output: + utils.push_task_output(task_id, f"\n{'='*60}\n") + utils.push_task_output( + task_id, f"Configuration diff for {device.name}:\n" + ) + utils.push_task_output(task_id, f"{'='*60}\n") + utils.push_task_output(task_id, f"{diff_output}\n") + utils.push_task_output(task_id, f"{'='*60}\n\n") + elif task_id and netbox_changed and not diff_output: + # First-time configuration (no diff available) + utils.push_task_output( + task_id, + f"First-time configuration created for {device.name}\n", + ) + else: + netbox_changed = save_config_to_netbox(device, sonic_config) - logger.info(f"Generated SONiC configurations for {len(device_configs)} devices") + # Export the generated configuration to local file (only if changed) + file_changed = export_config_to_file(device, sonic_config) - # Log cache statistics and cleanup - cache_stats = get_interface_cache_stats() - if cache_stats: - logger.debug( - f"Interface cache stats: {cache_stats['cached_devices']} devices, {cache_stats['total_interfaces']} interfaces" - ) + if netbox_changed or file_changed: + logger.info(f"Configuration updated for device {device.name}") + else: + logger.info(f"No configuration changes for device {device.name}") - clear_interface_cache() - clear_all_caches() - clear_vip_addresses_cache() - logger.debug("Cleared all caches after sync_sonic task completion") + logger.info( + f"Generated SONiC config for device {device.name} with {len(sonic_config.get('PORT', {}))} ports" + ) + except Exception as e: + logger.error( + f"Failed to sync SONiC configuration for device {device.name}: {e}" + ) + rc = 1 + + logger.info(f"Generated SONiC configurations for {len(device_configs)} devices") - # Finish task output if task_id is available - if task_id: - utils.finish_task_output(task_id, rc=0) + # Log cache statistics + cache_stats = get_interface_cache_stats() + if cache_stats: + logger.debug( + f"Interface cache stats: {cache_stats['cached_devices']} devices, {cache_stats['total_interfaces']} interfaces" + ) + finally: + # Cleanup must run on every exit path — the caches are module-level + # and would otherwise leak into the next run + clear_interface_cache() + clear_all_caches() + clear_vip_addresses_cache() + logger.debug("Cleared all caches after sync_sonic task completion") + + # Finish task output if task_id is available + if task_id: + utils.finish_task_output(task_id, rc=rc) # Return the dictionary with all device configurations return device_configs diff --git a/tests/unit/tasks/conductor/sonic/test_exporter.py b/tests/unit/tasks/conductor/sonic/test_exporter.py new file mode 100644 index 00000000..44981699 --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_exporter.py @@ -0,0 +1,543 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for ``osism.tasks.conductor.sonic.exporter``. + +Covers ``save_config_to_netbox`` (NetBox local-context persistence with diff +checking + journal logging) and ``export_config_to_file`` (on-disk export with +diff checking, filename selection, and the serial-number→hostname symlink). + +``save_config_to_netbox`` reuses the shared ``mock_nb`` fixture (it patches +``osism.utils.nb``, which ``exporter.utils.nb`` resolves to). The file-export +tests drive a real filesystem under ``tmp_path`` where that reads more clearly +than asserting on mocked ``os`` calls, and fall back to patching ``os.*`` only +to inject the two error paths (symlink / makedirs failures). +""" + +import json +import os +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from osism.tasks.conductor.sonic.exporter import ( + export_config_to_file, + save_config_to_netbox, +) + + +def _has_log(records, level, substring): + return any(r["level"] == level and substring in r["message"] for r in records) + + +# --------------------------------------------------------------------------- +# save_config_to_netbox +# --------------------------------------------------------------------------- + + +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( + id=device_id, + name=name, + local_context_data=local_context_data, + save=MagicMock(), + ) + + +def test_save_config_first_time_saves_and_returns_true(mock_nb): + """No existing local context → first-time path saves and reports change.""" + device = _make_save_device(local_context_data=None) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet0": {}}}) + + assert result is True + device.save.assert_called_once_with() + assert device.local_context_data == {"sonic_config": {"PORT": {"Ethernet0": {}}}} + # First-time configuration creates no journal entry. + mock_nb.extras.journal_entries.create.assert_not_called() + + +def test_save_config_first_time_return_diff_tuple(mock_nb): + """``return_diff=True`` on the first-time path yields ``(True, None)``.""" + device = _make_save_device(local_context_data=None) + + result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert result == (True, None) + + +def test_save_config_no_change_skips_save(mock_nb): + """Identical existing context → ``DeepDiff`` empty, no save, returns False.""" + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device(local_context_data={"sonic_config": config}) + + result = save_config_to_netbox(device, config) + + assert result is False + device.save.assert_not_called() + + +def test_save_config_no_change_return_diff_tuple(mock_nb): + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device(local_context_data={"sonic_config": config}) + + result = save_config_to_netbox(device, config, return_diff=True) + + assert result == (False, None) + + +def test_save_config_change_writes_journal_and_saves(mock_nb): + """A changed config generates a diff, a journal entry, and a save.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + changed, diff_text = save_config_to_netbox( + device, {"PORT": {"Ethernet1": {}}}, return_diff=True + ) + + assert changed is True + # The unified diff names both the removed and the added port. + assert "Ethernet0" in diff_text + assert "Ethernet1" in diff_text + device.save.assert_called_once_with() + mock_nb.extras.journal_entries.create.assert_called_once() + kwargs = mock_nb.extras.journal_entries.create.call_args.kwargs + assert kwargs["assigned_object_type"] == "dcim.device" + assert kwargs["assigned_object_id"] == device.id + assert kwargs["kind"] == "info" + assert "```diff" in kwargs["comments"] + + +def test_save_config_change_bool_form(mock_nb): + """Without ``return_diff`` the changed path returns a bare ``True``.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}) + + assert result is True + + +def test_save_config_preserves_sibling_context_keys(mock_nb): + """Only the ``sonic_config`` key is owned — sibling keys like + ``frr_parameters`` must survive a config update untouched.""" + frr = {"asn": 4200000001, "loopback": "10.0.0.1"} + device = _make_save_device( + local_context_data={ + "frr_parameters": frr, + "sonic_config": {"PORT": {"Ethernet0": {}}}, + } + ) + + result = save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}) + + assert result is True + device.save.assert_called_once_with() + assert device.local_context_data == { + "frr_parameters": frr, + "sonic_config": {"PORT": {"Ethernet1": {}}}, + } + + +def test_save_config_sibling_only_context_is_first_time(mock_nb): + """A context holding only sibling keys has no ``sonic_config`` to diff: + the first-time path adds the key and keeps the siblings.""" + frr = {"asn": 4200000001} + device = _make_save_device(local_context_data={"frr_parameters": frr}) + + result = save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert result == (True, None) + device.save.assert_called_once_with() + assert device.local_context_data == { + "frr_parameters": frr, + "sonic_config": {"PORT": {}}, + } + # First-time configuration creates no journal entry. + mock_nb.extras.journal_entries.create.assert_not_called() + + +def test_save_config_sibling_keys_do_not_trigger_change(mock_nb): + """Diffing covers only ``sonic_config`` — sibling keys must not register + as removals and force a save when the SONiC config itself is unchanged.""" + config = {"PORT": {"Ethernet0": {}}} + device = _make_save_device( + local_context_data={"frr_parameters": {"asn": 1}, "sonic_config": config} + ) + + result = save_config_to_netbox(device, config) + + assert result is False + device.save.assert_not_called() + + +def test_save_config_journal_failure_still_saves(mock_nb, loguru_logs): + """A failing journal create is logged but must not block the save.""" + mock_nb.extras.journal_entries.create.side_effect = RuntimeError("journal down") + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + + changed, diff_text = save_config_to_netbox( + device, {"PORT": {"Ethernet1": {}}}, return_diff=True + ) + + assert changed is True + assert diff_text is not None + device.save.assert_called_once_with() + assert _has_log(loguru_logs, "ERROR", "Failed to save diff to journal") + + +def test_save_config_save_failure_raises(mock_nb, loguru_logs): + """A raising ``device.save()`` is logged and re-raised so a failed save + stays distinguishable from "no changes" at the task layer.""" + device = _make_save_device(local_context_data=None) + device.save.side_effect = RuntimeError("netbox write failed") + + with pytest.raises(RuntimeError, match="netbox write failed"): + save_config_to_netbox(device, {"PORT": {}}, return_diff=True) + + assert _has_log(loguru_logs, "ERROR", "Failed to save local context") + + +def test_save_config_changed_path_save_failure_creates_no_journal(mock_nb, loguru_logs): + """A failed save on the changed path must not leave an orphaned journal + entry claiming the update succeeded — journal creation follows the save.""" + device = _make_save_device( + local_context_data={"sonic_config": {"PORT": {"Ethernet0": {}}}} + ) + device.save.side_effect = RuntimeError("netbox write failed") + + with pytest.raises(RuntimeError, match="netbox write failed"): + save_config_to_netbox(device, {"PORT": {"Ethernet1": {}}}, return_diff=True) + + mock_nb.extras.journal_entries.create.assert_not_called() + assert _has_log(loguru_logs, "ERROR", "Failed to save local context") + + +# --------------------------------------------------------------------------- +# export_config_to_file +# --------------------------------------------------------------------------- + + +@pytest.fixture +def export_settings(mocker): + """Patch the four ``SONIC_EXPORT_*`` settings exporter reads at call time.""" + + def _set( + export_dir, + identifier="serial-number", + prefix="osism_", + suffix="_config_db.json", + ): + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_DIR", + str(export_dir), + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_PREFIX", + prefix, + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_SUFFIX", + suffix, + ) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.settings.SONIC_EXPORT_IDENTIFIER", + identifier, + ) + + return _set + + +@pytest.fixture +def patch_hostname(mocker): + """Pin ``get_device_hostname`` so filenames are deterministic.""" + return mocker.patch( + "osism.tasks.conductor.sonic.exporter.get_device_hostname", + return_value="sw-1", + ) + + +# --- filename selection ----------------------------------------------------- + + +def test_export_hostname_identifier_uses_hostname( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, config) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert json.loads(target.read_text()) == config + + +def test_export_serial_identifier_uses_serial( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert (tmp_path / "osism_ABC123_config_db.json").exists() + + +@pytest.mark.parametrize("serial", ["", None]) +def test_export_serial_missing_falls_back_to_hostname( + tmp_path, export_settings, patch_hostname, loguru_logs, serial +): + """An empty or absent serial in serial-number mode warns and uses hostname, + and the symlink branch is skipped entirely (debug logged).""" + export_settings(tmp_path, identifier="serial-number") + device = ( + SimpleNamespace(name="sw-1", serial=serial) + if serial is not None + else SimpleNamespace(name="sw-1") + ) + + assert export_config_to_file(device, {"PORT": {}}) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + assert _has_log(loguru_logs, "WARNING", "Serial number not found") + assert _has_log(loguru_logs, "DEBUG", "Symlink conditions not met") + + +# --- diff handling ---------------------------------------------------------- + + +def test_export_no_change_returns_false(tmp_path, export_settings, patch_hostname): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_sw-1_config_db.json").write_text(json.dumps(config)) + + assert export_config_to_file(device, config) is False + + +def test_export_changed_content_rewrites_file( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + target.write_text(json.dumps({"PORT": {"Ethernet9": {}}})) + new_config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, new_config) is True + assert json.loads(target.read_text()) == new_config + + +def test_export_unreadable_file_is_overwritten( + tmp_path, export_settings, patch_hostname, loguru_logs +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + target.write_text("not valid json {") + new_config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, new_config) is True + assert json.loads(target.read_text()) == new_config + assert _has_log(loguru_logs, "WARNING", "Could not read existing config file") + + +def test_export_write_failure_preserves_previous_export( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + """The export is written atomically: a mid-write failure must leave the + previous export intact instead of truncating it, and not leak a temp file.""" + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + target = tmp_path / "osism_sw-1_config_db.json" + old_config = {"PORT": {"Ethernet9": {}}} + target.write_text(json.dumps(old_config)) + mocker.patch( + "osism.tasks.conductor.sonic.exporter.json.dump", + side_effect=OSError("no space left on device"), + ) + + with pytest.raises(OSError, match="no space left on device"): + export_config_to_file(device, {"PORT": {"Ethernet0": {}}}) + + assert json.loads(target.read_text()) == old_config + assert not (tmp_path / "osism_sw-1_config_db.json.tmp").exists() + assert _has_log(loguru_logs, "ERROR", "Failed to export config") + + +# --- symlink handling ------------------------------------------------------- + + +def test_export_creates_symlink_when_hostname_absent( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + link = tmp_path / "osism_sw-1_config_db.json" + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_replaces_existing_regular_file_with_symlink( + tmp_path, export_settings, patch_hostname +): + """A regular file at the hostname path is removed before the symlink is + created — observable because the path ends up as a symlink, not a file.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + link = tmp_path / "osism_sw-1_config_db.json" + link.write_text("stale regular file") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_replaces_dangling_symlink(tmp_path, export_settings, patch_hostname): + """A dangling symlink (``exists`` False, ``islink`` True) is removed and + repointed at the serial-number file.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("does_not_exist_target") + assert not os.path.exists(link) and os.path.islink(link) + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_unchanged_config_repairs_missing_symlink( + tmp_path, export_settings, patch_hostname +): + """Symlink reconciliation is independent of a content change: an unchanged + config with a missing hostname link still gets the link created.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + + assert export_config_to_file(device, config) is False + + link = tmp_path / "osism_sw-1_config_db.json" + assert link.is_symlink() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_unchanged_config_repoints_stale_symlink( + tmp_path, export_settings, patch_hostname +): + """A hostname link pointing at the wrong target is repointed even when + the exported content is unchanged.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("osism_OTHER_config_db.json") + + assert export_config_to_file(device, config) is False + + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_existing_correct_symlink_left_untouched( + tmp_path, export_settings, patch_hostname, mocker +): + """A link that already points at the config file is not removed and + recreated on every run.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="ABC123") + config = {"PORT": {"Ethernet0": {}}} + (tmp_path / "osism_ABC123_config_db.json").write_text(json.dumps(config)) + link = tmp_path / "osism_sw-1_config_db.json" + link.symlink_to("osism_ABC123_config_db.json") + remove_spy = mocker.spy(os, "remove") + symlink_spy = mocker.spy(os, "symlink") + + assert export_config_to_file(device, config) is False + + remove_spy.assert_not_called() + symlink_spy.assert_not_called() + assert os.readlink(link) == "osism_ABC123_config_db.json" + + +def test_export_hostname_equals_serial_keeps_config_file( + tmp_path, export_settings, patch_hostname +): + """When the serial equals the hostname the config file already lives at + the hostname path — no self-referential symlink may replace it.""" + export_settings(tmp_path, identifier="serial-number") + device = SimpleNamespace(name="sw-1", serial="sw-1") + config = {"PORT": {"Ethernet0": {}}} + + assert export_config_to_file(device, config) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + assert json.loads(target.read_text()) == config + + +def test_export_symlink_failure_still_returns_true( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + """A failing ``os.symlink`` is logged but the written config still counts + as a change (the file was exported successfully).""" + export_settings(tmp_path, identifier="serial-number") + mocker.patch( + "osism.tasks.conductor.sonic.exporter.os.symlink", + side_effect=OSError("permission denied"), + ) + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + assert (tmp_path / "osism_ABC123_config_db.json").exists() + assert _has_log(loguru_logs, "ERROR", "Failed to create hostname symlink") + + +def test_export_hostname_mode_makes_no_symlink( + tmp_path, export_settings, patch_hostname +): + export_settings(tmp_path, identifier="hostname") + device = SimpleNamespace(name="sw-1", serial="ABC123") + + assert export_config_to_file(device, {"PORT": {}}) is True + + target = tmp_path / "osism_sw-1_config_db.json" + assert target.exists() + assert not target.is_symlink() + + +# --- error handling --------------------------------------------------------- + + +def test_export_makedirs_failure_raises( + tmp_path, export_settings, patch_hostname, mocker, loguru_logs +): + export_settings(tmp_path, identifier="hostname") + mocker.patch( + "osism.tasks.conductor.sonic.exporter.os.makedirs", + side_effect=OSError("read-only filesystem"), + ) + device = SimpleNamespace(name="sw-1", serial="ABC123") + + with pytest.raises(OSError, match="read-only filesystem"): + export_config_to_file(device, {"PORT": {}}) + + assert _has_log(loguru_logs, "ERROR", "Failed to export config") diff --git a/tests/unit/tasks/conductor/sonic/test_sync.py b/tests/unit/tasks/conductor/sonic/test_sync.py new file mode 100644 index 00000000..1b13f626 --- /dev/null +++ b/tests/unit/tasks/conductor/sonic/test_sync.py @@ -0,0 +1,581 @@ +# SPDX-License-Identifier: Apache-2.0 + +"""Unit tests for ``osism.tasks.conductor.sonic.sync.sync_sonic``. + +``sync_sonic`` is the orchestration entry point: it manages the cache +lifecycle, resolves the device set (single-device or NetBox-filtered), +computes spine/superspine AS mappings, and drives config generation, NetBox +persistence, and file export per device. Every heavy collaborator is stubbed +at its import site in ``sync`` so only the orchestration glue is exercised +here — ``find_interconnected_devices`` and ``generate_sonic_config`` have their +own coverage (see #2199) and are intentionally mocked. + +``utils.nb`` is supplied by the shared ``mock_nb`` fixture (it patches +``osism.utils.nb``, which ``sync.utils.nb`` resolves to). +""" + +from types import SimpleNamespace +from unittest.mock import call + +import pytest + +from osism.tasks.conductor.sonic.sync import sync_sonic + + +def _has_log(records, level, substring): + return any(r["level"] == level and substring in r["message"] for r in records) + + +def make_device( + name="sw-1", + device_id=1, + role_slug="leaf", + hwsku="Accton-AS7326-56X", + config_version=None, +): + """Build a NetBox-shaped device the orchestrator can consume. + + ``role_slug=None`` yields ``role=None``; ``hwsku=None`` yields an empty + ``sonic_parameters`` so the HWSKU-missing branch is reachable. + """ + params = {} + if hwsku is not None: + params["hwsku"] = hwsku + if config_version is not None: + params["config_version"] = config_version + role = SimpleNamespace(slug=role_slug) if role_slug is not None else None + return SimpleNamespace( + id=device_id, + name=name, + serial="ABC", + role=role, + local_context_data=None, + custom_fields={"sonic_parameters": params}, + config_context={}, + ) + + +@pytest.fixture +def patch_sync_deps(mocker): + """Stub every heavy collaborator ``sync_sonic`` imports. + + Returns a ``SimpleNamespace`` of the mocks so tests can tune individual + return values / side effects. Each name is patched at its ``sync`` import + site, not at its source module, so the rebound reference is the one that + gets replaced. + """ + + def patch(name, **kw): + return mocker.patch(f"osism.tasks.conductor.sonic.sync.{name}", **kw) + + return SimpleNamespace( + get_nb_device_query_list_sonic=patch( + "get_nb_device_query_list_sonic", return_value=[] + ), + find_interconnected_devices=patch( + "find_interconnected_devices", return_value=[] + ), + calculate_minimum_as_for_group=patch( + "calculate_minimum_as_for_group", return_value=None + ), + generate_sonic_config=patch( + "generate_sonic_config", return_value={"PORT": {"Ethernet0": {}}} + ), + save_config_to_netbox=patch("save_config_to_netbox", return_value=(True, None)), + export_config_to_file=patch("export_config_to_file", return_value=False), + clear_interface_cache=patch("clear_interface_cache"), + clear_all_caches=patch("clear_all_caches"), + clear_vip_addresses_cache=patch("clear_vip_addresses_cache"), + _load_metalbox_devices_cache=patch("_load_metalbox_devices_cache"), + load_vip_addresses_cache=patch("load_vip_addresses_cache"), + get_interface_cache_stats=patch("get_interface_cache_stats", return_value={}), + push_task_output=patch("utils.push_task_output"), + finish_task_output=patch("utils.finish_task_output"), + ) + + +# --------------------------------------------------------------------------- +# Cache lifecycle +# --------------------------------------------------------------------------- + + +def test_cache_lifecycle_clears_in_order(mock_nb, patch_sync_deps, mocker): + """Caches are cleared at start (interface, all) and end (interface, all, + vip), with the two cache loads in between — pinned as an exact sequence.""" + deps = patch_sync_deps + manager = mocker.Mock() + manager.attach_mock(deps.clear_interface_cache, "clear_interface_cache") + manager.attach_mock(deps.clear_all_caches, "clear_all_caches") + manager.attach_mock(deps.clear_vip_addresses_cache, "clear_vip_addresses_cache") + manager.attach_mock(deps._load_metalbox_devices_cache, "load_metalbox") + manager.attach_mock(deps.load_vip_addresses_cache, "load_vip") + + sync_sonic() + + assert manager.mock_calls == [ + call.clear_interface_cache(), + call.clear_all_caches(), + call.load_metalbox(), + call.load_vip(), + call.clear_interface_cache(), + call.clear_all_caches(), + call.clear_vip_addresses_cache(), + ] + + +def test_caches_loaded_once_per_invocation(mock_nb, patch_sync_deps): + deps = patch_sync_deps + + sync_sonic() + + deps._load_metalbox_devices_cache.assert_called_once_with() + deps.load_vip_addresses_cache.assert_called_once_with() + + +# --------------------------------------------------------------------------- +# Single-device path +# --------------------------------------------------------------------------- + + +def test_single_device_allowed_role_is_processed(mock_nb, patch_sync_deps): + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {"sw-1": {"PORT": {"Ethernet0": {}}}} + + +def test_single_device_disallowed_role_returns_empty( + mock_nb, patch_sync_deps, loguru_logs +): + device = make_device(name="sw-1", role_slug="router") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + patch_sync_deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "WARNING", "not in allowed SONiC roles") + + +def test_single_device_role_none_returns_empty(mock_nb, patch_sync_deps, loguru_logs): + device = make_device(name="sw-1", role_slug=None) + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + assert _has_log(loguru_logs, "WARNING", "role 'None'") + + +def test_single_device_not_found_returns_empty(mock_nb, patch_sync_deps, loguru_logs): + mock_nb.dcim.devices.get.return_value = None + + result = sync_sonic(device_name="missing") + + assert result == {} + assert _has_log(loguru_logs, "ERROR", "not found in NetBox") + + +def test_single_device_lookup_raises_returns_empty( + mock_nb, patch_sync_deps, loguru_logs +): + mock_nb.dcim.devices.get.side_effect = RuntimeError("netbox down") + + result = sync_sonic(device_name="sw-1") + + assert result == {} + assert _has_log(loguru_logs, "ERROR", "Error fetching device") + + +def test_single_spine_fetches_all_spine_devices(mock_nb, patch_sync_deps): + """A single spine triggers a full spine/superspine fetch for group + detection, so ``nb.dcim.devices.filter`` runs via the query list.""" + deps = patch_sync_deps + device = make_device(name="spine-1", role_slug="spine") + mock_nb.dcim.devices.get.return_value = device + deps.get_nb_device_query_list_sonic.return_value = [{"status": "active"}] + mock_nb.dcim.devices.filter.return_value = [device] + + sync_sonic(device_name="spine-1") + + mock_nb.dcim.devices.filter.assert_called_once_with(status="active") + deps.find_interconnected_devices.assert_called_once_with( + [device], ["spine", "superspine"] + ) + + +def test_single_leaf_uses_device_list_without_extra_fetch(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="leaf-1") + + mock_nb.dcim.devices.filter.assert_not_called() + deps.find_interconnected_devices.assert_called_once_with( + [device], ["spine", "superspine"] + ) + + +# --------------------------------------------------------------------------- +# Multi-device path +# --------------------------------------------------------------------------- + + +def test_multi_device_keeps_only_allowed_roles(mock_nb, patch_sync_deps): + deps = patch_sync_deps + leaf = make_device(name="leaf-1", device_id=1, role_slug="leaf") + router = make_device(name="router-1", device_id=2, role_slug="router") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [leaf, router] + + result = sync_sonic() + + assert "leaf-1" in result + assert "router-1" not in result + + +def test_multi_device_skips_devices_without_role(mock_nb, patch_sync_deps): + deps = patch_sync_deps + leaf = make_device(name="leaf-1", device_id=1, role_slug="leaf") + norole = make_device(name="x-1", device_id=2, role_slug=None) + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [norole, leaf] + + result = sync_sonic() + + assert set(result) == {"leaf-1"} + + +# --------------------------------------------------------------------------- +# Per-device processing +# --------------------------------------------------------------------------- + + +def test_hwsku_missing_skips_device(mock_nb, patch_sync_deps, loguru_logs): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", hwsku=None) + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "DEBUG", "no HWSKU configured") + + +def test_hwsku_unsupported_skips_device(mock_nb, patch_sync_deps, loguru_logs): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", hwsku="Bogus-HWSKU") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + assert result == {} + deps.generate_sonic_config.assert_not_called() + assert _has_log(loguru_logs, "WARNING", "unsupported HWSKU") + + +def test_hwsku_valid_generates_and_stores_config(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + result = sync_sonic(device_name="sw-1") + + deps.generate_sonic_config.assert_called_once_with( + device, "Accton-AS7326-56X", {}, None + ) + assert result == {"sw-1": {"PORT": {"Ethernet0": {}}}} + + +def test_config_version_read_from_custom_fields(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf", config_version="4_2_0") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1") + + assert deps.generate_sonic_config.call_args.args[3] == "4_2_0" + + +# --------------------------------------------------------------------------- +# AS mapping +# --------------------------------------------------------------------------- + + +def test_as_mapping_calculated_per_spine_group(mock_nb, patch_sync_deps): + deps = patch_sync_deps + g1 = make_device(name="spine-1", device_id=1, role_slug="spine") + g2 = make_device(name="spine-2", device_id=2, role_slug="spine") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [g1, g2] + deps.find_interconnected_devices.return_value = [[g1], [g2]] + deps.calculate_minimum_as_for_group.side_effect = [4200000001, 4200000002] + + sync_sonic() + + assert deps.calculate_minimum_as_for_group.call_count == 2 + # The full mapping is handed to every per-device generation call. + assert deps.generate_sonic_config.call_args_list[0].args[2] == { + 1: 4200000001, + 2: 4200000002, + } + + +def test_as_mapping_passed_into_generate(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", device_id=7, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [device] + deps.find_interconnected_devices.return_value = [[device]] + deps.calculate_minimum_as_for_group.return_value = 4200000001 + + sync_sonic() + + assert deps.generate_sonic_config.call_args.args == ( + device, + "Accton-AS7326-56X", + {7: 4200000001}, + None, + ) + + +def test_group_with_none_min_as_not_propagated(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="leaf-1", device_id=7, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [device] + deps.find_interconnected_devices.return_value = [[device]] + deps.calculate_minimum_as_for_group.return_value = None + + sync_sonic() + + assert deps.generate_sonic_config.call_args.args[2] == {} + + +# --------------------------------------------------------------------------- +# Diff handling +# --------------------------------------------------------------------------- + + +def test_diff_streamed_to_task_output(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (True, "the-diff") + + sync_sonic(device_name="sw-1", task_id="t") + + deps.save_config_to_netbox.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}}, return_diff=True + ) + assert call("t", "the-diff\n") in deps.push_task_output.call_args_list + + +def test_first_time_configuration_message(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (True, None) + + sync_sonic(device_name="sw-1", task_id="t") + + assert ( + call("t", "First-time configuration created for sw-1\n") + in deps.push_task_output.call_args_list + ) + + +def test_no_change_skips_diff_output_but_still_exports(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = (False, None) + + sync_sonic(device_name="sw-1", task_id="t") + + deps.export_config_to_file.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}} + ) + # Only the "Processing device" line is pushed — no diff / first-time lines. + assert deps.push_task_output.call_args_list == [ + call("t", "Processing device: sw-1\n") + ] + + +def test_show_diff_false_calls_save_without_return_diff(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.return_value = True + + sync_sonic(device_name="sw-1", show_diff=False) + + deps.save_config_to_netbox.assert_called_once_with( + device, {"PORT": {"Ethernet0": {}}} + ) + + +# --------------------------------------------------------------------------- +# Task output +# --------------------------------------------------------------------------- + + +def test_task_id_pushes_per_device_and_finishes(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1", task_id="t") + + assert ( + call("t", "Processing device: sw-1\n") in deps.push_task_output.call_args_list + ) + deps.finish_task_output.assert_called_once_with("t", rc=0) + + +def test_no_task_id_suppresses_task_output(mock_nb, patch_sync_deps): + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + + sync_sonic(device_name="sw-1") + + deps.push_task_output.assert_not_called() + deps.finish_task_output.assert_not_called() + + +# --------------------------------------------------------------------------- +# Failure handling and cleanup +# --------------------------------------------------------------------------- + + +def _assert_caches_cleaned(deps): + """The end-of-run cleanup must have run: interface and generator caches + are cleared a second time (after the initial clear) and the VIP cache once.""" + assert deps.clear_interface_cache.call_count == 2 + assert deps.clear_all_caches.call_count == 2 + deps.clear_vip_addresses_cache.assert_called_once_with() + + +@pytest.mark.parametrize("path", ["disallowed_role", "not_found", "lookup_raises"]) +def test_early_return_cleans_caches_and_reports_failure(mock_nb, patch_sync_deps, path): + """Single-device early returns happen after the module-level caches are + loaded — cleanup and task finalization must still run, with rc=1.""" + deps = patch_sync_deps + if path == "disallowed_role": + mock_nb.dcim.devices.get.return_value = make_device( + name="sw-1", role_slug="router" + ) + elif path == "not_found": + mock_nb.dcim.devices.get.return_value = None + else: + mock_nb.dcim.devices.get.side_effect = RuntimeError("netbox down") + + result = sync_sonic(device_name="sw-1", task_id="t") + + assert result == {} + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + + +def test_mid_loop_exception_continues_and_reports_failure( + mock_nb, patch_sync_deps, loguru_logs +): + """A device failing mid-loop must not abort the remaining devices, must + not leak the module-level caches, and must surface as rc=1.""" + deps = patch_sync_deps + bad = make_device(name="bad-1", device_id=1, role_slug="leaf") + good = make_device(name="good-1", device_id=2, role_slug="leaf") + deps.get_nb_device_query_list_sonic.return_value = [{}] + mock_nb.dcim.devices.filter.return_value = [bad, good] + deps.generate_sonic_config.side_effect = [ + RuntimeError("generation failed"), + {"PORT": {"Ethernet0": {}}}, + ] + + result = sync_sonic(task_id="t") + + assert result == {"good-1": {"PORT": {"Ethernet0": {}}}} + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device bad-1" + ) + + +def test_netbox_save_failure_reports_nonzero_rc(mock_nb, patch_sync_deps, loguru_logs): + """A raising ``save_config_to_netbox`` must not finish the task as rc=0 + — a failed write is not "no changes".""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.save_config_to_netbox.side_effect = RuntimeError("netbox write failed") + + sync_sonic(device_name="sw-1", task_id="t") + + deps.export_config_to_file.assert_not_called() + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device sw-1" + ) + + +def test_file_export_failure_reports_nonzero_rc(mock_nb, patch_sync_deps, loguru_logs): + """A raising ``export_config_to_file`` surfaces as rc=1 as well.""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.export_config_to_file.side_effect = OSError("disk full") + + sync_sonic(device_name="sw-1", task_id="t") + + _assert_caches_cleaned(deps) + deps.finish_task_output.assert_called_once_with("t", rc=1) + assert _has_log( + loguru_logs, "ERROR", "Failed to sync SONiC configuration for device sw-1" + ) + + +def test_config_without_port_section_is_handled(mock_nb, patch_sync_deps, loguru_logs): + """A config lacking the PORT section must not raise in the summary log.""" + deps = patch_sync_deps + device = make_device(name="sw-1", role_slug="leaf") + mock_nb.dcim.devices.get.return_value = device + deps.generate_sonic_config.return_value = {"VLAN": {}} + + result = sync_sonic(device_name="sw-1", task_id="t") + + assert result == {"sw-1": {"VLAN": {}}} + deps.finish_task_output.assert_called_once_with("t", rc=0) + assert _has_log(loguru_logs, "INFO", "with 0 ports") + + +# --------------------------------------------------------------------------- +# Cache stats +# --------------------------------------------------------------------------- + + +def test_cache_stats_logged_when_present(mock_nb, patch_sync_deps, loguru_logs): + patch_sync_deps.get_interface_cache_stats.return_value = { + "cached_devices": 2, + "total_interfaces": 10, + } + + sync_sonic() + + assert _has_log( + loguru_logs, "DEBUG", "Interface cache stats: 2 devices, 10 interfaces" + ) + + +def test_cache_stats_not_logged_when_empty(mock_nb, patch_sync_deps, loguru_logs): + patch_sync_deps.get_interface_cache_stats.return_value = {} + + sync_sonic() + + assert not _has_log(loguru_logs, "DEBUG", "Interface cache stats")