[ovs-dev,v6] Text respresntations 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 <jtanenba@redhat.com>
Suggested-by: Dumitru Ceara <dceara@redhat.com>
Reported-at: https://issues.redhat.com/browse/FDP-307

---

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
---
 NEWS                |  2 ++
 lib/ovn-util.h      | 26 +++++++++++++++++++++++
 northd/lflow-mgr.c  | 25 +++++++++++++++--------
 northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
 northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
 ovn-sb.ovsschema    |  8 +++++---
 ovn-sb.xml          |  5 +++++
 tests/ovn-northd.at | 15 ++++++++++++++
 8 files changed, 119 insertions(+), 39 deletions(-)

On Tue, Jul 16, 2024 at 9:45 AM 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
>

Hi Jacob,

Thanks for the patch.

I've a question.  Do you have a specific reason to add a string wrapper ?
Looks to me, you can use "const char *flow_desc" in the "struct
ovn_lflow".  Is it because in the future, to add
dynamic strings ?

I'd suggest just use "const char *" for now and use "struct ds" in the
future patches if we need to add memory allocated
strings ?  What do you think ?

Otherwise the patch LGTM.

Thanks
Numan

> ---
>
> 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
> ---
>  NEWS                |  2 ++
>  lib/ovn-util.h      | 26 +++++++++++++++++++++++
>  northd/lflow-mgr.c  | 25 +++++++++++++++--------
>  northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
>  northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
>  ovn-sb.ovsschema    |  8 +++++---
>  ovn-sb.xml          |  5 +++++
>  tests/ovn-northd.at | 15 ++++++++++++++
>  8 files changed, 119 insertions(+), 39 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3e392ff08..0039b04be 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,8 @@ Post v24.03.0
>      ability to disable "VXLAN mode" to extend available tunnel IDs space for
>      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5) for
>      mentioned option.
> +  - 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
> index f75b821b6..2da5cd086 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -466,6 +466,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
> index b2c60b5de..c81d9afcd 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, 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
> index 83b087f47..56cda8507 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, 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
> index 6898daa00..878e66b3d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5877,9 +5877,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);
>  }
> @@ -8695,10 +8695,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);
> @@ -8711,10 +8712,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 unbownd router ports",
> +                        lflow_ref);
>                  }
>
>                  ds_clear(&match);
> @@ -8725,12 +8727,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);
>              }
>          }
> @@ -8756,8 +8759,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",
> @@ -8793,31 +8797,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
> index b6c051ae6..b512dc2a5 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": "831370701 31501",
>      "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 73a1be5ed..2703cb6ea 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2913,6 +2913,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 a389d1988..51fdd993e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12492,6 +12492,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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On Tue, Jul 23, 2024 at 12:41 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Tue, Jul 16, 2024 at 9:45 AM 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
> >
>
> Hi Jacob,
>
> Thanks for the patch.
>
> I've a question.  Do you have a specific reason to add a string wrapper ?
> Looks to me, you can use "const char *flow_desc" in the "struct
> ovn_lflow".  Is it because in the future, to add
> dynamic strings ?
>
> I'd suggest just use "const char *" for now and use "struct ds" in the
> future patches if we need to add memory allocated
> strings ?  What do you think ?
>
> Otherwise the patch LGTM.

Also there are a few typos.

In the commit message - s/respresntations/representations

And a few below.


>
> Thanks
> Numan
>
> > ---
> >
> > 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
> > ---
> >  NEWS                |  2 ++
> >  lib/ovn-util.h      | 26 +++++++++++++++++++++++
> >  northd/lflow-mgr.c  | 25 +++++++++++++++--------
> >  northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
> >  northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
> >  ovn-sb.ovsschema    |  8 +++++---
> >  ovn-sb.xml          |  5 +++++
> >  tests/ovn-northd.at | 15 ++++++++++++++
> >  8 files changed, 119 insertions(+), 39 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 3e392ff08..0039b04be 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -38,6 +38,8 @@ Post v24.03.0
> >      ability to disable "VXLAN mode" to extend available tunnel IDs space for
> >      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5) for
> >      mentioned option.
> > +  - 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
> > index f75b821b6..2da5cd086 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -466,6 +466,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
> > index b2c60b5de..c81d9afcd 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, 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
> > index 83b087f47..56cda8507 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, 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
> > index 6898daa00..878e66b3d 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -5877,9 +5877,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);
> >  }
> > @@ -8695,10 +8695,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);
> > @@ -8711,10 +8712,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 unbownd router ports",

