diff mbox series

[ovs-dev,v1,5/6] controller: only sample flow if Collector Set exists

Message ID 20230124151632.695912-6-amorenoz@redhat.com
State Accepted
Headers show
Series drop sampling: Fixes and optimizations | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Adrian Moreno Jan. 24, 2023, 3:16 p.m. UTC
From: Adrian Moreno <amorenoz@redhat.com>

Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental
in terms of performance) if a Flow_Sample_Collector_Set row does not
exist with the correspondent id. The sample (i.e: upcall) would take
place but ovs-vswitchd would not have a target IPFIX collector to send
the sample to.

In order to be able to control what Chassis perform sampling without
incurring in any performance cost if not needed, this patch makes the
controller only encode the OFPACT_SAMPLE if there's a valid
Flow_Sample_Collector_Set configured on that chassis.

In order to do that, a new input is added to the en_lflow_output engine
node that is associated with the OVSDB table. But since the information
has to be available in the lib/actions layer, an intermediate helper
struct is added (in lib/ovn-util.{h,c}) to help store and query the
configured collector set ids instead of using the idl directly.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 controller/lflow.c          |  1 +
 controller/lflow.h          |  8 ++++--
 controller/ovn-controller.c | 56 ++++++++++++++++++++++++++++++++++++-
 include/ovn/actions.h       |  4 +++
 lib/actions.c               |  9 +++++-
 lib/ovn-util.c              | 51 +++++++++++++++++++++++++++++++++
 lib/ovn-util.h              | 26 ++++++++++++++---
 tests/ovn-performance.at    | 24 ++++++++++++++++
 tests/ovn.at                |  8 ++++++
 tests/test-ovn.c            |  9 ++++++
 10 files changed, 187 insertions(+), 9 deletions(-)
diff mbox series

Patch

diff --git a/controller/lflow.c b/controller/lflow.c
index 4b1cfe318..08ce0386f 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -911,6 +911,7 @@  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
         .is_switch = ldp->is_switch,
         .group_table = l_ctx_out->group_table,
         .meter_table = l_ctx_out->meter_table,
+        .collector_ids = l_ctx_in->collector_ids,
         .lflow_uuid = lflow->header_.uuid,
         .dp_key = ldp->datapath->tunnel_key,
 
diff --git a/controller/lflow.h b/controller/lflow.h
index 9e8f9afd3..9bb61c039 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -43,11 +43,12 @@ 
 #include "openvswitch/uuid.h"
 #include "openvswitch/list.h"
 
-struct ovn_extend_table;
-struct ovsdb_idl_index;
-struct ovn_desired_flow_table;
 struct hmap;
 struct hmap_node;
+struct ovn_desired_flow_table;
+struct ovn_extend_table;
+struct ovsdb_idl_index;
+struct ovsrec_flow_sample_collector_set_table;
 struct sbrec_chassis;
 struct sbrec_load_balancer;
 struct sbrec_logical_flow_table;
@@ -114,6 +115,7 @@  struct lflow_ctx_in {
     const struct hmap *dhcpv6_opts;
     const struct controller_event_options *controller_event_opts;
     const struct smap *template_vars;
+    const struct flow_collector_ids *collector_ids;
     bool lb_hairpin_use_ct_mark;
 };
 
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index f20972c76..088389a54 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1047,6 +1047,8 @@  ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_column(ovs_idl, &ovsrec_ssl_col_private_key);
     ovsdb_idl_add_table(ovs_idl, &ovsrec_table_datapath);
     ovsdb_idl_add_column(ovs_idl, &ovsrec_datapath_col_capabilities);
+    ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_sample_collector_set);
+
     chassis_register_ovs_idl(ovs_idl);
     encaps_register_ovs_idl(ovs_idl);
     binding_register_ovs_idl(ovs_idl);
@@ -1063,6 +1065,10 @@  ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_name);
     ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_interfaces);
     ovsdb_idl_track_add_column(ovs_idl, &ovsrec_port_col_external_ids);
+    ovsdb_idl_track_add_column(ovs_idl,
+                               &ovsrec_flow_sample_collector_set_col_bridge);
+    ovsdb_idl_track_add_column(ovs_idl,
+                               &ovsrec_flow_sample_collector_set_col_id);
     mirror_register_ovs_idl(ovs_idl);
 }
 
