Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-14 Thread Adrian Moreno 

Hi Han,

I don't really understand why I did not get any email alerting about that the 
latest rebase had broken the build.


Thanks for fixing it so quickly.

On 2/14/23 07:45, Han Zhou wrote:



On Mon, Feb 13, 2023 at 8:56 AM Mark Michelson > wrote:

 >
 > I have merged this to main. Thanks Adrian and Ales!
 >
 > On 2/10/23 03:30, Ales Musil wrote:
 > > On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno > wrote:

 > >
 > >> While testing, I discovered some problems with drop sampling (first 4
 > >> patches).
 > >>
 > >> Also, this series introduces an optimization. In order to avoid adding
 > >> sample actions on Chassis that do not have a Flow_Sample_Collector_Set
 > >> configured (which would generate a useless upcall), make the controller
 > >> monitor this table in OVS and recompute flows when it's changed.
 > >>
 > >> The engine logic is pretty simple since this table is assumed to change
 > >> very rarely.
 > >>
 > >> --
 > >> v1:
 > >> - Fixed commit message in patch 4.
 > >>
 > >> Adrian Moreno (6):
 > >>    controller: fix recompute pflows if sampling changes
 > >>    northd: fix unsampled drops and unit test
 > >>    controller: add missing drop to loopback check table
 > >>    controller: set sampling port to OFP_NONE for drops
 > >>    controller: only sample flow if Collector Set exists
 > >>    controller: only sample pflow if Collector Set exists
 > >>
 > >>   controller/lflow.c          |   1 +
 > >>   controller/lflow.h          |   8 +-
 > >>   controller/ovn-controller.c | 161 +---
 > >>   controller/physical.c       |   2 +
 > >>   include/ovn/actions.h       |   4 +
 > >>   lib/actions.c               |   9 +-
 > >>   lib/ovn-util.c              |  51 
 > >>   lib/ovn-util.h              |  26 +-
 > >>   northd/northd.c             |  17 ++--
 > >>   tests/ovn-performance.at     |  24 ++
 > >>   tests/ovn.at                 |  15 +++-
 > >>   tests/test-ovn.c            |   9 ++
 > >>   12 files changed, 280 insertions(+), 47 deletions(-)
 > >>
 > >> --
 > >> 2.39.1
 > >>
 > >> ___
 > >> dev mailing list
 > >> d...@openvswitch.org 
 > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev 


 > >>
 > >>
 > > Looks good to me, thanks.
 > >
 > > Reviewed-by: Ales Musil mailto:amu...@redhat.com>>
 > >
 >
 > ___
 > dev mailing list
 > d...@openvswitch.org 
 > https://mail.openvswitch.org/mailman/listinfo/ovs-dev 



Hi Mark, Arian,

Sorry that I still didn't get time to review this in detail, but I noticed that 
the last two patches break the build, and all the CI builds are broken.

So I just applied the below hotfix:

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 7c640a9d7dbe..6ea96e2dd6a2 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3166,7 +3166,7 @@ lflow_output_flow_sample_collector_set_handler(struct 
engine_node *node,

      }

      const struct ovsrec_bridge *br_int;
-    br_int = get_bridge(bridge_table, br_int_name(cfg));
+    br_int = get_bridge(bridge_table, br_int_name(ovs_table));
      if (!br_int) {
          return true;
      }
@@ -3212,7 +3212,7 @@ pflow_output_get_debug(struct engine_node *node, struct 
physical_debug *debug)

      }

      const struct ovsrec_bridge *br_int;
-    br_int = get_bridge(bridge_table, br_int_name(ovs_cfg));
+    br_int = get_bridge(bridge_table, br_int_name(ovs_table));
      if (!br_int) {
          return;
      }
=

Thanks,
Han


--
Adrián Moreno

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-14 Thread Mark Michelson 

Thanks Han! Sorry for the mistake.

On 2/14/23 01:45, Han Zhou wrote:



On Mon, Feb 13, 2023 at 8:56 AM Mark Michelson > wrote:

 >
 > I have merged this to main. Thanks Adrian and Ales!
 >
 > On 2/10/23 03:30, Ales Musil wrote:
 > > On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno > wrote:

 > >
 > >> While testing, I discovered some problems with drop sampling (first 4
 > >> patches).
 > >>
 > >> Also, this series introduces an optimization. In order to avoid adding
 > >> sample actions on Chassis that do not have a Flow_Sample_Collector_Set
 > >> configured (which would generate a useless upcall), make the 
controller

 > >> monitor this table in OVS and recompute flows when it's changed.
 > >>
 > >> The engine logic is pretty simple since this table is assumed to 
change

 > >> very rarely.
 > >>
 > >> --
 > >> v1:
 > >> - Fixed commit message in patch 4.
 > >>
 > >> Adrian Moreno (6):
 > >>    controller: fix recompute pflows if sampling changes
 > >>    northd: fix unsampled drops and unit test
 > >>    controller: add missing drop to loopback check table
 > >>    controller: set sampling port to OFP_NONE for drops
 > >>    controller: only sample flow if Collector Set exists
 > >>    controller: only sample pflow if Collector Set exists
 > >>
 > >>   controller/lflow.c          |   1 +
 > >>   controller/lflow.h          |   8 +-
 > >>   controller/ovn-controller.c | 161 
