Improved snmp uptime parsing with new snmp parsing function while maintaining old behavior

Removed if64 workaround and moved tests to uptime tests

SUP-28325

Change-Id: I476f6432101f668667fbc595eae9dc4ab9b1a7a4

Author: matt-rindel-checkmk

cmk/plugins/collection/agent_based/if64.py

@@ -6,7 +6,7 @@
 import dataclasses
 import time
 from collections.abc import Mapping, MutableMapping, Sequence
-from typing import Any, NamedTuple
+from typing import Any
 
 from cmk.agent_based.v2 import (
     CheckPlugin,
@@ -35,43 +35,13 @@ def parse_if64adm(string_table: StringTable) -> If64AdmSection:
     }
 
 
-class UptimePair(NamedTuple):
-    sys_uptime_sec: float | None
-    hr_sys_uptime_sec: float | None
-
-    def __bool__(self) -> bool:
-        return self.hr_sys_uptime_sec is not None or self.sys_uptime_sec is not None
-
-    def to_list(self) -> list[float]:
-        return [
-            value for value in [self.hr_sys_uptime_sec, self.sys_uptime_sec] if value is not None
-        ]
-
-
-# parse_snmp_uptime currently decides on an uptime
-# based on whether it is defined, thus 0 would be preferred over a real uptime
-# in case of bad data. This is a workaround to force parsing of each value.
-def parse_snmp_uptime_pair(string_table: StringTable) -> UptimePair | None:
-    try:
-        sys_uptime_sec = uptime.parse_snmp_uptime([[string_table[0][0], ""]])
-        hr_sys_uptime_sec = uptime.parse_snmp_uptime([["", string_table[0][1]]])
-        return UptimePair(
-            sys_uptime_sec=sys_uptime_sec.uptime_sec if sys_uptime_sec else None,
-            hr_sys_uptime_sec=hr_sys_uptime_sec.uptime_sec if hr_sys_uptime_sec else None,
-        )
-    except TypeError:
-        return None
-    except IndexError:
-        return None
-
-
 def parse_if64_with_uptime(
     string_table: Sequence[StringByteTable],
 ) -> interfaces.Section[interfaces.InterfaceWithCounters]:
     raw_uptime, raw_if64 = string_table
-    parsed_uptimes = parse_snmp_uptime_pair([[str(w) for w in l] for l in raw_uptime])
+    parsed_uptimes = uptime.parse_snmp_uptime_pair([[str(w) for w in l] for l in raw_uptime])
 
-    timestamp = max(parsed_uptimes.to_list()) if parsed_uptimes else time.time()
+    timestamp = parsed_uptimes.max_uptime_sec if parsed_uptimes else time.time()
     return if64.parse_if64(
         raw_if64,
         timestamp,

packages/cmk-plugins/cmk/plugins/lib/uptime.py

@@ -75,6 +75,50 @@ def check(params: Mapping[str, Any], section: Section) -> CheckResult:
             )
 
 
+class UptimePair(NamedTuple):
+    sys_uptime_sec: float | None
+    hr_sys_uptime_sec: float | None
+
+    @property
+    def max_uptime_sec(self) -> float:
+        return max(
+            [value for value in [self.hr_sys_uptime_sec, self.sys_uptime_sec] if value is not None]
+        )
+
+    def __bool__(self) -> bool:
+        return self.hr_sys_uptime_sec is not None or self.sys_uptime_sec is not None
+
+
+def parse_snmp_uptime_pair(string_table: StringTable) -> UptimePair | None:
+    def parse(raw_value: str) -> int | None:
+        if ":" in raw_value:
+            try:
+                days, h, m, s = raw_value.split(":")
+                return (int(days) * 86400) + (int(h) * 3600) + (int(m) * 60) + int(float(s))
+            except Exception:
+                return None
+
+        if raw_value == "0":
+            return 0
+
+        if len(raw_value) < 3:
+            return None
+
+        try:
+            return int(raw_value[:-2])
+        except Exception:
+            return None
+
+    try:
+        return UptimePair(
+            sys_uptime_sec=parse(string_table[0][0]), hr_sys_uptime_sec=parse(string_table[0][1])
+        )
+    except TypeError:
+        return None
+    except IndexError:
+        return None
+
+
 def parse_snmp_uptime(string_table: StringTable) -> Section | None:
     """
     >>> parse_snmp_uptime([['2297331594', '']])
@@ -87,29 +131,17 @@ def parse_snmp_uptime(string_table: StringTable) -> Section | None:
     Section(uptime_sec=0, message=None)
 
     """
