BGP Labelpool : Releasing the label in labelpool when VPN session gets removed #17580
Conversation
|
no "fix it" commits, please. to make updates or edits to a commit, please rebase and force-push. note that our CI verifies our development workflow rules about commits; please read the developer doc and update your commit to match our guidelines. |
| address-family ipv4 vpn | ||
| neighbor 192.168.0.2 activate | ||
| exit-address-family | ||
| ! |
There was a problem hiding this comment.
please reformulate commit log: title + detail
topotets: add test to control release of bgp labels
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
97e7c96 to
42e8fc4
Compare
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
42e8fc4 to
1bb51d5
Compare
bgpd/bgp_mplsvpn.c
Outdated
|
|
||
| bgp_lp_release(LP_TYPE_VRF, &bgp->vpn_policy[afi], bgp->vpn_policy[afi].tovpn_label); | ||
|
|
||
| if (reset) { |
There was a problem hiding this comment.
don't follow this: why release a label, but leave it in tovpn_label ?
There was a problem hiding this comment.
This is to avoid doing unnecessary changes because this function is reused in vty to change the allocation-mode.
This said, I think it can be acceptable to move:
bgp->vpn_policy[afi].tovpn_label = MPLS_LABEL_NONE;
just after
bgp_lp_release
There was a problem hiding this comment.
moved bgp->vpn_policy[afi].tovpn_label = MPLS_LABEL_NONE;
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
4 times, most recently
from
ffaf7ab to
88088d1
Compare
| tgen = get_topogen() | ||
| cmds_list = [ | ||
| "ip link add vrf{} type vrf table {}", | ||
| "echo 100000 > /proc/sys/net/mpls/platform_labels", |
There was a problem hiding this comment.
please check the test lib code - I think this setup is already being done
There was a problem hiding this comment.
While reproducing, I did check on couple of existing suite, but I was unable to find it, so added a new topo.
| "show bgp labelpool summary json", | ||
| expected, | ||
| ) | ||
| _, result = topotest.run_and_expect(test_func, None, count=10, wait=3) |
There was a problem hiding this comment.
please follow the test guidelines for timeouts:
https://docs.frrouting.org/projects/dev-guide/en/latest/topotests.html#guidelines
There was a problem hiding this comment.
Referred one of the new bgp topotest and added the appropriate count and wait time of 30, 1
| _populate_iface() | ||
|
|
||
| for rname, router in router_list.items(): | ||
| router.load_config( |
There was a problem hiding this comment.
new tests should use unified config please
There was a problem hiding this comment.
Replacing the old cfgs with new unified config
| @@ -0,0 +1,27 @@ | |||
| log stdout | |||
| @@ -0,0 +1,21 @@ | |||
| log stdout | |||
There was a problem hiding this comment.
remove all of these statements
| @@ -0,0 +1,24 @@ | |||
| hostname r3 | |||
| log file ldpd.log | |||
| # test_bgp_vpnv4_ebgp.py | ||
| # Part of NetDEF Topology Tests | ||
| # | ||
| # Copyright (c) 2022 by 6WIND |
There was a problem hiding this comment.
is this the correct copyright statement?
There was a problem hiding this comment.
Changed the copyright appropriately
| @@ -4074,6 +4074,32 @@ void bgp_vpn_leak_export(struct bgp *from_bgp) | |||
| } | |||
| } | |||
|
|
|||
| void bgp_vpn_release_label(struct bgp *bgp, afi_t afi, bool reset) | |||
There was a problem hiding this comment.
please add a comment block explaining the dual uses of this new function
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
88088d1 to
202e252
Compare
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
202e252 to
3ebb795
Compare
|
|
||
| for rname, router in router_list.items(): | ||
| router.load_config( | ||
| TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname)) |
There was a problem hiding this comment.
Please use unified config (frr.conf).
| # | ||
|
|
||
| """ | ||
| test_bgp_vpnv4_ebgp.py: Test the FRR BGP daemon with EBGP direct connection |
There was a problem hiding this comment.
changed the file name and description
| output = json.loads( | ||
| router.vtysh_cmd("show bgp labelpool summary json") | ||
| ) | ||
| expected = json.loads('{"ledger":5,"inUse":5,"requests":0,"labelChunks":1,"pending":0,"reconnects":1}') |
There was a problem hiding this comment.
Please use Python dict directly, e.g.:
expected = {
"ledger":5,"inUse":5,"requests":0,"labelChunks":1,"pending":0,"reconnects":1
}|
|
||
| # checking the initial label pool chunk's free labels | ||
| logger.info("checking the initial label pool chunk's free labels") | ||
| expected = json.loads('[{"first":80,"last":207,"size":128,"numberFree":123}]') |
| no router bgp 65500 vrf vrf2 | ||
| """ | ||
| ) | ||
| expected = json.loads('[{"first":80,"last":207,"size":128,"numberFree":125}]') |
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
3ebb795 to
42371da
Compare
| tgen = get_topogen() | ||
| cmds_list = [ | ||
| "ip link add vrf{} type vrf table {}", | ||
| "echo 100000 > /proc/sys/net/mpls/platform_labels", |
There was a problem hiding this comment.
isn't this already in lib/topotest.py ?
There was a problem hiding this comment.
Yes was able to find that cmd in topotest.py itself and removed from here
Adding the topotest which verifies whether label beloning to corresponding chunk has been released properly or not once we remove the vpn session Signed-off-by: Varun Hegde <[email protected]>
Releasing the vpn label from label pool chunk using bgp_lp_release routine whenever vpn session is removed. bgp_lp_release will clear corresponding bit in the allocated map of the label pool chunk and increases nfree by 1 Signed-off-by: Philippe Guibert <[email protected]>
varuntumbe
force-pushed
the
dev/label_pool_release_fix
branch
from
42371da to
d5c2f2d
Compare
What is the problem ?
When we remove the BGP VPN session which had been configured using ( label vpn export auto ), corresponding label is not getting freed up in the label pool structures.
What is the Impact ?
In higher scale deployment, more we remove sessions and rollback or add new session, earlier assigned labels wont get freed up leading to label pool expansion
What is the fix ?
Fix to release the corresponding label pool using bgp_lp_release in bgp_delete flow and verifying the same using topotest