s/unbownd/unbound


Thanks
Numan

> > +                        lflow_ref);
> >                  }
> >
> >                  ds_clear(&match);
> > @@ -8725,12 +8727,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);
> >              }
> >          }
> > @@ -8756,8 +8759,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",
> > @@ -8793,31 +8797,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
> > index b6c051ae6..b512dc2a5 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": "831370701 31501",
> >      "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 73a1be5ed..2703cb6ea 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2913,6 +2913,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 a389d1988..51fdd993e 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -12492,6 +12492,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
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >

On Tue, Jul 23, 2024 at 6:42 PM Numan Siddique <numans@ovn.org> wrote:

> On Tue, Jul 16, 2024 at 9:45 AM 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
> >
>
> Hi Jacob,
>
> Thanks for the patch.
>
> I've a question.  Do you have a specific reason to add a string wrapper ?
> Looks to me, you can use "const char *flow_desc" in the "struct
> ovn_lflow".  Is it because in the future, to add
> dynamic strings ?
>


It was suggested by Adrian that we should be able to accept both dynamic
and static
strings. The version 4 AFAIR was using just const char *.


> I'd suggest just use "const char *" for now and use "struct ds" in the
> future patches if we need to add memory allocated
> strings ?  What do you think ?
>
> Otherwise the patch LGTM.
>
> Thanks
> Numan
>
> > ---
> >
> > 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
> > ---
> >  NEWS                |  2 ++
> >  lib/ovn-util.h      | 26 +++++++++++++++++++++++
> >  northd/lflow-mgr.c  | 25 +++++++++++++++--------
> >  northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
> >  northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
> >  ovn-sb.ovsschema    |  8 +++++---
> >  ovn-sb.xml          |  5 +++++
> >  tests/ovn-northd.at | 15 ++++++++++++++
> >  8 files changed, 119 insertions(+), 39 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 3e392ff08..0039b04be 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -38,6 +38,8 @@ Post v24.03.0
> >      ability to disable "VXLAN mode" to extend available tunnel IDs
> space for
> >      datapaths from 4095 to 16711680.  For more details see man
> ovn-nb(5) for
> >      mentioned option.
> > +  - 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
> > index f75b821b6..2da5cd086 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -466,6 +466,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
> > index b2c60b5de..c81d9afcd 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, 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
> > index 83b087f47..56cda8507 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, 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
> > index 6898daa00..878e66b3d 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -5877,9 +5877,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);
> >  }
> > @@ -8695,10 +8695,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);
> > @@ -8711,10 +8712,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 unbownd router
> ports",
> > +                        lflow_ref);
> >                  }
> >
> >                  ds_clear(&match);
> > @@ -8725,12 +8727,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);
> >              }
> >          }
> > @@ -8756,8 +8759,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",
> > @@ -8793,31 +8797,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
> > index b6c051ae6..b512dc2a5 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": "831370701 31501",
> >      "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 73a1be5ed..2703cb6ea 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -2913,6 +2913,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 a389d1988..51fdd993e 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -12492,6 +12492,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
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Thanks,
Ales

On Wed, Jul 24, 2024 at 1:34 AM Ales Musil <amusil@redhat.com> wrote:
>
> On Tue, Jul 23, 2024 at 6:42 PM Numan Siddique <numans@ovn.org> wrote:
>
> > On Tue, Jul 16, 2024 at 9:45 AM 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
> > >
> >
> > Hi Jacob,
> >
> > Thanks for the patch.
> >
> > I've a question.  Do you have a specific reason to add a string wrapper ?
> > Looks to me, you can use "const char *flow_desc" in the "struct
> > ovn_lflow".  Is it because in the future, to add
> > dynamic strings ?
> >
>
>
> It was suggested by Adrian that we should be able to accept both dynamic
> and static
> strings. The version 4 AFAIR was using just const char *.

I see.  @Ales - Would you mind taking a look at v6 and provide your
Ack if it LGTY ?

Thanks
Numan

