Text representations for drop sampling.

Created a new column in the southbound database to hardcode a human readable description for flows. This first use is describing why the flow is dropping packets. The new column is called flow_desc and will create southbound database entries like this _uuid : 20f1897b-477e-47ae-a32c-c546d83ec097 actions : "sample(probability=65535,collector_set=123,obs_domain=1,obs_point=$cookie); /* drop */" controller_meter : [] external_ids : {source="northd.c:8721", stage-name=ls_in_l2_unknown} flow_desc : "No L2 destination" logical_datapath : [] logical_dp_group : ee3c3db5-98a2-4f34-8a84-409deae140a7 match : "outport == \"none\"" pipeline : ingress priority : 50 table_id : 27 tags : {} hash : 0 future work includes entering more flow_desc for more flows and adding flow_desc to the actions as a comment. Signed-off-by: Jacob Tanenbaum <[email protected]> Suggested-by: Dumitru Ceara <[email protected]> Reported-at: https://issues.redhat.com/browse/FDP-307 Acked-by: Ales Musil <[email protected]>
dceara · Aug 6, 2024 · 33d04aa · 33d04aa
1 parent 6234e09
commit 33d04aa
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 40 deletions.
diff --git a/NEWS b/NEWS
@@ -50,6 +50,8 @@ Post v24.03.0
   - A new LSP option "disable_garp_rarp" has been added to prevent OVN from
     sending GARP or RARP announcements when a VIF is created on a bridged
     logical switch.
+  - Added a new column in the southbound database "flow_desc" to provide
+    human readable context to flows.
 
 OVN v24.03.0 - 01 Mar 2024
 --------------------------

diff --git a/lib/ovn-util.h b/lib/ovn-util.h
@@ -469,6 +469,32 @@ void sorted_array_apply_diff(const struct sorted_array *a1,
                                                     bool add),
                              const void *arg);
 
+/* A wrapper that holds strings */
+struct string_wrapper
+{
+    char *str;
+    bool owns_string;
+};
+
+#define EMPTY_STRING_WRAPPER (struct string_wrapper){NULL, false}
+
+static inline struct string_wrapper
+string_wrapper_create(char *str, bool take_ownership)
+{
+    return (struct string_wrapper) {
+        .str = str,
+        .owns_string = take_ownership,
+    };
+}
+
+static inline void
+string_wrapper_destroy(struct string_wrapper *s)
+{
+    if (s->owns_string) {
+        free(s->str);
+    }
+}
+
 /* Utilities around properly handling exit command. */
 struct ovn_exit_args {
     struct unixctl_conn **conns;

diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
@@ -25,6 +25,7 @@
 #include "debug.h"
 #include "lflow-mgr.h"
 #include "lib/ovn-parallel-hmap.h"
+#include "lib/ovn-util.h"
 
 VLOG_DEFINE_THIS_MODULE(lflow_mgr);
 
@@ -36,7 +37,7 @@ static void ovn_lflow_init(struct ovn_lflow *, struct ovn_datapath *od,
                            uint16_t priority, char *match,
                            char *actions, char *io_port,
                            char *ctrl_meter, char *stage_hint,
-                           const char *where);
+                           const char *where, struct string_wrapper flow_desc);
 static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows,
                                         enum ovn_stage stage,
                                         uint16_t priority, const char *match,
