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
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
