diff --git a/CLAUDE.md b/CLAUDE.md index a95b6342..0b05db21 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -182,6 +182,50 @@ Key constraints from AGENTS.md (see that file for complete details): - be sure to always run mix format after you modify an elixir file - when you run mix format, don't specify a file, let it format everything +### Database Schema Critical Notes + +**Binary UUID Primary Keys**: All tables MUST use `:binary_id` for primary keys, not `bigint`. + +When creating new migrations with tables that have UUID primary keys: +```elixir +create table(:table_name, primary_key: false) do + add :id, :binary_id, primary_key: true + # ... +end +``` + +**Issue History**: The `snmp_neighbors` table was initially created with a `bigint` primary key instead of `:binary_id`, causing `DBConnection.EncodeError` when trying to insert records. This happened because the migration file was correct but the table was created before the fix. Always verify the actual database schema matches the migration: + +```bash +psql towerops_dev -c "\d table_name" # Check actual schema +``` + +If a table has the wrong primary key type, create a fix migration that drops and recreates the table. + +**SNMP Binary Data Handling**: All SNMP values (chassis IDs, port IDs, system names) must be converted to printable strings before saving to the database. Non-printable binaries (like MAC addresses in binary format) should be converted to colon-separated hex format using `String.printable?/1` checks. See `lib/towerops/snmp/neighbor_discovery.ex:sanitize_string_field/1` for the pattern. + +### SNMP Polling Architecture + +The application has two separate SNMP collection mechanisms: + +**1. Discovery (`Towerops.Snmp.Discovery`)** - One-time or manual collection +- Triggered manually by user or on a schedule +- Collects: system info, device identification, interfaces, sensors, **neighbors** +- Creates/updates the device record and all associated data +- Typically runs when equipment is first added or when user clicks "Rediscover" + +**2. Polling (`Towerops.Snmp.PollerWorker`)** - Continuous time-series collection +- Runs automatically every 60 seconds (configurable per equipment) +- Collects: sensor readings, interface statistics, **neighbor topology** +- Updates time-series data without recreating device records +- Keeps neighbor data fresh by polling LLDP/CDP every minute + +**Neighbor Data Lifecycle:** +- Discovery saves initial neighbors with 5-minute stale threshold +- Poller continuously updates neighbors every 60 seconds with 5-minute stale threshold +- `NeighborCleanupWorker` deletes neighbors older than 24 hours (safety net) +- This ensures neighbors stay current and topology changes are detected quickly + ## Kubernetes Deployment ### Prerequisites for Talos Kubernetes Cluster diff --git a/lib/towerops/snmp.ex b/lib/towerops/snmp.ex index c4da6d1b..d955cec7 100644 --- a/lib/towerops/snmp.ex +++ b/lib/towerops/snmp.ex @@ -358,6 +358,107 @@ defmodule Towerops.Snmp do |> Repo.all() end + @doc """ + Lists all neighbors for equipment with enriched data linking to known equipment. + Adds a `matched_equipment` field to each neighbor if we can identify the remote device. + """ + def list_neighbors_with_equipment(equipment_id) do + neighbors = list_neighbors(equipment_id) + + # Get the organization_id from the equipment to search within the same org + equipment = Towerops.Equipment.get_equipment!(equipment_id) + + Enum.map(neighbors, fn neighbor -> + matched_equipment = find_matching_equipment(neighbor, equipment.site.organization_id) + Map.put(neighbor, :matched_equipment, matched_equipment) + end) + end + + # Try to match a neighbor to known equipment in the organization + defp find_matching_equipment(neighbor, organization_id) do + # Try matching by chassis ID (MAC address) first + equipment_by_chassis = + if neighbor.remote_chassis_id && is_mac_address?(neighbor.remote_chassis_id) do + find_equipment_by_mac(neighbor.remote_chassis_id, organization_id) + end + + # Try matching by port ID (often contains MAC address in LLDP) + equipment_by_port = + if is_nil(equipment_by_chassis) && neighbor.remote_port_id && + is_mac_address?(neighbor.remote_port_id) do + find_equipment_by_mac(neighbor.remote_port_id, organization_id) + end + + # Try matching by system name if MAC matches didn't work + equipment_by_name = + if is_nil(equipment_by_chassis) && is_nil(equipment_by_port) && + neighbor.remote_system_name do + find_equipment_by_name(neighbor.remote_system_name, organization_id) + end + + equipment_by_chassis || equipment_by_port || equipment_by_name + end + + # Check if a string looks like a MAC address + defp is_mac_address?(str) when is_binary(str) do + # Match patterns like "AA:BB:CC:DD:EE:FF" or "aa-bb-cc-dd-ee-ff" + String.match?(str, ~r/^([0-9A-Fa-f]{2}[:-]){5}[0-9A-Fa-f]{2}$/) + end + + defp is_mac_address?(_), do: false + + defp find_equipment_by_mac(mac_address, organization_id) do + # MAC address might be in format "aa:bb:cc:dd:ee:ff", "aa-bb-cc-dd-ee-ff", or "aabbccddeeff" + # Normalize to just the hex digits for comparison + normalized_mac = mac_address |> String.downcase() |> String.replace(~r/[:-]/, "") + + Repo.one( + from(i in Interface, + join: d in Device, + on: i.snmp_device_id == d.id, + join: e in Equipment, + on: d.equipment_id == e.id, + join: s in assoc(e, :site), + where: s.organization_id == ^organization_id, + where: + fragment("REPLACE(REPLACE(LOWER(?), ':', ''), '-', '')", i.if_phys_address) == + ^normalized_mac, + select: e, + limit: 1 + ) + ) + end + + defp find_equipment_by_name(system_name, organization_id) do + # Try exact match first, then partial match + normalized_name = String.downcase(system_name) + + from(e in Equipment, + join: s in assoc(e, :site), + where: s.organization_id == ^organization_id, + where: fragment("LOWER(?)", e.name) == ^normalized_name, + select: e, + limit: 1 + ) + |> Repo.one() + |> case do + nil -> + # Try partial match as fallback + Repo.one( + from(e in Equipment, + join: s in assoc(e, :site), + where: s.organization_id == ^organization_id, + where: fragment("LOWER(?) LIKE ?", e.name, ^"%#{normalized_name}%"), + select: e, + limit: 1 + ) + ) + + equipment -> + equipment + end + end + @doc """ Gets a specific neighbor. """ @@ -371,7 +472,7 @@ defmodule Towerops.Snmp do def upsert_neighbor(attrs) do case Repo.get_by(Neighbor, interface_id: attrs.interface_id, - remote_chassis_id: attrs.remote_chassis_id, + remote_chassis_id: attrs[:remote_chassis_id], protocol: attrs.protocol ) do nil -> diff --git a/lib/towerops/snmp/neighbor.ex b/lib/towerops/snmp/neighbor.ex index c24a0ba2..e8ba58dc 100644 --- a/lib/towerops/snmp/neighbor.ex +++ b/lib/towerops/snmp/neighbor.ex @@ -26,6 +26,12 @@ defmodule Towerops.Snmp.Neighbor do @doc false def changeset(neighbor, attrs) do + # Ensure UUIDs are properly formatted strings before casting + attrs = + attrs + |> Map.update(:equipment_id, nil, &normalize_uuid/1) + |> Map.update(:interface_id, nil, &normalize_uuid/1) + neighbor |> cast(attrs, [ :equipment_id, @@ -52,4 +58,15 @@ defmodule Towerops.Snmp.Neighbor do |> foreign_key_constraint(:interface_id) |> unique_constraint([:interface_id, :remote_chassis_id, :protocol]) end + + # Normalize UUID to string format for Ecto casting + defp normalize_uuid(nil), do: nil + + defp normalize_uuid(uuid) when is_binary(uuid) and byte_size(uuid) == 16 do + # Convert 16-byte binary UUID to string format + Ecto.UUID.cast!(uuid) + end + + defp normalize_uuid(uuid) when is_binary(uuid), do: uuid + defp normalize_uuid(uuid), do: uuid end diff --git a/lib/towerops/snmp/neighbor_discovery.ex b/lib/towerops/snmp/neighbor_discovery.ex index 22707acb..1e2795c7 100644 --- a/lib/towerops/snmp/neighbor_discovery.ex +++ b/lib/towerops/snmp/neighbor_discovery.ex @@ -124,19 +124,36 @@ defmodule Towerops.Snmp.NeighborDiscovery do protocol: protocol, interface_id: interface.id, equipment_id: interface.equipment_id, - remote_chassis_id: neighbor_map[:remote_chassis_id], - remote_system_name: neighbor_map[:remote_system_name], - remote_system_description: neighbor_map[:remote_system_description], - remote_platform: neighbor_map[:remote_platform], - remote_port_id: neighbor_map[:remote_port_id], - remote_port_description: neighbor_map[:remote_port_description], - remote_address: neighbor_map[:remote_address], + remote_chassis_id: sanitize_string_field(neighbor_map[:remote_chassis_id]), + remote_system_name: sanitize_string_field(neighbor_map[:remote_system_name]), + remote_system_description: sanitize_string_field(neighbor_map[:remote_system_description]), + remote_platform: sanitize_string_field(neighbor_map[:remote_platform]), + remote_port_id: sanitize_string_field(neighbor_map[:remote_port_id]), + remote_port_description: sanitize_string_field(neighbor_map[:remote_port_description]), + remote_address: sanitize_string_field(neighbor_map[:remote_address]), remote_capabilities: neighbor_map[:remote_capabilities] || [], last_discovered_at: DateTime.truncate(DateTime.utc_now(), :second) } end end + # Ensure all string fields are actually strings, not raw binaries + defp sanitize_string_field(nil), do: nil + + defp sanitize_string_field(value) when is_binary(value) do + if String.valid?(value) and String.printable?(value) do + String.trim(value) + else + # Convert non-printable binary to hex string + value + |> :binary.bin_to_list() + |> Enum.map_join(":", &String.pad_leading(Integer.to_string(&1, 16), 2, "0")) + |> String.downcase() + end + end + + defp sanitize_string_field(value), do: to_string(value) + # CDP Discovery (Cisco proprietary) @cdp_cache_table_oid "1.3.6.1.4.1.9.9.23.1.2.1.1" @@ -230,8 +247,8 @@ defmodule Towerops.Snmp.NeighborDiscovery do defp format_port_id(value) when is_binary(value) do # Try to convert to string first, if it fails use MAC format - if String.valid?(value) do - value + if String.valid?(value) and String.printable?(value) do + String.trim(value) else format_chassis_id(value) end @@ -247,7 +264,15 @@ defmodule Towerops.Snmp.NeighborDiscovery do defp format_ip_address(value), do: to_string_safe(value) - defp to_string_safe(value) when is_binary(value), do: String.trim(value) + defp to_string_safe(value) when is_binary(value) do + if String.valid?(value) and String.printable?(value) do + String.trim(value) + else + # Convert non-printable binary to hex string + format_chassis_id(value) + end + end + defp to_string_safe(value) when is_integer(value), do: Integer.to_string(value) defp to_string_safe(value), do: inspect(value) diff --git a/lib/towerops/snmp/poller_worker.ex b/lib/towerops/snmp/poller_worker.ex index a6f61bc8..2aaa1423 100644 --- a/lib/towerops/snmp/poller_worker.ex +++ b/lib/towerops/snmp/poller_worker.ex @@ -1,18 +1,21 @@ defmodule Towerops.Snmp.PollerWorker do @moduledoc """ - GenServer that regularly polls SNMP sensors and interfaces for a device. + GenServer that regularly polls SNMP data for a device. - This worker continuously collects time-series data for: + This worker continuously collects: - Sensor readings (temperature, voltage, CPU, memory, etc.) - Interface statistics (bandwidth, errors, discards, etc.) + - Neighbor discovery (LLDP/CDP topology information) Runs independently of the connectivity monitoring in EquipmentMonitor. + Poll interval is configurable per equipment (default: 60 seconds). """ use GenServer alias Towerops.Equipment alias Towerops.Snmp alias Towerops.Snmp.Client + alias Towerops.Snmp.NeighborDiscovery alias Towerops.Snmp.PollerRegistry alias Towerops.Snmp.Sensor @@ -128,6 +131,7 @@ defmodule Towerops.Snmp.PollerWorker do poll_device_sensors(device, equipment, client_opts, now) poll_device_interfaces(device, equipment, client_opts, now) check_device_interface_changes(device, equipment, client_opts, now) + poll_device_neighbors(device, equipment, client_opts) end defp poll_device_sensors(device, equipment, client_opts, now) do @@ -155,6 +159,27 @@ defmodule Towerops.Snmp.PollerWorker do ) end + defp poll_device_neighbors(device, equipment, client_opts) do + # Add equipment_id to interfaces for neighbor discovery + interfaces_with_equipment = Enum.map(device.interfaces, &Map.put(&1, :equipment_id, equipment.id)) + + {:ok, neighbors} = NeighborDiscovery.discover_neighbors(client_opts, interfaces_with_equipment) + + # Delete stale neighbors (not seen in last 5 minutes) + cutoff = DateTime.add(DateTime.utc_now(), -5, :minute) + Snmp.delete_stale_neighbors(equipment.id, cutoff) + + # Upsert each discovered neighbor + Enum.each(neighbors, fn neighbor_data -> + Snmp.upsert_neighbor(neighbor_data) + end) + + Logger.debug("Polled and saved #{length(neighbors)} neighbors for #{equipment.name}") + rescue + error -> + Logger.error("Error polling neighbors for #{equipment.name}: #{inspect(error)}\n#{Exception.format_stacktrace()}") + end + defp poll_sensors(sensors, client_opts, timestamp) do Enum.each(sensors, fn sensor -> result = poll_sensor_value(sensor, client_opts) diff --git a/lib/towerops_web/live/equipment_live/show.ex b/lib/towerops_web/live/equipment_live/show.ex index a6389503..97965c92 100644 --- a/lib/towerops_web/live/equipment_live/show.ex +++ b/lib/towerops_web/live/equipment_live/show.ex @@ -52,7 +52,7 @@ defmodule ToweropsWeb.EquipmentLive.Show do recent_checks = Monitoring.list_equipment_checks(equipment_id, 50) snmp_data = load_snmp_data(equipment_id) events = Equipment.list_equipment_events(equipment_id, 100) - neighbors = Snmp.list_neighbors(equipment_id) + neighbors = Snmp.list_neighbors_with_equipment(equipment_id) cpu_chart_data = load_sensor_chart_data(equipment_id, ["cpu_load"]) memory_chart_data = load_sensor_chart_data(equipment_id, ["memory_usage"]) storage_chart_data = load_sensor_chart_data(equipment_id, ["disk_usage"]) diff --git a/lib/towerops_web/live/equipment_live/show.html.heex b/lib/towerops_web/live/equipment_live/show.html.heex index 648b1c8d..0899e850 100644 --- a/lib/towerops_web/live/equipment_live/show.html.heex +++ b/lib/towerops_web/live/equipment_live/show.html.heex @@ -515,23 +515,40 @@ {neighbor.interface.if_name || neighbor.interface.if_descr} -
- {neighbor.remote_system_name || "Unknown"} -
- <%= if neighbor.remote_address do %> -
- {neighbor.remote_address} + <%= if neighbor.matched_equipment do %> + <.link + navigate={ + ~p"/orgs/#{@equipment.site.organization_id}/equipment/#{neighbor.matched_equipment.id}" + } + class="font-medium text-blue-600 hover:text-blue-800 dark:text-blue-400 dark:hover:text-blue-300" + > + {neighbor.matched_equipment.name} + + <% else %> +
+ {neighbor.remote_system_name || "Unknown"}
<% end %> -
- {neighbor.remote_port_id || "-"} -
- <%= if neighbor.remote_port_description do %> -
- {neighbor.remote_port_description} + <%= if neighbor.matched_equipment do %> +
+ {neighbor.remote_system_name || "-"}
+ <%= if neighbor.remote_port_description do %> +
+ {neighbor.remote_port_description} +
+ <% end %> + <% else %> +
+ {neighbor.remote_port_id || "-"} +
+ <%= if neighbor.remote_chassis_id && neighbor.remote_chassis_id != neighbor.remote_port_id do %> +
+ {neighbor.remote_chassis_id} +
+ <% end %> <% end %> diff --git a/priv/repo/migrations/20260116195231_fix_snmp_neighbors_primary_key.exs b/priv/repo/migrations/20260116195231_fix_snmp_neighbors_primary_key.exs new file mode 100644 index 00000000..bbed714e --- /dev/null +++ b/priv/repo/migrations/20260116195231_fix_snmp_neighbors_primary_key.exs @@ -0,0 +1,55 @@ +defmodule Towerops.Repo.Migrations.FixSnmpNeighborsPrimaryKey do + use Ecto.Migration + + def up do + # Drop the existing table and recreate with correct primary key type + drop_if_exists table(:snmp_neighbors) + + create table(:snmp_neighbors, primary_key: false) do + add :id, :binary_id, primary_key: true + + add :equipment_id, references(:equipment, type: :binary_id, on_delete: :delete_all), + null: false + + add :interface_id, references(:snmp_interfaces, type: :binary_id, on_delete: :delete_all), + null: false + + # Discovery protocol (lldp, cdp) + add :protocol, :string, null: false + + # Remote device identification + add :remote_chassis_id, :string + add :remote_system_name, :string + add :remote_system_description, :text + add :remote_platform, :string + + # Remote interface/port information + add :remote_port_id, :string + add :remote_port_description, :string + + # Remote device address (IP) + add :remote_address, :string + + # Capabilities as a list of strings (router, switch, bridge, etc.) + add :remote_capabilities, {:array, :string}, default: [] + + # Last time this neighbor was seen + add :last_discovered_at, :utc_datetime, null: false + + timestamps(type: :utc_datetime) + end + + # Indexes for performance + create index(:snmp_neighbors, [:equipment_id]) + create index(:snmp_neighbors, [:interface_id]) + create index(:snmp_neighbors, [:protocol]) + create index(:snmp_neighbors, [:last_discovered_at]) + + # Unique constraint: one neighbor per interface + remote chassis + protocol + create unique_index(:snmp_neighbors, [:interface_id, :remote_chassis_id, :protocol]) + end + + def down do + drop table(:snmp_neighbors) + end +end