On 5/21/24 11:34, [email protected] wrote: > On Mon, May 20, 2024 at 08:24:24AM GMT, Jacob Tanenbaum wrote: >> created a new column in the database to hardcode drop reasons for >> default drop actions. 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-reason (NO L2 DEST) */" >> controller_meter : [] >> external_ids : {source="northd.c:8721", stage-name=ls_in_l2_unknown} >> flow_desc : "NO L2 DEST" >> logical_datapath : [] >> logical_dp_group : ee3c3db5-98a2-4f34-8a84-409deae140a7 >> match : "outport == \"none\"" >> pipeline : ingress >> priority : 50 >> table_id : 27 >> tags : {} >> hash : 0 >> >> where the action includes the flow_desc as the drop reason >> > > Thanks for working on this Jacob! > I've added some comments inline. > >> Signed-off-by: Jacob Tanenbaum <[email protected]> >> Suggested-by: Dumitru Ceara <[email protected]> >> Reported-at: https://issues.redhat.com/browse/FDP-307 >> >> diff --git a/NEWS b/NEWS >> index 3b5e93dc9..95d641905 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -17,6 +17,8 @@ Post v24.03.0 >> external-ids, the option is no longer needed as it became effectively >> "true" for all scenarios. >> - Added DHCPv4 relay support. >> + - 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..24c2c17c5 100644 >> --- a/northd/lflow-mgr.c >> +++ b/northd/lflow-mgr.c >> @@ -36,7 +36,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 +52,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 +173,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 +660,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 +680,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 +734,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 +857,7 @@ 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 +868,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; > > IIUC, and please correct me if I'm wrong as I'm not very familiar with > lflow-mgr, the lflow object outlives lflow_table_add_lflow and I don't > see a call to "free(lflow->flow_desc)". This means we can only add static > strings. Do you see any use for a dynamically generated string? > >> lflow->dpg = NULL; >> lflow->where = where; >> lflow->sb_uuid = UUID_ZERO; >> @@ -960,7 +962,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 +984,7 @@ 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 +1052,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..4ad200e7e 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,19 @@ 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_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) >> > > Have you considered making the functionality applicable to all logical > flows, not only for default drops? > > It would be very useful for other non-default drops or even non-drops > flows. > >
+1 it would be a way of documenting the intention of the pipeline so in an ideal world a user could "ovn-sbctl lflow-list" and get a "story" of what will happen to the packet in the pipeline. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