+---

 > >>   controller/physical.c       |   2 +
 > >>   include/ovn/actions.h       |   4 +
 > >>   lib/actions.c               |   9 +-
 > >>   lib/ovn-util.c              |  51 
 > >>   lib/ovn-util.h              |  26 +-
 > >>   northd/northd.c             |  17 ++--
 > >>   tests/ovn-performance.at     |  24 ++
 > >>   tests/ovn.at                 |  15 +++-
 > >>   tests/test-ovn.c            |   9 ++
 > >>   12 files changed, 280 insertions(+), 47 deletions(-)
 > >>
 > >> --
 > >> 2.39.1
 > >>
 > >> ___
 > >> dev mailing list
 > >> d...@openvswitch.org 
 > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev 


 > >>
 > >>
 > > Looks good to me, thanks.
 > >
 > > Reviewed-by: Ales Musil mailto:amu...@redhat.com>>
 > >
 >
 > ___
 > dev mailing list
 > d...@openvswitch.org 
 > https://mail.openvswitch.org/mailman/listinfo/ovs-dev 



Hi Mark, Arian,

Sorry that I still didn't get time to review this in detail, but I 
noticed that the last two patches break the build, and all the CI builds 
are broken.

So I just applied the below hotfix:

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 7c640a9d7dbe..6ea96e2dd6a2 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3166,7 +3166,7 @@ 
lflow_output_flow_sample_collector_set_handler(struct engine_node *node,

      }

      const struct ovsrec_bridge *br_int;
-    br_int = get_bridge(bridge_table, br_int_name(cfg));
+    br_int = get_bridge(bridge_table, br_int_name(ovs_table));
      if (!br_int) {
          return true;
      }
@@ -3212,7 +3212,7 @@ pflow_output_get_debug(struct engine_node *node, 
struct physical_debug *debug)

      }

      const struct ovsrec_bridge *br_int;
-    br_int = get_bridge(bridge_table, br_int_name(ovs_cfg));
+    br_int = get_bridge(bridge_table, br_int_name(ovs_table));
      if (!br_int) {
          return;
      }
=

Thanks,
Han


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-13 Thread Han Zhou 
On Mon, Feb 13, 2023 at 8:56 AM Mark Michelson  wrote:
>
> I have merged this to main. Thanks Adrian and Ales!
>
> On 2/10/23 03:30, Ales Musil wrote:
> > On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno 
wrote:
> >
> >> While testing, I discovered some problems with drop sampling (first 4
> >> patches).
> >>
> >> Also, this series introduces an optimization. In order to avoid adding
> >> sample actions on Chassis that do not have a Flow_Sample_Collector_Set
> >> configured (which would generate a useless upcall), make the controller
> >> monitor this table in OVS and recompute flows when it's changed.
> >>
> >> The engine logic is pretty simple since this table is assumed to change
> >> very rarely.
> >>
> >> --
> >> v1:
> >> - Fixed commit message in patch 4.
> >>
> >> Adrian Moreno (6):
> >>controller: fix recompute pflows if sampling changes
> >>northd: fix unsampled drops and unit test
> >>controller: add missing drop to loopback check table
> >>controller: set sampling port to OFP_NONE for drops
> >>controller: only sample flow if Collector Set exists
> >>controller: only sample pflow if Collector Set exists
> >>
> >>   controller/lflow.c  |   1 +
> >>   controller/lflow.h  |   8 +-
> >>   controller/ovn-controller.c | 161
+---
> >>   controller/physical.c   |   2 +
> >>   include/ovn/actions.h   |   4 +
> >>   lib/actions.c   |   9 +-
> >>   lib/ovn-util.c  |  51 
> >>   lib/ovn-util.h  |  26 +-
> >>   northd/northd.c |  17 ++--
> >>   tests/ovn-performance.at|  24 ++
> >>   tests/ovn.at|  15 +++-
> >>   tests/test-ovn.c|   9 ++
> >>   12 files changed, 280 insertions(+), 47 deletions(-)
> >>
> >> --
> >> 2.39.1
> >>
> >> ___
> >> dev mailing list
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> > Looks good to me, thanks.
> >
> > Reviewed-by: Ales Musil 
> >
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Hi Mark, Arian,

Sorry that I still didn't get time to review this in detail, but I noticed
that the last two patches break the build, and all the CI builds are broken.
So I just applied the below hotfix:

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 7c640a9d7dbe..6ea96e2dd6a2 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3166,7 +3166,7 @@ lflow_output_flow_sample_collector_set_handler(struct
engine_node *node,
 }

 const struct ovsrec_bridge *br_int;
-br_int = get_bridge(bridge_table, br_int_name(cfg));
+br_int = get_bridge(bridge_table, br_int_name(ovs_table));
 if (!br_int) {
 return true;
 }
