On 8/10/23 18:15, Han Zhou wrote: > Thanks Vladislav and Dumitru for reporting and fixing the issue. > The impact of the issue is more than just the memory spikes in > ovn-controller. More importantly, it incurs much higher load on SB DB > because the conditions are flooded with all the localnet ports regardless > of whether they belong to local datapaths. > Please see more details in a related fix I just posted: > https://patchwork.ozlabs.org/project/ovn/patch/20230810155351.2853369-1-hz...@ovn.org/ > > In addition, I want to comment on Dumitru's patch (sorry I still didn't see > you posted for review yet) > https://github.com/dceara/ovn/commit/f5e8b9bcba61a2528b67854bb4211981a99feaa8, > I think it is better to avoid using the "optional" version for port_binding > monitoring. For the same reason, if my patch above is not applied, the SB > DB overload problem is still there because the unconditional monitoring of > port_binding initially adds all localnet ports to local_lports and later to > the monitor conditions. In addition, if there are a huge amount of PBs, it > may still cause memory pressure. I understand the intention of avoiding > churns of building local_datapaths, but it may be better to be addressed in > a separate patch and we can review and discuss accordingly. >
The unfortunate part is the fact that the local datapath "churn" will also cause conntrack zones to be flushed and disrupt traffic. What's worse (in the ovn-k8s case) is that some of the zones (zone 0) might be shared with the host. So, just restarting ovn-controller (with conditional monitoring enabled) will trigger conntrack entries to be cleared for non-OVN sessions too. > Still, looking forward to the formal patch for this. > It's still on my todo list, I didn't get a chance to do this yet, sorry. Regards, Dumitru > Thanks, > Han > > On Tue, Aug 1, 2023 at 3:09 AM Vladislav Odintsov <odiv...@gmail.com> wrote: >> >> Hi Dumitru! >> >> I’ve performed test on the host, where ovn-controller (22.09.x) without > your patch and without any local datapaths consumed 3+GiB of ram after > start and the process start took 100% CPU during ~40 seconds after start. >> With your patch ovn-controller starts during ~5 seconds with max cpu > consumption seen in top ~35-36%. >> Memory consumption doesn’t exceed 220-222 MiB on the same host with same > NB/SB contents. >> >> Many thanks for this improvement! >> >> I guess after this patch got merged to main branch, it should be > backported down to 22.03 (branch, to which initial commit [0] introduced > such behaviour was backported). >> >> 0: > https://github.com/ovn-org/ovn/commit/525829c47d09a114de8536d23784c458977d8321 >> >>> On 31 Jul 2023, at 14:43, Vladislav Odintsov <odiv...@gmail.com> wrote: >>> >>> Thanks Dumitru! >>> >>> I’ll test this patch in a few days. >>> >>>> On 28 Jul 2023, at 14:36, Dumitru Ceara <dce...@redhat.com> wrote: >>>> >>>> Hi Vladislav, >>>> >>>> After quite some time trying to implement the IDL API change to allow >>>> setting a different default monitor condition and mostly struggling > with >>>> ovn-controller using that properly I kind of gave up and decided to >>>> approach this in a different way. >>>> >>>> We have guidelines about supported upgrade scenarios [0] so we can use >>>> the same guidelines for defining which tables ovn-controller is > entitled >>>> to assume exist in the SB (without having to check). >>>> >>>> I ended up with: >>>> > https://github.com/dceara/ovn/commit/f5e8b9bcba61a2528b67854bb4211981a99feaa8 >>>> >>>> I know it's not perfect but it might be the least risky and a good >>>> enough solution. >>>> >>>> It would be great if you could try it out on your data set too. >>>> >>>> Thanks, >>>> Dumitru >>>> >>>> [0] >>>> > https://github.com/ovn-org/ovn/blob/5a1d82cb28c554276e0c17718f808b8f244cb162/Documentation/intro/install/ovn-upgrades.rst?plain=1#L28 >>>> >>>> On 7/25/23 10:19, Vladislav Odintsov wrote: >>>>> Many thanks for the information! >>>>> >>>>>> On 25 Jul 2023, at 11:14, Dumitru Ceara <dce...@redhat.com> wrote: >>>>>> >>>>>> On 7/24/23 21:10, Vladislav Odintsov wrote: >>>>>>> Hi Dumitru, >>>>>>> >>>>>> >>>>>> Hi Vladislav, >>>>>> >>>>>>> I just wanted to ask wether you need any help (maybe, testing) in > this? >>>>>>> I’m ready to check this on my dataset if you were successful to >>>>>>> implement a fix. >>>>>>> >>>>>> >>>>>> Thanks for offering to help. I didn't get the chance to properly > write >>>>>> and test the patches for this (we need a change in OVS IDL first and >>>>>> then one in OVN). It would be great if you could try them out on > your >>>>>> data sets so I'll CC you on the patches when posting them. I hope > to do >>>>>> that this week. >>>>>> >>>>>> Regards, >>>>>> Dumitru >>>>>> >>>>>>>> On 12 Jul 2023, at 12:15, Dumitru Ceara <dce...@redhat.com> wrote: >>>>>>>> >>>>>>>> On 7/12/23 00:01, Ilya Maximets wrote: >>>>>>>>> On 7/11/23 19:01, Dumitru Ceara wrote: >>>>>>>>>> On 7/11/23 18:33, Vladislav Odintsov wrote: >>>>>>>>>>> Hi Dumitru, >>>>>>>>>>> >>>>>>>>>>> The system on which I reproduced this issue is running 22.09.x >>>>>>>>>>> version. I’ve tried to upgrade ovn-controller to main branch + > your >>>>>>>>>>> patch. Please, note that it has test error: [1]. >>>>>>>>>>> After two minutes after upgrade it still consumed 3.3G. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Ack, I need to re-think the patch then. Maybe a hard deadline > to run >>>>>>>>>> malloc_trim() at least once every X seconds. I'll see what I > can come >>>>>>>>>> up with. >>>>>>>>>> >>>>>>>>>>> I tried to backport your patch to 22.09, it required to backport >>>>>>>>>>> also this commit: [2] and it failed some tests: [3]. >>>>>>>>>>> >>>>>>>>>>> But I’ve got general question: prior to commit that I mentioned > in >>>>>>>>>>> initial mail, ovn-controller even didn’t try load such amount of >>>>>>>>>>> data. And now it does and IIUC, your patch just releases memory >>>>>>>>>>> that was freed after ovn-controller fully loaded. >>>>>>>>>>> I’m wonder wether it should load that excess data at all? Seems >>>>>>>>>>> like it did. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Looking again at 1b0dbde94070 ("ovn-controller: Only set monitor >>>>>>>>>> conditions on available tables.") it's kind of expected indeed: >>>>>>>>>> >>>>>>>>>> Initially all tables are "unavailable" because we didn't get the > schema >>>>>>>>>> so we don't set any condition for any table. >>>>>>>>>> >>>>>>>>>> After ovn-controller connects to the SB for the first time it > will >>>>>>>>>> determine that the SB tables are in the schema so it will > explicitly add >>>>>>>>>> them to the monitor condition and restrict the SB data it is >>>>>>>>>> interested in. >>>>>>>>>> >>>>>>>>>> Maybe we need to change the IDL/CS modules to wait with the >>>>>>>>>> monitor_cond/monitor_cond_since until instructed by the client >>>>>>>>>> (ovn-controller). Ilya do you have any thoughts on this matter? >>>>>>>>> >>>>>>>>> So, AFAICT, the issue is that we're running with >>>>>>>>> 'monitor_everything_by_default' >>>>>>>>> option, the default condition is 'true' and the monitor request > for >>>>>>>>> the main >>>>>>>>> database is sent out immediately after receiving the schema, so > the >>>>>>>>> application >>>>>>>>> has no time to react. >>>>>>>>> >>>>>>>>> I think, there are few possible solutions for this: >>>>>>>>> >>>>>>>>> 1. Introduce a new state in the CS state machine, e.g. >>>>>>>>> CS_S_SERVER_SCHEMA_RCEIVED, and move out from this state in the > run() >>>>>>>>> callback. This way the application will have a chance to set up >>>>>>>>> conditions >>>>>>>>> before they are sent. Slightly not intuitive. >>>>>>>>> >>>>>>>>> 2. A variation on what you suggested, i.e. enter the >>>>>>>>> CS_S_SERVER_SCHEMA_RCEIVED >>>>>>>>> state and wait for some sort of the signal from the application to >>>>>>>>> proceed. >>>>>>>>> Sounds a bit counter-intuitive for an IDL user. >>>>>>>>> >>>>>>>>> 3. Introduce an application callback that can be called from the >>>>>>>>> ovsdb_idl_compose_monitor_request() the same way as this function >>>>>>>>> is getting >>>>>>>>> called form the ovsdb_cs_send_monitor_request(). An application >>>>>>>>> will be >>>>>>>>> able to influence conditions before they are sent. >>>>>>>>> Might be tricky due to new->req->ack state transition. >>>>>>>>> >>>>>>>>> 4. Make the default condition configurable, e.g. by an additional >>>>>>>>> argument >>>>>>>>> 'default_condition' = true/false for an ovsdb_idl_create(). This >>>>>>>>> way the >>>>>>>>> application will not get any data until conditions are actually > set. >>>>>>>>> >>>>>>>>> 5. Or it maybe just a separate config function that will set > default >>>>>>>>> conditions >>>>>>>>> to 'false' and will need to be called before the first run(). >>>>>>>>> >>>>>>>>> 6. Change behavior of 'monitor_everything_by_default' argument. > Make it >>>>>>>>> actually add all the tables to the monitor, but with the 'false' >>>>>>>>> condition. >>>>>>>>> Result should technically be the same. Might be tricky to get >>>>>>>>> right though >>>>>>>>> with all the backward compatibility. >>>>>>>>> >>>>>>>>> Option 5 might be the better option of these. >>>>>>>>> >>>>>>>>> What do you think? >>>>>>>>> >>>>>>>> >>>>>>>> I think option 5 sounds the simplest to implement indeed. It also >>>>>>>> doesn't induce any compatibility issues as you mentioned. >>>>>>>> >>>>>>>> The only "issue" is we'd probably want this backported to stable > OVN >>>>>>>> releases so it means we need to bump the submodule version to an >>>>>>>> unreleased version of OVS. But that's an OVN problem and we > discussed >>>>>>>> similar instances of it before. >>>>>>>> >>>>>>>> I'll prepare a patch soon. >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Dumitru >>>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Vladislav Odintsov >>>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> dev mailing list >>>>>> d...@openvswitch.org <mailto:d...@openvswitch.org> >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>> >>>>> >>>>> Regards, >>>>> Vladislav Odintsov >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> d...@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> d...@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >>> Regards, >>> Vladislav Odintsov >>> >>> _______________________________________________ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> Regards, >> Vladislav Odintsov >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
