responses inline, thanks for the review!
On Tue, May 21, 2024 at 5:35 AM <[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?
>
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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
