Re: [ovs-dev] [PATCH ovn v4] Text respresntations for drop sampling.

2024-06-06 Thread Dumitru Ceara 
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 
>> Suggested-by: Dumitru Ceara 
>> 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 
>> *

Re: [ovs-dev] [PATCH ovn v4] Text respresntations for drop sampling.

2024-06-06 Thread Adrián Moreno 
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?

>
> Signed-off-by: Jacob Tanenbaum 
> Suggested-by: Dumitru Ceara 
> 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,
>

Re: [ovs-dev] [PATCH ovn v4] Text respresntations for drop sampling.

2024-06-03 Thread Ales Musil 
On Mon, Jun 3, 2024 at 9:47 PM 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.
>
> Signed-off-by: Jacob Tanenbaum 
> Suggested-by: Dumitru Ceara 
> 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
>

[ovs-dev] [PATCH ovn v4] Text respresntations for drop sampling.

2024-06-03 Thread Jacob Tanenbaum 
Created a new column in the southbound database to hardcode a human readable
description for flows. This first use is describing why the flow is dropping 
packets.
The new column is called flow_desc and will create southbound database entries 
like this

_uuid   : 20f1897b-477e-47ae-a32c-c546d83ec097
actions : 
"sample(probability=65535,collector_set=123,obs_domain=1,obs_point=$cookie); /* 
drop */"
controller_meter: []
external_ids: {source="northd.c:8721", stage-name=ls_in_l2_unknown}
flow_desc   : "No L2 destination"
logical_datapath: []
logical_dp_group: ee3c3db5-98a2-4f34-8a84-409deae140a7
match   : "outport == \"none\""
pipeline: ingress
priority: 50
table_id: 27
tags: {}
hash: 0

future work includes entering more flow_desc for more flows and adding
flow_desc to the actions as a comment.

Signed-off-by: Jacob Tanenbaum 
Suggested-by: Dumitru Ceara 
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, s