diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index 8d26c038..2786a1eb 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -37,7 +37,6 @@ defmodule Towerops.Snmp.Discovery do alias Towerops.Snmp.Storage alias Towerops.Snmp.Transceiver alias Towerops.Snmp.Vlan - # alias Towerops.Topology # Commented out - auto-inference disabled alias Towerops.Workers.DiscoveryWorker require Logger @@ -100,37 +99,40 @@ defmodule Towerops.Snmp.Discovery do """ @spec discover_device(DeviceSchema.t()) :: {:ok, Device.t()} | {:error, term()} + def discover_device(%DeviceSchema{snmp_enabled: false}) do + {:error, :snmp_not_enabled} + end + def discover_device(%DeviceSchema{} = device) do - if device.snmp_enabled do - Logger.info( - "Starting SNMP discovery: #{device.name} (#{device.ip_address})", - device_id: device.id, - device_name: device.name, - device_ip: device.ip_address - ) + Logger.info( + "Starting SNMP discovery: #{device.name} (#{device.ip_address})", + device_id: device.id, + device_name: device.name, + device_ip: device.ip_address + ) - client_opts = build_client_opts(device) + client_opts = build_client_opts(device) - # Categorize device speed for adaptive timeouts - Logger.info("Categorizing device speed...", device_id: device.id) - device_speed = DeferredDiscovery.categorize_device_speed(client_opts) - timeouts = DeferredDiscovery.timeouts_for_speed(device_speed) + Logger.info("Categorizing device speed...", device_id: device.id) + device_speed = DeferredDiscovery.categorize_device_speed(client_opts) + timeouts = DeferredDiscovery.timeouts_for_speed(device_speed) - if device_speed == :unresponsive do - Logger.warning("Device #{device.name} is unresponsive, aborting discovery") - {:error, :device_unresponsive} - else - Logger.info( - "Device categorized as #{device_speed}, proceeding with discovery", - device_id: device.id, - device_speed: device_speed - ) + discover_with_speed(device, client_opts, device_speed, timeouts) + end - do_discover_device(device, client_opts, timeouts) - end - else - {:error, :snmp_not_enabled} - end + defp discover_with_speed(device, _client_opts, :unresponsive, _timeouts) do + Logger.warning("Device #{device.name} is unresponsive, aborting discovery") + {:error, :device_unresponsive} + end + + defp discover_with_speed(device, client_opts, device_speed, timeouts) do + Logger.info( + "Device categorized as #{device_speed}, proceeding with discovery", + device_id: device.id, + device_speed: device_speed + ) + + do_discover_device(device, client_opts, timeouts) end @doc """ @@ -150,50 +152,49 @@ defmodule Towerops.Snmp.Discovery do """ @spec discover_device_with_opts(DeviceSchema.t(), keyword()) :: {:ok, Device.t()} | {:error, term()} + def discover_device_with_opts(%DeviceSchema{snmp_enabled: false}, _client_opts) do + {:error, :snmp_not_enabled} + end + def discover_device_with_opts(%DeviceSchema{} = device, client_opts) do - if device.snmp_enabled do - Logger.debug("Starting discovery with custom opts for: #{device.name}") + Logger.debug("Starting discovery with custom opts for: #{device.name}") + {device_speed, timeouts} = categorize_speed(client_opts) + discover_with_speed(device, client_opts, device_speed, timeouts) + end - # Skip speed categorization for Replay adapter (agent-based discovery) - # Agent data is pre-collected, so network checks are not needed - adapter = Keyword.get(client_opts, :adapter) + defp categorize_speed(client_opts) do + categorize_speed_for_adapter(client_opts, Keyword.get(client_opts, :adapter)) + end - {device_speed, timeouts} = - if adapter == Replay do - Logger.debug("Using Replay adapter, skipping speed categorization") - {:fast, DeferredDiscovery.timeouts_for_speed(:fast)} - else - # Categorize device speed for adaptive timeouts (real SNMP) - speed = DeferredDiscovery.categorize_device_speed(client_opts) - {speed, DeferredDiscovery.timeouts_for_speed(speed)} - end + defp categorize_speed_for_adapter(_client_opts, Replay) do + Logger.debug("Using Replay adapter, skipping speed categorization") + {:fast, DeferredDiscovery.timeouts_for_speed(:fast)} + end - if device_speed == :unresponsive do - Logger.warning("Device #{device.name} is unresponsive, aborting discovery") - {:error, :device_unresponsive} - else - do_discover_device(device, client_opts, timeouts) - end - else - {:error, :snmp_not_enabled} - end + defp categorize_speed_for_adapter(client_opts, _adapter) do + speed = DeferredDiscovery.categorize_device_speed(client_opts) + {speed, DeferredDiscovery.timeouts_for_speed(speed)} + end + + # Connection test with pattern matching on Replay adapter + defp test_connection(client_opts, device_id) do + test_connection_for_adapter(client_opts, device_id, Keyword.get(client_opts, :adapter)) + end + + defp test_connection_for_adapter(_client_opts, device_id, Replay) do + Logger.debug("Using Replay adapter, skipping connection test", device_id: device_id) + {:ok, :replay} + end + + defp test_connection_for_adapter(client_opts, device_id, _adapter) do + Logger.info("Testing SNMP connection...", device_id: device_id) + Client.test_connection(client_opts) end # Internal discovery with adaptive timeouts based on device speed defp do_discover_device(device, client_opts, timeouts) do - adapter = Keyword.get(client_opts, :adapter) + connection_result = test_connection(client_opts, device.id) - # Skip connection test for Replay adapter (agent-provided data) - connection_result = - if adapter == Replay do - Logger.debug("Using Replay adapter, skipping connection test", device_id: device.id) - {:ok, :replay} - else - Logger.info("Testing SNMP connection...", device_id: device.id) - Client.test_connection(client_opts) - end - - # Critical path: connection, system info, device info must succeed with {:ok, _} <- connection_result, Logger.info("Discovering system info...", device_id: device.id), {:ok, system_info} <- discover_system(client_opts), @@ -205,91 +206,45 @@ defmodule Towerops.Snmp.Discovery do {:ok, device_info} <- build_device_info(client_opts, system_info, profile), {:ok, interfaces} <- discover_interfaces_with_timeout_safe(client_opts, profile, timeouts, device.id), {:ok, sensors} <- discover_sensors_with_timeout_safe(client_opts, profile, timeouts, device.id) do - # Secondary discovery: VLANs, IPs, processors, storage (already fall back to [] on timeout) Logger.info("Discovering VLANs...", device_id: device.id) - {:ok, vlans} = discover_vlans_with_timeout(client_opts, profile, timeouts) + {:ok, vlans} = with_timeout(client_opts, fn -> discover_vlans(client_opts, profile) end, timeouts) Logger.info("Discovering IP addresses...", device_id: device.id) - {:ok, ip_addresses} = discover_ip_addresses_with_timeout(client_opts, timeouts) + {:ok, ip_addresses} = with_timeout(client_opts, fn -> Base.discover_all_ip_addresses(client_opts) end, timeouts) _ = log_ip_discovery_results(device, ip_addresses) Logger.info("Discovering processors...", device_id: device.id) - {:ok, processors} = discover_processors_with_timeout(client_opts, timeouts) + {:ok, processors} = with_timeout(client_opts, fn -> Base.discover_processors(client_opts) end, timeouts) Logger.info("Discovering storage...", device_id: device.id) - {:ok, storage} = discover_storage_with_timeout(client_opts, timeouts) + {:ok, storage} = with_timeout(client_opts, fn -> Base.discover_storage(client_opts) end, timeouts) Logger.info("Discovering memory pools...", device_id: device.id) - {:ok, mempools} = discover_mempools_with_timeout(client_opts, timeouts) + {:ok, mempools} = with_timeout(client_opts, fn -> Base.discover_memory_pools(client_opts) end, timeouts) Logger.info("Discovering entity physical inventory...", device_id: device.id) - {:ok, entity_physical} = discover_entity_physical_with_timeout(client_opts, timeouts) + {:ok, entity_physical} = with_timeout(client_opts, fn -> Base.discover_entity_physical(client_opts) end, timeouts) Logger.info("Discovering transceivers...", device_id: device.id) - {:ok, transceivers} = discover_transceivers_with_timeout(client_opts, timeouts) + {:ok, transceivers} = with_timeout(client_opts, fn -> Base.discover_transceivers(client_opts) end, timeouts) Logger.info("Discovering printer supplies...", device_id: device.id) - {:ok, printer_supplies} = discover_printer_supplies_with_timeout(client_opts, timeouts) + {:ok, printer_supplies} = + with_timeout(client_opts, fn -> Base.discover_printer_supplies(client_opts) end, timeouts) - Logger.info("Saving discovery results (including IP addresses and processors)...", device_id: device.id) + Logger.info("Saving discovery results...", device_id: device.id) - case save_discovery_results(device, device_info, interfaces, sensors, vlans, ip_addresses, processors) do - {:ok, discovered_device} -> - # Post-save discovery: neighbors, ARP (best-effort) - Logger.info("Syncing storage...", device_id: device.id) - _ = sync_storage(discovered_device, storage) + save_result = save_discovery_results(device, device_info, interfaces, sensors, vlans, ip_addresses, processors) - Logger.info("Syncing memory pools...", device_id: device.id) - _ = sync_mempools(discovered_device, mempools) + post_data = %{ + storage: storage, + mempools: mempools, + entity_physical: entity_physical, + transceivers: transceivers, + printer_supplies: printer_supplies + } - Logger.info("Syncing entity physical inventory...", device_id: device.id) - _ = sync_entity_physical(discovered_device, entity_physical) - - Logger.info("Syncing transceivers...", device_id: device.id) - _ = sync_transceivers(discovered_device, transceivers) - - Logger.info("Syncing printer supplies...", device_id: device.id) - _ = sync_printer_supplies(discovered_device, printer_supplies) - - Logger.info("Discovering neighbors...", device_id: device.id) - {:ok, neighbors} = discover_neighbors_with_timeout(client_opts, discovered_device.interfaces, timeouts) - - Logger.info("Saving neighbors...", device_id: device.id) - _ = save_neighbors(discovered_device.device_id, neighbors) - - Logger.info("Discovering ARP entries...", device_id: device.id) - {:ok, arp_entries} = discover_arp_with_timeout(client_opts, timeouts) - _ = save_arp_entries(discovered_device.device_id, arp_entries, discovered_device.interfaces) - - _ = update_device_discovery_time(device) - Logger.debug("SNMP discovery completed successfully for: #{device.name}") - - is_rediscovery = device.last_discovery_at != nil - _ = log_discovery_event(device, discovered_device, is_rediscovery) - - # Create monitoring checks for discovered entities - Logger.info("Creating monitoring checks from discovered entities...", device_id: device.id) - - {:ok, check_counts} = Towerops.Snmp.create_checks_from_discovery(device, discovered_device) - log_check_creation_results(device.id, check_counts) - - # Auto-inference disabled - device type is now manual-only - # Logger.info("Inferring device role...", device_id: device.id) - # _ = Topology.maybe_update_device_role(device) - - _ = - Phoenix.PubSub.broadcast( - Towerops.PubSub, - "device:#{device.id}", - {:discovery_completed, device.id} - ) - - {:ok, discovered_device} - - {:error, reason} -> - Logger.error("Failed to save discovery results for #{device.name}: #{inspect(reason)}") - {:error, reason} - end + complete_discovery(device, client_opts, timeouts, save_result, post_data) else {:error, reason} = error -> Logger.error("SNMP discovery failed for #{device.name}: #{inspect(reason)}") @@ -297,40 +252,74 @@ defmodule Towerops.Snmp.Discovery do end end - # Interface discovery with timeout - returns error on timeout to preserve existing data - defp discover_interfaces_with_timeout(client_opts, profile, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> discover_interfaces(client_opts, profile) end, - timeout: timeouts[:slow], - default: {:error, :interface_discovery_timeout} - ) + defp complete_discovery(_device, _client_opts, _timeouts, {:error, _} = error, _post_data) do + error end - # Sensor discovery with timeout - returns error on timeout to preserve existing data - defp discover_sensors_with_timeout(client_opts, profile, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> discover_sensors(client_opts, profile) end, - timeout: timeouts[:slow], - default: {:error, :sensor_discovery_timeout} - ) + defp complete_discovery(device, client_opts, timeouts, {:ok, discovered_device}, post_data) do + _ = sync_storage(discovered_device, post_data.storage) + _ = sync_mempools(discovered_device, post_data.mempools) + _ = sync_entity_physical(discovered_device, post_data.entity_physical) + _ = sync_transceivers(discovered_device, post_data.transceivers) + _ = sync_printer_supplies(discovered_device, post_data.printer_supplies) + + Logger.info("Discovering neighbors...", device_id: device.id) + + neighbors_fn = fn -> NeighborDiscovery.discover_neighbors(client_opts, discovered_device.interfaces) end + + {:ok, neighbors} = with_deferred_timeout(client_opts, neighbors_fn, timeouts) + + _ = save_neighbors(discovered_device.device_id, neighbors) + + Logger.info("Discovering ARP entries...", device_id: device.id) + + arp_fn = fn -> ArpDiscovery.discover_arp_table(client_opts) end + + {:ok, arp_entries} = with_deferred_timeout(client_opts, arp_fn, timeouts) + _ = save_arp_entries(discovered_device.device_id, arp_entries, discovered_device.interfaces) + + _ = update_device_discovery_time(device) + Logger.debug("SNMP discovery completed successfully for: #{device.name}") + + is_rediscovery = device.last_discovery_at != nil + _ = log_discovery_event(device, discovered_device, is_rediscovery) + + Logger.info("Creating monitoring checks from discovered entities...", device_id: device.id) + {:ok, check_counts} = Towerops.Snmp.create_checks_from_discovery(device, discovered_device) + log_check_creation_results(device.id, check_counts) + + _ = + Phoenix.PubSub.broadcast( + Towerops.PubSub, + "device:#{device.id}", + {:discovery_completed, device.id} + ) + + {:ok, discovered_device} end + # Generic timeout wrapper for slow discovery operations (falls back to [] on timeout) + defp with_timeout(client_opts, discovery_fn, timeouts), + do: DeferredDiscovery.slow_check(client_opts, discovery_fn, timeout: timeouts[:slow], default: []) + + # Timeout wrapper for deferred discovery operations (neighbors, ARP) + defp with_deferred_timeout(client_opts, discovery_fn, timeouts), + do: DeferredDiscovery.slow_check(client_opts, discovery_fn, timeout: timeouts[:deferred], default: []) + # Safe wrapper for interface discovery - aborts discovery on timeout to prevent data loss defp discover_interfaces_with_timeout_safe(client_opts, profile, timeouts, device_id) do Logger.info("Discovering interfaces...", device_id: device_id) - case discover_interfaces_with_timeout(client_opts, profile, timeouts) do - {:ok, ifaces} -> - {:ok, ifaces} + case DeferredDiscovery.slow_check( + client_opts, + fn -> discover_interfaces(client_opts, profile) end, + timeout: timeouts[:slow], + default: {:error, :interface_discovery_timeout} + ) do + {:ok, _ifaces} = ok -> ok {:error, :interface_discovery_timeout} -> - Logger.error( - "Interface discovery timed out - aborting to prevent data loss", - device_id: device_id - ) - + Logger.error("Interface discovery timed out - aborting to prevent data loss", device_id: device_id) {:error, :interface_discovery_timeout} end end @@ -339,116 +328,20 @@ defmodule Towerops.Snmp.Discovery do defp discover_sensors_with_timeout_safe(client_opts, profile, timeouts, device_id) do Logger.info("Discovering sensors...", device_id: device_id) - case discover_sensors_with_timeout(client_opts, profile, timeouts) do - {:ok, sensors} -> - {:ok, sensors} + case DeferredDiscovery.slow_check( + client_opts, + fn -> discover_sensors(client_opts, profile) end, + timeout: timeouts[:slow], + default: {:error, :sensor_discovery_timeout} + ) do + {:ok, _sensors} = ok -> ok {:error, :sensor_discovery_timeout} -> - Logger.error( - "Sensor discovery timed out - aborting to prevent data loss", - device_id: device_id - ) - + Logger.error("Sensor discovery timed out - aborting to prevent data loss", device_id: device_id) {:error, :sensor_discovery_timeout} end end - # Neighbor discovery with timeout - falls back to empty list on timeout - defp discover_neighbors_with_timeout(client_opts, interfaces, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> NeighborDiscovery.discover_neighbors(client_opts, interfaces) end, - timeout: timeouts[:deferred], - default: [] - ) - end - - # ARP table discovery with timeout - falls back to empty list on timeout - defp discover_arp_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> ArpDiscovery.discover_arp_table(client_opts) end, - timeout: timeouts[:deferred], - default: [] - ) - end - - # VLAN discovery with timeout - falls back to empty list on timeout - defp discover_vlans_with_timeout(client_opts, profile, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> discover_vlans(client_opts, profile) end, - timeout: timeouts[:slow], - default: [] - ) - end - - # IP address discovery with timeout - falls back to empty list on timeout - defp discover_ip_addresses_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_all_ip_addresses(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - # Processor discovery with timeout - falls back to empty list on timeout - defp discover_processors_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_processors(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - # Storage discovery with timeout - falls back to empty list on timeout - defp discover_storage_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_storage(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - defp discover_mempools_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_memory_pools(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - defp discover_entity_physical_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_entity_physical(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - defp discover_transceivers_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_transceivers(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - - defp discover_printer_supplies_with_timeout(client_opts, timeouts) do - DeferredDiscovery.slow_check( - client_opts, - fn -> Base.discover_printer_supplies(client_opts) end, - timeout: timeouts[:slow], - default: [] - ) - end - @doc """ Enqueues Oban discovery jobs for all SNMP-enabled devices in an organization. @@ -652,18 +545,21 @@ defmodule Towerops.Snmp.Discovery do device_id: device_id ) - if check_counts.errors != [] do - Logger.warning( - "Failed to create #{length(check_counts.errors)} checks for device #{device_id}", - device_id: device_id, - errors: check_counts.errors - ) - end + log_check_errors(device_id, check_counts.errors) + end + + defp log_check_errors(_device_id, []), do: :ok + + defp log_check_errors(device_id, errors) do + Logger.warning( + "Failed to create #{length(errors)} checks for device #{device_id}", + device_id: device_id, + errors: errors + ) end @spec build_client_opts(DeviceSchema.t()) :: Client.connection_opts() defp build_client_opts(device) do - # Get SNMP config with hierarchical fallback (device -> site -> organization) snmp_config = Devices.get_snmp_config(device) base_opts = [ @@ -672,22 +568,25 @@ defmodule Towerops.Snmp.Discovery do port: device.snmp_port || 161 ] - # Add version-specific credentials - if snmp_config.version == "3" do - v3_config = Devices.get_snmpv3_config(device) + add_credentials(base_opts, snmp_config, device) + end - base_opts ++ - [ - security_name: v3_config.username, - security_level: v3_config.security_level, - auth_protocol: v3_config.auth_protocol, - auth_password: v3_config.auth_password, - priv_protocol: v3_config.priv_protocol, - priv_password: v3_config.priv_password - ] - else - base_opts ++ [community: snmp_config.community] - end + defp add_credentials(base_opts, %{version: "3"} = _snmp_config, device) do + v3_config = Devices.get_snmpv3_config(device) + + base_opts ++ + [ + security_name: v3_config.username, + security_level: v3_config.security_level, + auth_protocol: v3_config.auth_protocol, + auth_password: v3_config.auth_password, + priv_protocol: v3_config.priv_protocol, + priv_password: v3_config.priv_password + ] + end + + defp add_credentials(base_opts, snmp_config, _device) do + base_opts ++ [community: snmp_config.community] end @spec discover_system(Client.connection_opts()) :: {:ok, system_info()} | {:error, term()} @@ -768,37 +667,15 @@ defmodule Towerops.Snmp.Discovery do "build_device_info: system_info has _raw_discovery_data: #{inspect(Map.has_key?(system_info, :_raw_discovery_data))}" ) - # Get profile-specific system info if profile has discover_system_info/1 - enriched_system_info = - if function_exported?(profile, :discover_system_info, 1) do - case profile.discover_system_info(client_opts) do - {:ok, profile_info} -> - Logger.debug("Enriched system_info with profile-specific data") - Map.merge(system_info, profile_info) + has_sys_info = function_exported?(profile, :discover_system_info, 1) - {:error, _reason} -> - Logger.warning("Failed to get profile-specific system info, using base info") - system_info - end - else - system_info - end + enriched_system_info = enrich_with_profile_info(client_opts, system_info, profile, has_sys_info) - # Let the hard-coded profile identify the device identified_info = profile.identify_device(enriched_system_info) - # Collect raw debug data if profile supports it - identified_info_with_debug = - if function_exported?(profile, :collect_raw_debug_data, 1) do - raw_data = profile.collect_raw_debug_data(client_opts) - Logger.debug("Collected raw debug data: #{map_size(raw_data)} keys") + has_collect_debug = function_exported?(profile, :collect_raw_debug_data, 1) - identified_info - |> Map.put(:_raw_discovery_data, raw_data) - |> Map.put(:_last_discovery_at, DateTime.utc_now()) - else - identified_info - end + identified_info_with_debug = maybe_attach_debug_data(client_opts, identified_info, profile, has_collect_debug) Logger.debug( "build_device_info: identified_info has _raw_discovery_data: #{inspect(Map.has_key?(identified_info_with_debug, :_raw_discovery_data))}" @@ -808,10 +685,34 @@ defmodule Towerops.Snmp.Discovery do rescue _error -> Logger.error("Failed to identify device with profile, falling back to Base profile") - # Fall back to Base profile identification to ensure manufacturer/model are set {:ok, Base.identify_device(system_info)} end + defp enrich_with_profile_info(_client_opts, system_info, _profile, false), do: system_info + + defp enrich_with_profile_info(client_opts, system_info, profile, true) do + case profile.discover_system_info(client_opts) do + {:ok, profile_info} -> + Logger.debug("Enriched system_info with profile-specific data") + Map.merge(system_info, profile_info) + + {:error, _reason} -> + Logger.warning("Failed to get profile-specific system info, using base info") + system_info + end + end + + defp maybe_attach_debug_data(_client_opts, identified_info, _profile, false), do: identified_info + + defp maybe_attach_debug_data(client_opts, identified_info, profile, true) do + raw_data = profile.collect_raw_debug_data(client_opts) + Logger.debug("Collected raw debug data: #{map_size(raw_data)} keys") + + identified_info + |> Map.put(:_raw_discovery_data, raw_data) + |> Map.put(:_last_discovery_at, DateTime.utc_now()) + end + @spec discover_interfaces(Client.connection_opts(), profile()) :: {:ok, [interface_data()]} defp discover_interfaces(client_opts, {:yaml, profile}) do @@ -857,16 +758,19 @@ defmodule Towerops.Snmp.Discovery do end defp discover_vlans(client_opts, profile) when is_atom(profile) do - if function_exported?(profile, :discover_vlans, 1) do - {:ok, vlans} = profile.discover_vlans(client_opts) - Logger.debug("Discovered #{length(vlans)} VLANs") - {:ok, vlans} - else - # Fall back to Base profile for VLAN discovery - {:ok, vlans} = Base.discover_vlans(client_opts) - Logger.debug("Discovered #{length(vlans)} VLANs using Base profile") - {:ok, vlans} - end + discover_vlans_for_profile(client_opts, profile, function_exported?(profile, :discover_vlans, 1)) + end + + defp discover_vlans_for_profile(client_opts, profile, true) do + {:ok, vlans} = profile.discover_vlans(client_opts) + Logger.debug("Discovered #{length(vlans)} VLANs") + {:ok, vlans} + end + + defp discover_vlans_for_profile(client_opts, _profile, false) do + {:ok, vlans} = Base.discover_vlans(client_opts) + Logger.debug("Discovered #{length(vlans)} VLANs using Base profile") + {:ok, vlans} end @spec save_discovery_results( @@ -914,17 +818,21 @@ defmodule Towerops.Snmp.Discovery do duration = System.monotonic_time(:millisecond) - start - if duration > 5_000 do - Logger.warning("Discovery transaction took #{duration}ms", - device_id: device.id, - interface_count: length(interfaces), - sensor_count: length(sensors) - ) - end + log_slow_transaction(duration, device.id, interfaces, sensors) result end + defp log_slow_transaction(duration, _device_id, _interfaces, _sensors) when duration <= 5_000, do: :ok + + defp log_slow_transaction(duration, device_id, interfaces, sensors) do + Logger.warning("Discovery transaction took #{duration}ms", + device_id: device_id, + interface_count: length(interfaces), + sensor_count: length(sensors) + ) + end + # Prepares device_info for database save by sanitizing raw_discovery_data # This is done OUTSIDE the transaction to avoid DB timeout on large data defp prepare_device_info_for_save(device_info, device_id) do @@ -952,45 +860,46 @@ defmodule Towerops.Snmp.Discovery do @spec upsert_device(DeviceSchema.t(), map()) :: Device.t() defp upsert_device(device, device_attrs) do Logger.info("Upserting SNMP device for #{device.name}") + do_upsert_device(device_attrs, Repo.get_by(Device, device_id: device.id)) + end - case Repo.get_by(Device, device_id: device.id) do - nil -> - snmp_device = - %Device{} - |> Device.changeset(device_attrs) - |> Repo.insert!() + defp do_upsert_device(device_attrs, nil) do + snmp_device = + %Device{} + |> Device.changeset(device_attrs) + |> Repo.insert!() - # Log initial firmware version (no old version) - if snmp_device.firmware_version do - Firmware.log_firmware_change( - snmp_device.id, - nil, - snmp_device.firmware_version - ) - end + log_initial_firmware(snmp_device) + snmp_device + end - snmp_device + defp do_upsert_device(device_attrs, existing_device) do + old_version = existing_device.firmware_version + new_version = device_attrs[:firmware_version] - existing_device -> - old_version = existing_device.firmware_version - new_version = device_attrs[:firmware_version] + snmp_device = + existing_device + |> Device.changeset(device_attrs) + |> Repo.update!() - snmp_device = - existing_device - |> Device.changeset(device_attrs) - |> Repo.update!() + log_firmware_if_changed(snmp_device, old_version, new_version) + snmp_device + end - # Detect and log firmware version changes - if version_changed?(old_version, new_version) do - Firmware.log_firmware_change( - snmp_device.id, - old_version, - new_version - ) - end + defp log_initial_firmware(%{firmware_version: fw} = snmp_device) when not is_nil(fw) do + Firmware.log_firmware_change(snmp_device.id, nil, fw) + end - snmp_device - end + defp log_initial_firmware(_snmp_device), do: :ok + + defp log_firmware_if_changed(snmp_device, old_version, new_version) do + log_firmware_when_changed(snmp_device, old_version, new_version, version_changed?(old_version, new_version)) + end + + defp log_firmware_when_changed(_snmp_device, _old, _new, false), do: :ok + + defp log_firmware_when_changed(snmp_device, old, new, true) do + Firmware.log_firmware_change(snmp_device.id, old, new) end defp version_changed?(nil, new_version) when is_binary(new_version), do: false @@ -1187,41 +1096,45 @@ defmodule Towerops.Snmp.Discovery do Logger.debug("Deleted #{MapSet.size(removed_keys)} removed IP addresses") end - # Upsert each discovered IP address Enum.each(discovered_ip_addresses, fn ip_data -> upsert_ip_address(ip_data, if_index_to_interface, existing_ip_addresses) end) - if Application.get_env(:towerops, :env) == :dev do - ipv4_count = Enum.count(discovered_ip_addresses, &(&1.ip_type == "ipv4")) - ipv6_count = Enum.count(discovered_ip_addresses, &(&1.ip_type == "ipv6")) - removed_count = MapSet.size(removed_keys) - - Logger.info( - "Synced IP addresses: #{ipv4_count} IPv4, #{ipv6_count} IPv6, removed #{removed_count}", - device_id: device.device_id - ) - else - Logger.debug("Synced #{length(discovered_ip_addresses)} IP addresses for device") - end + log_sync_ip_summary(device, discovered_ip_addresses, removed_keys, Application.get_env(:towerops, :env) == :dev) :ok end + defp log_sync_ip_summary(_device, _discovered, _removed_keys, false) do + Logger.debug("Synced IP addresses for device") + end + + defp log_sync_ip_summary(device, discovered_ip_addresses, removed_keys, true) do + ipv4_count = Enum.count(discovered_ip_addresses, &(&1.ip_type == "ipv4")) + ipv6_count = Enum.count(discovered_ip_addresses, &(&1.ip_type == "ipv6")) + removed_count = MapSet.size(removed_keys) + + Logger.info( + "Synced IP addresses: #{ipv4_count} IPv4, #{ipv6_count} IPv6, removed #{removed_count}", + device_id: device.device_id + ) + end + defp upsert_ip_address(ip_data, if_index_to_interface, _existing_ip_addresses) do - interface_id = Map.get(if_index_to_interface, ip_data.if_index) + upsert_ip_with_iface(ip_data, Map.get(if_index_to_interface, ip_data.if_index)) + end - if interface_id do - ip_attrs = Map.put(ip_data, :snmp_interface_id, interface_id) + defp upsert_ip_with_iface(_ip_data, nil), do: :ok - # Use on_conflict to handle race conditions gracefully - %IpAddress{} - |> IpAddress.changeset(ip_attrs) - |> Repo.insert( - on_conflict: {:replace_all_except, [:id, :inserted_at]}, - conflict_target: [:snmp_interface_id, :ip_address] - ) - end + defp upsert_ip_with_iface(ip_data, interface_id) do + ip_attrs = Map.put(ip_data, :snmp_interface_id, interface_id) + + %IpAddress{} + |> IpAddress.changeset(ip_attrs) + |> Repo.insert( + on_conflict: {:replace_all_except, [:id, :inserted_at]}, + conflict_target: [:snmp_interface_id, :ip_address] + ) end @type snmp_device_ref :: Device.t() | %{required(:id) => String.t()} @@ -1312,10 +1225,7 @@ defmodule Towerops.Snmp.Discovery do end end) - if invalid_count > 0 do - Logger.warning("Dropped #{invalid_count} storage entries with malformed storage_index") - end - + log_invalid_storage_entries(invalid_count) discovered_indices = MapSet.new(valid_storage, & &1.storage_index) # Delete storage that no longer exists on the device @@ -1354,6 +1264,12 @@ defmodule Towerops.Snmp.Discovery do :ok end + defp log_invalid_storage_entries(0), do: :ok + + defp log_invalid_storage_entries(count) do + Logger.warning("Dropped #{count} storage entries with malformed storage_index") + end + # Normalize storage index to integer (Base.discover_storage may return string from OID parsing). # Returns nil for malformed values — OID suffixes come from SNMP responses and must not be trusted. defp normalize_storage_index(index) when is_integer(index), do: index @@ -1622,30 +1538,28 @@ defmodule Towerops.Snmp.Discovery do {:ok, synced} end - # Resolve parent_id from parent_index for all entities on a device defp resolve_entity_parent_relationships(snmp_device_id) do - # Get all entities for this device entities = Repo.all(from(e in EntityPhysical, where: e.snmp_device_id == ^snmp_device_id)) - - # Build index map: entity_index -> id index_to_id = Map.new(entities, &{&1.entity_index, &1.id}) - # Update each entity's parent_id based on parent_index Enum.each(entities, fn entity -> - parent_id = - if entity.parent_index && entity.parent_index != "0" do - Map.get(index_to_id, entity.parent_index) - end - - # Only update if parent_id changed - if entity.parent_id != parent_id do - entity - |> Ecto.Changeset.change(parent_id: parent_id) - |> Repo.update!() - end + parent_id = resolve_parent_id(index_to_id, entity) + update_parent_if_changed(entity, parent_id) end) end + defp resolve_parent_id(_index_to_id, %{parent_index: idx}) when is_nil(idx), do: nil + defp resolve_parent_id(_index_to_id, %{parent_index: "0"}), do: nil + defp resolve_parent_id(index_to_id, %{parent_index: idx}), do: Map.get(index_to_id, idx) + + defp update_parent_if_changed(%{parent_id: existing} = _entity, existing), do: :ok + + defp update_parent_if_changed(entity, parent_id) do + entity + |> Ecto.Changeset.change(parent_id: parent_id) + |> Repo.update!() + end + @spec update_device_discovery_time(DeviceSchema.t()) :: {:ok, DeviceSchema.t()} | {:error, Ecto.Changeset.t()} defp update_device_discovery_time(device) do @@ -1690,65 +1604,69 @@ defmodule Towerops.Snmp.Discovery do @spec update_device_name_from_snmp(DeviceSchema.t(), system_info()) :: {:ok, DeviceSchema.t()} | {:error, term()} + defp update_device_name_from_snmp(%{name: name} = device, _system_info) + when is_binary(name) and name != "" do + Logger.info("Device name already set or no sysName, skipping update", device_id: device.id) + {:ok, device} + end + defp update_device_name_from_snmp(device, system_info) do - # Only update device name if it's currently empty and we got a sysName from SNMP - name_empty? = is_nil(device.name) or device.name == "" + do_update_device_name(device, system_info, Map.has_key?(system_info, :sys_name)) + end - if name_empty? and Map.has_key?(system_info, :sys_name) do - sys_name = Map.get(system_info, :sys_name) + defp do_update_device_name(device, _system_info, false) do + Logger.info("Device name already set or no sysName, skipping update", device_id: device.id) + {:ok, device} + end - if sys_name && sys_name != "" do - Logger.info( - "Updating device name from SNMP sysName: #{sys_name}", - device_id: device.id - ) + defp do_update_device_name(device, %{sys_name: sys_name}, true) when is_nil(sys_name) or sys_name == "" do + Logger.info("sysName empty, keeping device name unchanged", device_id: device.id) + {:ok, device} + end - result = - device - |> Ecto.Changeset.change(name: sys_name) - |> Repo.update() + defp do_update_device_name(device, %{sys_name: sys_name}, true) do + Logger.info("Updating device name from SNMP sysName: #{sys_name}", device_id: device.id) - Logger.info("Device name update completed", device_id: device.id) - result - else - Logger.info("sysName empty, keeping device name unchanged", device_id: device.id) - {:ok, device} - end - else - Logger.info("Device name already set or no sysName, skipping update", device_id: device.id) - {:ok, device} - end + result = + device + |> Ecto.Changeset.change(name: sys_name) + |> Repo.update() + + Logger.info("Device name update completed", device_id: device.id) + result end defp log_ip_discovery_results(device, ip_addresses) do - if Application.get_env(:towerops, :env) == :dev do - ipv4_count = Enum.count(ip_addresses, &(&1.ip_type == "ipv4")) - ipv6_count = Enum.count(ip_addresses, &(&1.ip_type == "ipv6")) - - Logger.info( - "Discovered IP addresses: #{ipv4_count} IPv4, #{ipv6_count} IPv6 (total: #{length(ip_addresses)})", - device_id: device.id - ) - end - + log_ip_if_dev_env(device, ip_addresses, Application.get_env(:towerops, :env) == :dev) :ok end - defp log_discovery_event(device, discovered_device, is_rediscovery) do - event_type = if is_rediscovery, do: "device_rediscovered", else: "device_discovered" + defp log_ip_if_dev_env(_device, _ip_addresses, false), do: :ok - message = - if is_rediscovery do - "Device rediscovered: #{device.name} (#{discovered_device.manufacturer} #{discovered_device.model})" - else - "Device discovered: #{device.name} (#{discovered_device.manufacturer} #{discovered_device.model})" - end + defp log_ip_if_dev_env(device, ip_addresses, true) do + ipv4_count = Enum.count(ip_addresses, &(&1.ip_type == "ipv4")) + ipv6_count = Enum.count(ip_addresses, &(&1.ip_type == "ipv6")) - interface_count = - case discovered_device.interfaces do - %Ecto.Association.NotLoaded{} -> 0 - interfaces when is_list(interfaces) -> length(interfaces) - end + Logger.info( + "Discovered IP addresses: #{ipv4_count} IPv4, #{ipv6_count} IPv6 (total: #{length(ip_addresses)})", + device_id: device.id + ) + end + + defp interface_count(%Ecto.Association.NotLoaded{}), do: 0 + defp interface_count(interfaces) when is_list(interfaces), do: length(interfaces) + + defp log_discovery_event(device, discovered_device, true) do + log_discovery_event_common(device, discovered_device, "device_rediscovered", "Device rediscovered") + end + + defp log_discovery_event(device, discovered_device, false) do + log_discovery_event_common(device, discovered_device, "device_discovered", "Device discovered") + end + + defp log_discovery_event_common(device, discovered_device, event_type, event_label) do + message = "#{event_label}: #{device.name} (#{discovered_device.manufacturer} #{discovered_device.model})" + interface_count = interface_count(discovered_device.interfaces) metadata = %{ manufacturer: discovered_device.manufacturer, @@ -1819,13 +1737,7 @@ defmodule Towerops.Snmp.Discovery do end defp sanitize_for_json(binary) when is_binary(binary) do - if String.printable?(binary) do - binary - else - # Convert non-printable binary to base64 with marker - # Use Elixir.Base to avoid conflict with Towerops.Snmp.Profiles.Base alias - %{"_binary_base64" => Elixir.Base.encode64(binary)} - end + sanitize_binary(binary, String.printable?(binary)) end defp sanitize_for_json(tuple) when is_tuple(tuple) do @@ -1836,6 +1748,12 @@ defmodule Towerops.Snmp.Discovery do defp sanitize_for_json(other), do: other + defp sanitize_binary(binary, true), do: binary + + defp sanitize_binary(binary, false) do + %{"_binary_base64" => Elixir.Base.encode64(binary)} + end + # Deduplicate sensors by OID - when multiple sensors share the same OID, # prefer the one with a proper description (not containing "MIB::"). # Normalizes OIDs by stripping leading dots so ".1.3.6.1..." and "1.3.6.1..."