Nit: typo in subject: "respresntations". And maybe it should be
something like:
Add text representations for drop flows.
On 6/6/24 11:17, Adrián Moreno wrote:
> On Mon, Jun 03, 2024 at 03:47:25PM GMT, Jacob Tanenbaum 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.
>
> Is this future work planned for the next OVN version?
>
Given that OVN release cadence is every 6 months I'd suggest we do this
now so we have a usable feature in v24.09.0. The patch will be a bit
larger but it seems rather straightforward.
Wdyt Jacob?
>>
>> Signed-off-by: Jacob Tanenbaum <[email protected]>
>> Suggested-by: Dumitru Ceara <[email protected]>
>> Reported-at: https://issues.redhat.com/browse/FDP-307
>>
>> ---
>>
>> v1: initial version
>> v2: correct commit message
>> make the flow_desc a char*
>> correct a typo in the ovn-sb.xml
>> correct the test
>> v3: rebase issue with NEWS file
>> v4: remove options:debug_drop_domain_id="1" from testing as we
>> do not depend on it
>>
>> merge
>>
>> diff --git a/NEWS b/NEWS
>> index 81c958f9a..04c441ada 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -21,6 +21,8 @@ Post v24.03.0
>> MAC addresses configured on the LSP with "unknown", are learnt via the
>> OVN native FDB.
>> - Add support for ovsdb-server `--config-file` option in ovn-ctl.
>> + - 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..e27558a32 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, 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;
>> + 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, 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;
>> @@ -946,6 +948,7 @@ ovn_lflow_destroy(struct lflow_table *lflow_table,
>> struct ovn_lflow *lflow)
>> free(lflow->io_port);
>> free(lflow->stage_hint);
>> free(lflow->ctrl_meter);
>> + free(lflow->flow_desc);
>> ovn_lflow_clear_dp_refcnts_map(lflow);
>> struct lflow_ref_node *lrn;
>> LIST_FOR_EACH_SAFE (lrn, ref_list_node, &lflow->referenced_by) {
>> @@ -960,7 +963,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 +985,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 ? xstrdup(flow_desc): NULL);
>>
>
> For now the only "flow_desc" is a static string, but I guess the goal is
> to support dynamically created strings. When we get there, it's likely
> that we end up copying the string unnecessarily. I was wondering if it's
> worth moving the xstrdup to the macros and have a "_nocopy" version that
> just takes ownership of the string.
Sounds good to me too.
> I see match and actions are always copied, but they might have more
> reuse than "flow_desc", so I'm not sure, maybe it's not worth it.
> What do you think?
>
>
>> if (parallelization_state != STATE_USE_PARALLELIZATION) {
>> hmap_insert(&lflow_table->entries, &lflow->hmap_node, hash);
>> @@ -1050,6 +1054,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 a78cbcd53..dfb2a1cd0 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -8743,8 +8743,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 destination",
>> 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",
The version needs to be updated too to ensure CMS upgrades/downgrades
work properly.
>> - "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..496c5a242 100644
>> --- a/ovn-sb.xml
>> +++ b/ovn-sb.xml
>> @@ -2913,6 +2913,11 @@ tcp.flags = RST;
>> <code>ovn-controller</code>.
>> </column>
>>
>> + <column name="flow_desc">
>> + Human-readable explanation of the flow, this is optional and used
>> + to provide context for the given flow.
>> + </column>
>> +
>> <column name="external_ids" key="stage-name">
>> Human-readable name for this flow's stage in the pipeline.
>> </column>
>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>> index f3ffb4a6d..fc9abdeaf 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -12439,6 +12439,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"
>> +ovn-nbctl ls-add ls1
Missing "check".
>> +
>> +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.42.0
>>
>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