@@ -52,7 +53,7 @@ static struct ovn_lflow *do_ovn_lflow_add(
     const char *actions, const char *io_port,
     const char *ctrl_meter,
     const struct ovsdb_idl_row *stage_hint,
-    const char *where);
+    const char *where, struct string_wrapper flow_desc);
 
 
 static struct ovs_mutex *lflow_hash_lock(const struct hmap *lflow_table,
@@ -173,6 +174,7 @@ struct ovn_lflow {
                                   * 'dpg_bitmap'. */
     struct ovn_dp_group *dpg;    /* Link to unique Sb datapath group. */
     const char *where;
+    struct string_wrapper flow_desc;
 
     struct uuid sb_uuid;         /* SB DB row uuid, specified by northd. */
     struct ovs_list referenced_by;  /* List of struct lflow_ref_node. */
@@ -659,7 +661,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
                       const char *match, const char *actions,
                       const char *io_port, const char *ctrl_meter,
                       const struct ovsdb_idl_row *stage_hint,
-                      const char *where,
+                      const char *where, struct string_wrapper flow_desc,
                       struct lflow_ref *lflow_ref)
     OVS_EXCLUDED(fake_hash_mutex)
 {
@@ -679,7 +681,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table,
         do_ovn_lflow_add(lflow_table,
                          od ? ods_size(od->datapaths) : dp_bitmap_len,
                          hash, stage, priority, match, actions,
-                         io_port, ctrl_meter, stage_hint, where);
+                         io_port, ctrl_meter, stage_hint, where, flow_desc);
 
     if (lflow_ref) {
         struct lflow_ref_node *lrn =
@@ -733,7 +735,7 @@ lflow_table_add_lflow_default_drop(struct lflow_table *lflow_table,
 {
     lflow_table_add_lflow(lflow_table, od, NULL, 0, stage, 0, "1",
                           debug_drop_action(), NULL, NULL, NULL,
-                          where, lflow_ref);
+                          where, EMPTY_STRING_WRAPPER, lflow_ref);
 }
 
 struct ovn_dp_group *
@@ -856,7 +858,8 @@ static void
 ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
                size_t dp_bitmap_len, enum ovn_stage stage, uint16_t priority,
                char *match, char *actions, char *io_port, char *ctrl_meter,
-               char *stage_hint, const char *where)
+               char *stage_hint, const char *where,
+               struct string_wrapper flow_desc)
 {
     lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
     lflow->od = od;
@@ -867,6 +870,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
     lflow->io_port = io_port;
     lflow->stage_hint = stage_hint;
     lflow->ctrl_meter = ctrl_meter;
+    lflow->flow_desc = flow_desc;
     lflow->dpg = NULL;
     lflow->where = where;
     lflow->sb_uuid = UUID_ZERO;
@@ -946,6 +950,7 @@ ovn_lflow_destroy(struct lflow_table *lflow_table, struct ovn_lflow *lflow)
     free(lflow->io_port);
     free(lflow->stage_hint);
     free(lflow->ctrl_meter);
+    string_wrapper_destroy(&lflow->flow_desc);
     ovn_lflow_clear_dp_refcnts_map(lflow);
     struct lflow_ref_node *lrn;
     LIST_FOR_EACH_SAFE (lrn, ref_list_node, &lflow->referenced_by) {
@@ -960,7 +965,7 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, size_t dp_bitmap_len,
                  const char *match, const char *actions,
                  const char *io_port, const char *ctrl_meter,
                  const struct ovsdb_idl_row *stage_hint,
-                 const char *where)
+                 const char *where, struct string_wrapper flow_desc)
     OVS_REQUIRES(fake_hash_mutex)
 {
     struct ovn_lflow *old_lflow;
@@ -982,7 +987,8 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, size_t dp_bitmap_len,
                    xstrdup(match), xstrdup(actions),
                    io_port ? xstrdup(io_port) : NULL,
                    nullable_xstrdup(ctrl_meter),
-                   ovn_lflow_hint(stage_hint), where);
+                   ovn_lflow_hint(stage_hint), where,
+                   flow_desc);
 
     if (parallelization_state != STATE_USE_PARALLELIZATION) {
         hmap_insert(&lflow_table->entries, &lflow->hmap_node, hash);
@@ -1050,6 +1056,9 @@ sync_lflow_to_sb(struct ovn_lflow *lflow,
         sbrec_logical_flow_set_priority(sbflow, lflow->priority);
         sbrec_logical_flow_set_match(sbflow, lflow->match);
         sbrec_logical_flow_set_actions(sbflow, lflow->actions);
+        if (lflow->flow_desc.str) {
+            sbrec_logical_flow_set_flow_desc(sbflow, lflow->flow_desc.str);
+        }
         if (lflow->io_port) {
             struct smap tags = SMAP_INITIALIZER(&tags);
             smap_add(&tags, "in_out_port", lflow->io_port);

diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
@@ -78,7 +78,8 @@ void lflow_table_add_lflow(struct lflow_table *, const struct ovn_datapath *,
                            const char *actions, const char *io_port,
                            const char *ctrl_meter,
                            const struct ovsdb_idl_row *stage_hint,
-                           const char *where, struct lflow_ref *);
+                           const char *where, struct string_wrapper flow_desc,
+                           struct lflow_ref *);
 void lflow_table_add_lflow_default_drop(struct lflow_table *,
                                         const struct ovn_datapath *,
                                         enum ovn_stage stage,
@@ -91,20 +92,20 @@ void lflow_table_add_lflow_default_drop(struct lflow_table *,
                                   STAGE_HINT, LFLOW_REF) \
     lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
                           ACTIONS, IN_OUT_PORT, CTRL_METER, STAGE_HINT, \
-                          OVS_SOURCE_LOCATOR, LFLOW_REF)
+                          OVS_SOURCE_LOCATOR, EMPTY_STRING_WRAPPER, LFLOW_REF)
 
 #define ovn_lflow_add_with_hint(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, \
                                 ACTIONS, STAGE_HINT, LFLOW_REF) \
     lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
                           ACTIONS, NULL, NULL, STAGE_HINT,  \
