graham/towerops
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: resolve MikroTik sensor values and empty SNMP communities

Browse source 
Fixes three critical issues affecting agent-based SNMP polling:

1. MikroTik gauge sensors incorrectly showing 10x values (223°C instead of 22.3°C)
   - Root cause: get_int_value() rejected zero values, causing mtxrGaugeUnit
     to be treated as nil and bypassing divisor logic
   - Fixed: Accept zero values and apply correct divisor=10 for temp/voltage/current/power

2. Empty SNMP community strings sent to agents causing authentication failures
   - Root cause: Devices with source="device" but null community weren't falling
     back to organization/site inheritance
   - Fixed: Enhanced resolve_snmp_community() to fall back to inheritance chain
     even when source="device" but community is empty

3. Continuous MikroTik API commands sent every 60s instead of once per 24h
   - Root cause: MikroTik jobs built on every poll cycle, not just discovery
   - Fixed: Only build MikroTik jobs during discovery phase

Migrations update existing data to fix incorrect divisor values and community
source tracking. Added comprehensive debug logging to diagnose future issues.
This commit is contained in:
Graham McIntire 2026-02-02 17:12:36 -06:00
parent aaf8c38b78
commit e181c17f78
No known key found for this signature in database
7 changed files with 244 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

43
lib/towerops/devices.ex
View file

@ -646,25 +646,52 @@ defmodule Towerops.Devices do
Resolves the effective SNMP community string for a device.
Resolution order:
1. If device has its own community string (source = "device"), use it
2. If source = "site", inherit from site's community string
1. If device has its own community string (source = "device" AND community is non-empty), use it
2. Otherwise, inherit from site's community string
3. If site has no community, inherit from organization's community string
4. If no community found anywhere, return nil
Note: Even if source is "device", we fall back to inheritance if the device community is empty.
This handles cases where the source field is incorrect or the community was cleared.
"""
def resolve_snmp_community(%DeviceSchema{} = device) do
device = Repo.preload(device, site: :organization)
case device.snmp_community_source do
"device" ->
# Use device-specific community
# Try device-specific community first (if source is "device" AND it's non-empty)
device_community =
if device.snmp_community_source == "device" && present?(device.snmp_community) do
device.snmp_community
end
_ ->
# Inherit from site or organization
device.site.snmp_community || device.site.organization.snmp_community
# Fall back to inheritance chain
community =
device_community ||
device.site.snmp_community ||
device.site.organization.snmp_community
# Debug logging to track empty community strings
if is_nil(community) or community == "" do
require Logger
Logger.warning("Empty SNMP community resolved for device",
device_id: device.id,
device_name: device.name,
community_source: device.snmp_community_source,
device_community: device.snmp_community,
site_community: device.site.snmp_community,
org_community: device.site.organization.snmp_community,
site_id: device.site_id,
org_id: device.site.organization_id
)
end
community
end
# Helper to check if a string is present (not nil and not empty)
defp present?(value) when is_binary(value), do: String.trim(value) != ""
defp present?(_), do: false
@doc """
Propagates SNMP community string changes from a site to all devices
that inherit from it (source = "site").

2
lib/towerops/devices/device.ex
View file

@ -34,7 +34,7 @@ defmodule Towerops.Devices.Device do
field :snmp_enabled, :boolean, default: true
field :snmp_version, :string, default: "2c"
field :snmp_community, :string
field :snmp_community_source, :string, default: "device"
field :snmp_community_source, :string, default: "organization"
field :snmp_port, :integer, default: 161
field :last_discovery_at, :utc_datetime
field :last_snmp_poll_at, :utc_datetime

2
lib/towerops/snmp/profiles/vendors/routeros.ex vendored
View file

@ -869,7 +869,7 @@ defmodule Towerops.Snmp.Profiles.Vendors.Routeros do
# Helper: Get integer value from grouped results
defp get_int_value(values, sub_oid) do
case Map.get(values, sub_oid) do
value when is_integer(value) and value != 0 -> value
value when is_integer(value) -> value
_ -> nil
end
end

34
lib/towerops_web/channels/agent_channel.ex
View file