@@ -3212,7 +3212,7 @@ pflow_output_get_debug(struct engine_node *node,
struct physical_debug *debug)
 }

 const struct ovsrec_bridge *br_int;
-br_int = get_bridge(bridge_table, br_int_name(ovs_cfg));
+br_int = get_bridge(bridge_table, br_int_name(ovs_table));
 if (!br_int) {
 return;
 }
=

Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-13 Thread Mark Michelson 

I have merged this to main. Thanks Adrian and Ales!

On 2/10/23 03:30, Ales Musil wrote:

On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno  wrote:


While testing, I discovered some problems with drop sampling (first 4
patches).

Also, this series introduces an optimization. In order to avoid adding
sample actions on Chassis that do not have a Flow_Sample_Collector_Set
configured (which would generate a useless upcall), make the controller
monitor this table in OVS and recompute flows when it's changed.

The engine logic is pretty simple since this table is assumed to change
very rarely.

--
v1:
- Fixed commit message in patch 4.

Adrian Moreno (6):
   controller: fix recompute pflows if sampling changes
   northd: fix unsampled drops and unit test
   controller: add missing drop to loopback check table
   controller: set sampling port to OFP_NONE for drops
   controller: only sample flow if Collector Set exists
   controller: only sample pflow if Collector Set exists

  controller/lflow.c  |   1 +
  controller/lflow.h  |   8 +-
  controller/ovn-controller.c | 161 +---
  controller/physical.c   |   2 +
  include/ovn/actions.h   |   4 +
  lib/actions.c   |   9 +-
  lib/ovn-util.c  |  51 
  lib/ovn-util.h  |  26 +-
  northd/northd.c |  17 ++--
  tests/ovn-performance.at|  24 ++
  tests/ovn.at|  15 +++-
  tests/test-ovn.c|   9 ++
  12 files changed, 280 insertions(+), 47 deletions(-)

--
2.39.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



Looks good to me, thanks.

Reviewed-by: Ales Musil 



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-10 Thread Ales Musil 
On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno  wrote:

> While testing, I discovered some problems with drop sampling (first 4
> patches).
>
> Also, this series introduces an optimization. In order to avoid adding
> sample actions on Chassis that do not have a Flow_Sample_Collector_Set
> configured (which would generate a useless upcall), make the controller
> monitor this table in OVS and recompute flows when it's changed.
>
> The engine logic is pretty simple since this table is assumed to change
> very rarely.
>
> --
> v1:
> - Fixed commit message in patch 4.
>
> Adrian Moreno (6):
>   controller: fix recompute pflows if sampling changes
>   northd: fix unsampled drops and unit test
>   controller: add missing drop to loopback check table
>   controller: set sampling port to OFP_NONE for drops
>   controller: only sample flow if Collector Set exists
>   controller: only sample pflow if Collector Set exists
>
>  controller/lflow.c  |   1 +
>  controller/lflow.h  |   8 +-
>  controller/ovn-controller.c | 161 +---
>  controller/physical.c   |   2 +
>  include/ovn/actions.h   |   4 +
>  lib/actions.c   |   9 +-
>  lib/ovn-util.c  |  51 
>  lib/ovn-util.h  |  26 +-
>  northd/northd.c |  17 ++--
>  tests/ovn-performance.at|  24 ++
>  tests/ovn.at|  15 +++-
>  tests/test-ovn.c|   9 ++
>  12 files changed, 280 insertions(+), 47 deletions(-)
>
> --
> 2.39.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Reviewed-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.comIM: amusil

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

2023-02-09 Thread Mark Michelson 

Hi Adrian,

Thanks for these patches. They all look good to me. For the series,

Acked-by: Mark Michelson 

On 1/24/23 10:16, Adrián Moreno wrote:

While testing, I discovered some problems with drop sampling (first 4
patches).

Also, this series introduces an optimization. In order to avoid adding
sample actions on Chassis that do not have a Flow_Sample_Collector_Set
configured (which would generate a useless upcall), make the controller
monitor this table in OVS and recompute flows when it's changed.

The engine logic is pretty simple since this table is assumed to change
very rarely.

--
v1:
- Fixed commit message in patch 4.

Adrian Moreno (6):
   controller: fix recompute pflows if sampling changes
   northd: fix unsampled drops and unit test
   controller: add missing drop to loopback check table
   controller: set sampling port to OFP_NONE for drops
   controller: only sample flow if Collector Set exists
   controller: only sample pflow if Collector Set exists

  controller/lflow.c  |   1 +
  controller/lflow.h  |   8 +-
  controller/ovn-controller.c | 161 +---
  controller/physical.c   |   2 +
  include/ovn/actions.h   |   4 +
  lib/actions.c   |   9 +-
  lib/ovn-util.c  |  51 
  lib/ovn-util.h  |  26 +-
  northd/northd.c |  17 ++--
  tests/ovn-performance.at|  24 ++
  tests/ovn.at|  15 +++-
  tests/test-ovn.c|   9 ++
  12 files changed, 280 insertions(+), 47 deletions(-)



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev