diff --git a/steward/monitors/ping.py b/steward/monitors/ping.py index 62f93a0..275331b 100644 --- a/steward/monitors/ping.py +++ b/steward/monitors/ping.py @@ -1,17 +1,38 @@ -"""ICMP / TCP reachability checks — pure probes returning a result dict.""" +"""ICMP / TCP reachability checks — pure probes returning a result dict. + +A failed check says why in `error_msg` (#5430): a down monitor that can't +say whether nothing answered or something refused leaves the operator to +guess. +""" from __future__ import annotations import asyncio import logging +import socket import time logger = logging.getLogger(__name__) TCP_TIMEOUT = 5.0 DEFAULT_TCP_PORT = 80 +PING_WAIT_S = 2 +PING_TIMEOUT = 5.0 + + +def tcp_failure(exc: BaseException, address: str, port: int) -> str: + """Why a TCP connect failed, in a phrase.""" + if isinstance(exc, (asyncio.TimeoutError, TimeoutError)): + return f"No answer on port {port} within {TCP_TIMEOUT:g} s" + if isinstance(exc, ConnectionRefusedError): + return f"Connection refused on port {port}" + if isinstance(exc, socket.gaierror): + return f"Can't resolve {address}: {exc.strerror or exc}" + if isinstance(exc, OSError): + return f"Can't connect to port {port}: {exc.strerror or exc}" + return str(exc) or type(exc).__name__ async def tcp_check(address: str, port: int) -> dict: - """TCP connect probe. Returns {is_up, response_ms}.""" + """TCP connect probe. Returns {is_up, response_ms, error_msg}.""" start = time.monotonic() try: reader, writer = await asyncio.wait_for( @@ -20,28 +41,53 @@ async def tcp_check(address: str, port: int) -> dict: ) writer.close() await writer.wait_closed() - return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2)} - except Exception: - return {"is_up": False, "response_ms": None} + return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2), + "error_msg": None} + except Exception as exc: + return {"is_up": False, "response_ms": None, "error_msg": tcp_failure(exc, address, port)} + + +def ping_failure(returncode: int, stderr: str) -> str: + """Why `ping -c 1` failed: exit 1 is no reply; anything else is ping's + own complaint (an unknown name, no route), first line of it.""" + if returncode == 1: + return f"No reply within {PING_WAIT_S} s" + said = stderr.strip().splitlines()[0] if stderr.strip() else "" + return said or f"ping failed (exit {returncode})" async def icmp_check(address: str) -> dict: """ICMP echo via the system ping binary (setuid; no raw-socket privilege). Falls back to a TCP connect on the default port if the ping binary is - unavailable or errors, so a host with ICMP filtered still gets a signal. + unavailable or errors, so a host with ICMP filtered still gets a signal; + a failure then says the TCP check stood in. """ start = time.monotonic() try: proc = await asyncio.create_subprocess_exec( - "ping", "-c", "1", "-W", "2", address, + "ping", "-c", "1", "-W", str(PING_WAIT_S), address, stdout=asyncio.subprocess.DEVNULL, - stderr=asyncio.subprocess.DEVNULL, + stderr=asyncio.subprocess.PIPE, ) - await asyncio.wait_for(proc.wait(), timeout=5.0) - if proc.returncode == 0: - return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2)} - return {"is_up": False, "response_ms": None} except Exception as exc: logger.warning("ICMP ping failed for %s: %s, falling back to TCP", address, exc) - return await tcp_check(address, DEFAULT_TCP_PORT) + result = await tcp_check(address, DEFAULT_TCP_PORT) + if not result["is_up"]: + result["error_msg"] = f"ping unavailable, TCP check instead: {result['error_msg']}" + return result + try: + _, stderr = await asyncio.wait_for(proc.communicate(), timeout=PING_TIMEOUT) + except asyncio.TimeoutError: + proc.kill() + try: + await asyncio.wait_for(proc.wait(), timeout=PING_TIMEOUT) + except asyncio.TimeoutError: + logger.warning("ping for %s did not exit after kill", address) + return {"is_up": False, "response_ms": None, + "error_msg": f"No reply within {PING_TIMEOUT:g} s"} + if proc.returncode == 0: + return {"is_up": True, "response_ms": round((time.monotonic() - start) * 1000, 2), + "error_msg": None} + return {"is_up": False, "response_ms": None, + "error_msg": ping_failure(proc.returncode, (stderr or b"").decode(errors="replace"))} diff --git a/tests/core/test_ping_checks.py b/tests/core/test_ping_checks.py new file mode 100644 index 0000000..7bd8f49 --- /dev/null +++ b/tests/core/test_ping_checks.py @@ -0,0 +1,65 @@ +"""A failed ping or TCP check says why (#5430): no reply, refused, no answer +in time, a name that doesn't resolve, or ping's own complaint; an ICMP check +that fell back to TCP says so.""" +import asyncio +import socket + +import pytest + +from steward.monitors import ping + + +@pytest.mark.parametrize("exc,want", [ + (asyncio.TimeoutError(), "No answer on port 22 within 5 s"), + (ConnectionRefusedError(111, "Connection refused"), "Connection refused on port 22"), + (socket.gaierror(-2, "Name or service not known"), "Can't resolve nas.lan: Name or service not known"), + (OSError(113, "No route to host"), "Can't connect to port 22: No route to host"), + (RuntimeError("odd"), "odd"), +]) +def test_a_tcp_failure_says_why(monkeypatch, exc, want): + async def boom(*a, **kw): + raise exc + monkeypatch.setattr(ping.asyncio, "open_connection", boom) + got = asyncio.run(ping.tcp_check("nas.lan", 22)) + assert got == {"is_up": False, "response_ms": None, "error_msg": want} + + +class _Proc: + def __init__(self, returncode, stderr=b""): + self.returncode, self._stderr = returncode, stderr + + async def communicate(self): + return b"", self._stderr + + +@pytest.mark.parametrize("code,stderr,want", [ + (1, b"", "No reply within 2 s"), + (2, b"ping: nas.lan: Name or service not known\n", "ping: nas.lan: Name or service not known"), + (2, b"", "ping failed (exit 2)"), +]) +def test_a_ping_failure_says_why(monkeypatch, code, stderr, want): + async def spawn(*a, **kw): + return _Proc(code, stderr) + monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", spawn) + got = asyncio.run(ping.icmp_check("nas.lan")) + assert got == {"is_up": False, "response_ms": None, "error_msg": want} + + +def test_a_reply_is_up_with_no_error(monkeypatch): + async def spawn(*a, **kw): + return _Proc(0) + monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", spawn) + got = asyncio.run(ping.icmp_check("nas.lan")) + assert got["is_up"] is True and got["error_msg"] is None + + +def test_without_ping_the_tcp_stand_in_says_so(monkeypatch): + async def no_ping(*a, **kw): + raise FileNotFoundError("ping") + + async def refused(*a, **kw): + raise ConnectionRefusedError(111, "Connection refused") + monkeypatch.setattr(ping.asyncio, "create_subprocess_exec", no_ping) + monkeypatch.setattr(ping.asyncio, "open_connection", refused) + got = asyncio.run(ping.icmp_check("nas.lan")) + assert got["error_msg"] == "ping unavailable, TCP check instead: Connection refused on port 80"