Message ID | 20240716134506.47622-1-jtanenba@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,v6] Text respresntations for drop sampling. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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
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
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(-)