fix neighbors

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

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 ->

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

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)
 

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)

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"])

lib/towerops_web/live/equipment_live/show.html.heex

@@ -515,23 +515,40 @@
                         {neighbor.interface.if_name || neighbor.interface.if_descr}
                       </td>
                       <td class="px-4 py-3">
-                        <div class="font-medium text-zinc-900 dark:text-zinc-100">
-                          {neighbor.remote_system_name || "Unknown"}
-                        </div>
-                        <%= if neighbor.remote_address do %>
-                          <div class="text-xs font-mono text-zinc-500 dark:text-zinc-400">
-                            {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}
+                          </.link>
+                        <% else %>
+                          <div class="text-zinc-900 dark:text-zinc-100">
+                            {neighbor.remote_system_name || "Unknown"}
                           </div>
                         <% end %>
                       </td>
                       <td class="px-4 py-3">
-                        <div class="text-zinc-900 dark:text-zinc-100">
-                          {neighbor.remote_port_id || "-"}
-                        </div>
-                        <%= if neighbor.remote_port_description do %>
-                          <div class="text-xs text-zinc-500 dark:text-zinc-400">
-                            {neighbor.remote_port_description}
+                        <%= if neighbor.matched_equipment do %>
+                          <div class="text-zinc-900 dark:text-zinc-100">
+                            {neighbor.remote_system_name || "-"}
                           </div>
+                          <%= if neighbor.remote_port_description do %>
+                            <div class="text-xs text-zinc-500 dark:text-zinc-400">
+                              {neighbor.remote_port_description}
+                            </div>
+                          <% end %>
+                        <% else %>
+                          <div class="text-zinc-900 dark:text-zinc-100">
+                            {neighbor.remote_port_id || "-"}
+                          </div>
+                          <%= if neighbor.remote_chassis_id && neighbor.remote_chassis_id != neighbor.remote_port_id do %>
+                            <div class="text-xs font-mono text-zinc-500 dark:text-zinc-400">
+                              {neighbor.remote_chassis_id}
+                            </div>
+                          <% end %>
                         <% end %>
                       </td>
                       <td class="px-4 py-3 text-xs text-zinc-600 dark:text-zinc-400">

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