From 1d4569924078ee941f639992e76e1c3df7b844ac Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 20 Jan 2026 17:29:58 -0600 Subject: [PATCH] discovery no longer deletes historical data --- lib/towerops/snmp/discovery.ex | 112 ++++++-- test/towerops/snmp/discovery_test.exs | 390 ++++++++++++++++++++++++++ 2 files changed, 477 insertions(+), 25 deletions(-) diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index bb248659..9b46d22b 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -376,14 +376,13 @@ defmodule Towerops.Snmp.Discovery do # Upsert Device snmp_device = upsert_device(device, device_info) - # Delete old interfaces/sensors and insert new ones - _ = delete_old_data(snmp_device) - new_interfaces = insert_interfaces(snmp_device, interfaces) - _ = insert_sensors(snmp_device, sensors) + # Sync interfaces and sensors (preserving historical data) + synced_interfaces = sync_interfaces(snmp_device, interfaces) + _ = sync_sensors(snmp_device, sensors) # Return device with interfaces loaded and device_id added # Use snmp_device.device_id which references the Equipment table - %{snmp_device | interfaces: Enum.map(new_interfaces, &Map.put(&1, :device_id, snmp_device.device_id))} + %{snmp_device | interfaces: Enum.map(synced_interfaces, &Map.put(&1, :device_id, snmp_device.device_id))} end) end @@ -421,32 +420,95 @@ defmodule Towerops.Snmp.Discovery do end end - @spec delete_old_data(Device.t()) :: {non_neg_integer(), nil | [term()]} - defp delete_old_data(device) do - # Delete old interfaces and sensors (cascade will handle stats/readings) - Repo.delete_all(from i in Interface, where: i.snmp_device_id == ^device.id) - Repo.delete_all(from s in Sensor, where: s.snmp_device_id == ^device.id) - end + @spec sync_interfaces(Device.t(), [interface_data()]) :: [Interface.t()] + defp sync_interfaces(device, discovered_interfaces) do + # Get existing interfaces for this device, indexed by if_index + existing_interfaces = + from(i in Interface, where: i.snmp_device_id == ^device.id) + |> Repo.all() + |> Map.new(&{&1.if_index, &1}) - @spec insert_interfaces(Device.t(), [interface_data()]) :: [Interface.t()] - defp insert_interfaces(device, interfaces) do - Enum.map(interfaces, fn interface_data -> - %Interface{} - |> Interface.changeset(Map.put(interface_data, :snmp_device_id, device.id)) - |> Repo.insert!() + # Get the set of discovered if_indices + discovered_indices = MapSet.new(discovered_interfaces, & &1.if_index) + + # Delete interfaces that no longer exist on the device + existing_indices = MapSet.new(Map.keys(existing_interfaces)) + removed_indices = MapSet.difference(existing_indices, discovered_indices) + + if MapSet.size(removed_indices) != 0 do + removed_list = MapSet.to_list(removed_indices) + + Repo.delete_all( + from i in Interface, + where: i.snmp_device_id == ^device.id and i.if_index in ^removed_list + ) + + Logger.debug("Deleted #{MapSet.size(removed_indices)} removed interfaces") + end + + # Upsert each discovered interface + Enum.map(discovered_interfaces, fn interface_data -> + case Map.get(existing_interfaces, interface_data.if_index) do + nil -> + # New interface - insert + %Interface{} + |> Interface.changeset(Map.put(interface_data, :snmp_device_id, device.id)) + |> Repo.insert!() + + existing -> + # Existing interface - update (preserves ID and associated stats) + existing + |> Interface.changeset(interface_data) + |> Repo.update!() + end end) end - @spec insert_sensors(Device.t(), [sensor_data()]) :: [Sensor.t()] - defp insert_sensors(device, sensors) do + @spec sync_sensors(Device.t(), [sensor_data()]) :: [Sensor.t()] + defp sync_sensors(device, discovered_sensors) do # Deduplicate sensors by sensor_index (keep first occurrence) # This prevents duplicate key errors when profiles generate duplicate indices - sensors - |> Enum.uniq_by(& &1.sensor_index) - |> Enum.map(fn sensor_data -> - %Sensor{} - |> Sensor.changeset(Map.put(sensor_data, :snmp_device_id, device.id)) - |> Repo.insert!() + discovered_sensors = Enum.uniq_by(discovered_sensors, & &1.sensor_index) + + # Get existing sensors for this device, indexed by sensor_index + existing_sensors = + from(s in Sensor, where: s.snmp_device_id == ^device.id) + |> Repo.all() + |> Map.new(&{&1.sensor_index, &1}) + + # Get the set of discovered sensor_indices + discovered_indices = MapSet.new(discovered_sensors, & &1.sensor_index) + + # Delete sensors that no longer exist on the device + existing_indices = MapSet.new(Map.keys(existing_sensors)) + removed_indices = MapSet.difference(existing_indices, discovered_indices) + + if MapSet.size(removed_indices) != 0 do + removed_list = MapSet.to_list(removed_indices) + + Repo.delete_all( + from s in Sensor, + where: s.snmp_device_id == ^device.id and s.sensor_index in ^removed_list + ) + + Logger.debug("Deleted #{MapSet.size(removed_indices)} removed sensors") + end + + # Upsert each discovered sensor + Enum.map(discovered_sensors, fn sensor_data -> + case Map.get(existing_sensors, sensor_data.sensor_index) do + nil -> + # New sensor - insert + %Sensor{} + |> Sensor.changeset(Map.put(sensor_data, :snmp_device_id, device.id)) + |> Repo.insert!() + + existing -> + # Existing sensor - update (preserves ID and associated readings) + existing + |> Sensor.changeset(sensor_data) + |> Repo.update!() + end end) end diff --git a/test/towerops/snmp/discovery_test.exs b/test/towerops/snmp/discovery_test.exs index 4d0c7851..f8f77d35 100644 --- a/test/towerops/snmp/discovery_test.exs +++ b/test/towerops/snmp/discovery_test.exs @@ -405,6 +405,396 @@ defmodule Towerops.Snmp.DiscoveryTest do assert updated_device.model == "Linux" assert updated_device.sys_name == "updated-name" end + + test "preserves sensor readings when rediscovering device with same sensor_index", %{ + site: site + } do + alias Towerops.Snmp.SensorReading + + {:ok, device} = + Towerops.Devices.create_device(%{ + name: "Device With Sensors", + ip_address: "192.168.1.50", + site_id: site.id, + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161 + }) + + # Create initial SNMP device with a sensor + {:ok, snmp_device} = + %Device{} + |> Device.changeset(%{ + device_id: device.id, + manufacturer: "Cisco", + model: "C2960", + sys_name: "test-switch" + }) + |> Repo.insert() + + {:ok, sensor} = + %Sensor{} + |> Sensor.changeset(%{ + snmp_device_id: snmp_device.id, + sensor_type: "temperature", + sensor_index: "1001", + sensor_oid: "1.3.6.1.4.1.9.9.91.1.1.1.1.4.1001", + sensor_descr: "CPU Temperature", + sensor_unit: "celsius", + sensor_divisor: 1, + last_value: 45.0 + }) + |> Repo.insert() + + # Create historical sensor readings + readings_to_create = [ + %{ + sensor_id: sensor.id, + value: 42.0, + status: "ok", + checked_at: DateTime.add(DateTime.utc_now(), -3600, :second) + }, + %{ + sensor_id: sensor.id, + value: 44.0, + status: "ok", + checked_at: DateTime.add(DateTime.utc_now(), -1800, :second) + }, + %{ + sensor_id: sensor.id, + value: 45.0, + status: "ok", + checked_at: DateTime.utc_now() + } + ] + + for reading_attrs <- readings_to_create do + %SensorReading{} + |> SensorReading.changeset(reading_attrs) + |> Repo.insert!() + end + + # Verify readings exist before rediscovery + initial_reading_count = + Repo.aggregate(from(r in SensorReading, where: r.sensor_id == ^sensor.id), :count) + + assert initial_reading_count == 3 + + # Mock SNMP responses for rediscovery + stub(SnmpMock, :get, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.3.0" -> {:ok, {:timeticks, 54_321}} + "1.3.6.1.2.1.1.1.0" -> {:ok, {:octet_string, "Cisco IOS Software, C2960"}} + "1.3.6.1.2.1.1.2.0" -> {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 9, 1, 1208]}} + "1.3.6.1.2.1.1.4.0" -> {:ok, {:octet_string, "admin@example.com"}} + "1.3.6.1.2.1.1.5.0" -> {:ok, {:octet_string, "test-switch"}} + "1.3.6.1.2.1.1.6.0" -> {:ok, {:octet_string, "Data Center"}} + _ -> {:error, :no_such_object} + end + end) + + stub(SnmpMock, :walk, fn _, oid, _ -> + # CISCO-ENTITY-SENSOR-MIB - sensor type table (ent_sensor_type) + # This is what Cisco profile walks first to find sensors + if oid == "1.3.6.1.4.1.9.9.91.1.1.1.1.1" do + {:ok, [%{oid: "1.3.6.1.4.1.9.9.91.1.1.1.1.1.1001", value: {:integer, 8}}]} + else + {:ok, []} + end + end) + + # Mock the individual sensor GETs that Cisco profile does after finding sensor indices + stub(SnmpMock, :get, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.3.0" -> {:ok, {:timeticks, 54_321}} + "1.3.6.1.2.1.1.1.0" -> {:ok, {:octet_string, "Cisco IOS Software, C2960"}} + "1.3.6.1.2.1.1.2.0" -> {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 9, 1, 1208]}} + "1.3.6.1.2.1.1.4.0" -> {:ok, {:octet_string, "admin@example.com"}} + "1.3.6.1.2.1.1.5.0" -> {:ok, {:octet_string, "test-switch"}} + "1.3.6.1.2.1.1.6.0" -> {:ok, {:octet_string, "Data Center"}} + # Cisco sensor details for index 1001 + "1.3.6.1.4.1.9.9.91.1.1.1.1.1.1001" -> {:ok, {:integer, 8}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.2.1001" -> {:ok, {:integer, 9}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.3.1001" -> {:ok, {:integer, 0}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.4.1001" -> {:ok, {:integer, 47}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.5.1001" -> {:ok, {:integer, 1}} + "1.3.6.1.2.1.47.1.1.1.1.2.1001" -> {:ok, {:octet_string, "CPU Temperature"}} + "1.3.6.1.2.1.47.1.1.1.1.7.1001" -> {:ok, {:octet_string, "TempSensor1"}} + _ -> {:error, :no_such_object} + end + end) + + # Run rediscovery + assert {:ok, rediscovered_device} = Discovery.discover_device(device) + + # Verify the SNMP device was updated (same ID) + assert rediscovered_device.id == snmp_device.id + + # Find the sensor - it should still exist (same ID preserved) + sensors = Repo.all(from s in Sensor, where: s.snmp_device_id == ^snmp_device.id) + assert length(sensors) == 1 + [updated_sensor] = sensors + + # The key assertion: sensor ID should be the same, preserving readings + assert updated_sensor.id == sensor.id + + # Verify all historical readings are still there + final_reading_count = + Repo.aggregate(from(r in SensorReading, where: r.sensor_id == ^sensor.id), :count) + + assert final_reading_count == 3 + + # Verify readings still have the correct values + readings = Repo.all(from r in SensorReading, where: r.sensor_id == ^sensor.id, order_by: r.checked_at) + assert length(readings) == 3 + assert Enum.map(readings, & &1.value) == [42.0, 44.0, 45.0] + end + + test "preserves interface stats when rediscovering device with same if_index", %{site: site} do + alias Towerops.Snmp.InterfaceStat + + {:ok, device} = + Towerops.Devices.create_device(%{ + name: "Device With Interfaces", + ip_address: "192.168.1.51", + site_id: site.id, + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161 + }) + + # Create initial SNMP device with an interface + {:ok, snmp_device} = + %Device{} + |> Device.changeset(%{ + device_id: device.id, + manufacturer: "Cisco", + model: "C2960", + sys_name: "test-switch" + }) + |> Repo.insert() + + {:ok, interface} = + %Interface{} + |> Interface.changeset(%{ + snmp_device_id: snmp_device.id, + if_index: 1, + if_descr: "GigabitEthernet0/1", + if_name: "Gi0/1", + if_type: 6, + if_speed: 1_000_000_000 + }) + |> Repo.insert() + + # Create historical interface stats + stats_to_create = [ + %{ + interface_id: interface.id, + if_in_octets: 1_000_000, + if_out_octets: 500_000, + if_in_errors: 0, + if_out_errors: 0, + checked_at: DateTime.add(DateTime.utc_now(), -3600, :second) + }, + %{ + interface_id: interface.id, + if_in_octets: 2_000_000, + if_out_octets: 1_000_000, + if_in_errors: 0, + if_out_errors: 0, + checked_at: DateTime.add(DateTime.utc_now(), -1800, :second) + }, + %{ + interface_id: interface.id, + if_in_octets: 3_000_000, + if_out_octets: 1_500_000, + if_in_errors: 1, + if_out_errors: 0, + checked_at: DateTime.utc_now() + } + ] + + for stat_attrs <- stats_to_create do + %InterfaceStat{} + |> InterfaceStat.changeset(stat_attrs) + |> Repo.insert!() + end + + # Verify stats exist before rediscovery + initial_stat_count = + Repo.aggregate(from(s in InterfaceStat, where: s.interface_id == ^interface.id), :count) + + assert initial_stat_count == 3 + + # Mock SNMP responses for rediscovery + stub(SnmpMock, :get, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.3.0" -> {:ok, {:timeticks, 54_321}} + "1.3.6.1.2.1.1.1.0" -> {:ok, {:octet_string, "Cisco IOS Software, C2960"}} + "1.3.6.1.2.1.1.2.0" -> {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 9, 1, 1208]}} + "1.3.6.1.2.1.1.4.0" -> {:ok, {:octet_string, "admin@example.com"}} + "1.3.6.1.2.1.1.5.0" -> {:ok, {:octet_string, "test-switch"}} + "1.3.6.1.2.1.1.6.0" -> {:ok, {:octet_string, "Data Center"}} + # Interface details for if_index 1 + "1.3.6.1.2.1.2.2.1.2.1" -> {:ok, {:octet_string, "GigabitEthernet0/1"}} + "1.3.6.1.2.1.2.2.1.3.1" -> {:ok, {:integer, 6}} + "1.3.6.1.2.1.2.2.1.5.1" -> {:ok, {:gauge32, 1_000_000_000}} + "1.3.6.1.2.1.2.2.1.6.1" -> {:ok, {:octet_string, <<0x00, 0x11, 0x22, 0x33, 0x44, 0x55>>}} + "1.3.6.1.2.1.2.2.1.7.1" -> {:ok, {:integer, 1}} + "1.3.6.1.2.1.2.2.1.8.1" -> {:ok, {:integer, 1}} + "1.3.6.1.2.1.31.1.1.1.1.1" -> {:ok, {:octet_string, "Gi0/1"}} + "1.3.6.1.2.1.31.1.1.1.18.1" -> {:ok, {:octet_string, "Uplink"}} + _ -> {:error, :no_such_object} + end + end) + + stub(SnmpMock, :walk, fn _, oid, _ -> + # Interface discovery - return same interface with if_index 1 + if oid == "1.3.6.1.2.1.2.2.1.1" do + {:ok, [%{oid: "1.3.6.1.2.1.2.2.1.1.1", value: {:integer, 1}}]} + else + {:ok, []} + end + end) + + # Run rediscovery + assert {:ok, rediscovered_device} = Discovery.discover_device(device) + + # Verify the SNMP device was updated (same ID) + assert rediscovered_device.id == snmp_device.id + + # Find the interface - it should still exist (same ID preserved) + interfaces = Repo.all(from i in Interface, where: i.snmp_device_id == ^snmp_device.id) + assert length(interfaces) == 1 + [updated_interface] = interfaces + + # The key assertion: interface ID should be the same, preserving stats + assert updated_interface.id == interface.id + + # Verify all historical stats are still there + final_stat_count = + Repo.aggregate(from(s in InterfaceStat, where: s.interface_id == ^interface.id), :count) + + assert final_stat_count == 3 + + # Verify stats still have the correct values + stats = Repo.all(from s in InterfaceStat, where: s.interface_id == ^interface.id, order_by: s.checked_at) + assert length(stats) == 3 + assert Enum.map(stats, & &1.if_in_octets) == [1_000_000, 2_000_000, 3_000_000] + end + + test "deletes sensors that no longer exist during rediscovery", %{site: site} do + {:ok, device} = + Towerops.Devices.create_device(%{ + name: "Device With Removed Sensor", + ip_address: "192.168.1.52", + site_id: site.id, + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161 + }) + + # Create initial SNMP device with two sensors + {:ok, snmp_device} = + %Device{} + |> Device.changeset(%{ + device_id: device.id, + manufacturer: "Cisco", + model: "C2960", + sys_name: "test-switch" + }) + |> Repo.insert() + + {:ok, sensor1} = + %Sensor{} + |> Sensor.changeset(%{ + snmp_device_id: snmp_device.id, + sensor_type: "temperature", + sensor_index: "1001", + sensor_oid: "1.3.6.1.4.1.9.9.91.1.1.1.1.4.1001", + sensor_descr: "CPU Temperature", + sensor_unit: "celsius", + sensor_divisor: 1 + }) + |> Repo.insert() + + {:ok, _sensor2} = + %Sensor{} + |> Sensor.changeset(%{ + snmp_device_id: snmp_device.id, + sensor_type: "temperature", + sensor_index: "1002", + sensor_oid: "1.3.6.1.4.1.9.9.91.1.1.1.1.4.1002", + sensor_descr: "Chassis Temperature", + sensor_unit: "celsius", + sensor_divisor: 1 + }) + |> Repo.insert() + + # Verify two sensors exist before rediscovery + initial_sensor_count = + Repo.aggregate(from(s in Sensor, where: s.snmp_device_id == ^snmp_device.id), :count) + + assert initial_sensor_count == 2 + + # Mock SNMP responses - only return sensor 1001, not 1002 + stub(SnmpMock, :get, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.3.0" -> {:ok, {:timeticks, 54_321}} + "1.3.6.1.2.1.1.1.0" -> {:ok, {:octet_string, "Cisco IOS Software, C2960"}} + "1.3.6.1.2.1.1.2.0" -> {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 9, 1, 1208]}} + "1.3.6.1.2.1.1.4.0" -> {:ok, {:octet_string, "admin@example.com"}} + "1.3.6.1.2.1.1.5.0" -> {:ok, {:octet_string, "test-switch"}} + "1.3.6.1.2.1.1.6.0" -> {:ok, {:octet_string, "Data Center"}} + _ -> {:error, :no_such_object} + end + end) + + stub(SnmpMock, :walk, fn _, oid, _ -> + # CISCO-ENTITY-SENSOR-MIB - only return sensor 1001 (1002 is "removed") + if oid == "1.3.6.1.4.1.9.9.91.1.1.1.1.1" do + {:ok, [%{oid: "1.3.6.1.4.1.9.9.91.1.1.1.1.1.1001", value: {:integer, 8}}]} + else + {:ok, []} + end + end) + + # Mock the individual sensor GETs for Cisco profile + stub(SnmpMock, :get, fn _, oid, _ -> + case oid do + "1.3.6.1.2.1.1.3.0" -> {:ok, {:timeticks, 54_321}} + "1.3.6.1.2.1.1.1.0" -> {:ok, {:octet_string, "Cisco IOS Software, C2960"}} + "1.3.6.1.2.1.1.2.0" -> {:ok, {:object_identifier, [1, 3, 6, 1, 4, 1, 9, 1, 1208]}} + "1.3.6.1.2.1.1.4.0" -> {:ok, {:octet_string, "admin@example.com"}} + "1.3.6.1.2.1.1.5.0" -> {:ok, {:octet_string, "test-switch"}} + "1.3.6.1.2.1.1.6.0" -> {:ok, {:octet_string, "Data Center"}} + # Cisco sensor details for index 1001 only + "1.3.6.1.4.1.9.9.91.1.1.1.1.1.1001" -> {:ok, {:integer, 8}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.2.1001" -> {:ok, {:integer, 9}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.3.1001" -> {:ok, {:integer, 0}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.4.1001" -> {:ok, {:integer, 47}} + "1.3.6.1.4.1.9.9.91.1.1.1.1.5.1001" -> {:ok, {:integer, 1}} + "1.3.6.1.2.1.47.1.1.1.1.2.1001" -> {:ok, {:octet_string, "CPU Temperature"}} + "1.3.6.1.2.1.47.1.1.1.1.7.1001" -> {:ok, {:octet_string, "TempSensor1"}} + _ -> {:error, :no_such_object} + end + end) + + # Run rediscovery + assert {:ok, _rediscovered_device} = Discovery.discover_device(device) + + # Verify only sensor 1001 remains + sensors = Repo.all(from s in Sensor, where: s.snmp_device_id == ^snmp_device.id) + assert length(sensors) == 1 + [remaining_sensor] = sensors + + # The preserved sensor should be sensor1 (index 1001) + assert remaining_sensor.id == sensor1.id + assert remaining_sensor.sensor_index == "1001" + end end describe "discover_all/1" do