On 12/20/23 19:03, Numan Siddique wrote: > On Mon, Dec 18, 2023 at 9:08 AM Dumitru Ceara <dce...@redhat.com> wrote: >> >> On 11/28/23 03:38, num...@ovn.org wrote: >>> From: Numan Siddique <num...@ovn.org> >>> >>> Any changes to northd engine node due to load balancers >>> are now handled in 'sync_to_sb_lb' node to sync the changed >>> load balancers to SB load balancers. >>> >>> The logic to sync the SB load balancers is changed a bit and it >>> now mimics the SB lflow sync. >>> >>> Below are the scale testing results done with all the patches applied >>> in this series using ovn-heater. The test ran the scenario - >>> ocp-500-density-heavy.yml [1]. >>> >>> The resuts are: >>> >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> Min (s) Median (s) 90%ile (s) >>> 99%ile (s) Max (s) Mean (s) Total (s) Count >>> Failed >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> Iteration Total 0.136883 1.129016 1.192001 >>> 1.204167 1.212728 0.665017 83.127099 125 0 >>> Namespace.add_ports 0.005216 0.005736 0.007034 >>> 0.015486 0.018978 0.006211 0.776373 125 0 >>> WorkerNode.bind_port 0.035030 0.046082 0.052469 >>> 0.058293 0.060311 0.045973 11.493259 250 0 >>> WorkerNode.ping_port 0.005057 0.006727 1.047692 >>> 1.069253 1.071336 0.266896 66.724094 250 0 >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> >>> The results with the present main [2] are: >>> >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> Min (s) Median (s) 90%ile (s) >>> 99%ile (s) Max (s) Mean (s) Total (s) Count >>> Failed >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> Iteration Total 0.135491 2.223805 3.311270 >>> 3.339078 3.345346 1.729172 216.146495 125 0 >>> Namespace.add_ports 0.005380 0.005744 0.006819 >>> 0.018773 0.020800 0.006292 0.786532 125 0 >>> WorkerNode.bind_port 0.034179 0.046055 0.053488 >>> 0.058801 0.071043 0.046117 11.529311 250 0 >>> WorkerNode.ping_port 0.004956 0.006952 3.086952 >>> 3.191743 3.192807 0.791544 197.886026 250 0 >>> ------------------------------------------------------------------------------------------------------------------------------------------------------- >>> >>> [1] - >>> https://github.com/ovn-org/ovn-heater/blob/main/test-scenarios/ocp-500-density-heavy.yml >>> [2] - 2a12cda890a7("controller, northd: Wait for cleanup before replying to >>> exit") >>> >>> Signed-off-by: Numan Siddique <num...@ovn.org> >>> --- >>> northd/en-sync-sb.c | 445 +++++++++++++++++++++++++++++++++++++-- >>> northd/inc-proc-northd.c | 1 + >>> northd/lflow-mgr.c | 196 ++++++----------- >>> northd/lflow-mgr.h | 23 +- >>> northd/northd.c | 238 --------------------- >>> northd/northd.h | 6 - >>> tests/ovn-northd.at | 103 +++++---- >>> 7 files changed, 585 insertions(+), 427 deletions(-) >>> >> >> Hi Numan, >> >> The only reason why we need to not ignore SB.LB updates is because we >> need to check for duplicates that under normal circumstances do not >> exist. They can appear due to "spurious" [0] inserts because the LB >> table doesn't have an index defined. >> >> Would it be less of an effort to just have a periodic task (we do >> similar work for mac binding aging) and in that task (I-P node run >> function) check for SB load balancer duplicates, remove them and trigger >> a full recompute? >> >> Wouldn't that be less error prone than all the I-P code? > > Hi Dumitru, > > Thanks for the reviews. > > Do you mean that by adding this periodic task, we can drop this patch ? > > This patch doesn't do any changes to the existing code which handles > the duplicates. > It adds I-P to the Load balancer changes to the sb lb sync in the > sync_to_sb_lb_northd_handler() function. > > northd node in its tracked data provides the information about deleted > and added/updated load balancers > and sync_to_sb_lb_northd_handler() calls sync_changed_lbs() to sync > the changed LBs in the SB DB. > > Before this patch any change to the NB load balancers/lb groups > resulted in the sync_to_sb_lb full recompute > and this was expensive. Hence added the I-P. > > Since this patch doesn't change anything related to the duplicates, I > think we could still add a periodic task > for that (which would be independent to this patch). >
You're right Numan, I mixed things up. The periodic task would only allow us to avoid the need for sync_to_sb_lb_sb_load_balancer() which checks for duplicates. I'll have another pass at reviewing this patch, sorry for the noise, please disregard my initial review. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
