backpressure bgp zebra client

[raja-rajasekar]

1. Modify the bgp master to hold a type safe list for bgp_dests that need to be passed to zebra.

2. Since installing/withdrawing routes into zebra is going to be changed around to be dest based in a list,

   • Retrieve the afi/safi to use based upon the dest's afi/safi instead of passing it in.
   • Prefix is known by the dest. Remove this arg as well

3. BGP is now keeping a list of dests with the dest having a pointer to the bgp_path_info that it will be working on.

4. When bgp receives a prefix, process it, add the bgp_dest of the prefix into the new Fifo list if not present, update the flags (Ex: earlier if the prefix was advertised and now it is a withdrawn), increment the ref_count and DO NOT advertise the install/withdraw to zebra yet.

5. Schedule an event to wake up to invoke the new function which will walk the list one by one and installs/withdraws the routes into zebra.
   a) if BUFFER_EMPTY, process the next item on the list
   b) if BUFFER_PENDING, bail out and the callback in
   zclient_flush_data() will invoke the same function when BUFFER_EMPTY

Changes

• rename old bgp_zebra_announce to bgp_zebra_announce_actual
• rename old bgp_zebra_withdrw to bgp_zebra_withdraw_actual
• Handle new fifo list cleanup in bgp_exit()
• New funcs: bgp_handle_route_announcements_to_zebra() and bgp_zebra_route_install()
• Define a callback function to invoke bgp_handle_route_announcements_to_zebra() when BUFFER_EMPTY in zclient_flush_data()

The current change deals with bgp installing routes via bgp_process_main_one()

Will add EVPN changes in a seperate commit shortly.

[donaldsharp]

I am converting this to a draft because I believe we are discussing some problems found internally

[raja-rajasekar]

I am converting this to a draft because I believe we are discussing some problems found internally

EVPN code changes have issues which are being discussing internally. This review is Non evpn code change. EVPN will be either as a seperate commit in this review or a different MR on top of this code base.

[ton31337]

Shouldn't we stop the thread (t_bgp_zebra_route) on BGP termination/stop?

[raja-rajasekar]

Shouldn't we stop the thread (t_bgp_zebra_route) on BGP termination/stop?

Agree.. i missed this . Fixing this now

[raja-rajasekar]

RUN_CI

[riw777]

looks good, waiting on other's comments

[raja-rajasekar]

Internal Test for ref:
ZClient_Testing.pdf

OBSERVATION:

Without code changes Buffer data on this simple trigger jumped from 700KB to 78MB
With code changes Buffer data on this simple trigger jumped from 12KB to 8.4MB

[ton31337]

Just a question on bgp_path_info_lock(), see inline.

one nit (not a blocker): Just not sure about the naming of bgp_zebra_route_install(). Previously it was easier to read with bgp_zebra_announce(), and bgp_zebra_withdraw(). Can't we preserve the naming it was before? By converting to macros at least:

#define bgp_zebra_announce(...) bgp_zebra_route_install(..., true)
#define bgp_zebra_withdraw(...) bgp_zebra_route_install(..., false)

Other than that - LGTM.

[raja-rajasekar]

Just a question on bgp_path_info_lock(), see inline.

one nit (not a blocker): Just not sure about the naming of bgp_zebra_route_install(). Previously it was easier to read with bgp_zebra_announce(), and bgp_zebra_withdraw(). Can't we preserve the naming it was before? By converting to macros at least:

#define bgp_zebra_announce(...) bgp_zebra_route_install(..., true)
#define bgp_zebra_withdraw(...) bgp_zebra_route_install(..., false)

Other than that - LGTM.

In the next MR, the changes will be for evpn and we will use evpn_zebra_install and evpn_zebra_uninstall in this function..
Let me make it generic and retain old names in the next MR.

[raja-rajasekar]

RUN_CI

[ton31337]

Why are these commits: 3f0ce9c, 27d5cdb split into two while one is initializing zebra_announce, other does finish? I assume _fini() should be bundled with the previous commit.

Other than that a couple nits from my side.