-                          OVS_SOURCE_LOCATOR, LFLOW_REF)
+                          OVS_SOURCE_LOCATOR, EMPTY_STRING_WRAPPER, LFLOW_REF)
 
 #define ovn_lflow_add_with_dp_group(LFLOW_TABLE, DP_BITMAP, DP_BITMAP_LEN, \
                                     STAGE, PRIORITY, MATCH, ACTIONS, \
                                     STAGE_HINT, LFLOW_REF) \
     lflow_table_add_lflow(LFLOW_TABLE, NULL, DP_BITMAP, DP_BITMAP_LEN, STAGE, \
                           PRIORITY, MATCH, ACTIONS, NULL, NULL, STAGE_HINT, \
-                          OVS_SOURCE_LOCATOR, LFLOW_REF)
+                          OVS_SOURCE_LOCATOR, EMPTY_STRING_WRAPPER, LFLOW_REF)
 
 #define ovn_lflow_add_default_drop(LFLOW_TABLE, OD, STAGE, LFLOW_REF)   \
     lflow_table_add_lflow_default_drop(LFLOW_TABLE, OD, STAGE, \
@@ -126,12 +127,28 @@ void lflow_table_add_lflow_default_drop(struct lflow_table *,
                                           STAGE_HINT, LFLOW_REF) \
     lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
                           ACTIONS, IN_OUT_PORT, NULL, STAGE_HINT, \
-                          OVS_SOURCE_LOCATOR, LFLOW_REF)
+                          OVS_SOURCE_LOCATOR, EMPTY_STRING_WRAPPER, LFLOW_REF)
 
 #define ovn_lflow_add(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \
                       LFLOW_REF) \
     lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
                           ACTIONS, NULL, NULL, NULL, OVS_SOURCE_LOCATOR, \
+                          EMPTY_STRING_WRAPPER, LFLOW_REF)
+
+#define ovn_lflow_add_drop_with_desc(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, \
+                                DESCRIPTION, LFLOW_REF) \
+    lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
+                          debug_drop_action(), NULL, NULL, NULL,  \
+                          OVS_SOURCE_LOCATOR, \
+                          string_wrapper_create(DESCRIPTION, false), LFLOW_REF)
+
+#define ovn_lflow_add_drop_with_lport_hint_and_desc(LFLOW_TABLE, OD, STAGE, \
+                                          PRIORITY, MATCH,  IN_OUT_PORT, \
+                                          STAGE_HINT, DESCRIPTION, LFLOW_REF) \
+    lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, MATCH, \
+                          debug_drop_action(), IN_OUT_PORT, NULL, STAGE_HINT, \
+                          OVS_SOURCE_LOCATOR, \
+                          string_wrapper_create(DESCRIPTION, false), \
                           LFLOW_REF)
 
 #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \

diff --git a/northd/northd.c b/northd/northd.c
@@ -5883,9 +5883,9 @@ build_lswitch_output_port_sec_od(struct ovn_datapath *od,
                   REGBIT_PORT_SEC_DROP" = check_out_port_sec(); next;",
                   lflow_ref);
 
-    ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50,
-                  REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(),
-                  lflow_ref);
+    ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50,
+                  REGBIT_PORT_SEC_DROP" == 1",
+                  "Packet does not follow port security rules", lflow_ref);
     ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 0,
                   "1", "output;", lflow_ref);
 }
@@ -8672,10 +8672,11 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
                         port->json_key,
                         op->lsp_addrs[i].ea_s, op->json_key,
                         rp->lsp_addrs[k].ipv4_addrs[l].addr_s);
-                    ovn_lflow_add_with_lport_and_hint(
+                    ovn_lflow_add_drop_with_lport_hint_and_desc(
                         lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
-                        ds_cstr(&match),  debug_drop_action(), port->key,
-                        &op->nbsp->header_, lflow_ref);
+                        ds_cstr(&match), port->key,
+                        &op->nbsp->header_,
+                        "Drop arp for unknown router ports", lflow_ref);
                 }
                 for (size_t l = 0; l < rp->lsp_addrs[k].n_ipv6_addrs; l++) {
                     ds_clear(&match);
@@ -8688,10 +8689,11 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
                         rp->lsp_addrs[k].ipv6_addrs[l].addr_s,
                         rp->lsp_addrs[k].ipv6_addrs[l].sn_addr_s,
                         rp->lsp_addrs[k].ipv6_addrs[l].addr_s);
