There are some drops that I don't add descriptions to in this patch. I plan on adding more but I want to clean up the code and hopefully combine/rebase some of the macros to make the code more readable as I add more.
Jacob On Thu, Aug 8, 2024 at 1:09 PM Jacob Tanenbaum <[email protected]> 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 <[email protected]> > Suggested-by: Dumitru Ceara <[email protected]> > Reported-at: https://issues.redhat.com/browse/FDP-307 > Acked-by: Ales Musil <[email protected]> > > --- > > v1: initial version > v2: correct commit message > make the flow_desc a char* > correct a typo in the ovn-sb.xml > correct the test > v3: rebase issue with NEWS file > v4: remove options:debug_drop_domain_id="1" from testing as we > do not depend on it > v5: introduce string wrapper > increment ovs-sb.ovsschema version > correct the testing > added descriptions to more dropped packets > v6: v5 was not the correct branch, this is... > added descriptions to more dropped packets > changed the names of the macros to be more descriptive > v7: rebased, corrected a few typos, updated the ovs-sb.ovsschema > checksum, and added Ales ACK > v8: rebased to main, took dceara's patch to use const char*, minor > formatting issues fixed > --- > NEWS | 2 ++ > northd/lflow-mgr.c | 22 +++++++++----- > northd/lflow-mgr.h | 27 +++++++++++++---- > northd/northd.c | 73 +++++++++++++++++++++++++-------------------- > ovn-sb.ovsschema | 8 +++-- > ovn-sb.xml | 5 ++++ > tests/ovn-northd.at | 15 ++++++++++ > 7 files changed, 103 insertions(+), 49 deletions(-) > > diff --git a/NEWS b/NEWS > index 17bb50af9..5b939211f 100644 > --- a/NEWS > +++ b/NEWS > @@ -62,6 +62,8 @@ Post v24.03.0 > - Add "external_ids:ovn-encap-ip-default" config for ovn-controller to > determine the default encap IP when there are multiple encap IPs > configured. > + - Added a new column in the southbound database "flow_desc" to provide > + human readable context to flows. > > OVN v24.03.0 - 01 Mar 2024 > -------------------------- > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index b2c60b5de..88ce7ce56 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -25,6 +25,7 @@ > #include "debug.h" > #include "lflow-mgr.h" > #include "lib/ovn-parallel-hmap.h" > +#include "lib/ovn-util.h" > > VLOG_DEFINE_THIS_MODULE(lflow_mgr); > > @@ -36,7 +37,7 @@ static void ovn_lflow_init(struct ovn_lflow *, struct > ovn_datapath *od, > uint16_t priority, char *match, > char *actions, char *io_port, > char *ctrl_meter, char *stage_hint, > - const char *where); > + const char *where, const char *flow_desc); > static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows, > enum ovn_stage stage, > uint16_t priority, const char > *match, > @@ -52,7 +53,7 @@ static struct ovn_lflow *do_ovn_lflow_add( > const char *actions, const char *io_port, > const char *ctrl_meter, > const struct ovsdb_idl_row *stage_hint, > - const char *where); > + const char *where, const char *flow_desc); > > > static struct ovs_mutex *lflow_hash_lock(const struct hmap *lflow_table, > @@ -173,6 +174,7 @@ struct ovn_lflow { > * 'dpg_bitmap'. */ > struct ovn_dp_group *dpg; /* Link to unique Sb datapath group. */ > const char *where; > + const char *flow_desc; > > struct uuid sb_uuid; /* SB DB row uuid, specified by northd. > */ > struct ovs_list referenced_by; /* List of struct lflow_ref_node. */ > @@ -659,7 +661,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > const char *match, const char *actions, > const char *io_port, const char *ctrl_meter, > const struct ovsdb_idl_row *stage_hint, > - const char *where, > + const char *where, const char *flow_desc, > struct lflow_ref *lflow_ref) > OVS_EXCLUDED(fake_hash_mutex) > { > @@ -679,7 +681,7 @@ lflow_table_add_lflow(struct lflow_table *lflow_table, > do_ovn_lflow_add(lflow_table, > od ? ods_size(od->datapaths) : dp_bitmap_len, > hash, stage, priority, match, actions, > - io_port, ctrl_meter, stage_hint, where); > + io_port, ctrl_meter, stage_hint, where, > flow_desc); > > if (lflow_ref) { > struct lflow_ref_node *lrn = > @@ -733,7 +735,7 @@ lflow_table_add_lflow_default_drop(struct lflow_table > *lflow_table, > { > lflow_table_add_lflow(lflow_table, od, NULL, 0, stage, 0, "1", > debug_drop_action(), NULL, NULL, NULL, > - where, lflow_ref); > + where, NULL, lflow_ref); > } > > struct ovn_dp_group * > @@ -856,7 +858,8 @@ static void > ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od, > size_t dp_bitmap_len, enum ovn_stage stage, uint16_t > priority, > char *match, char *actions, char *io_port, char > *ctrl_meter, > - char *stage_hint, const char *where) > + char *stage_hint, const char *where, > + const char *flow_desc) > { > lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len); > lflow->od = od; > @@ -867,6 +870,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct > ovn_datapath *od, > lflow->io_port = io_port; > lflow->stage_hint = stage_hint; > lflow->ctrl_meter = ctrl_meter; > + lflow->flow_desc = flow_desc; > lflow->dpg = NULL; > lflow->where = where; > lflow->sb_uuid = UUID_ZERO; > @@ -960,7 +964,7 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, > size_t dp_bitmap_len, > const char *match, const char *actions, > const char *io_port, const char *ctrl_meter, > const struct ovsdb_idl_row *stage_hint, > - const char *where) > + const char *where, const char *flow_desc) > OVS_REQUIRES(fake_hash_mutex) > { > struct ovn_lflow *old_lflow; > @@ -982,7 +986,8 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, > size_t dp_bitmap_len, > xstrdup(match), xstrdup(actions), > io_port ? xstrdup(io_port) : NULL, > nullable_xstrdup(ctrl_meter), > - ovn_lflow_hint(stage_hint), where); > + ovn_lflow_hint(stage_hint), where, > + flow_desc); > > if (parallelization_state != STATE_USE_PARALLELIZATION) { > hmap_insert(&lflow_table->entries, &lflow->hmap_node, hash); > @@ -1050,6 +1055,7 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > sbrec_logical_flow_set_priority(sbflow, lflow->priority); > sbrec_logical_flow_set_match(sbflow, lflow->match); > sbrec_logical_flow_set_actions(sbflow, lflow->actions); > + sbrec_logical_flow_set_flow_desc(sbflow, lflow->flow_desc); > if (lflow->io_port) { > struct smap tags = SMAP_INITIALIZER(&tags); > smap_add(&tags, "in_out_port", lflow->io_port); > diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h > index 83b087f47..2c05b352d 100644 > --- a/northd/lflow-mgr.h > +++ b/northd/lflow-mgr.h > @@ -78,7 +78,8 @@ void lflow_table_add_lflow(struct lflow_table *, const > struct ovn_datapath *, > const char *actions, const char *io_port, > const char *ctrl_meter, > const struct ovsdb_idl_row *stage_hint, > - const char *where, struct lflow_ref *); > + const char *where, const char *flow_desc, > + struct lflow_ref *); > void lflow_table_add_lflow_default_drop(struct lflow_table *, > const struct ovn_datapath *, > enum ovn_stage stage, > @@ -91,20 +92,20 @@ void lflow_table_add_lflow_default_drop(struct > lflow_table *, > STAGE_HINT, LFLOW_REF) \ > lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > ACTIONS, IN_OUT_PORT, CTRL_METER, STAGE_HINT, \ > - OVS_SOURCE_LOCATOR, LFLOW_REF) > + OVS_SOURCE_LOCATOR, NULL, LFLOW_REF) > > #define ovn_lflow_add_with_hint(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, \ > ACTIONS, STAGE_HINT, LFLOW_REF) \ > lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > ACTIONS, NULL, NULL, STAGE_HINT, \ > - OVS_SOURCE_LOCATOR, LFLOW_REF) > + OVS_SOURCE_LOCATOR, NULL, LFLOW_REF) > > #define ovn_lflow_add_with_dp_group(LFLOW_TABLE, DP_BITMAP, > DP_BITMAP_LEN, \ > STAGE, PRIORITY, MATCH, ACTIONS, \ > STAGE_HINT, LFLOW_REF) \ > lflow_table_add_lflow(LFLOW_TABLE, NULL, DP_BITMAP, DP_BITMAP_LEN, > STAGE, \ > PRIORITY, MATCH, ACTIONS, NULL, NULL, > STAGE_HINT, \ > - OVS_SOURCE_LOCATOR, LFLOW_REF) > + OVS_SOURCE_LOCATOR, NULL, LFLOW_REF) > > #define ovn_lflow_add_default_drop(LFLOW_TABLE, OD, STAGE, LFLOW_REF) \ > lflow_table_add_lflow_default_drop(LFLOW_TABLE, OD, STAGE, \ > @@ -126,13 +127,27 @@ void lflow_table_add_lflow_default_drop(struct > lflow_table *, > STAGE_HINT, LFLOW_REF) \ > lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > ACTIONS, IN_OUT_PORT, NULL, STAGE_HINT, \ > - OVS_SOURCE_LOCATOR, LFLOW_REF) > + OVS_SOURCE_LOCATOR, NULL, LFLOW_REF) > > #define ovn_lflow_add(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \ > LFLOW_REF) \ > lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > ACTIONS, NULL, NULL, NULL, OVS_SOURCE_LOCATOR, \ > - LFLOW_REF) > + NULL, LFLOW_REF) > + > +#define ovn_lflow_add_drop_with_desc(LFLOW_TABLE, OD, STAGE, PRIORITY, > MATCH, \ > + DESCRIPTION, LFLOW_REF) \ > + lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > + debug_drop_action(), NULL, NULL, NULL, \ > + OVS_SOURCE_LOCATOR, DESCRIPTION, LFLOW_REF) > + > +#define ovn_lflow_add_drop_with_lport_hint_and_desc(LFLOW_TABLE, OD, > STAGE, \ > + PRIORITY, MATCH, \ > + IN_OUT_PORT, > STAGE_HINT, \ > + DESCRIPTION, > LFLOW_REF) \ > + lflow_table_add_lflow(LFLOW_TABLE, OD, NULL, 0, STAGE, PRIORITY, > MATCH, \ > + debug_drop_action(), IN_OUT_PORT, NULL, > STAGE_HINT, \ > + OVS_SOURCE_LOCATOR, DESCRIPTION, LFLOW_REF) > > #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, > ACTIONS, \ > CTRL_METER, LFLOW_REF) \ > diff --git a/northd/northd.c b/northd/northd.c > index 588edadce..04647e614 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -5982,9 +5982,10 @@ build_lswitch_output_port_sec_od(struct > ovn_datapath *od, > REGBIT_PORT_SEC_DROP" = check_out_port_sec(); next;", > lflow_ref); > > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50, > - REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(), > - lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50, > + REGBIT_PORT_SEC_DROP" == 1", > + "Packet does not follow port security rules", lflow_ref); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 0, > "1", "output;", lflow_ref); > } > @@ -9308,10 +9309,11 @@ > build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op, > port->json_key, > op->lsp_addrs[i].ea_s, op->json_key, > rp->lsp_addrs[k].ipv4_addrs[l].addr_s); > - ovn_lflow_add_with_lport_and_hint( > + ovn_lflow_add_drop_with_lport_hint_and_desc( > lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100, > - ds_cstr(&match), debug_drop_action(), port->key, > - &op->nbsp->header_, lflow_ref); > + ds_cstr(&match), port->key, > + &op->nbsp->header_, > + "Drop ARP for unknown router ports", lflow_ref); > } > for (size_t l = 0; l < rp->lsp_addrs[k].n_ipv6_addrs; > l++) { > ds_clear(&match); > @@ -9324,10 +9326,11 @@ > build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op, > rp->lsp_addrs[k].ipv6_addrs[l].addr_s, > rp->lsp_addrs[k].ipv6_addrs[l].sn_addr_s, > rp->lsp_addrs[k].ipv6_addrs[l].addr_s); > - ovn_lflow_add_with_lport_and_hint( > + ovn_lflow_add_drop_with_lport_hint_and_desc( > lflows, op->od, S_SWITCH_IN_EXTERNAL_PORT, 100, > - ds_cstr(&match), debug_drop_action(), port->key, > - &op->nbsp->header_, lflow_ref); > + ds_cstr(&match), port->key, > + &op->nbsp->header_, "Drop ND for unbound router > ports", > + lflow_ref); > } > > ds_clear(&match); > @@ -9338,13 +9341,14 @@ > build_drop_arp_nd_flows_for_unbound_router_ports(struct ovn_port *op, > port->json_key, > op->lsp_addrs[i].ea_s, rp->lsp_addrs[k].ea_s, > op->json_key); > - ovn_lflow_add_with_lport_and_hint(lflows, op->od, > - > S_SWITCH_IN_EXTERNAL_PORT, > - 100, ds_cstr(&match), > - debug_drop_action(), > - port->key, > - &op->nbsp->header_, > - lflow_ref); > + ovn_lflow_add_drop_with_lport_hint_and_desc( > + lflows, op->od, > + S_SWITCH_IN_EXTERNAL_PORT, > + 100, ds_cstr(&match), > + port->key, > + &op->nbsp->header_, > + "Packet does not come from a chassis resident", > + lflow_ref); > } > } > } > @@ -9369,9 +9373,9 @@ build_lswitch_lflows_l2_unknown(struct ovn_datapath > *od, > "outport = \""MC_UNKNOWN "\"; output;", > lflow_ref); > } else { > - ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, > - "outport == \"none\"", debug_drop_action(), > - lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, "outport == \"none\"", > + "No L2 destination", lflow_ref); > } > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", > "output;", lflow_ref); > @@ -9405,31 +9409,36 @@ build_lswitch_lflows_admission_control(struct > ovn_datapath *od, > ovs_assert(od->nbs); > > /* Default action for recirculated ICMP error 'packet too big'. */ > - ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105, > - "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > - " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&" > - " flags.tunnel_rx == 1", debug_drop_action(), > lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105, > + "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||" > + " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&" > + " flags.tunnel_rx == 1", "ICMP: packet too big", lflow_ref); > > /* Logical VLANs not supported. */ > if (!is_vlan_transparent(od)) { > /* Block logical VLANs. */ > - ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100, > - "vlan.present", debug_drop_action(), > - lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, > + 100, "vlan.present", > + "VLANs blocked due to vlan-passthru option", > + lflow_ref); > } > > /* Broadcast/multicast source address is invalid. */ > - ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100, > - "eth.src[40]", debug_drop_action(), > - lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 100, > + "eth.src[40]", "Incoming Broadcast/multicast source" \ > + " address is invalid", lflow_ref); > > ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 50, "1", > REGBIT_PORT_SEC_DROP" = check_in_port_sec(); next;", > lflow_ref); > > - ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50, > - REGBIT_PORT_SEC_DROP" == 1", debug_drop_action(), > - lflow_ref); > + ovn_lflow_add_drop_with_desc( > + lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 50, > + REGBIT_PORT_SEC_DROP" == 1", > + "Broadcast/multicast port security invalid", lflow_ref); > > ovn_lflow_add(lflows, od, S_SWITCH_IN_APPLY_PORT_SEC, 0, "1", "next;", > lflow_ref); > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index b6c051ae6..ec39fdd81 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > - "version": "20.34.0", > - "cksum": "2786607656 31376", > + "version": "20.35.0", > + "cksum": "2897301415 31493", > "tables": { > "SB_Global": { > "columns": { > @@ -116,7 +116,9 @@ > "min": 0, "max": 1}}, > "external_ids": { > "type": {"key": "string", "value": "string", > - "min": 0, "max": "unlimited"}}}, > + "min": 0, "max": "unlimited"}}, > + "flow_desc": {"type": {"key": {"type": "string"}, > + "min": 0, "max": 1}}}, > "isRoot": true}, > "Logical_DP_Group": { > "columns": { > diff --git a/ovn-sb.xml b/ovn-sb.xml > index d91d77f83..cbcbbfb53 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -2904,6 +2904,11 @@ tcp.flags = RST; > <code>ovn-controller</code>. > </column> > > + <column name="flow_desc"> > + Human-readable explanation of the flow, this is optional and used > + to provide context for the given flow. > + </column> > + > <column name="external_ids" key="stage-name"> > Human-readable name for this flow's stage in the pipeline. > </column> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 2830ab598..159d61a16 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12360,6 +12360,21 @@ AT_CHECK([grep -e "DHCP_RELAY_" lflows | sed > 's/table=../table=??/'], [0], [dnl > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([check for flow_desc]) > +ovn_start > + > +check ovn-nbctl -- set NB_Global . options:debug_drop_collector_set="123" > +check ovn-nbctl ls-add ls1 > + > +check ovn-nbctl --wait=hv sync > + > +flow_desc=$(fetch_column Logical_flow flow_desc match='"outport == > \"none\""') > +AT_CHECK([test "$flow_desc" != ""]) > + > +AT_CLEANUP > +]) > + > AT_SETUP([NB_Global and SB_Global incremental processing]) > > ovn_start > -- > 2.45.2 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
