Handle FDB meta data together with FDB data in fdborch to avoid race …

[jipanyang]

…condition

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

Move FDB meta data data processing to fdborch. This a first step try to solve issue #440

[kcudnik]

where this is called ?

[kcudnik]

can you give scenario where you get race condition ?

[lguohan]

issue #440

[kcudnik]

you moved some functionality to handle_meta_fdb_event, but this function is not called anywhere, basically you remove meta_ call which is required, and issues in OA are caused because OA is listening on redis notifications for fdb, and not handling them in SAI FDB callback ! https://github.com/Azure/sonic-swss/blob/master/orchagent/notifications.cpp#L8

which is totally wrong !, "concurency" in the DB can be solved other way! OA should use SAI callback to handle fdb notifications, this is only valid way to handle them

[lguohan]

@jipanyang , can you check the comments here?

[jipanyang]

handle_meta_fdb_event() is used in sonic-net/sonic-swss#824

[jipanyang]

you moved some functionality to handle_meta_fdb_event, but this function is not called anywhere, basically you remove meta_ call which is required, and issues in OA are caused because OA is listening on redis notifications for fdb, and not handling them in SAI FDB callback ! https://github.com/Azure/sonic-swss/blob/master/orchagent/notifications.cpp#L8

which is totally wrong !, "concurency" in the DB can be solved other way! OA should use SAI callback to handle fdb notifications, this is only valid way to handle them

@kcudnik Not sure if I got your point. The current ntf_thread() is also a thread in OA, both fdborch in the main orchagent thread and ntf_thread operate on fdb meta data, there is race condition between them.

[kcudnik]

there always will be race condition on 2 indepentent processes. interprocess synchronization could be used here.
fdb notification should be handled by using SAI notification callback, not by .listening to db directly.

you can use lock on ntf_callback from SAI when it will be used, but this does not guarantee anything, since more notifications can be in the redis queue waiting to be consumed by OA
because notifications are asynchronous the same issue also exist's when you would use vendor SAI directly instead of sairedis SAI. in vendor case, they jsut don't care about any reference count, bhats why we need, and this lead to finding a log of vendor bugs in the past, and prevent users writing OA to make simple mistakes like removing objects which is still in use

and again, you moved some functionality and not using using which opens possible race condition

[jipanyang]

@kcudnik I think you are talking about the notification callback in syncd process. We are not touching that part.

[kcudnik]

no, im talking about notification callback in OA, take a look here:
https://github.com/Azure/sonic-swss/blob/master/orchagent/notifications.cpp#L8

i already pointed this in my previous comment

[wendani]

retest this please

[kcudnik]

Please address commments

[jipanyang]

no, im talking about notification callback in OA, take a look here:
https://github.com/Azure/sonic-swss/blob/master/orchagent/notifications.cpp#L8

i already pointed this in my previous comment

The comment is correct. While the change in meta_fdb_event_snoop_oid() simply conflicts with what is stated in the comment, in orchangent, now you have two threads handling the FDB related meta data reference. Please check #440 .

fdb is asynchronous. Having them processed in one thread in orchagent doesn't prevent possible pending notification in syncd or the db channel, but that should not be the excuse for introducing further race condition at OA module.

[kcudnik]

i already mentioned the same issue, it does not matter whether those pending notifications are in syncd or in internal vendors sai, always is issue of having them come ant any time.
please fix OA using correct notifications procedure, and please don't break existing stuff since it will create more issues

[kcudnik]

closing this as obsolete, code drifted to cpp objects instead of global functions, if there is need to still address this, please reopen