| Message ID | 20230124151632.695912-1-amorenoz@redhat.com |
|---|---|
| Headers | show |
| Series | drop sampling: Fixes and optimizations | expand |
Hi Adrian, Thanks for these patches. They all look good to me. For the series, Acked-by: Mark Michelson <mmichels@redhat.com> 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(-) >
On Tue, Jan 24, 2023 at 4:18 PM Adrián Moreno <amorenoz@redhat.com> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
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 <amorenoz@redhat.com> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Reviewed-by: Ales Musil <amusil@redhat.com> >
On Mon, Feb 13, 2023 at 8:56 AM Mark Michelson <mmichels@redhat.com> 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 <amorenoz@redhat.com> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > Looks good to me, thanks. > > > > Reviewed-by: Ales Musil <amusil@redhat.com> > > > > _______________________________________________ > dev mailing list > dev@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
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 <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 <amorenoz@redhat.com > <mailto:amorenoz@redhat.com>> 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 <http://ovn-performance.at> | 24 ++++++ > > >> tests/ovn.at <http://ovn.at> | 15 +++- > > >> tests/test-ovn.c | 9 ++ > > >> 12 files changed, 280 insertions(+), 47 deletions(-) > > >> > > >> -- > > >> 2.39.1 > > >> > > >> _______________________________________________ > > >> dev mailing list > > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > >> > > >> > > > Looks good to me, thanks. > > > > > > Reviewed-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>> > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <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
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 <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 <amorenoz@redhat.com > <mailto:amorenoz@redhat.com>> 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 <http://ovn-performance.at> | 24 ++++++ > > >> tests/ovn.at <http://ovn.at> | 15 +++- > > >> tests/test-ovn.c | 9 ++ > > >> 12 files changed, 280 insertions(+), 47 deletions(-) > > >> > > >> -- > > >> 2.39.1 > > >> > > >> _______________________________________________ > > >> dev mailing list > > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > > >> > > >> > > > Looks good to me, thanks. > > > > > > Reviewed-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com>> > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <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