@ -248,9 +248,10 @@ defmodule ToweropsWeb.AgentChannel do
build_polling_job(device)
end
# Build MikroTik job if device is MikroTik and MikroTik API is enabled
# Build MikroTik job only during discovery (not during regular polling)
# MikroTik commands are discovery-type operations that should run once per 24h
mikrotik_job =
if mikrotik_device?(device) do
if needs_discovery?(device) and mikrotik_device?(device) do
build_mikrotik_job(device)
end
@ -273,13 +274,22 @@ defmodule ToweropsWeb.AgentChannel do
end
defp build_discovery_job(device) do
community = Devices.resolve_snmp_community(device)
maybe_debug_log(%{assigns: %{agent_token_id: "system"}}, "Building discovery job",
device_id: device.id,
device_name: device.name,
community_present: !is_nil(community) && community != "",
community_source: device.snmp_community_source
)
%AgentJob{
job_id: "discover:#{device.id}",
job_type: :DISCOVER,
device_id: device.id,
snmp_device: %SnmpDevice{
ip: device.ip_address,
community: Devices.resolve_snmp_community(device),
community: community,
version: device.snmp_version,
port: device.snmp_port || 161
},
@ -289,6 +299,22 @@ defmodule ToweropsWeb.AgentChannel do
defp build_polling_job(device) do
_snmp_device = device.snmp_device
community = Devices.resolve_snmp_community(device)
maybe_debug_log(%{assigns: %{agent_token_id: "system"}}, "Building polling job",
device_id: device.id,
device_name: device.name,
community_present: !is_nil(community) && community != "",
community_source: device.snmp_community_source,
site_id: device.site_id,
site_loaded: not is_nil(Map.get(device, :site)),
site_community_present:
case Map.get(device, :site) do
nil -> false
%Ecto.Association.NotLoaded{} -> false
site -> !is_nil(site.snmp_community) && site.snmp_community != ""
end
)
%AgentJob{
job_id: "poll:#{device.id}",
@ -296,7 +322,7 @@ defmodule ToweropsWeb.AgentChannel do
device_id: device.id,
snmp_device: %SnmpDevice{
ip: device.ip_address,
community: Devices.resolve_snmp_community(device),
community: community,
version: device.snmp_version,
port: device.snmp_port || 161
},

15
lib/towerops_web/live/device_live/show.html.heex
View file

@ -1488,7 +1488,10 @@
<%= if length(@mikrotik_backups) > 1 do %>
<div class="px-4 py-3 bg-blue-50 dark:bg-blue-950/30 border-b border-blue-100 dark:border-blue-900/50">
<div class="flex items-start gap-2">
<.icon name="hero-information-circle" class="h-5 w-5 text-blue-600 dark:text-blue-400 flex-shrink-0 mt-0.5" />
<.icon
name="hero-information-circle"
class="h-5 w-5 text-blue-600 dark:text-blue-400 flex-shrink-0 mt-0.5"
/>
<div class="text-sm text-blue-900 dark:text-blue-200">
<span class="font-medium">Compare configurations:</span>
Select any 2 backups using the checkboxes to compare their differences
@ -1500,7 +1503,10 @@
<table class="min-w-full divide-y divide-gray-200 dark:divide-white/10">
<thead class="bg-gray-50 dark:bg-gray-800/75">
<tr class="text-xs text-gray-600 dark:text-gray-400">
<th class="px-4 py-3 text-center font-medium w-12" title="Select backups to compare">
<th
class="px-4 py-3 text-center font-medium w-12"
title="Select backups to compare"
>
<.icon name="hero-check-circle" class="h-4 w-4 mx-auto" />
</th>
<th class="px-4 py-3 text-left font-medium">Date</th>
@ -1614,7 +1620,10 @@
<div class="flex flex-col gap-2">
<%!-- Selection status --%>
<div class="flex items-center gap-2 text-sm">
<.icon name="hero-check-circle" class="h-5 w-5 text-blue-600 dark:text-blue-400" />
<.icon
name="hero-check-circle"
class="h-5 w-5 text-blue-600 dark:text-blue-400"
/>
<div class="text-gray-700 dark:text-gray-300">
<span class="font-medium">{MapSet.size(@selected_backup_ids)}</span>
of 2 backups selected

130
priv/repo/migrations/20260202225627_fix_mikrotik_gauge_sensors_with_wrong_divisor.exs Normal file
View file

@ -0,0 +1,130 @@
defmodule Towerops.Repo.Migrations.FixMikrotikGaugeSensorsWithWrongDivisor do
use Ecto.Migration
def up do
# Fix MikroTik gauge table sensors that were discovered with divisor 1
# due to the bug in get_int_value() that rejected 0 values.
#
# This affects sensors where:
# - mtxrGaugeUnit was 0, nil, or an unknown value
# - Name-based override should have set divisor to 10
# - But the override didn't run or failed, leaving divisor at 1
#
# We identify these by:
# - OID matches gauge table pattern (1.3.6.1.4.1.14988.1.1.3.100.1.3.*)
# - sensor_type should be temperature/voltage/current/power (tenths of unit)
# - sensor_divisor is currently 1 (wrong)
# Temperature sensors (should be tenths of degrees)
execute """
UPDATE snmp_sensors
SET sensor_divisor = 10,
sensor_unit = '°C',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value / 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'temperature'
AND sensor_divisor = 1
"""
# Voltage sensors (should be tenths of volts)
execute """
UPDATE snmp_sensors
SET sensor_divisor = 10,
sensor_unit = 'V',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value / 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'voltage'
AND sensor_divisor = 1
"""
# Current sensors (should be tenths of amps)
execute """
UPDATE snmp_sensors
SET sensor_divisor = 10,
sensor_unit = 'A',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value / 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'current'
AND sensor_divisor = 1
"""
# Power sensors (should be tenths of watts)
execute """
UPDATE snmp_sensors
SET sensor_divisor = 10,
sensor_unit = 'W',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value / 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'power'
AND sensor_divisor = 1
"""
end
def down do
# Revert to divisor 1 (incorrect, but reversible)
execute """
UPDATE snmp_sensors
SET sensor_divisor = 1,
sensor_unit = '',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value * 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'temperature'
AND sensor_divisor = 10
"""
execute """
UPDATE snmp_sensors
SET sensor_divisor = 1,
sensor_unit = '',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value * 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'voltage'
AND sensor_divisor = 10
"""
execute """
UPDATE snmp_sensors
SET sensor_divisor = 1,
sensor_unit = '',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value * 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'current'
AND sensor_divisor = 10
"""
execute """
UPDATE snmp_sensors
SET sensor_divisor = 1,
sensor_unit = '',
last_value = CASE
WHEN last_value IS NOT NULL THEN last_value * 10.0
ELSE NULL
END
WHERE sensor_oid LIKE '1.3.6.1.4.1.14988.1.1.3.100.1.3.%'
AND sensor_type = 'power'
AND sensor_divisor = 10
"""
end
end

35
priv/repo/migrations/20260202230847_fix_snmp_community_source_for_empty_communities.exs Normal file
View file

@ -0,0 +1,35 @@
defmodule Towerops.Repo.Migrations.FixSnmpCommunitySourceForEmptyCommunities do
use Ecto.Migration
def up do
# Fix devices that have snmp_community_source = "device" but no actual community string.
# These should inherit from organization/site instead.
#
# Root cause: The changeset logic in update_community_source/1 was setting source = "device"
# as the default, even when the device had no community string.
#
# This migration changes devices with:
# - snmp_community_source = "device"
# - snmp_community IS NULL or empty string
#
# To:
# - snmp_community_source = "organization" (will cascade through site → org inheritance)
execute """
UPDATE devices
SET snmp_community_source = 'organization'
WHERE snmp_community_source = 'device'
AND (snmp_community IS NULL OR snmp_community = '')
"""
end
def down do
# Revert to "device" source (not ideal, but reversible)
execute """
UPDATE devices
SET snmp_community_source = 'device'
WHERE snmp_community_source = 'organization'
AND (snmp_community IS NULL OR snmp_community = '')
"""
end
end