>
>
> > I'd suggest just use "const char *" for now and use "struct ds" in the
> > future patches if we need to add memory allocated
> > strings ?  What do you think ?
> >
> > Otherwise the patch LGTM.
> >
> > Thanks
> > Numan
> >
> > > ---
> > >
> > > 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
> > > ---
> > >  NEWS                |  2 ++
> > >  lib/ovn-util.h      | 26 +++++++++++++++++++++++
> > >  northd/lflow-mgr.c  | 25 +++++++++++++++--------
> > >  northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
> > >  northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
> > >  ovn-sb.ovsschema    |  8 +++++---
> > >  ovn-sb.xml          |  5 +++++
> > >  tests/ovn-northd.at | 15 ++++++++++++++
> > >  8 files changed, 119 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 3e392ff08..0039b04be 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -38,6 +38,8 @@ Post v24.03.0
> > >      ability to disable "VXLAN mode" to extend available tunnel IDs
> > space for
> > >      datapaths from 4095 to 16711680.  For more details see man
> > ovn-nb(5) for
> > >      mentioned option.
> > > +  - 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
> > > index f75b821b6..2da5cd086 100644
> > > --- a/lib/ovn-util.h
> > > +++ b/lib/ovn-util.h
> > > @@ -466,6 +466,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
> > > index b2c60b5de..c81d9afcd 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, 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
> > > index 83b087f47..56cda8507 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, 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
> > > index 6898daa00..878e66b3d 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -5877,9 +5877,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);
> > >  }
> > > @@ -8695,10 +8695,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);
> > > @@ -8711,10 +8712,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 unbownd router
> > ports",
> > > +                        lflow_ref);
> > >                  }
> > >
> > >                  ds_clear(&match);
> > > @@ -8725,12 +8727,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);
> > >              }
> > >          }
> > > @@ -8756,8 +8759,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",
> > > @@ -8793,31 +8797,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
> > > index b6c051ae6..b512dc2a5 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": "831370701 31501",
> > >      "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 73a1be5ed..2703cb6ea 100644
> > > --- a/ovn-sb.xml
> > > +++ b/ovn-sb.xml
> > > @@ -2913,6 +2913,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 a389d1988..51fdd993e 100644
> > > --- a/tests/ovn-northd.at
> > > +++ b/tests/ovn-northd.at
> > > @@ -12492,6 +12492,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
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> Thanks,
> Ales
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amusil@redhat.com
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Tue, Jul 16, 2024 at 3:45 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
>
> ---
>

Hi Jacob,

I have just two small nits down below, so if maintainers are
willing to fold them in:

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
> ---
>  NEWS                |  2 ++
>  lib/ovn-util.h      | 26 +++++++++++++++++++++++
>  northd/lflow-mgr.c  | 25 +++++++++++++++--------
>  northd/lflow-mgr.h  | 27 +++++++++++++++++++-----
>  northd/northd.c     | 50 ++++++++++++++++++++++++---------------------
>  ovn-sb.ovsschema    |  8 +++++---
>  ovn-sb.xml          |  5 +++++
>  tests/ovn-northd.at | 15 ++++++++++++++
>  8 files changed, 119 insertions(+), 39 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3e392ff08..0039b04be 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -38,6 +38,8 @@ Post v24.03.0
>      ability to disable "VXLAN mode" to extend available tunnel IDs space
> for
>      datapaths from 4095 to 16711680.  For more details see man ovn-nb(5)
> for
>      mentioned option.
> +  - 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
> index f75b821b6..2da5cd086 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -466,6 +466,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
> index b2c60b5de..c81d9afcd 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, 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);
>

nit: Two spaces


>  }
>
>  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
> index 83b087f47..56cda8507 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, 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
> index 6898daa00..878e66b3d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5877,9 +5877,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);
>  }
> @@ -8695,10 +8695,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);
> @@ -8711,10 +8712,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 unbownd router
> ports",
> +                        lflow_ref);
>                  }
>
>                  ds_clear(&match);
> @@ -8725,12 +8727,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);
>              }
>          }
> @@ -8756,8 +8759,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",
> @@ -8793,31 +8797,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
> index b6c051ae6..b512dc2a5 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": "831370701 31501",
>      "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}}},
>

nit: Formatting


>              "isRoot": true},
>          "Logical_DP_Group": {
>              "columns": {
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 73a1be5ed..2703cb6ea 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -2913,6 +2913,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 a389d1988..51fdd993e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -12492,6 +12492,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
>
>
Thanks,
Ales