From 1712e7ac9de30e02ee650fbf65ed3d148bde89a7 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 17 Feb 2026 07:43:35 -0600 Subject: [PATCH] Skip SNMP check execution in CheckExecutorWorker for agent-polled devices CheckExecutorWorker was missing the phoenix_snmp_disabled and should_phoenix_poll_device guards that DevicePollerWorker and DeviceMonitorWorker already have. This caused SNMP checks to execute through Phoenix even when devices are assigned to agents, creating an infinite loop of failing jobs that fill the Oban queue. --- lib/towerops/workers/check_executor_worker.ex | 27 ++- .../controllers/admin_controller.ex | 4 +- .../workers/check_executor_worker_test.exs | 169 ++++++++++++++++++ 3 files changed, 193 insertions(+), 7 deletions(-) diff --git a/lib/towerops/workers/check_executor_worker.ex b/lib/towerops/workers/check_executor_worker.ex index 7727a4eb..332b5324 100644 --- a/lib/towerops/workers/check_executor_worker.ex +++ b/lib/towerops/workers/check_executor_worker.ex @@ -23,6 +23,8 @@ defmodule Towerops.Workers.CheckExecutorWorker do queue: :check_executors, max_attempts: 3 + alias Towerops.Agents + alias Towerops.Devices.Device alias Towerops.Monitoring alias Towerops.Monitoring.Executors.DnsExecutor alias Towerops.Monitoring.Executors.HttpExecutor @@ -31,10 +33,13 @@ defmodule Towerops.Workers.CheckExecutorWorker do alias Towerops.Monitoring.Executors.SnmpSensorExecutor alias Towerops.Monitoring.Executors.SnmpStorageExecutor alias Towerops.Monitoring.Executors.TcpExecutor + alias Towerops.Snmp.Client alias Towerops.Workers.PollingOffset require Logger + @snmp_check_types ~w(snmp_sensor snmp_interface snmp_processor snmp_storage) + @impl Oban.Worker def perform(%Oban.Job{args: %{"check_id" => check_id}}) do case Monitoring.get_check(check_id) do @@ -43,17 +48,29 @@ defmodule Towerops.Workers.CheckExecutorWorker do :ok check -> - if check.enabled do - execute_and_record(check) - schedule_next_check(check) - else - Logger.debug("Check #{check_id} disabled, skipping execution") + cond do + not check.enabled -> + Logger.debug("Check #{check_id} disabled, skipping execution") + + should_skip_snmp_check?(check) -> + Logger.debug("Skipping SNMP check #{check_id} - Phoenix SNMP disabled or device assigned to agent") + + true -> + execute_and_record(check) + schedule_next_check(check) end :ok end end + defp should_skip_snmp_check?(%{check_type: check_type} = check) when check_type in @snmp_check_types do + Client.phoenix_snmp_disabled() or + (check.device_id != nil and not Agents.should_phoenix_poll_device?(%Device{id: check.device_id})) + end + + defp should_skip_snmp_check?(_check), do: false + defp execute_and_record(check) do Logger.debug("Executing check #{check.id} (#{check.check_type})") diff --git a/lib/towerops_web/controllers/admin_controller.ex b/lib/towerops_web/controllers/admin_controller.ex index bb886914..2b4087cb 100644 --- a/lib/towerops_web/controllers/admin_controller.ex +++ b/lib/towerops_web/controllers/admin_controller.ex @@ -28,7 +28,7 @@ defmodule ToweropsWeb.AdminController do import Ecto.Query valid_states = ~w(scheduled retryable available) - requested = String.split(states_param, ",") |> Enum.map(&String.trim/1) + requested = states_param |> String.split(",") |> Enum.map(&String.trim/1) states = Enum.filter(requested, &(&1 in valid_states)) if states == [] do @@ -39,7 +39,7 @@ defmodule ToweropsWeb.AdminController do |> where([j], j.state in ^states) |> Towerops.Repo.delete_all() - conn |> json(%{ok: true, deleted: count, states: states}) + json(conn, %{ok: true, deleted: count, states: states}) end end end diff --git a/test/towerops/workers/check_executor_worker_test.exs b/test/towerops/workers/check_executor_worker_test.exs index 32276fcf..abe4bedf 100644 --- a/test/towerops/workers/check_executor_worker_test.exs +++ b/test/towerops/workers/check_executor_worker_test.exs @@ -312,4 +312,173 @@ defmodule Towerops.Workers.CheckExecutorWorkerTest do assert :ok = perform_job(CheckExecutorWorker, %{check_id: Ecto.UUID.generate()}) end end + + describe "perform/1 - SNMP check skipping" do + test "skips SNMP check when device is assigned to a local agent", %{ + device: device, + organization: organization, + snmp_device: snmp_device + } do + # Create a local agent and assign device to it + {:ok, agent_token, _token} = Towerops.Agents.create_agent_token(organization.id, "Local Agent") + {:ok, _assignment} = Towerops.Agents.assign_device_to_agent(agent_token.id, device.id) + + sensor = + %Sensor{} + |> Sensor.changeset(%{ + snmp_device_id: snmp_device.id, + sensor_descr: "Agent Sensor", + sensor_type: "temperature", + sensor_index: "1", + sensor_oid: ".1.2.3.4", + sensor_unit: "°C", + sensor_divisor: 1 + }) + |> Repo.insert!() + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "Agent Sensor Check", + check_type: "snmp_sensor", + source_type: "auto_discovery", + source_id: sensor.id, + interval_seconds: 60, + enabled: true, + config: %{} + }) + + # Clear existing jobs + Repo.delete_all(Oban.Job) + + # Should NOT call SNMP - device is agent-polled + assert :ok = perform_job(CheckExecutorWorker, %{check_id: check.id}) + + # No results should be recorded + results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) + assert results == [] + + # Should NOT reschedule + jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) + assert jobs == [] + end + + test "skips snmp_interface check when device is assigned to agent", %{ + device: device, + organization: organization + } do + {:ok, agent_token, _token} = Towerops.Agents.create_agent_token(organization.id, "Local Agent") + {:ok, _assignment} = Towerops.Agents.assign_device_to_agent(agent_token.id, device.id) + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "Interface Check", + check_type: "snmp_interface", + source_type: "auto_discovery", + source_id: Ecto.UUID.generate(), + interval_seconds: 60, + enabled: true, + config: %{"if_index" => "1", "if_descr" => "eth0"} + }) + + Repo.delete_all(Oban.Job) + + assert :ok = perform_job(CheckExecutorWorker, %{check_id: check.id}) + + results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) + assert results == [] + end + + test "still executes non-SNMP checks when device is assigned to agent", %{ + device: device, + organization: organization + } do + {:ok, agent_token, _token} = Towerops.Agents.create_agent_token(organization.id, "Local Agent") + {:ok, _assignment} = Towerops.Agents.assign_device_to_agent(agent_token.id, device.id) + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "HTTP Check", + check_type: "http", + source_type: "manual", + interval_seconds: 60, + enabled: true, + config: %{"url" => "https://example.com"} + }) + + Repo.delete_all(Oban.Job) + + # HTTP checks should still execute even when device has an agent + assert :ok = perform_job(CheckExecutorWorker, %{check_id: check.id}) + + # Should have a result (even if error from network) + results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) + assert length(results) == 1 + end + + test "skips SNMP check when phoenix_snmp_disabled is true", %{ + device: device, + snmp_device: snmp_device + } do + # Override env to simulate non-test environment with SNMP disabled + old_env = Application.get_env(:towerops, :env) + old_disable = Application.get_env(:towerops, :disable_phoenix_snmp) + Application.put_env(:towerops, :env, :prod) + Application.put_env(:towerops, :disable_phoenix_snmp, true) + + on_exit(fn -> + Application.put_env(:towerops, :env, old_env) + + if old_disable == nil do + Application.delete_env(:towerops, :disable_phoenix_snmp) + else + Application.put_env(:towerops, :disable_phoenix_snmp, old_disable) + end + end) + + sensor = + %Sensor{} + |> Sensor.changeset(%{ + snmp_device_id: snmp_device.id, + sensor_descr: "Disabled SNMP Sensor", + sensor_type: "temperature", + sensor_index: "2", + sensor_oid: ".1.2.3.5", + sensor_unit: "°C", + sensor_divisor: 1 + }) + |> Repo.insert!() + + {:ok, check} = + Monitoring.create_check(%{ + organization_id: device.organization_id, + device_id: device.id, + name: "Disabled SNMP Check", + check_type: "snmp_sensor", + source_type: "auto_discovery", + source_id: sensor.id, + interval_seconds: 60, + enabled: true, + config: %{} + }) + + Repo.delete_all(Oban.Job) + + # Should skip without calling SNMP + assert :ok = perform_job(CheckExecutorWorker, %{check_id: check.id}) + + # No results recorded + results = Repo.all(from(r in CheckResult, where: r.check_id == ^check.id)) + assert results == [] + + # No rescheduling + jobs = Repo.all(from(j in Oban.Job, where: j.worker == "Towerops.Workers.CheckExecutorWorker")) + assert jobs == [] + end + end end