diff mbox series

[ovs-dev,v8] Text representations for drop sampling.

Message ID 20240808170909.418899-1-jtanenba@redhat.com
State Accepted
Headers show
Series [ovs-dev,v8] Text representations for drop sampling. | expand

Checks

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

Commit Message

Jacob Tanenbaum Aug. 8, 2024, 5:09 p.m. UTC
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 <jtanenba@redhat.com>
Suggested-by: Dumitru Ceara <dceara@redhat.com>
Reported-at: https://issues.redhat.com/browse/FDP-307
Acked-by: Ales Musil <amusil@redhat.com>

---

v1: initial version
v2: correct commit message
    make the flow_desc a char*
    correct a typo in the ovn-sb.xml
    correct the test
v3: rebase issue with NEWS file
v4: remove options:debug_drop_domain_id="1" from testing as we
    do not depend on it
v5: introduce string wrapper
    increment ovs-sb.ovsschema version
    correct the testing
    added descriptions to more dropped packets
v6: v5 was not the correct branch, this is...
    added descriptions to more dropped packets
    changed the names of the macros to be more descriptive
v7: rebased, corrected a few typos, updated the ovs-sb.ovsschema
    checksum, and added Ales ACK
v8: rebased to main, took dceara's patch to use const char*, minor
    formatting issues fixed
---
 NEWS                |  2 ++
 northd/lflow-mgr.c  | 22 +++++++++-----
 northd/lflow-mgr.h  | 27 +++++++++++++----
 northd/northd.c     | 73 +++++++++++++++++++++++++--------------------
 ovn-sb.ovsschema    |  8 +++--
 ovn-sb.xml          |  5 ++++
 tests/ovn-northd.at | 15 ++++++++++
 7 files changed, 103 insertions(+), 49 deletions(-)

Comments

Jacob Tanenbaum Aug. 8, 2024, 5:44 p.m. UTC | #1
There are some drops that I don't add descriptions to in this patch. I plan
on adding more but I want to clean up the code and hopefully combine/rebase
some of the macros to make the code more readable as I add more.

Jacob

On Thu, Aug 8, 2024 at 1:09 PM Jacob Tanenbaum <jtanenba@redhat.com> wrote:

> 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 <jtanenba@redhat.com>
> Suggested-by: Dumitru Ceara <dceara@redhat.com>
> Reported-at: https://issues.redhat.com/browse/FDP-307
> Acked-by: Ales Musil <amusil@redhat.com>
>
> ---
>
> v1: initial version
> v2: correct commit message
>     make the flow_desc a char*
>     correct a typo in the ovn-sb.xml
>     correct the test
> v3: rebase issue with NEWS file
> v4: remove options:debug_drop_domain_id="1" from testing as we
>     do not depend on it
> v5: introduce string wrapper
>     increment ovs-sb.ovsschema version
>     correct the testing
>     added descriptions to more dropped packets
> v6: v5 was not the correct branch, this is...
>     added descriptions to more dropped packets
>     changed the names of the macros to be more descriptive
> v7: rebased, corrected a few typos, updated the ovs-sb.ovsschema
>     checksum, and added Ales ACK
> v8: rebased to main, took dceara's patch to use const char*, minor
>     formatting issues fixed
> ---
>  NEWS                |  2 ++
>  northd/lflow-mgr.c  | 22 +++++++++-----
>  northd/lflow-mgr.h  | 27 +++++++++++++----
>  northd/northd.c     | 73 +++++++++++++++++++++++++--------------------
>  ovn-sb.ovsschema    |  8 +++--
>  ovn-sb.xml          |  5 ++++
>  tests/ovn-northd.at | 15 ++++++++++
>  7 files changed, 103 insertions(+), 49 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 17bb50af9..5b939211f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -62,6 +62,8 @@ Post v24.03.0
>    - Add "external_ids:ovn-encap-ip-default" config for ovn-controller to
>      determine the default encap IP when there are multiple encap IPs
>      configured.
> +  - 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/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index b2c60b5de..88ce7ce56 100644
> --- 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, const char *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, const char *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;
> +    const char *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, const char *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, NULL, 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,
> +               const char *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;
> @@ -960,7 +964,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, const char *flow_desc)
>      OVS_REQUIRES(fake_hash_mutex)
>  {
>      struct ovn_lflow *old_lflow;
> @@ -982,7 +986,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 +1055,7 @@ 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);
> +        sbrec_logical_flow_set_flow_desc(sbflow, lflow->flow_desc);
>          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
> index 83b087f47..2c05b352d 100644
> --- 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, const char *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, NULL, 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, NULL, 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, NULL, 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,13 +127,27 @@ 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, NULL, 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, \
> -                          LFLOW_REF)
> +                          NULL, 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, DESCRIPTION, 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, DESCRIPTION, LFLOW_REF)
>
>  #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH,
> ACTIONS, \
>                            CTRL_METER, LFLOW_REF) \
> diff --git a/northd/northd.c b/northd/northd.c
> index 588edadce..04647e614 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5982,9 +5982,10 @@ 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);
>  }
> @@ -9308,10 +9309,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);
> @@ -9324,10 +9326,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);
> @@ -9338,13 +9341,14 @@
> 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,
> -
> S_SWITCH_IN_EXTERNAL_PORT,
> -                                                  100, ds_cstr(&match),
> -                                                  debug_drop_action(),
> -                                                  port->key,
> -                                                  &op->nbsp->header_,
> -                                                  lflow_ref);
> +                ovn_lflow_add_drop_with_lport_hint_and_desc(
> +                    lflows, op->od,
> +                    S_SWITCH_IN_EXTERNAL_PORT,
> +                    100, ds_cstr(&match),
> +                    port->key,
> +                    &op->nbsp->header_,
> +                    "Packet does not come from a chassis resident",
> +                    lflow_ref);
>              }
>          }
>      }
> @@ -9369,9 +9373,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(),
> -                      lflow_ref);
> +        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",
>                    "output;", lflow_ref);
> @@ -9405,31 +9409,36 @@ 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,
> -                  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
> -                  " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
> -                  " flags.tunnel_rx == 1", debug_drop_action(),
> lflow_ref);
> +    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", "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(),
> -                      lflow_ref);
> +        ovn_lflow_add_drop_with_desc(
> +            lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
> +            100, "vlan.present",
> +            "VLANs blocked due to vlan-passthru option",
> +            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
> index b6c051ae6..ec39fdd81 100644
> --- 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": "2897301415 31493",
>      "tables": {
>          "SB_Global": {
>              "columns": {
> @@ -116,7 +116,9 @@
>                                       "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
> index d91d77f83..cbcbbfb53 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2904,6 +2904,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
> index 2830ab598..159d61a16 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12360,6 +12360,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
> --
> 2.45.2
>
>
Dumitru Ceara Aug. 8, 2024, 7 p.m. UTC | #2
On 8/8/24 19:09, Jacob Tanenbaum wrote:
> 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 <jtanenba@redhat.com>
> Suggested-by: Dumitru Ceara <dceara@redhat.com>
> Reported-at: https://issues.redhat.com/browse/FDP-307
> Acked-by: Ales Musil <amusil@redhat.com>
> 
> ---

Thanks, Jacob and Ales!

I only have a minor comment below.  I think that can be fixed up when
this patch is merged so there's probably no need to send v9.

Acked-by: Dumitru Ceara <dceara@redhat.com>

[...]

> @@ -9405,31 +9409,36 @@ 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,
> -                  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
> -                  " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
> -                  " flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
> +    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", "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(),
> -                      lflow_ref);
> +        ovn_lflow_add_drop_with_desc(
> +            lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
> +            100, "vlan.present",
> +            "VLANs blocked due to vlan-passthru option",
> +            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" \

Nit: no need for "\".

> +        " address is invalid", lflow_ref);
>  

Regards,
Dumitru
Numan Siddique Aug. 9, 2024, 3:30 p.m. UTC | #3
On Thu, Aug 8, 2024 at 3:01 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 8/8/24 19:09, Jacob Tanenbaum wrote:
> > 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 <jtanenba@redhat.com>
> > Suggested-by: Dumitru Ceara <dceara@redhat.com>
> > Reported-at: https://issues.redhat.com/browse/FDP-307
> > Acked-by: Ales Musil <amusil@redhat.com>
> >
> > ---
>
> Thanks, Jacob and Ales!
>
> I only have a minor comment below.  I think that can be fixed up when
> this patch is merged so there's probably no need to send v9.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks.  Applied to main after addressing the comment below.

Numan

>
> [...]
>
> > @@ -9405,31 +9409,36 @@ 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,
> > -                  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
> > -                  " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
> > -                  " flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
> > +    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", "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(),
> > -                      lflow_ref);
> > +        ovn_lflow_add_drop_with_desc(
> > +            lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
> > +            100, "vlan.present",
> > +            "VLANs blocked due to vlan-passthru option",
> > +            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" \
>
> Nit: no need for "\".
>
> > +        " address is invalid", lflow_ref);
> >
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 17bb50af9..5b939211f 100644
--- a/NEWS
+++ b/NEWS
@@ -62,6 +62,8 @@  Post v24.03.0
   - Add "external_ids:ovn-encap-ip-default" config for ovn-controller to
     determine the default encap IP when there are multiple encap IPs
     configured.
+  - 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/northd/lflow-mgr.c b/northd/lflow-mgr.c
index b2c60b5de..88ce7ce56 100644
--- 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, const char *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, const char *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;
+    const char *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, const char *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, NULL, 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,
+               const char *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;
@@ -960,7 +964,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, const char *flow_desc)
     OVS_REQUIRES(fake_hash_mutex)
 {
     struct ovn_lflow *old_lflow;
@@ -982,7 +986,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 +1055,7 @@  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);
+        sbrec_logical_flow_set_flow_desc(sbflow, lflow->flow_desc);
         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
index 83b087f47..2c05b352d 100644
--- 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, const char *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, NULL, 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, NULL, 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, NULL, 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,13 +127,27 @@  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, NULL, 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, \
-                          LFLOW_REF)
+                          NULL, 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, DESCRIPTION, 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, DESCRIPTION, LFLOW_REF)
 
 #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \
                           CTRL_METER, LFLOW_REF) \