-                    ovn_lflow_add_with_lport_and_hint(
+                    ovn_lflow_add_drop_with_lport_hint_and_desc(
                         lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100,
-                        ds_cstr(&match), debug_drop_action(), port->key,
-                        &op->nbsp->header_, lflow_ref);
+                        ds_cstr(&match), port->key,
+                        &op->nbsp->header_, "Drop ND for unbound router ports",
+                        lflow_ref);
                 }
 
                 ds_clear(&match);
@@ -8702,12 +8704,13 @@ build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op,
                     port->json_key,
                     op->lsp_addrs[i].ea_s, rp->lsp_addrs[k].ea_s,
                     op->json_key);
-                ovn_lflow_add_with_lport_and_hint(lflows, op->od,
+                ovn_lflow_add_drop_with_lport_hint_and_desc(lflows, op->od,
                                                   S_SWITCH_IN_EXTERNAL_PORT,
                                                   100, ds_cstr(&match),
-                                                  debug_drop_action(),
                                                   port->key,
                                                   &op->nbsp->header_,
+                                                  "Packet does not come from" \
+                                                  " a chassis resident",
                                                   lflow_ref);
             }
         }
@@ -8733,8 +8736,9 @@ build_lswitch_lflows_l2_unknown(struct ovn_datapath *od,
                       "outport = \""MC_UNKNOWN "\"; output;",
                       lflow_ref);
     } else {
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
-                      "outport == \"none\"",  debug_drop_action(),
+        ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50,
+                      "outport == \"none\"",
+                      "No L2 destination",
                       lflow_ref);
     }
     ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1",
@@ -8769,31 +8773,31 @@ build_lswitch_lflows_admission_control(struct ovn_datapath *od,
     ovs_assert(od->nbs);
 
     /* Default action for recirculated ICMP error 'packet too big'. */
-    ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
+    ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
                   "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
                   " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
-                  " flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
+                  " flags.tunnel_rx == 1", "ICMP: packet too big", lflow_ref);
 
     /* Logical VLANs not supported. */
     if (!is_vlan_transparent(od)) {
         /* Block logical VLANs. */
-        ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
-                      "vlan.present", debug_drop_action(),
+        ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
+                      100, "vlan.present", "VLANs blocked",
                       lflow_ref);
     }
 
     /* Broadcast/multicast source address is invalid. */
-    ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
-                  "eth.src[40]", debug_drop_action(),
-                  lflow_ref);
+    ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100,
+                  "eth.src[40]", "incoming Broadcast/multicast source" \
+                  " address is invalid", lflow_ref);
 
     ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 50, "1",
                   REGBIT_PORT_SEC_DROP" = check_in_port_sec(); next;",
                   lflow_ref);
 
-    ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50,
-                  REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(),
-                  lflow_ref);
+    ovn_lflow_add_drop_with_desc(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50,
+                  REGBIT_PORT_SEC_DROP" == 1",
+                  "Broadcast/multicast port security invalid", lflow_ref);
 
     ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 0, "1", "next;",
                   lflow_ref);

diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
 {
     "name": "OVN_Southbound",
-    "version": "20.34.0",
-    "cksum": "2786607656 31376",
+    "version": "20.35.0",
+    "cksum": "3665804961 31485",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -113,10 +113,12 @@
                     "type": {"key": "string", "value": "string",
                              "min": 0, "max": "unlimited"}},
                 "controller_meter": {"type": {"key": {"type": "string"},
-                                     "min": 0, "max": 1}},
+                             "min": 0, "max": 1}},
                 "external_ids": {
                     "type": {"key": "string", "value": "string",
-                             "min": 0, "max": "unlimited"}}},
+                             "min": 0, "max": "unlimited"}},
+                "flow_desc": {"type": {"key": {"type": "string"},
+                             "min": 0, "max": 1}}},
             "isRoot": true},
         "Logical_DP_Group": {
             "columns": {

diff --git a/ovn-sb.xml b/ovn-sb.xml
@@ -2894,6 +2894,11 @@ tcp.flags = RST;
       <code>ovn-controller</code>.
     </column>
 
+    <column name="flow_desc">
+      Human-readable explanation of the flow, this is optional and used
+      to provide context for the given flow.
+    </column>
+
     <column name="external_ids" key="stage-name">
       Human-readable name for this flow's stage in the pipeline.
     </column>

diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
@@ -12256,6 +12256,21 @@ AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed 's/table=../table=??/'], [0], [dnl
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check for flow_desc])
+ovn_start
+
+check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123"
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl --wait=hv sync
+
+flow_desc=$(fetch_column Logical_flow flow_desc match='"outport == \"none\""')
+AT_CHECK([test "$flow_desc" != ""])
+
+AT_CLEANUP
+])
+
 AT_SETUP([NB_Global and SB_Global incremental processing])
 
 ovn_start