Merge ~hemanth-n/charm-telegraf:lp1903499 into charm-telegraf:master

[Hemanth Nakkina]

OvsDumpFlowsCmdMetric: Use openflow protocol version to dump the flows

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Benjamin Allot (ballot)]

LGTM.

Ran the tests locally with success and looked at the commands and their output on a compute node.

Not top approving as it requires bootstack reviews.

review: Approve

[Peter Sabaini (peter-sabaini)]

Hey, thanks for the contribution.

I have only one minor nit inline.

Would it be possible to get some functional test coverage for this too?

Thank you,
peter.

review: Needs Fixing

Unmerged commits

8cb08fd... by Hemanth Nakkina on 2020-12-06

Fix OvsDumpFlowsCmdMetric to use correct openflow protocol

Currently ovs-ofctl dump the flows without specifying any
openflow protocol which leads to openflow version negotiation
warning messages in ovs-vswitchd daemon.

The fix gets the supported openflow protocol versions and use
the latest one for dump flows command.

Closes: #1903499

diff --git a/src/files/telegraf_exec_metrics.py b/src/files/telegraf_exec_metrics.py
index 7f9bfc3..fe31c81 100755
--- a/src/files/telegraf_exec_metrics.py
+++ b/src/files/telegraf_exec_metrics.py
@@ -360,8 +360,37 @@ class OvsDumpFlowsCmdMetric(CmdMetric):
 
         # 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]
+            # find openflow protocol supported
+            bridge_details_cmd = [
+                "sudo",
+                ovs_vsctl,
+                "-f",
+                "json",
+                "find",
+                "bridge",
+                "name=" + bridge,
+            ]
+            bridge_details = self.get_cmd_output(bridge_details_cmd, is_json=True)
+            protocol_index = bridge_details["headings"].index("protocols")
+            # Ex: br_int_details['data'][0][protocol_index] is in format
+            # ['set', ['OpenFlow13', 'OpenFlow15']]]
+            supported_protocols = bridge_details["data"][0][protocol_index][1]
+
+            if supported_protocols:
+                # pick the latest openflow protocol version
+                cmd = [
+                    "sudo",
+                    ovs_ofctl,
+                    "dump-flows",
+                    "-O",
+                    supported_protocols[-1],
+                    bridge,
+                ]
+            else:
+                cmd = ["sudo", ovs_ofctl, "dump-flows", bridge]
+
             output = self.get_cmd_output(cmd)
             flows[bridge] = output
 
diff --git a/src/tests/unit/test_telegraf.py b/src/tests/unit/test_telegraf.py
index afc3625..d13dedb 100644
--- a/src/tests/unit/test_telegraf.py
+++ b/src/tests/unit/test_telegraf.py
@@ -457,8 +457,12 @@ 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"
+        if cmd == "ovs-vsctl" and args[0][2] == "list-br":
+            return b"br-ex\nbr-data\n"
+        elif cmd == "ovs-vsctl" and args[0][6] == "name=br-ex":
+            return b'{"data":[[["uuid","8267c260-81d5-4a06-848f-711b82d9bf4b"],["set",[]],["set",[]],"000062c26782064a","system","<unknown>",["map",[]],["set",[]],["set",[]],["map",[]],["set",[]],false,["set",[]],"br-ex",["set",[]],["map",[]],["uuid","c1b763d1-a706-46af-98a8-4b4bd0125249"],["set",[]],false,["map",[]],["set",[]],["map",[]],false]],"headings":["_uuid","auto_attach","controller","datapath_id","datapath_type","datapath_version","external_ids","fail_mode","flood_vlans","flow_tables","ipfix","mcast_snooping_enable","mirrors","name","netflow","other_config","ports","protocols","rstp_enable","rstp_status","sflow","status","stp_enable"]}'  # noqa E501
+        elif cmd == "ovs-vsctl" and args[0][6] == "name=br-data":
+            return b'{"data":[[["uuid","8267c260-81d5-4a06-848f-711b82d9bf4c"],["set",[]],["set",[]],"000062c26782064b","system","<unknown>",["map",[]],["set",[]],["set",[]],["map",[]],["set",[]],false,["set",[]],"br-data",["set",[]],["map",[]],["uuid","c1b763d1-a706-46af-98a8-4b4bd0125250"],["set",["OpenFlow13", "OpenFlow15"]],false,["map",[]],["set",[]],["map",[]],false]],"headings":["_uuid","auto_attach","controller","datapath_id","datapath_type","datapath_version","external_ids","fail_mode","flood_vlans","flow_tables","ipfix","mcast_snooping_enable","mirrors","name","netflow","other_config","ports","protocols","rstp_enable","rstp_status","sflow","status","stp_enable"]}'  # noqa E501
         elif (
             cmd == "ovs-ofctl" and args[0][3] == "br-ex"
         ):  # sudo ovs-ofctl dump-flows br-ex
@@ -467,6 +471,14 @@ def test_ovs_dump_flows_cmd_metric(monkeypatch):
                 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
             )
+        elif (
+            cmd == "ovs-ofctl" and args[0][5] == "br-data"
+        ):  # sudo ovs-ofctl dump-flows br-data
+            return (
+                b"NXST_FLOW reply (xid=0x4):\n"
+                b' cookie=0x3711311ec90146a0, duration=247966.151s, table=0, n_packets=0, n_bytes=0, priority=2,in_port="phy-br-data" actions=drop\n'  # noqa E501
+                b" cookie=0x3711311ec90146a0, 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))
 
@@ -479,7 +491,10 @@ def test_ovs_dump_flows_cmd_metric(monkeypatch):
 
     raw_output = metric.get_input_content()
     parsed_output = metric.parse(raw_output)
-    expected_output = [{"bridge": "br-ex", "count": 2}]
+    expected_output = [
+        {"bridge": "br-ex", "count": 2},
+        {"bridge": "br-data", "count": 2},
+    ]
     assert parsed_output == expected_output