Merge ~jfguedez/charm-telegraf:backport-1891781-1892129 into charm-telegraf:stable/20.08

[Jose Guedez]

Backport LP1891781/LP1892129

[Celia Wang (ziyiwang)]

LGTM

review: Approve

[James Hebden (ec0)]

LGTM

review: Approve

diff --git a/src/files/telegraf_exec_metrics.py b/src/files/telegraf_exec_metrics.py
index d9b3402..7f9bfc3 100755
--- a/src/files/telegraf_exec_metrics.py
+++ b/src/files/telegraf_exec_metrics.py
@@ -313,6 +313,12 @@ class NetNSCmdMetric(CmdMetric):
         return items
 
 
+# Openvswitch metric collectors
+# The OVS collectors use the "ovs_" prefix and should not be installed in a non-OVS box
+# the deployment logic will skip them by including them in the "disabled_plugins" list
+# more context in LP1891781
+
+
 @register_metric
 class OvsDumpFlowsCmdMetric(CmdMetric):
     name = "ovs_dump_flows"
@@ -342,15 +348,20 @@ class OvsDumpFlowsCmdMetric(CmdMetric):
         """
         flows = {}
 
-        ovs_cmd = "ovs-ofctl"
-
-        if not shutil.which(ovs_cmd):
-            LOG.error('openvswitch command "%s" is not available, exit', ovs_cmd)
+        ovs_vsctl = "ovs-vsctl"
+        ovs_ofctl = "ovs-ofctl"
 
-            return flows
+        for ovs_cmd in [ovs_vsctl, ovs_ofctl]:
+            if not shutil.which(ovs_cmd):
+                LOG.error(
+                    'openvswitch command "%s" is not available, aborting', ovs_cmd
+                )
+                return flows
 
-        for bridge in ["br-int", "br-tun", "br-data"]:
-            cmd = ["sudo", ovs_cmd, "dump-flows", bridge]
+        # query OVS for the list of bridges
+        bridges = self.get_cmd_output(["sudo", ovs_vsctl, "list-br"]).split()
+        for bridge in bridges:
+            cmd = ["sudo", ovs_ofctl, "dump-flows", bridge]
             output = self.get_cmd_output(cmd)
             flows[bridge] = output
 
diff --git a/src/files/telegraf_sudoers b/src/files/telegraf_sudoers
index d1776b3..6be9f54 100644
--- a/src/files/telegraf_sudoers
+++ b/src/files/telegraf_sudoers
@@ -1,4 +1,5 @@
 Defaults:telegraf !requiretty
 
 telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-ofctl
+telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-vsctl
 telegraf ALL = (root) NOPASSWD: /usr/bin/ovs-appctl
diff --git a/src/reactive/telegraf.py b/src/reactive/telegraf.py
index 4b72eab..058380c 100644
--- a/src/reactive/telegraf.py
+++ b/src/reactive/telegraf.py
@@ -48,12 +48,15 @@ from charms.reactive import (
 )
 from charms.reactive.bus import get_states
 
+from files.telegraf_exec_metrics import METRICS
+
 from jinja2 import Environment, FileSystemLoader, Template, exceptions
 
 import netifaces
 
 import yaml
 
+
 DEB_BASE_DIR = "/etc/telegraf"
 SNAP_BASE_DIR = "/var/snap/telegraf/current"
 SUDOERS_DIR = "/etc/sudoers.d"
@@ -219,7 +222,6 @@ def get_base_inputs():
         "python": sys.executable,
         "files_dir": get_files_dir(),
         "disabled_plugins": disabled_plugins,
-        "openvswitch": host.service_running("openvswitch-switch"),
         "conntrack": conntrack,
     }
 
@@ -502,12 +504,43 @@ def configure_telegraf():  # noqa: C901
 
     render_socket_listener_config(context=context)
 
+    disabled_plugins = config["disabled_plugins"]
+
+    # handle plugin configuration for OVS
+    if host.service_running("openvswitch-switch"):
+        # add sudoers file for telegraf if openvswitch is running
+        sudoers_filename = "telegraf_sudoers"
+        src = os.path.join(get_files_dir(), sudoers_filename)
+        dst = os.path.join(SUDOERS_DIR, sudoers_filename)
+        shutil.copy(src, dst)
+        os.chmod(dst, 0o640)
+    else:
+        # disable the OVS checks if the service is not curently running
+        # no need to handle duplicates here, as those are handled during
+        # template rendering later
+        ovs_metric_names = [
+            metric_name for metric_name in METRICS if metric_name.startswith("ovs_")
+        ]
+
+        hookenv.log(
+            "disabling the following metrics, since OVS is not available: {}".format(
+                ovs_metric_names
+            )
+        )
+
+        ovs_disabled_plugins = ":".join(ovs_metric_names)
+
+        if disabled_plugins:
+            disabled_plugins = disabled_plugins + ":" + ovs_disabled_plugins
+        else:
+            disabled_plugins = ovs_disabled_plugins
+
     telegraf_exec_metrics = os.path.join(get_files_dir(), "telegraf_exec_metrics.py")
     cmd = [
         telegraf_exec_metrics,
         "--render-config-files",
         "--disabled-metrics",
-        config["disabled_plugins"],
+        disabled_plugins,
         "--configs-dir",
         get_configs_dir(),  # unit test can monkeypatch this path
         "--python",
@@ -524,15 +557,6 @@ def configure_telegraf():  # noqa: C901
         context=context,
     )
 
-    # add sudoers file for telegraf if openvswitch is running
-
-    if host.service_running("openvswitch-switch"):
-        sudoers_filename = "telegraf_sudoers"
-        src = os.path.join(get_files_dir(), sudoers_filename)
-        dst = os.path.join(SUDOERS_DIR, sudoers_filename)
-        shutil.copy(src, dst)
-        os.chmod(dst, 0o640)
-
     for service in [DEB_SERVICE, SNAP_SERVICE]:
         if service == get_service():
             host.service_resume(service)
diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
index 57e7e3c..8ef82ec 100644
--- a/src/tests/unit/test_telegraf.py
+++ b/src/tests/unit/test_telegraf.py
@@ -20,6 +20,7 @@ import getpass
 import grp
 import json
 import os
+import shutil
 import subprocess
 import sys
 import unittest
@@ -27,6 +28,7 @@ from textwrap import dedent
 from unittest import mock
 from unittest.mock import MagicMock, call, patch
 
+from charmhelpers.core import host
 from charmhelpers.core.hookenv import Config
 from charmhelpers.core.templating import render
 
@@ -40,11 +42,18 @@ import pytest
 import yaml
 
 # Mock layer modules
+apt = MagicMock()
+layer = MagicMock()
 promreg = MagicMock()
+charms.apt = apt
+charms.layer = layer
 charms.promreg = promreg
+sys.modules["charms.apt"] = apt
+sys.modules["charms.layer"] = layer
 sys.modules["charms.promreg"] = promreg
 
 import reactive  # noqa E402
+from files.telegraf_exec_metrics import METRICS  # noqa E402
 from reactive import telegraf  # noqa E402
 
 UNIT_TESTS_DIR = os.path.dirname(os.path.abspath(__file__))
@@ -165,6 +174,12 @@ def persist_state():
         helpers.data_changed("active_plugins", states)
 
 
+def run_telegraf_exec_metrics(*args):
+    script = os.path.join(telegraf.get_files_dir(), "telegraf_exec_metrics.py")
+    cmd = [script] + list(args)
+    assert 0 == subprocess.call(cmd)
+
+
 # Tests
 
 
@@ -350,11 +365,6 @@ def test_sadc_options_s_missing(monkeypatch):
 
 
 def test_telegraf_exec_metrics(monkeypatch, temp_config_dir):
-    def run_telegraf_exec_metrics(*args):
-        script = os.path.join(telegraf.get_files_dir(), "telegraf_exec_metrics.py")
-        cmd = [script] + list(args)
-        assert 0 == subprocess.call(cmd)
-
     metrics = set(
         ["sockstat", "sockstat6", "softnet_stat", "buddyinfo", "zoneinfo", "netns"]
     )
@@ -402,6 +412,66 @@ def test_telegraf_exec_metrics(monkeypatch, temp_config_dir):
         assert os.path.isfile(os.path.join(configs_dir(), "{}.conf".format(metric)))
 
 
+def test_telegraf_exec_metrics_enable_ovs(monkeypatch, config, temp_config_dir):
+    config["disabled_plugins"] = ""  # ensure no metrics are initially disabled
+    monkeypatch.setattr(telegraf, "check_prometheus_port", lambda a, b: None)
+    monkeypatch.setattr(host, "service_running", lambda svc: True)
+    telegraf.configure_telegraf()  # this will generate the check definitions
+
+    # get the names of all the OVS metrics
+    ovs_metric_names = [
+        metric_name for metric_name in METRICS if metric_name.startswith("ovs_")
+    ]
+
+    # check that the check definitions for the OVS checks are in place
+    for metric in ovs_metric_names:
+        assert os.path.isfile(os.path.join(configs_dir(), "{}.conf".format(metric)))
+
+
+def test_telegraf_exec_metrics_disable_ovs(monkeypatch, config):
+    def mock_check_call(*args, **kwargs):
+        disabled_metrics = args[0][3]
+        assert disabled_metrics  # this should not be empty
+        for metric in disabled_metrics.split(":"):
+            assert metric.startswith("ovs_")
+
+    config["disabled_plugins"] = ""  # ensure no metrics are initially disabled
+    monkeypatch.setattr(telegraf, "check_prometheus_port", lambda a, b: None)
+    monkeypatch.setattr(host, "service_running", lambda svc: False)
+    monkeypatch.setattr(subprocess, "check_call", mock_check_call)
+    telegraf.configure_telegraf()
+
+
+def test_ovs_dump_flows_cmd_metric(monkeypatch):
+    def mock_check_output(*args, **kwargs):
+        """Mock the subprocess calls to the OVS tools."""
+        cmd = args[0][1]
+        if cmd == "ovs-vsctl":
+            return b"br-ex\n"
+        elif (
+            cmd == "ovs-ofctl" and args[0][3] == "br-ex"
+        ):  # sudo ovs-ofctl dump-flows br-ex
+            return (
+                b"NXST_FLOW reply (xid=0x4):\n"
+                b' cookie=0x3711311ec90146a9, duration=247966.151s, table=0, n_packets=0, n_bytes=0, priority=2,in_port="phy-br-ex" actions=drop\n'  # noqa E501
+                b" cookie=0x3711311ec90146a9, duration=247966.175s, table=0, n_packets=2192, n_bytes=156148, priority=0 actions=NORMAL\n"  # noqa E501
+            )
+        else:
+            raise Exception("unexpected argument: {}".format(args))
+
+    monkeypatch.setattr(subprocess, "check_output", mock_check_output)
+    monkeypatch.setattr(shutil, "which", lambda path: True)
+
+    from files.telegraf_exec_metrics import OvsDumpFlowsCmdMetric
+
+    metric = OvsDumpFlowsCmdMetric()
+
+    raw_output = metric.get_input_content()
+    parsed_output = metric.parse(raw_output)
+    expected_output = [{"bridge": "br-ex", "count": 2}]
+    assert parsed_output == expected_output
+
+
 def test_get_remote_unit_name_juju2(monkeypatch):
     monkeypatch.undo()
     monkeypatch.setitem(os.environ, "JUJU_PRINCIPAL_UNIT", "remote/0")