| Message ID | 20240520122424.1701398-1-jtanenba@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev] text respresntations for drop sampling. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
I am aware of the mistake in the commit message, this patch does not add the flow_desc to the commit message of the action. This will be fixed in v2. Thank you On Mon, May 20, 2024 at 8:24 AM Jacob Tanenbaum <jtanenba@redhat.com> 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 > > Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com> > Suggested-by: Dumitru Ceara <dceara@redhat.com> > 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; > 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) > > #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, > ACTIONS, \ > CTRL_METER, LFLOW_REF) \ > @@ -186,4 +193,4 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct > ovn_dp_group *dpg) > } > } > > -#endif /* LFLOW_MGR_H */ > \ No newline at end of file > +#endif /* LFLOW_MGR_H */ > diff --git a/northd/northd.c b/northd/northd.c > index 0cabda7ea..14be8347f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -8733,8 +8733,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_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, > + "outport == \"none\"", > + "NO L2 DEST", > lflow_ref); > } > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index b6c051ae6..dc3384d29 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > "version": "20.34.0", > - "cksum": "2786607656 31376", > + "cksum": "3752487770 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 507a0b571..93a57cd06 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 > + 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 680d96675..2adc2a529 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12371,6 +12371,22 @@ 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" \ > + -- set NB_Global . options:debug_drop_domain_id="1" > +check ovn-nbctl --wait=hv sync > + > +ovn-nbctl ls-add ls1 > + > +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.42.0 > >
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 <jtanenba@redhat.com> > Suggested-by: Dumitru Ceara <dceara@redhat.com> > 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. > #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \ > CTRL_METER, LFLOW_REF) \ > @@ -186,4 +193,4 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct ovn_dp_group *dpg) > } > } > > -#endif /* LFLOW_MGR_H */ > \ No newline at end of file > +#endif /* LFLOW_MGR_H */ > diff --git a/northd/northd.c b/northd/northd.c > index 0cabda7ea..14be8347f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -8733,8 +8733,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_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, > + "outport == \"none\"", > + "NO L2 DEST", > lflow_ref); Are you planning to add this flow reference to more flows? > } > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index b6c051ae6..dc3384d29 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > "version": "20.34.0", > - "cksum": "2786607656 31376", > + "cksum": "3752487770 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 507a0b571..93a57cd06 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 > + provide context for the given flow. s/used provide/used to provide/ > + </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 680d96675..2adc2a529 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12371,6 +12371,22 @@ 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" \ > + -- set NB_Global . options:debug_drop_domain_id="1" > +check ovn-nbctl --wait=hv sync > + > +ovn-nbctl ls-add ls1 > + > +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.42.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 5/21/24 11:34, amorenoz@redhat.com 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 <jtanenba@redhat.com> >> Suggested-by: Dumitru Ceara <dceara@redhat.com> >> 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.
On Mon, May 20, 2024 at 2:24 PM Jacob Tanenbaum <jtanenba@redhat.com> 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 > > 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, in addition to what was said already, I have two small comments. > 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; > 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) > > #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, > ACTIONS, \ > CTRL_METER, LFLOW_REF) \ > @@ -186,4 +193,4 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct > ovn_dp_group *dpg) > } > } > > -#endif /* LFLOW_MGR_H */ > \ No newline at end of file > +#endif /* LFLOW_MGR_H */ > diff --git a/northd/northd.c b/northd/northd.c > index 0cabda7ea..14be8347f 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -8733,8 +8733,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_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, > + "outport == \"none\"", > + "NO L2 DEST", > Maybe a personal preference, but it doesn't feel right being all capital. > lflow_ref); > } > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > index b6c051ae6..dc3384d29 100644 > --- a/ovn-sb.ovsschema > +++ b/ovn-sb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Southbound", > "version": "20.34.0", > - "cksum": "2786607656 31376", > + "cksum": "3752487770 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 507a0b571..93a57cd06 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 > + 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 680d96675..2adc2a529 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -12371,6 +12371,22 @@ 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" \ > + -- set NB_Global . options:debug_drop_domain_id="1" > +check ovn-nbctl --wait=hv sync > The sync should be after the ls-add otherwise the test might be flaky on slower systems. + > +ovn-nbctl ls-add ls1 > + > +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.42.0 > > Thanks, Ales
responses inline, thanks for the review! On Tue, May 21, 2024 at 5:35 AM <amorenoz@redhat.com> 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 <jtanenba@redhat.com> > > Suggested-by: Dumitru Ceara <dceara@redhat.com> > > 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? > I do see a use for a dynamically generated string and am changing this for v2 > > > 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. > This could be useful for all flows, I am getting the ball rolling on adding the flow descriptions. This was just where where I decided to start > > > > #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, > ACTIONS, \ > > CTRL_METER, LFLOW_REF) \ > > @@ -186,4 +193,4 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct > ovn_dp_group *dpg) > > } > > } > > > > -#endif /* LFLOW_MGR_H */ > > \ No newline at end of file > > +#endif /* LFLOW_MGR_H */ > > diff --git a/northd/northd.c b/northd/northd.c > > index 0cabda7ea..14be8347f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -8733,8 +8733,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_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, > > + "outport == \"none\"", > > + "NO L2 DEST", > > lflow_ref); > > > Are you planning to add this flow reference to more flows? > yes, but that is future work > > > } > > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema > > index b6c051ae6..dc3384d29 100644 > > --- a/ovn-sb.ovsschema > > +++ b/ovn-sb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Southbound", > > "version": "20.34.0", > > - "cksum": "2786607656 31376", > > + "cksum": "3752487770 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 507a0b571..93a57cd06 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 > > + provide context for the given flow. > > s/used provide/used to provide/ > > > + </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 680d96675..2adc2a529 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -12371,6 +12371,22 @@ 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" \ > > + -- set NB_Global . options:debug_drop_domain_id="1" > > +check ovn-nbctl --wait=hv sync > > + > > +ovn-nbctl ls-add ls1 > > + > > +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.42.0 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
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; 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) #define ovn_lflow_metered(LFLOW_TABLE, OD, STAGE, PRIORITY, MATCH, ACTIONS, \ CTRL_METER, LFLOW_REF) \ @@ -186,4 +193,4 @@ dec_ovn_dp_group_ref(struct hmap *dp_groups, struct ovn_dp_group *dpg) } } -#endif /* LFLOW_MGR_H */ \ No newline at end of file +#endif /* LFLOW_MGR_H */ diff --git a/northd/northd.c b/northd/northd.c index 0cabda7ea..14be8347f 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -8733,8 +8733,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_with_desc(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 50, + "outport == \"none\"", + "NO L2 DEST", lflow_ref); } ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_UNKNOWN, 0, "1", diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema index b6c051ae6..dc3384d29 100644 --- a/ovn-sb.ovsschema +++ b/ovn-sb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Southbound", "version": "20.34.0", - "cksum": "2786607656 31376", + "cksum": "3752487770 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 507a0b571..93a57cd06 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 + 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 680d96675..2adc2a529 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -12371,6 +12371,22 @@ 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" \ + -- set NB_Global . options:debug_drop_domain_id="1" +check ovn-nbctl --wait=hv sync + +ovn-nbctl ls-add ls1 + +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 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 Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com> Suggested-by: Dumitru Ceara <dceara@redhat.com> Reported-at: https://issues.redhat.com/browse/FDP-307