| Message ID | 20220513004228.3947905-1-numans@ovn.org |
|---|---|
| Headers | show |
| Series | Adding generic port security flows. | expand |
Hi Numan, I've taken a close look at the patches in this series, and they seem really good. They're very well commented and well tested as well. It's quite easy to follow the change, and I couldn't find any flaws in my review. However, I do want to double-check that this does not put unnecessary load on ovn-controller. I suspect it won't be much of a problem since 1) The port security flows are calculated incrementally. 2) The reduced SB DB size likely lessens the overall load on ovn-controller . 3) The removed port security logical flows means there is less parsing of logical flows per iteration of ovn-controller. However, this does add new OF flow creation in ovn-controller, so it's worth checking to make sure ovn-controller does not see any noticeable performance decrease. If we can get confirmation, then I'll ack the series. On 5/12/22 20:42, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > This patch series adds generic logical flows for port security in > the logical switch pipeline and pushes the actual port security > implementation logic to ovn-controller from ovn-northd. > > ovn-northd will now add logical flows like: > > table=0 (ls_in_check_port_sec), priority=50 , match=(1), action=(reg0[14] = check_in_port_sec(); next;) > table=1 (ls_in_apply_port_sec), priority=50 , match=(reg0[14] == 1), action=(drop;) > table=1 (ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > > OVN action check_in_port_sec() resubmits the packet to openflow table > 73. ovn-controller will add port security flows in table 73,74 and 75 > for all the logical ports it has claimed. The port security information > is passed down the Port_Binding table in Southbound database. > > The main motivation for the patch is to address scale concerns. > This patch series reduces the number of logical flows and ovn-northd > CPU utilization time. > > Did some scale testing and below are the results: > > Used a Northbound database from a deployment of 120 node cluster. > Number of logical switch ports with port security configured: 13711 > > With vanilla ovn-northd > ----------------------- > Number of logical flows : 208061 > Avg time taken to run build_lflows() : 1301 msec > Size of Southbound database after compaction: 104M > > With ovn-northd using this feature > --------------------------------- > Number of logical flows : 83396 > Avg time taken to run build_lflows() : 560 msec > Size of Southbound database after compaction: 45M > > > > Numan Siddique (3): > ovn-controller: Add OF rules for port security. > actions: Add new actions check_in_port_sec and check_out_port_sec. > northd: Add generic port security logical flows. > > controller/binding.c | 78 +++- > controller/binding.h | 23 +- > controller/lflow.c | 792 ++++++++++++++++++++++++++++++++++- > controller/lflow.h | 4 + > controller/ovn-controller.c | 21 +- > include/ovn/actions.h | 6 + > include/ovn/logical-fields.h | 1 + > lib/actions.c | 75 +++- > northd/northd.c | 557 +++++------------------- > northd/ovn-northd.8.xml | 263 ++++++------ > ovn-sb.ovsschema | 7 +- > ovn-sb.xml | 54 +++ > tests/ovn-northd.at | 431 ++++++++++++------- > tests/ovn.at | 369 ++++++++++++++-- > tests/test-ovn.c | 2 + > utilities/ovn-trace.c | 313 ++++++++++++++ > 16 files changed, 2175 insertions(+), 821 deletions(-) >
On Mon, May 16, 2022 at 4:27 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Numan, > > I've taken a close look at the patches in this series, and they seem > really good. They're very well commented and well tested as well. It's > quite easy to follow the change, and I couldn't find any flaws in my > review. > > However, I do want to double-check that this does not put unnecessary > load on ovn-controller. I suspect it won't be much of a problem since > > 1) The port security flows are calculated incrementally. > 2) The reduced SB DB size likely lessens the overall load on > ovn-controller . > 3) The removed port security logical flows means there is less parsing > of logical flows per iteration of ovn-controller. > > However, this does add new OF flow creation in ovn-controller, so it's > worth checking to make sure ovn-controller does not see any noticeable > performance decrease. > > If we can get confirmation, then I'll ack the series. > Thanks Mark for the reviews. Sure. I'll do some tests and share the results. Numan > > On 5/12/22 20:42, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > This patch series adds generic logical flows for port security in > > the logical switch pipeline and pushes the actual port security > > implementation logic to ovn-controller from ovn-northd. > > > > ovn-northd will now add logical flows like: > > > > table=0 (ls_in_check_port_sec), priority=50 , match=(1), > action=(reg0[14] = check_in_port_sec(); next;) > > table=1 (ls_in_apply_port_sec), priority=50 , match=(reg0[14] == 1), > action=(drop;) > > table=1 (ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) > > > > OVN action check_in_port_sec() resubmits the packet to openflow table > > 73. ovn-controller will add port security flows in table 73,74 and 75 > > for all the logical ports it has claimed. The port security information > > is passed down the Port_Binding table in Southbound database. > > > > The main motivation for the patch is to address scale concerns. > > This patch series reduces the number of logical flows and ovn-northd > > CPU utilization time. > > > > Did some scale testing and below are the results: > > > > Used a Northbound database from a deployment of 120 node cluster. > > Number of logical switch ports with port security configured: 13711 > > > > With vanilla ovn-northd > > ----------------------- > > Number of logical flows : 208061 > > Avg time taken to run build_lflows() : 1301 msec > > Size of Southbound database after compaction: 104M > > > > With ovn-northd using this feature > > --------------------------------- > > Number of logical flows : 83396 > > Avg time taken to run build_lflows() : 560 msec > > Size of Southbound database after compaction: 45M > > > > > > > > Numan Siddique (3): > > ovn-controller: Add OF rules for port security. > > actions: Add new actions check_in_port_sec and check_out_port_sec. > > northd: Add generic port security logical flows. > > > > controller/binding.c | 78 +++- > > controller/binding.h | 23 +- > > controller/lflow.c | 792 ++++++++++++++++++++++++++++++++++- > > controller/lflow.h | 4 + > > controller/ovn-controller.c | 21 +- > > include/ovn/actions.h | 6 + > > include/ovn/logical-fields.h | 1 + > > lib/actions.c | 75 +++- > > northd/northd.c | 557 +++++------------------- > > northd/ovn-northd.8.xml | 263 ++++++------ > > ovn-sb.ovsschema | 7 +- > > ovn-sb.xml | 54 +++ > > tests/ovn-northd.at | 431 ++++++++++++------- > > tests/ovn.at | 369 ++++++++++++++-- > > tests/test-ovn.c | 2 + > > utilities/ovn-trace.c | 313 ++++++++++++++ > > 16 files changed, 2175 insertions(+), 821 deletions(-) > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, May 16, 2022 at 5:42 PM Numan Siddique <numans@ovn.org> wrote: > > > On Mon, May 16, 2022 at 4:27 PM Mark Michelson <mmichels@redhat.com> > wrote: > >> Hi Numan, >> >> I've taken a close look at the patches in this series, and they seem >> really good. They're very well commented and well tested as well. It's >> quite easy to follow the change, and I couldn't find any flaws in my >> review. >> >> However, I do want to double-check that this does not put unnecessary >> load on ovn-controller. I suspect it won't be much of a problem since >> >> 1) The port security flows are calculated incrementally. >> 2) The reduced SB DB size likely lessens the overall load on >> ovn-controller . >> 3) The removed port security logical flows means there is less parsing >> of logical flows per iteration of ovn-controller. >> >> However, this does add new OF flow creation in ovn-controller, so it's >> worth checking to make sure ovn-controller does not see any noticeable >> performance decrease. >> >> If we can get confirmation, then I'll ack the series. >> > > Thanks Mark for the reviews. > > Sure. I'll do some tests and share the results. > I did some testing and these are the findings. 1. I started 2 separate ovn-central components, one on central-1 with OVN main commit (as of today's) and the other on central-2 node with OVN main + these port security patches. Same OVN databases on both the nodes. central-1 SB DB has 208962 logical flows central-2 SB DB (with port sec patches) has 84294 logical flows. 2. Started 2 separate compute nodes - compute-1 (with ovn main ovn-controller) and compute-2 (with ovn main + port sec ovn-controller) Created around 1000 ovs ports on compute-1 and copied the conf.db to compute-2 - so that both the ovn-controllers claim the same logical ports. 3. After both the ovn-controllers settle down. I triggered recompute multiple times. This recompute will generate all the openflows (but ofcourse will not program ovs-vswitchd) compute-1 ovn-controller takes around 3100 ms to complete the loop. I see unreasonable long poll interval message and compute-2 ovn-controller takes around 1500ms to complete the loop. I think these patches also help ovn-controller as it has to do less logical flow processing. Below is the stopwatch/show for compute-1 ovn-controller [root@ovn-chassis-1 data]# ovn-appctl -t ovn-controller stopwatch/show flow-generation Statistics for 'flow-generation' Total samples: 679 Maximum: 3435 msec Minimum: 0 msec 95th percentile: 32.512116 msec Short term average: 15.137543 msec Long term average: 109.403161 msec And below is the same for compute-2 ovn-controller [root@ovn-chassis-2 /]# ovn-appctl -t ovn-controller stopwatch/show flow-generation Statistics for 'flow-generation' Total samples: 700 Maximum: 1341 msec Minimum: 0 msec 95th percentile: 2.987580 msec Short term average: 0.000000 msec Long term average: 24.980349 msec Thanks Numan > Numan > >> >> On 5/12/22 20:42, numans@ovn.org wrote: >> > From: Numan Siddique <numans@ovn.org> >> > >> > This patch series adds generic logical flows for port security in >> > the logical switch pipeline and pushes the actual port security >> > implementation logic to ovn-controller from ovn-northd. >> > >> > ovn-northd will now add logical flows like: >> > >> > table=0 (ls_in_check_port_sec), priority=50 , match=(1), >> action=(reg0[14] = check_in_port_sec(); next;) >> > table=1 (ls_in_apply_port_sec), priority=50 , match=(reg0[14] == 1), >> action=(drop;) >> > table=1 (ls_in_apply_port_sec), priority=0 , match=(1), >> action=(next;) >> > >> > OVN action check_in_port_sec() resubmits the packet to openflow table >> > 73. ovn-controller will add port security flows in table 73,74 and 75 >> > for all the logical ports it has claimed. The port security information >> > is passed down the Port_Binding table in Southbound database. >> > >> > The main motivation for the patch is to address scale concerns. >> > This patch series reduces the number of logical flows and ovn-northd >> > CPU utilization time. >> > >> > Did some scale testing and below are the results: >> > >> > Used a Northbound database from a deployment of 120 node cluster. >> > Number of logical switch ports with port security configured: 13711 >> > >> > With vanilla ovn-northd >> > ----------------------- >> > Number of logical flows : 208061 >> > Avg time taken to run build_lflows() : 1301 msec >> > Size of Southbound database after compaction: 104M >> > >> > With ovn-northd using this feature >> > --------------------------------- >> > Number of logical flows : 83396 >> > Avg time taken to run build_lflows() : 560 msec >> > Size of Southbound database after compaction: 45M >> > >> > >> > >> > Numan Siddique (3): >> > ovn-controller: Add OF rules for port security. >> > actions: Add new actions check_in_port_sec and check_out_port_sec. >> > northd: Add generic port security logical flows. >> > >> > controller/binding.c | 78 +++- >> > controller/binding.h | 23 +- >> > controller/lflow.c | 792 ++++++++++++++++++++++++++++++++++- >> > controller/lflow.h | 4 + >> > controller/ovn-controller.c | 21 +- >> > include/ovn/actions.h | 6 + >> > include/ovn/logical-fields.h | 1 + >> > lib/actions.c | 75 +++- >> > northd/northd.c | 557 +++++------------------- >> > northd/ovn-northd.8.xml | 263 ++++++------ >> > ovn-sb.ovsschema | 7 +- >> > ovn-sb.xml | 54 +++ >> > tests/ovn-northd.at | 431 ++++++++++++------- >> > tests/ovn.at | 369 ++++++++++++++-- >> > tests/test-ovn.c | 2 + >> > utilities/ovn-trace.c | 313 ++++++++++++++ >> > 16 files changed, 2175 insertions(+), 821 deletions(-) >> > >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
On 5/18/22 14:17, Numan Siddique wrote: > > > On Mon, May 16, 2022 at 5:42 PM Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> wrote: > > > > On Mon, May 16, 2022 at 4:27 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > Hi Numan, > > I've taken a close look at the patches in this series, and they > seem > really good. They're very well commented and well tested as > well. It's > quite easy to follow the change, and I couldn't find any flaws > in my review. > > However, I do want to double-check that this does not put > unnecessary > load on ovn-controller. I suspect it won't be much of a problem > since > > 1) The port security flows are calculated incrementally. > 2) The reduced SB DB size likely lessens the overall load on > ovn-controller . > 3) The removed port security logical flows means there is less > parsing > of logical flows per iteration of ovn-controller. > > However, this does add new OF flow creation in ovn-controller, > so it's > worth checking to make sure ovn-controller does not see any > noticeable > performance decrease. > > If we can get confirmation, then I'll ack the series. > > > Thanks Mark for the reviews. > > Sure. I'll do some tests and share the results. > > > I did some testing and these are the findings. > > 1. I started 2 separate ovn-central components, one on central-1 with > OVN main commit (as of today's) > and the other on central-2 node with OVN main + these port security > patches. > Same OVN databases on both the nodes. > > central-1 SB DB has 208962 logical flows > central-2 SB DB (with port sec patches) has 84294 logical flows. > > 2. Started 2 separate compute nodes - compute-1 (with ovn main > ovn-controller) and compute-2 (with ovn main + port sec ovn-controller) > Created around 1000 ovs ports on compute-1 and copied the conf.db > to compute-2 - so that both the ovn-controllers claim the same logical > ports. > > 3. After both the ovn-controllers settle down. I triggered recompute > multiple times. This recompute will generate all the openflows (but > ofcourse will not program ovs-vswitchd) > compute-1 ovn-controller takes around 3100 ms to complete the loop. > I see unreasonable long poll interval message and compute-2 > ovn-controller takes around 1500ms to > complete the loop. > > > I think these patches also help ovn-controller as it has to do less > logical flow processing. > > Below is the stopwatch/show for compute-1 ovn-controller > > [root@ovn-chassis-1 data]# ovn-appctl -t ovn-controller stopwatch/show > flow-generation > Statistics for 'flow-generation' > Total samples: 679 > Maximum: 3435 msec > Minimum: 0 msec > 95th percentile: 32.512116 msec > Short term average: 15.137543 msec > Long term average: 109.403161 msec > > And below is the same for compute-2 ovn-controller > > [root@ovn-chassis-2 /]# ovn-appctl -t ovn-controller stopwatch/show > flow-generation > Statistics for 'flow-generation' > Total samples: 700 > Maximum: 1341 msec > Minimum: 0 msec > 95th percentile: 2.987580 msec > Short term average: 0.000000 msec > Long term average: 24.980349 msec > > > Thanks > Numan Excellent work Numan. I suspected that ovn-controller might run the same or slightly slower with this change, but the result is that it's actually quicker! Acked-by: Mark Michelson <mmichels@redhat.com> > > > > Numan > > > On 5/12/22 20:42, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > This patch series adds generic logical flows for port security in > > the logical switch pipeline and pushes the actual port security > > implementation logic to ovn-controller from ovn-northd. > > > > ovn-northd will now add logical flows like: > > > > table=0 (ls_in_check_port_sec), priority=50 , match=(1), > action=(reg0[14] = check_in_port_sec(); next;) > > table=1 (ls_in_apply_port_sec), priority=50 , > match=(reg0[14] == 1), action=(drop;) > > table=1 (ls_in_apply_port_sec), priority=0 , match=(1), > action=(next;) > > > > OVN action check_in_port_sec() resubmits the packet to > openflow table > > 73. ovn-controller will add port security flows in table > 73,74 and 75 > > for all the logical ports it has claimed. The port security > information > > is passed down the Port_Binding table in Southbound database. > > > > The main motivation for the patch is to address scale concerns. > > This patch series reduces the number of logical flows and > ovn-northd > > CPU utilization time. > > > > Did some scale testing and below are the results: > > > > Used a Northbound database from a deployment of 120 node cluster. > > Number of logical switch ports with port security configured: > 13711 > > > > With vanilla ovn-northd > > ----------------------- > > Number of logical flows : 208061 > > Avg time taken to run build_lflows() : 1301 msec > > Size of Southbound database after compaction: 104M > > > > With ovn-northd using this feature > > --------------------------------- > > Number of logical flows : 83396 > > Avg time taken to run build_lflows() : 560 msec > > Size of Southbound database after compaction: 45M > > > > > > > > Numan Siddique (3): > > ovn-controller: Add OF rules for port security. > > actions: Add new actions check_in_port_sec and > check_out_port_sec. > > northd: Add generic port security logical flows. > > > > controller/binding.c | 78 +++- > > controller/binding.h | 23 +- > > controller/lflow.c | 792 > ++++++++++++++++++++++++++++++++++- > > controller/lflow.h | 4 + > > controller/ovn-controller.c | 21 +- > > include/ovn/actions.h | 6 + > > include/ovn/logical-fields.h | 1 + > > lib/actions.c | 75 +++- > > northd/northd.c | 557 +++++------------------- > > northd/ovn-northd.8.xml | 263 ++++++------ > > ovn-sb.ovsschema | 7 +- > > ovn-sb.xml | 54 +++ > > tests/ovn-northd.at <http://ovn-northd.at> | 431 > ++++++++++++------- > > tests/ovn.at <http://ovn.at> | 369 > ++++++++++++++-- > > tests/test-ovn.c | 2 + > > utilities/ovn-trace.c | 313 ++++++++++++++ > > 16 files changed, 2175 insertions(+), 821 deletions(-) > > > > _______________________________________________ > 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> >
From: Numan Siddique <numans@ovn.org> This patch series adds generic logical flows for port security in the logical switch pipeline and pushes the actual port security implementation logic to ovn-controller from ovn-northd. ovn-northd will now add logical flows like: table=0 (ls_in_check_port_sec), priority=50 , match=(1), action=(reg0[14] = check_in_port_sec(); next;) table=1 (ls_in_apply_port_sec), priority=50 , match=(reg0[14] == 1), action=(drop;) table=1 (ls_in_apply_port_sec), priority=0 , match=(1), action=(next;) OVN action check_in_port_sec() resubmits the packet to openflow table 73. ovn-controller will add port security flows in table 73,74 and 75 for all the logical ports it has claimed. The port security information is passed down the Port_Binding table in Southbound database. The main motivation for the patch is to address scale concerns. This patch series reduces the number of logical flows and ovn-northd CPU utilization time. Did some scale testing and below are the results: Used a Northbound database from a deployment of 120 node cluster. Number of logical switch ports with port security configured: 13711 With vanilla ovn-northd ----------------------- Number of logical flows : 208061 Avg time taken to run build_lflows() : 1301 msec Size of Southbound database after compaction: 104M With ovn-northd using this feature --------------------------------- Number of logical flows : 83396 Avg time taken to run build_lflows() : 560 msec Size of Southbound database after compaction: 45M Numan Siddique (3): ovn-controller: Add OF rules for port security. actions: Add new actions check_in_port_sec and check_out_port_sec. northd: Add generic port security logical flows. controller/binding.c | 78 +++- controller/binding.h | 23 +- controller/lflow.c | 792 ++++++++++++++++++++++++++++++++++- controller/lflow.h | 4 + controller/ovn-controller.c | 21 +- include/ovn/actions.h | 6 + include/ovn/logical-fields.h | 1 + lib/actions.c | 75 +++- northd/northd.c | 557 +++++------------------- northd/ovn-northd.8.xml | 263 ++++++------ ovn-sb.ovsschema | 7 +- ovn-sb.xml | 54 +++ tests/ovn-northd.at | 431 ++++++++++++------- tests/ovn.at | 369 ++++++++++++++-- tests/test-ovn.c | 2 + utilities/ovn-trace.c | 313 ++++++++++++++ 16 files changed, 2175 insertions(+), 821 deletions(-)