load nokia netconf counters, simple unit test (fixed merge conflicts)

brian_polling_manager/interface_stats/cli.py

@@ -12,7 +12,7 @@ from brian_polling_manager.interface_stats.services import (
     write_points_to_influx,
     write_points_to_stdout,
 )
-from brian_polling_manager.interface_stats.vendors import Vendor, juniper
+from brian_polling_manager.interface_stats.vendors import Vendor, juniper, nokia
 from brian_polling_manager.inventory import load_interfaces
 
 from . import config
@@ -46,6 +46,7 @@ LOGGING_DEFAULT_CONFIG = {
     "root": {"level": "INFO", "handlers": ["console"]},
 }
 
+# TODO: (smell) this makes the methods that use it stateful/non-functional (ER)
 _APP_CONFIG_PARAMS = {}
 
 
@@ -83,10 +84,16 @@ def write_points(points: Iterable[dict], influx_params: dict, **kwargs):
 def get_netconf(router_name, vendor=Vendor.JUNIPER, **kwargs):
     source_dir = _APP_CONFIG_PARAMS.get("testing", {}).get("netconf-source-dir")
     if source_dir:
-        return juniper.get_netconf_interface_info_from_source_dir(router_name, source_dir)
+        if vendor == Vendor.JUNIPER:
+            return juniper.get_netconf_interface_info_from_source_dir(router_name, source_dir)
+        else:
+            return nokia.get_netconf_interface_info_from_source_dir(router_name, source_dir)
 
     ssh_params = _APP_CONFIG_PARAMS[vendor.value]
-    return juniper.get_netconf_interface_info(router_name, ssh_params, **kwargs)
+    if vendor == Vendor.JUNIPER:
+        return juniper.get_netconf_interface_info(router_name, ssh_params, **kwargs)
+    else:
+        return nokia.get_netconf_interface_info(router_name, ssh_params, **kwargs)
 
 
 def validate_router_hosts(

brian_polling_manager/interface_stats/vendors/common.py

+import logging
 from functools import partial
 
+logger = logging.getLogger(__name__)
 
 PHYSICAL_INTERFACE_COUNTER_SCHEMA = {
     "$schema": "https://json-schema.org/draft/2020-12/schema",
@@ -113,6 +115,61 @@ BRIAN_POINT_FIELDS_SCHEMA = {
     "additionalProperties": False,
 }
 
+# TODO (POL1-798): unify the counter schemas
+NOKIA_INTERFACE_COUNTER_SCHEMA = {
+    "$schema": "https://json-schema.org/draft/2020-12/schema",
+    "definitions": {
+        "brian-counters": {
+            "type": "object",
+            "properties": {
+                "ingressOctets": {"type": "integer"},
+                "ingressPackets": {"type": "integer"},
+                "egressOctets": {"type": "integer"},
+                "egressPackets": {"type": "integer"},
+                "ingressErrors": {"type": "integer"},
+                "egressErrors": {"type": "integer"},
+                "ingressDiscards": {"type": "integer"},
+                "egressDiscards": {"type": "integer"},
+            },
+            "required": [
+                "ingressOctets",
+                "ingressPackets",
+                "egressOctets",
+                "egressPackets",
+                "ingressErrors",
+                "egressErrors",
+                "ingressDiscards",
+                "egressDiscards",
+            ],
+            "additionalProperties": False,
+        },
+        "error-counters": {
+            "type": "object",
+            "properties": {
+                "input_discards": {"type": "integer"},
+                "output_discards": {"type": "integer"},
+                "output_total_errors": {"type": "integer"},
+            },
+            "required": [
+                "input_discards",
+                "output_discards",
+                "output_total_errors",
+            ],
+            "additionalProperties": False,
+
+            "additionalProperties": False,
+        },
+    },
+    "type": "object",
+    "properties": {
+        "name": {"type": "string"},
+        "brian": {"$ref": "#/definitions/brian-counters"},
+        "errors": {"$ref": "#/definitions/error-counters"},
+    },
+    "required": ["name", "brian"],
+    "additionalProperties": False,
+}
+
 
 ERROR_POINT_FIELDS_SCHEMA = {
     "$schema": "https://json-schema.org/draft/2020-12/schema",
@@ -205,3 +262,46 @@ def error_points(router_fqdn, interfaces, timestamp, measurement_name):
         required=False,
     )
     return filter(lambda r: r is not None, map(point_creator, interfaces))
+
+
+def _read_counter(node, path, transform, required=False):
+    if isinstance(path, str):
+        path = [path]
+    for p in path:
+        _elems = node.xpath(p + "/text()")
+        if _elems:
+            if len(_elems) > 1:
+                logger.warning(f"found more than one {p} in {node}")
+            return transform(_elems[0])
+
+    if required:
+        logger.error(f"required path {p} not found in {node}")
+
+
+def parse_interface_xml(node, struct: dict, defaults=None):
+    """
+    struct should be one of:
+        juniper.PHYSICAL_INTERFACE_COUNTERS
+        juniper.LOGICAL_INTERFACE_COUNTERS
+        nokia.PORT_COUNTERS
+        nokia.LAG_COUNTERS
+
+    :param node: an lxml.etree.Element
+    :param struct: one of the dicts listed above
+    :param defaults:
+    :return:
+    """
+    defaults = struct.get("__defaults__", defaults) or {}
+    result = {}
+    for key, val in struct.items():
+        if key == "__defaults__":
+            continue
+        if "path" in val:
+            parsed = _read_counter(node, **{**defaults, **val})
+        else:
+            parsed = parse_interface_xml(node, val, defaults)
+        if parsed is None:
+            continue
+        result[key] = parsed
+    return result or None
+

brian_polling_manager/interface_stats/vendors/juniper.py

@@ -6,6 +6,8 @@ import ncclient.manager
 from ncclient.devices.junos import JunosDeviceHandler
 from ncclient.xml_ import NCElement
 
+from brian_polling_manager.interface_stats.vendors.common import parse_interface_xml
+
 logger = logging.getLogger(__name__)
 
 DEFAULT_NETCONF_PORT = 830
@@ -140,7 +142,7 @@ def _physical_interface_counters(ifc_doc):
         ' and normalize-space(oper-status)="up"'
         "]"
     ):
-        result = _parse_interface_xml(phy, PHYSICAL_INTERFACE_COUNTERS)
+        result = parse_interface_xml(phy, PHYSICAL_INTERFACE_COUNTERS)
         assert result
         yield result
 
@@ -158,41 +160,11 @@ def _logical_interface_counters(ifc_doc):
         ' and normalize-space(oper-status)="up"'
         "]/logical-interface"
     ):
-        result = _parse_interface_xml(logical, LOGICAL_INTERFACE_COUNTERS)
+        result = parse_interface_xml(logical, LOGICAL_INTERFACE_COUNTERS)
         assert result
         yield result
 
 
-def _parse_interface_xml(node, struct: dict, defaults=None):
-    defaults = struct.get("__defaults__", defaults) or {}
-    result = {}
-    for key, val in struct.items():
-        if key == "__defaults__":
-            continue
-        if "path" in val:
-            parsed = _read_counter(node, **{**defaults, **val})
-        else:
-            parsed = _parse_interface_xml(node, val, defaults)
-        if parsed is None:
-            continue
-        result[key] = parsed
-    return result or None
-
-
-def _read_counter(node, path, transform, required=False):
-    if isinstance(path, str):
-        path = [path]
-    for p in path:
-        _elems = node.xpath(p + "/text()")
-        if _elems:
-            if len(_elems) > 1:
-                logger.warning(f"found more than one {p} in {node}")
-            return transform(_elems[0])
-
-    if required:
-        logger.error(f"required path {p} not found in {node}")
-
-
 def interface_counters(ifc_doc):
     yield from _physical_interface_counters(ifc_doc)
 
@@ -254,9 +226,9 @@ def _rpc(
 
 
 def get_netconf_interface_info_from_source_dir(
-    router_name: str, source_dir: str, suffix="-interface-info.xml"
+    router_name: str, source_dir: str
 ):
-    file = pathlib.Path(source_dir) / f"{router_name}{suffix}"
+    file = pathlib.Path(source_dir) / f"{router_name}-interface-info.xml"
     if not file.is_file():
         raise ValueError(f"file {file} is not a valid file")
     return etree.fromstring(file.read_text())

brian_polling_manager/interface_stats/vendors/nokia.py

@@ -3,12 +3,15 @@
 """
 import contextlib
 import logging
+import pathlib
 import time
 import typing
 
 from lxml import etree
 import ncclient.manager
 
+from brian_polling_manager.interface_stats.vendors.common import parse_interface_xml
+
 if typing.TYPE_CHECKING:
     from ncclient.lxml_ import NCElement
 
@@ -82,9 +85,114 @@ def get_netconf_interface_info(router_name: str, ssh_params: dict):
         return rsp
 
 
-def interface_counters(raw_counter_docs):
+def get_netconf_interface_info_from_source_dir(
+    router_name: str, source_dir: str
+):
+    file = pathlib.Path(source_dir) / f"{router_name}-ports.xml"
+    if not file.is_file():
+        raise ValueError(f"file {file} is not a valid file")
+    ports_doc = etree.fromstring(file.read_text())
+
+    file = pathlib.Path(source_dir) / f"{router_name}-lags.xml"
+    if not file.is_file():
+        raise ValueError(f"file {file} is not a valid file")
+    lags_doc = etree.fromstring(file.read_text())
+
+    return {
+        'port': ports_doc,
+        'lag': lags_doc
+    }
+
+
+def _counter_parse_config(name_tag: str):
+    """
+    the stats info for both port and lag are the same, only the
+    name we use for reference is different, so the parsing is nearly
+    identical
+
+    :param name_tag: either 'port-id' or 'lag-name'
+    :return:
+    """
+    # sanity: special-purpose method for only these 2 cases
+    assert name_tag in ('port-id', 'lag-name')
+
+    return {
+        "__defaults__": {"transform": int, "required": False},
+        "name": {"path": f"./{name_tag}", "transform": lambda v: str(v).strip()},
+        "brian": {
+            "ingressOctets": {
+                "path": "./statistics/in-octets",
+                "required": True
+            },
+            "ingressPackets": {
+                "path": "./statistics/in-packets",
+                "required": True,
+            },
+            "egressOctets": {
+                "path": "./statistics/out-octets",
+                "required": True
+            },
+            "egressPackets": {
+                "path": "./statistics/out-packets",
+                "required": True
+            },
+            "ingressErrors": {"path": "./statistics/in-errors"},
+            "egressErrors": {"path": "./statistics/out-errors"},
+            "ingressDiscards": {"path": "./statistics/in-discards"},
+            "egressDiscards": {"path": "./statistics/out-discards"},
+        },
+        "errors": {
+            "output_total_errors": {"path": "./statistics/out-errors"},
+            "input_discards": {"path": "./statistics/in-discards"},
+            "output_discards": {"path": "./statistics/out-discards"},
+        },
+    }
+
+
+def _port_counters(state_doc: etree.Element):
+    """
+    :param state_doc: /nokia-state:state/port yang node
+    :return: counters for all monitored ports
+    """
+
+    parse_config = _counter_parse_config('port-id')
+
+    for port in state_doc.xpath(
+        "//data/state/port["
+        ' normalize-space(oper-state)="up"'
+        "]"
+    ):
+        result = parse_interface_xml(port, parse_config)
+        assert result
+        yield result
+
+
+def _lag_counters(state_doc: etree.Element):
+    """
+    :param state_doc: /nokia-state:state/lag yang node
+    :return: counters for all monitored lags
+    """
+
+    parse_config = _counter_parse_config('lag-name')
+
+    for port in state_doc.xpath(
+        "//data/state/lag["
+        ' normalize-space(oper-state)="up"'
+        "]"
+    ):
+        result = parse_interface_xml(port, parse_config)
+        assert result
+        yield result
+
+
+def interface_counters(raw_counter_docs: dict):
+    """
 
-    assert False, "TODO"
+    :param raw_counter_docs: output of a call to get_netconf_interface_info
+    :return: iterable of interface counters
+    """
+    yield from _port_counters(raw_counter_docs['port'])
+    yield from _lag_counters(raw_counter_docs['lag'])
 
 
 if __name__ == '__main__':

test/interface_stats/conftest.py

@@ -5,14 +5,21 @@ import pytest
 
 
 DATA_DIR = pathlib.Path(__file__).parent / "data"
-DATA_FILENAME_EXTENSION = "-interface-info.xml"
+JUNIPER_DATA_FILENAME_EXTENSION = "-interface-info.xml"
 
 JUNIPER_ROUTERS = [
-    path.name[: -len(DATA_FILENAME_EXTENSION)]
+    path.name[: -len(JUNIPER_DATA_FILENAME_EXTENSION)]
     for path in DATA_DIR.iterdir()
-    if path.name.endswith(DATA_FILENAME_EXTENSION)
+    if path.name.endswith(JUNIPER_DATA_FILENAME_EXTENSION)
 ]
 
+NOKIA_ROUTERS = list({
+    path.name[: -len(suffix)]
+    for suffix in {"-ports.xml", "-lags.xml"}
+    for path in DATA_DIR.iterdir()
+    if path.name.endswith(suffix)
+})
+
 
 @pytest.fixture
 def data_dir():
@@ -21,6 +28,12 @@ def data_dir():
 
 @pytest.fixture(autouse=True)
 def app_params():
+    """
+    ER: I think this is a smell, putting special-purpose code
+     in the production release that runs iff. a condition path
+     is not taken that runs in test
+     mocking isn't an anti-pattern, and "explicit is better than implicit"
+    """
     params = {
         "testing": {
             "dry_run": True,
@@ -52,11 +65,21 @@ def all_juniper_routers():
     return JUNIPER_ROUTERS
 
 
+@pytest.fixture
+def all_nokia_routers():
+    return NOKIA_ROUTERS
+
+
 @pytest.fixture(params=JUNIPER_ROUTERS)
-def router_fqdn(request):
+def juniper_router_fqdn(request):
     return request.param
 
 
+
+@pytest.fixture(params=NOKIA_ROUTERS)
+def nokia_router_fqdn(request):
+    return request.param
+
 @pytest.fixture()
 def single_router_fqdn():
     return JUNIPER_ROUTERS[0]

test/interface_stats/test_interface_stats.py

@@ -6,10 +6,18 @@ from unittest.mock import MagicMock, Mock, call, patch
 import jsonschema
 import pytest
 from brian_polling_manager.interface_stats import cli, services
-from brian_polling_manager.interface_stats.vendors import Vendor, common, juniper
+from brian_polling_manager.interface_stats.vendors import Vendor, common, juniper, nokia
 from lxml import etree
 from ncclient.operations.rpc import RPCReply
 
+"""
+@pelle: please restore the tests where all router snapshots are tested
+        at all levels, with the various schema validations - explicitly
+        to aid in future debugging as router configs change
+        ... we want to sanity check real-life router configurations, and to
+            have a convenient way of quickly replacing the test data with
+            fresh router configs to know if our code breaks
+"""
 
 def test_sanity_check_snapshot_data(polled_interfaces, all_juniper_routers):
     """
@@ -29,7 +37,11 @@ def test_verify_all_interfaces_present(single_router_fqdn, polled_interfaces):
     a snapshot of inventory /poller/interfaces
     (the snapshots were all taken around the same time)
     """
-
+    """
+    @pelle: please re-instate the running of this test
+            over all test data sets ... the point of the test
+            is a sanity check of our test data
+    """
     def _is_enabled(ifc_name, ifc_doc):
         m = re.match(r"^([^\.]+)\.?.*", ifc_name)
         assert m  # sanity: should never fail
@@ -58,19 +70,19 @@ class TestParseCounters:
     def test_parse_counters(self):
         xml = """<root><ab>42</ab></root>"""
         struct = {"something": {"path": "./ab", "transform": int}}
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result == {"something": 42}
 
     def test_parse_counters_multiple_path(self):
         xml = """<root><a>This is something</a></root>"""
         struct = {"something": {"path": ["./b", "./a"], "transform": str}}
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result == {"something": "This is something"}
 
     def test_parse_counters_nested(self):
         xml = """<root><a>This is something</a></root>"""
         struct = {"something": {"nested": {"path": "./a", "transform": str}}}
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result == {"something": {"nested": "This is something"}}
 
     def test_skips_unavailable_field(self):
@@ -79,7 +91,7 @@ class TestParseCounters:
             "something": {"path": "./a", "transform": str},
             "something_else": {"nested": {"path": "./b", "transform": str}},
         }
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result == {"something": "This is something"}
 
     def test_logs_on_missing_required_field(self, caplog):
@@ -87,7 +99,7 @@ class TestParseCounters:
         struct = {
             "something": {"path": "./a", "transform": str, "required": True},
         }
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result is None
         record = caplog.records[0]
         assert record.levelname == "ERROR"
@@ -98,7 +110,7 @@ class TestParseCounters:
         struct = {
             "something": {"path": "./a", "transform": str},
         }
-        result = juniper._parse_interface_xml(etree.fromstring(xml), struct)
+        result = common.parse_interface_xml(etree.fromstring(xml), struct)
         assert result == {"something": "This is something"}
         record = caplog.records[0]
         assert record.levelname == "WARNING"
@@ -231,18 +243,16 @@ def test_logical_interface_counters(juniper_router_doc_containing_every_field):
     }
 
 
-@pytest.fixture(
-    params=[juniper._physical_interface_counters, juniper._logical_interface_counters]
-)
-def generate_interface_counters(request):
-    return request.param
+def test_juniper_router_docs_do_not_generate_errors(juniper_router_fqdn, caplog):
+    doc = cli.get_netconf(juniper_router_fqdn, ssh_params=None, vendor=Vendor.JUNIPER)
+    counters = list(juniper.interface_counters(doc))
+    assert counters
+    assert not [r for r in caplog.records if r.levelname in ("ERROR", "WARNING")]
 
 
-def test_router_docs_do_not_generate_errors(
-    router_fqdn, generate_interface_counters, caplog
-):
-    doc = cli.get_netconf(router_fqdn, ssh_params=None)
-    counters = list(generate_interface_counters(doc))
+def test_nokia_router_docs_do_not_generate_errors(nokia_router_fqdn, caplog):
+    doc = cli.get_netconf(nokia_router_fqdn, ssh_params=None, vendor=Vendor.NOKIA)
+    counters = list(nokia.interface_counters(doc))
     assert counters
     assert not [r for r in caplog.records if r.levelname in ("ERROR", "WARNING")]
 
@@ -254,6 +264,11 @@ def test_router_docs_do_not_generate_errors(
     ]
 )
 def generate_points_with_schema(request):
+    """
+    @pelle: was this fixture meant to be used?
+      ... it appears that all tests that verify robust generated data
+          validates against the 'point' schemas have been removed
+    """
     return request.param
 
 
@@ -366,6 +381,10 @@ def test_main_for_all_juniper_routers(write_points, all_juniper_routers):
         vendor=Vendor.JUNIPER,
         raise_errors=True,
     )
+    """
+    @pelle: this is not a maintainable pattern (e.g. POL1-799 or similar things)
+      ... please do something nicer (actually > a few calls/points is enough)
+    """
     assert calls == 104
     assert total_points == 6819
 
@@ -441,6 +460,9 @@ def test_doesnt_validate_without_inprov_hosts(load_interfaces):
 
 
 def test_write_points_to_influx():
+    """
+    @pelle: this is broken ... the factory construct has been removed
+    """
     cli.set_app_params({})
     influx_factory = MagicMock()
     points = [{"point": "one"}, {"point": "two"}]
@@ -496,3 +518,20 @@ def test_prepare_influx_params(input_params, expected):
     defaults = dict(database="counters", username="user", password="pass", timeout=5)
     result = services.prepare_influx_params({**defaults, **input_params})
     assert result == {**defaults, **expected}
+
+
+@pytest.mark.parametrize(
+    "hostname",
+    [ 'rt0.lon.uk.lab.office.geant.net', 'rt0.ams.nl.lab.office.geant.net' ]
+)
+def test_nokia_counters(hostname):
+    """
+    quick poc
+    :return:
+    """
+    doc = cli.get_netconf(hostname, vendor=Vendor.NOKIA)
+    counters = list(nokia.interface_counters(doc))
+    assert counters
+    for counter in counters:
+        jsonschema.validate(counter, common.NOKIA_INTERFACE_COUNTER_SCHEMA)
+    print(len(counters))