-    if not string_table:
-        return None
-
-    ticks = string_table[0][1] or string_table[0][0]
-
-    if ticks == "0":
-        return Section(0, None)
 
-    if len(ticks) < 3:
+    # internally uses parse_snmp_uptime_pair to keep parsing logic
+    # in one place.
+    uptime_pair = parse_snmp_uptime_pair(string_table)
+    if not uptime_pair:
         return None
 
-    try:
-        return Section(int(ticks[:-2]), None)
-    except Exception:
-        pass
-
-    try:
-        days, h, m, s = ticks.split(":")
-        return Section(
-            (int(days) * 86400) + (int(h) * 3600) + (int(m) * 60) + int(float(s)),
-            None,
-        )
-    except Exception:
-        pass
-
-    return None
+    # prefer hr_sys_uptime_sec if possible, even if it may have weird values
+    result = (
+        uptime_pair.hr_sys_uptime_sec
+        if uptime_pair.hr_sys_uptime_sec is not None
+        else uptime_pair.sys_uptime_sec
+    )
+    return Section(result, None)

packages/cmk-plugins/tests/cmk/plugins/lib/test_uptime.py

@@ -0,0 +1,139 @@
+#!/usr/bin/env python3
+# Copyright (C) 2026 Checkmk GmbH - License: GNU General Public License v2
+# This file is part of Checkmk (https://checkmk.com). It is subject to the terms and
+# conditions defined in the file COPYING, which is part of this source code package.
+import pytest
+
+from cmk.plugins.lib.uptime import parse_snmp_uptime, parse_snmp_uptime_pair, Section
+
+
+def test_parse_snmp_uptime_pair_with_empty_section_is_none() -> None:
+    parsed_uptime = parse_snmp_uptime_pair([[]])
+    assert parsed_uptime is None
+
+
+def test_parse_snmp_uptime_pair_with_no_values_is_not_valid() -> None:
+    parsed_uptime = parse_snmp_uptime_pair([["", ""]])
+
+    assert parsed_uptime is not None
+    assert not parsed_uptime
+    with pytest.raises(ValueError):
+        _ = parsed_uptime.max_uptime_sec
+    assert parsed_uptime.sys_uptime_sec is None
+    assert parsed_uptime.hr_sys_uptime_sec is None
+
+
+def test_parse_snmp_uptime_pair_with_sys_uptime_only_has_parsed_value() -> None:
+    parsed_uptime = parse_snmp_uptime_pair([["500000", ""]])
+
+    assert parsed_uptime
+    assert parsed_uptime.max_uptime_sec == 5000
+    assert parsed_uptime.sys_uptime_sec == 5000
+    assert parsed_uptime.hr_sys_uptime_sec is None
+
+
+def test_parse_snmp_uptime_pair_with_hr_sys_uptime_only_has_parsed_value() -> None:
+    parsed_uptime = parse_snmp_uptime_pair([["", "500000"]])
+
+    assert parsed_uptime
+    assert parsed_uptime.max_uptime_sec == 5000
+    assert parsed_uptime.sys_uptime_sec is None
+    assert parsed_uptime.hr_sys_uptime_sec == 5000
+
+
+def test_parse_snmp_uptime_pair_with_both_uptime_values_has_parsed_values() -> None:
+    parsed_uptime = parse_snmp_uptime_pair([["500000", "500500"]])
+
+    assert parsed_uptime
+    assert parsed_uptime.max_uptime_sec == 5005
+    assert parsed_uptime.sys_uptime_sec == 5000
+    assert parsed_uptime.hr_sys_uptime_sec == 5005
+
+
+def test_parse_snmp_uptime_pair_with_both_values_but_hr_uptime_is_zero_second_value_is_not_none() -> (
+    None
+):
+    parsed_uptime = parse_snmp_uptime_pair([["500000", "0"]])
+
+    assert parsed_uptime
+    assert parsed_uptime.max_uptime_sec == 5000
+    assert parsed_uptime.sys_uptime_sec == 5000
+    assert parsed_uptime.hr_sys_uptime_sec == 0
+
+
+def test_parse_snmp_uptime_parses_with_zero_values() -> None:
+    assert parse_snmp_uptime([["0", ""]]) == Section(uptime_sec=0, message=None)
+    assert parse_snmp_uptime([["", "0"]]) == Section(uptime_sec=0, message=None)
+    assert parse_snmp_uptime([["0", "0"]]) == Section(uptime_sec=0, message=None)
+
+
+def test_parse_snmp_uptime_is_none_with_empty_data() -> None:
+    assert parse_snmp_uptime([["", ""]]) is None
+
+
+def test_parse_snmp_uptime_is_none_with_nonsense_data() -> None:
+    assert parse_snmp_uptime([["", "Fortigate 80C"]]) is None
+    assert parse_snmp_uptime([["Fortigate 80C", ""]]) is None
+
+
+def test_parse_snmp_uptime_parses_with_only_sys_up_time() -> None:
+    assert parse_snmp_uptime([["2297331594", ""]]) == Section(uptime_sec=22973315, message=None)
+
+
+def test_parse_snmp_uptime_parses_with_only_hr_system_up_time() -> None:
+    assert parse_snmp_uptime([["", "2297331594"]]) == Section(uptime_sec=22973315, message=None)
+
+
+def test_parse_snmp_uptime_prefers_sys_up_time_with_unexpected_hr_system_up_time() -> None:
+    # would have been None with the previous logic
+    assert parse_snmp_uptime([["2297331594", "9"]]) == Section(uptime_sec=22973315, message=None)
+    assert parse_snmp_uptime([["2297331594", "99"]]) == Section(uptime_sec=22973315, message=None)
+
+
+def test_parse_snmp_uptime_prefers_hr_system_up_time_with_with_valid_hr_value() -> None:
+    # keeping the old behavior and not trying to guess the correct value
+    assert parse_snmp_uptime([["2297331594", "0"]]) == Section(uptime_sec=0, message=None)
+    assert parse_snmp_uptime([["2297331594", "999"]]) == Section(uptime_sec=9, message=None)
+    assert parse_snmp_uptime([["2297331594", "2297331"]]) == Section(uptime_sec=22973, message=None)
+
+    assert parse_snmp_uptime([["0", "2297331694"]]) == Section(uptime_sec=22973316, message=None)
+    assert parse_snmp_uptime([["9", "2297331694"]]) == Section(uptime_sec=22973316, message=None)
+    assert parse_snmp_uptime([["99", "2297331694"]]) == Section(uptime_sec=22973316, message=None)
+    assert parse_snmp_uptime([["999", "2297331694"]]) == Section(uptime_sec=22973316, message=None)
+    assert parse_snmp_uptime([["2297331", "2297331694"]]) == Section(
+        uptime_sec=22973316, message=None
+    )
+
+
+def test_parse_snmp_uptime_prefers_hr_system_up_time() -> None:
+    assert parse_snmp_uptime([["2267331694", "2297331694"]]) == Section(
+        uptime_sec=22973316, message=None
+    )
+
+
+def test_parse_snmp_uptime_parses_with_sys_up_timestamp_and_hr_system_up_timestamp() -> None:
+    assert parse_snmp_uptime([["124:21:26:42.03", "124:21:29:01.14"]]) == Section(
+        uptime_sec=10790941, message=None
+    )
+
+
+def test_parse_snmp_uptime_parses_mixed_formats() -> None:
+    assert parse_snmp_uptime([["124:21:26:42.03", "1079094124"]]) == Section(
+        uptime_sec=10790941, message=None
+    )
+
+    assert parse_snmp_uptime([["1079094124", "124:21:26:42.03"]]) == Section(
+        uptime_sec=10790802, message=None
+    )
+
+
+def test_parse_snmp_uptime_parses_with_only_hr_system_up_timestamp() -> None:
+    assert parse_snmp_uptime([["", "124:21:29:01.14"]]) == Section(
+        uptime_sec=10790941, message=None
+    )
+
+
+def test_parse_snmp_uptime_parses_with_only_sys_up_timestamp() -> None:
+    assert parse_snmp_uptime([["124:21:26:42.03", ""]]) == Section(
+        uptime_sec=10790802, message=None
+    )

