On 2/13/25 3:22 PM, Felix Huettner wrote: > On Thu, Feb 13, 2025 at 01:28:02PM +0100, Ilya Maximets wrote: >> On 2/13/25 13:25, Ilya Maximets wrote: >>> On 2/13/25 13:18, Felix Huettner wrote: >>>> On Thu, Feb 13, 2025 at 01:12:29PM +0100, Ilya Maximets wrote: >>>>> On 2/11/25 15:37, Dumitru Ceara wrote: >>>>>> On 2/11/25 9:35 AM, Felix Huettner via dev wrote: >>>>>>> in order to exchange routes between OVN and the network fabric we >>>>>>> use the new Advertised_Route sb table. Northd here advertises all routes >>>>>>> where the user explicitly opted-in. >>>>>>> >>>>>>> ovn-controller will later use this table to share these routes to the >>>>>>> outside. >>>>>>> >>>>>>> Acked-by: Dumitru Ceara <[email protected]> >>>>>>> Signed-off-by: Felix Huettner <[email protected]> >>>>>>> --- >>>>>> >>>>>> Hi Felix, >>>>>> >>>>>> I applied this patch to main with the following minor style changes: >>>>> >>>>> <snip> >>>>> >>>>>>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>>>>>> +AT_SETUP([dynamic-routing incremental processing]) >>>>>>> +AT_KEYWORDS([dynamic-routing]) >>>>>>> +ovn_start >>>>>>> + >>>>>>> +# Test I-P for dynamic-routing. >>>>>>> +# Presently ovn-northd has no I-P for Advertised_Route. >>>>>>> +# Wait for sb to be connected before clearing stats. >>>>>>> +check ovn-nbctl --wait=sb sync >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl lr-add lr0 >>>>>>> +check ovn-nbctl --wait=sb set Logical_Router lr0 >>>>>>> option:dynamic-routing=true >>>>>>> + >>>>>>> +check_engine_stats northd recompute nocompute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 >>>>>>> 10.0.0.1/24 >>>>>>> +check_engine_stats northd recompute compute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 >>>>>>> 10.0.1.1/24 >>>>>>> +check_engine_stats northd recompute compute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lr-route-add lr0 192.168.0.0/24 10.0.0.10 >>>>>>> +check_engine_stats northd recompute nocompute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw2 00:00:00:00:ff:03 >>>>>>> 2001:db8::1/64 fe80::1/64 >>>>>>> +check_engine_stats northd recompute compute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb remove Logical_Router lr0 option >>>>>>> dynamic-routing >>>>>>> +check_engine_stats northd recompute nocompute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb set Logical_Router lr0 >>>>>>> option:dynamic-routing=true >>>>>>> +check_engine_stats northd recompute nocompute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lrp-del lr0-sw0 >>>>>>> +check_engine_stats northd recompute compute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>>>>>> +check ovn-nbctl --wait=sb lr-del lr0 >>>>>>> +check_engine_stats northd recompute nocompute >>>>>>> +check_engine_stats routes recompute nocompute >>>>>>> +check_engine_stats advertised_route_sync recompute nocompute >>>>>>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>>>>>> + >>>>>>> +AT_CLEANUP >>>>>>> +]) >>>>> >>>>> Hi, Felix and Dumitru. >>>>> >>>>> Just an FYI, this test seems to be unstable on arm with Cirrus CI and >>>>> fails regularly. >>>>> For example, on my fork: >>>>> https://cirrus-ci.com/task/5960613278515200?logs=build#L2624 >>>>> >>>>> If someone could take a look, that would be great. >>>> >>>> Hi Ilya, >>>> >>>> i have an arm machine available and will try to reproduce the issue. >>> >> >> CC: Felix (not sure why thunderbird dropped some people from To/Cc) >> >>> Thanks! FWIW, it's unlikely to be architecture-specific, more likely to >>> just >>> be a timing issue on a less powerful system. >>> >>> If you can't reproduce it yourself, cirrus allows to get a debug console >>> access >>> to the running test system for some decent, though limited, amount of time. >>> See the 'Re-run with a Terminal Access' option. So, that can also be used >>> for >>> debugging right inside the CI env, if necessary. > > Hi Ilya, > > i managed to reproduce this issue on an arm node, if i only have 1 cpu > and parallely run "stress --cpu 2" :) > > The issue seems to be that in some cases northd will batch multiple > northbound/southbound changes together in one inc-engine run. > Depending on what exactly that is that might mean that some runs will > handle in one recompute what others handle in a recompute + compute. > > My current idea is to add a new option to check_engine_stats that allows > you to ignore the compute value. > > I think we generally only care about 3 options on each engine node: > 1. Something was recomputed (no full incremental handling) > 2. Nothing was recomputed but something was computed (full incremental > handling) > 3. No recompute nor compute (no change at all) > > I think having a new option to ignore compute values would not interfere > with the above if only used in combination with checking for recomputes. > > If that sounds reasonable to you then i would send out a patch for that. >
I think that's exactly the reason why some of incremental processing tests for LB syncing to SB ignore "compute" stats: https://github.com/ovn-org/ovn/blob/1098d316be577f5bd5c2f9d4aedb192b6df08094/tests/ovn-northd.at#L10863 Added when ovn-northd LB I-P support was implemented: https://github.com/ovn-org/ovn/commit/5e5aeaa9f133ab021d48f61905fc3035ae4220b8#diff-ad86f2621d498f3f310f111c1a92fe47589f0c2d51529fa9ba98371b32c00a9e It sounds reasonable to me to ignore "compute" stats in these kinds of cases. Regards, Dumitru > Thanks a lot, > Felix > > >>> >>>> >>>> Thanks for bringing that up, >>>> Felix >>>> >>>>> >>>>> Best regards, Ilya Maximets. >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