@@ -1102,7 +1108,8 @@  enum sb_engine_node {
     OVS_NODE(bridge, "bridge") \
     OVS_NODE(port, "port") \
     OVS_NODE(interface, "interface") \
-    OVS_NODE(qos, "qos")
+    OVS_NODE(qos, "qos") \
+    OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set")
 
 enum ovs_engine_node {
 #define OVS_NODE(NAME, NAME_STR) OVS_##NAME,
@@ -2873,6 +2880,9 @@  struct ed_type_lflow_output {
 
     /* Fixed controller_event supported options. */
     struct controller_event_options controller_event_opts;
+
+    /* Configured Flow Sample Collector Sets. */
+    struct flow_collector_ids collector_ids;
 };
 
 static void
@@ -3011,6 +3021,7 @@  init_lflow_ctx(struct engine_node *node,
     l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts;
     l_ctx_in->controller_event_opts = &fo->controller_event_opts;
     l_ctx_in->template_vars = &template_vars->local_templates;
+    l_ctx_in->collector_ids = &fo->collector_ids;
 
     l_ctx_out->flow_table = &fo->flow_table;
     l_ctx_out->group_table = &fo->group_table;
@@ -3040,6 +3051,7 @@  en_lflow_output_init(struct engine_node *node OVS_UNUSED,
     data->hd.pool = id_pool_create(1, UINT32_MAX - 1);
     nd_ra_opts_init(&data->nd_ra_opts);
     controller_event_opts_init(&data->controller_event_opts);
+    flow_collector_ids_init(&data->collector_ids);
     return data;
 }
 
@@ -3066,6 +3078,7 @@  en_lflow_output_cleanup(void *data)
     id_pool_destroy(flow_output_data->hd.pool);
     nd_ra_opts_destroy(&flow_output_data->nd_ra_opts);
     controller_event_opts_destroy(&flow_output_data->controller_event_opts);
+    flow_collector_ids_destroy(&flow_output_data->collector_ids);
 }
 
 static void
@@ -3135,6 +3148,45 @@  lflow_output_sb_logical_flow_handler(struct engine_node *node, void *data)
     return handled;
 }
 
+static bool
+lflow_output_flow_sample_collector_set_handler(struct engine_node *node,
+                                               void *data OVS_UNUSED)
+{
+    const struct ovsrec_flow_sample_collector_set_table *flow_collector_table =
+        EN_OVSDB_GET(engine_get_input("OVS_flow_sample_collector_set", node));
+    const struct ovsrec_open_vswitch_table *ovs_table =
+        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
+    const struct ovsrec_bridge_table *bridge_table =
+        EN_OVSDB_GET(engine_get_input("OVS_bridge", node));
+
+    const struct ovsrec_open_vswitch *cfg =
+        ovsrec_open_vswitch_table_first(ovs_table);
+    if (!cfg) {
+        return true;
+    }
+
+    const struct ovsrec_bridge *br_int;
+    br_int = get_bridge(bridge_table, br_int_name(cfg));
+    if (!br_int) {
+        return true;
+    }
+
+    const struct ovsrec_flow_sample_collector_set *set;
+    OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH_TRACKED (set,
+                                                        flow_collector_table) {
+        if (set->bridge == br_int) {
+            struct ed_type_lflow_output *lfo = data;
+            flow_collector_ids_clear(&lfo->collector_ids);
+            flow_collector_ids_init_from_table(&lfo->collector_ids,
+                                               flow_collector_table);
+            return false;
+        }
+    }
+
+    engine_set_node_state(node, EN_UPDATED);
+    return true;
+}
+
 static bool
 lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data)
 {
@@ -4250,6 +4302,8 @@  main(int argc, char *argv[])
 
     engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
     engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
+    engine_add_input(&en_lflow_output, &en_ovs_flow_sample_collector_set,
+                     lflow_output_flow_sample_collector_set_handler);
 
     engine_add_input(&en_lflow_output, &en_sb_mac_binding,
                      lflow_output_sb_mac_binding_handler);
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 6ca08b3c6..28479ede1 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -33,6 +33,7 @@  struct ofpbuf;
 struct shash;
 struct simap;
 struct ovn_extend_table;
+struct collector_set_ids;
 
 /* List of OVN logical actions.
  *
@@ -831,6 +832,9 @@  struct ovnact_encode_params {
     /* A struct to figure out the meter_id for meter actions. */
     struct ovn_extend_table *meter_table;
 
+    /* A struct to lookup Flow_Sample_Collector_Set ids */
+    const struct flow_collector_ids *collector_ids;
+
     /* The logical flow uuid that drove this action. */
     struct uuid lflow_uuid;
 
diff --git a/lib/actions.c b/lib/actions.c
index 2da5a696b..781549d75 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -4384,7 +4384,14 @@  encode_SAMPLE(const struct ovnact_sample *sample,
               const struct ovnact_encode_params *ep,
               struct ofpbuf *ofpacts)
 {
-    struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts);
+    struct ofpact_sample *os;
+    if (!ep->collector_ids ||
+        !flow_collector_ids_lookup(ep->collector_ids,
+                                   sample->collector_set_id)) {
+        return;
+    }
+
+    os = ofpact_put_SAMPLE(ofpacts);
     os->probability = sample->probability;
     os->collector_set_id = sample->collector_set_id;
     os->obs_domain_id =
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index f3665b89f..b12c027b9 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -1044,3 +1044,54 @@  get_chassis_external_id_value_bool(const struct smap *external_ids,
     free(chassis_key);
     return ret;
 }
+
+void flow_collector_ids_init(struct flow_collector_ids *ids)
+{
+    ovs_list_init(&ids->list);
+}
+
+void flow_collector_ids_init_from_table(struct flow_collector_ids *ids,
+    const struct ovsrec_flow_sample_collector_set_table *table)
+{
+    flow_collector_ids_init(ids);
+    const struct ovsrec_flow_sample_collector_set *ovs_collector_set;
+    OVSREC_FLOW_SAMPLE_COLLECTOR_SET_TABLE_FOR_EACH (ovs_collector_set,
+                                                    table) {
+        flow_collector_ids_add(ids, ovs_collector_set->id);
+    }
+}
+
+void flow_collector_ids_add(struct flow_collector_ids *ids,
+                            uint64_t id)
+{
+    struct flow_collector_id *collector = xzalloc(sizeof *collector);
+    collector->id = id;
+    ovs_list_push_back(&ids->list, &collector->node);
+}
+
+bool flow_collector_ids_lookup(const struct flow_collector_ids *ids,
+                               uint32_t id)
+{
+    struct flow_collector_id *collector;
+    LIST_FOR_EACH (collector, node, &ids->list) {
+        if (collector->id == id) {
+          return true;
+        }
+    }
+    return false;
+}
+
+void flow_collector_ids_destroy(struct flow_collector_ids *ids)
+{
+    struct flow_collector_id *collector;
+    LIST_FOR_EACH_SAFE (collector, node, &ids->list) {
+        ovs_list_remove(&collector->node);
+        free(collector);
+    }
+}
+
+void flow_collector_ids_clear(struct flow_collector_ids *ids)
+{
+    flow_collector_ids_destroy(ids);
+    flow_collector_ids_init(ids);
+}
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index cd19919cb..88e745369 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -28,15 +28,16 @@ 
 #define ROUTE_ORIGIN_CONNECTED "connected"
 #define ROUTE_ORIGIN_STATIC "static"
 
+struct eth_addr;
 struct nbrec_logical_router_port;
+struct ovsrec_flow_sample_collector_set_table;
+struct sbrec_datapath_binding;
 struct sbrec_logical_flow;
+struct sbrec_port_binding;
+struct smap;
 struct svec;
 struct uuid;
-struct eth_addr;
-struct sbrec_port_binding;
-struct sbrec_datapath_binding;
 struct unixctl_conn;
-struct smap;
 
 struct ipv4_netaddr {
     ovs_be32 addr;            /* 192.168.10.123 */
@@ -344,4 +345,21 @@  get_chassis_external_id_value_bool(const struct smap *,
                                    const char *option_key,
                                    bool def);
 
+/* flow_collector_ids is a helper struct used to store and lookup
+ * Flow_Sample_Collector_Set ids. */
+struct flow_collector_id {
+    struct ovs_list node;  /* In flow_collector_ids->list */
+    uint64_t id;
+};
+struct flow_collector_ids {
+    struct ovs_list list;
+};
+void flow_collector_ids_init(struct flow_collector_ids *);
+void flow_collector_ids_init_from_table(struct flow_collector_ids *,
+    const struct ovsrec_flow_sample_collector_set_table *);
+void flow_collector_ids_add(struct flow_collector_ids *, uint64_t);
+bool flow_collector_ids_lookup(const struct flow_collector_ids *, uint32_t);
+void flow_collector_ids_destroy(struct flow_collector_ids *);
+void flow_collector_ids_clear(struct flow_collector_ids *);
+
 #endif /* OVN_UTIL_H */
diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
index 125c0141a..8ac0a392c 100644
--- a/tests/ovn-performance.at
+++ b/tests/ovn-performance.at
@@ -586,6 +586,30 @@  OVN_CONTROLLER_EXPECT_NO_HIT(
     [ovn-nbctl --wait=hv meter-del meter0]
 )
 
+OVN_CONTROLLER_EXPECT_HIT(
+    [hv1], [lflow_run],
+    [as hv1 ovs-vsctl --id=@br get Bridge br-int --  \
+        --id=@i create IPFIX targets=\"192.168.1.1\"  -- \
+        create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv3], [lflow_run],
+    [as hv2 ovs-vsctl --id=@br get Bridge br-int --  \
+        --id=@i create IPFIX targets=\"192.168.1.1\"  -- \
+        create Flow_Sample_Collector_Set bridge=@br id=1 ipfix=@i]
+)
+
+OVN_CONTROLLER_EXPECT_NO_HIT(
+    [hv1], [lflow_run],
+    [as hv1 ovs-vsctl set IPFIX . targets=\"192.168.1.2\"]
+)
+
+OVN_CONTROLLER_EXPECT_HIT(
+    [hv1], [lflow_run],
+    [as hv1 ovs-vsctl destroy Flow_Sample_Collector_Set .]
+)
+
 for i in 1 2; do
     j=$((i%2 + 1))
     lp=lp$i
diff --git a/tests/ovn.at b/tests/ovn.at
index 6bd1efb38..dd9985179 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -2216,6 +2216,10 @@  sample(probability=10);
     formats as sample(probability=10,collector_set=0,obs_domain=0,obs_point=0);
     encodes as sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0)
 
+# sample with a collector_set_id that is not configured in the chassis.
+sample(probability=100,collector_set=999,obs_domain=0,obs_point=1000);
+    encodes as drop
+
 sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000);
     probability must be greater than zero
 
@@ -33902,6 +33906,10 @@  check_sample_drops() {
     # Take match part of flows that contain "drop".
     drop_matches="$(grep 'drop' oflows_nosample | grep -oP 'table=\d*, priority=.* ')"
 
+    ovs-vsctl --id=@br get Bridge br-int --  \
+        --id=@i create IPFIX targets=\"192.168.1.1\"  -- \
+        create Flow_Sample_Collector_Set bridge=@br id=123 ipfix=@i
+
     check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" \
                     -- set NB_Global . options:debug_drop_domain_id="1"
     check ovn-nbctl --wait=hv sync
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index 64b0a2e20..d58350dcf 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -34,6 +34,7 @@ 
 #include "ovn/logical-fields.h"
 #include "lib/ovn-l7.h"
 #include "lib/extend-table.h"
+#include "lib/ovn-util.h"
 #include "ovs-thread.h"
 #include "ovstest.h"
 #include "openvswitch/shash.h"
@@ -1302,6 +1303,12 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
     struct ovn_extend_table meter_table;
     ovn_extend_table_init(&meter_table);
 
+    /* Initialize collector sets. */
+    struct flow_collector_ids collector_ids;
+    flow_collector_ids_init(&collector_ids);
+    flow_collector_ids_add(&collector_ids, 0);
+    flow_collector_ids_add(&collector_ids, 200);
+
     simap_init(&ports);
     simap_put(&ports, "eth0", 5);
     simap_put(&ports, "eth1", 6);
@@ -1348,6 +1355,7 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
                 .is_switch = true,
                 .group_table = &group_table,
                 .meter_table = &meter_table,
+                .collector_ids = &collector_ids,
 
                 .pipeline = OVNACT_P_INGRESS,
                 .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
@@ -1434,6 +1442,7 @@  test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
     smap_destroy(&template_vars);
     ovn_extend_table_destroy(&group_table);
     ovn_extend_table_destroy(&meter_table);
+    flow_collector_ids_destroy(&collector_ids);
     exit(ok ? EXIT_SUCCESS : EXIT_FAILURE);
 }