tests/unit/cmk/plugins/collection/agent_based/test_if64.py

@@ -75,56 +75,3 @@ def test_check_timestamps_valid() -> None:
     value_store: dict[str, object] = {}
     assert not list(if64._check_timestamps({"a": 1, "b": 2}, value_store))
     assert not list(if64._check_timestamps({"a": 61, "b": 62}, value_store))
-
-
-def test_parse_snmp_uptime_pair_with_empty_section_is_none() -> None:
-    parsed_uptime = if64.parse_snmp_uptime_pair([[]])
-    assert parsed_uptime is None
-
-
-def test_parse_snmp_uptime_pair_with_no_values_is_not_valid() -> None:
-    parsed_uptime = if64.parse_snmp_uptime_pair([["", ""]])
-
-    assert parsed_uptime is not None
-    assert not parsed_uptime
-    assert len(parsed_uptime.to_list()) == 0
-    assert parsed_uptime.sys_uptime_sec is None
-    assert parsed_uptime.hr_sys_uptime_sec is None
-
-
-def test_parse_snmp_uptime_pair_with_sys_uptime_only_has_parsed_value() -> None:
-    parsed_uptime = if64.parse_snmp_uptime_pair([["500000", ""]])
-
-    assert parsed_uptime
-    assert len(parsed_uptime.to_list()) == 1
-    assert parsed_uptime.sys_uptime_sec == 5000
-    assert parsed_uptime.hr_sys_uptime_sec is None
-
-
-def test_parse_snmp_uptime_pair_with_hr_sys_uptime_only_has_parsed_value() -> None:
-    parsed_uptime = if64.parse_snmp_uptime_pair([["", "500000"]])
-
-    assert parsed_uptime
-    assert len(parsed_uptime.to_list()) == 1
-    assert parsed_uptime.sys_uptime_sec is None
-    assert parsed_uptime.hr_sys_uptime_sec == 5000
-
-
-def test_parse_snmp_uptime_pair_with_both_uptime_values_has_parsed_values() -> None:
-    parsed_uptime = if64.parse_snmp_uptime_pair([["500000", "500500"]])
-
-    assert parsed_uptime
-    assert len(parsed_uptime.to_list()) == 2
-    assert parsed_uptime.sys_uptime_sec == 5000
-    assert parsed_uptime.hr_sys_uptime_sec == 5005
-
-
-def test_parse_snmp_uptime_pair_with_both_values_but_hr_uptime_is_zero_second_value_is_not_none() -> (
-    None
-):
-    parsed_uptime = if64.parse_snmp_uptime_pair([["500000", "0"]])
-
-    assert parsed_uptime
-    assert len(parsed_uptime.to_list()) == 2
-    assert parsed_uptime.sys_uptime_sec == 5000
-    assert parsed_uptime.hr_sys_uptime_sec == 0