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