diff --git a/northd/northd.c b/northd/northd.c
index 588edadce..04647e614 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5982,9 +5982,10 @@  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);
 }
@@ -9308,10 +9309,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);
@@ -9324,10 +9326,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);
@@ -9338,13 +9341,14 @@  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,
-                                                  S_SWITCH_IN_EXTERNAL_PORT,
-                                                  100, ds_cstr(&match),
-                                                  debug_drop_action(),
-                                                  port->key,
-                                                  &op->nbsp->header_,
-                                                  lflow_ref);
+                ovn_lflow_add_drop_with_lport_hint_and_desc(
+                    lflows, op->od,
+                    S_SWITCH_IN_EXTERNAL_PORT,
+                    100, ds_cstr(&match),
+                    port->key,
+                    &op->nbsp->header_,
+                    "Packet does not come from a chassis resident",
+                    lflow_ref);
             }
         }
     }
@@ -9369,9 +9373,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(),
-                      lflow_ref);
+        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",
                   "output;", lflow_ref);
@@ -9405,31 +9409,36 @@  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,
-                  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
-                  " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
-                  " flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
+    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", "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(),
-                      lflow_ref);
+        ovn_lflow_add_drop_with_desc(
+            lflows, od, S_SWITCH_IN_CHECK_PORT_SEC,
+            100, "vlan.present",
+            "VLANs blocked due to vlan-passthru option",
+            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
index b6c051ae6..ec39fdd81 100644
--- 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": "2897301415 31493",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -116,7 +116,9 @@ 
                                      "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
index d91d77f83..cbcbbfb53 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -2904,6 +2904,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
index 2830ab598..159d61a16 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -12360,6 +12360,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