-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bgpd, zebra: auto assign labels to regular labeled-unicast prefixes #3327
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f0520d7
to
82c7002
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Whitespace issues fixed but I don't understand why there are complaints about lines longer than 80 characters. I have this limit marked in my IDE and also checked manually -- the lines that I have committed are under the 80 char limit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One really minor question which probably isn't a problem; this code looks fine to me. Will wait for @louberger to review.
checkpatch reports the following:
|
82c7002
to
c64bc06
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@riw777 @louberger Thank you both for your input, all your notes have been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff, I tested this here and it worked smoothly.
The only problem I found is that bgpd is allocating labels dynamically even for FECs that have static label bindings configured (mpls label bind ...
). Shouldn't the static labels take precedence over auto-assigned labels?
Please check more review comments below (mostly style issues).
@louberger please recheck on your ❌ :) @adeg can you fix the style issues pointed out by @rwestphal? |
Will do.
…----------
On December 4, 2018 9:43:01 AM David Lamparter ***@***.***> wrote:
@louberger please recheck on your ❌ :)
@adeg can you fix the style issues pointed out by @rwestphal?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#3327 (comment)
|
@rwestphal I haven't been able to reproduce this (and also do not see how this could be happening if I read the code). I have tested this with a couple
Could you please share the config you've been using when the described behaviour was observed? |
c64bc06
to
ee58350
Compare
@rwestphal I have pushed all fixes to the issues pointed out by yourself in the recent code review in a separate commit for better visibility. Please check and let me know if everything is in order. If the PR is ready for merging, I will squash the fixes commit into the base one and then you can merge. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
@adeg thanks for the updates! The changes look good, feel free to squash the fixes into the original commit.
Sure, here's my BGP LU test topology: https://gist.github.com/rwestphal/9707df2ff6c2073c8b46a23e7d603859 Here's the output of some "show" commands in the rt2 router:
As it can be seen by the |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
This comment has been minimized.
This comment has been minimized.
CI:rerun |
This comment has been minimized.
This comment has been minimized.
54a50d3
to
2cf1866
Compare
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-6237/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:
Warnings Generated during build:Debian 8 amd64 build: Successful with additional warnings:Debian Package lintian failed for Debian 8 amd64 build:
Ubuntu 16.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 16.04 amd64 build:
Ubuntu 18.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 18.04 amd64 build:
Debian 9 amd64 build: Successful with additional warnings:Debian Package lintian failed for Debian 9 amd64 build:
Ubuntu 14.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 14.04 amd64 build:
Ubuntu 16.04 i386 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 16.04 i386 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
…n BGP labeled unicast This commit is the last missing piece to complete BGP LU support in bgpd. To this moment, bgpd (and zebra) supported auto label assignment only for prefixes leaked from VRFs to vpn and for MPLS SR prefixes. This adds auto label assignment to other routes types in bgpd. The following enhancements have been made: * bgp_route.c:bgp_process_main_one() now sets implicit-null local_label to all local, aggregate and redistributed routes. * bgp_route.c:bgp_process_main_one() now will request a label from the label pool for any prefix that loses the label for some reason (for example, when the static label assignment config is removed) * bgp_label.c:bgp_reg_dereg_for_label() now requests labels from label pool for routes which have no associated label index * zebra_mpls.c:zebra_mpls_fec_register() now expects both label and label_index from the calling function, one of which must be set to MPLS_INVALID_LABEL or MPLS_INVALID_LABEL_INDEX, based on this it will decide how to register the provided FEC. Signed-off-by: Anton Degtyarev <anton@cumulusnetworks.com>
2cf1866
to
57592a5
Compare
Squashed all code review commits into the base one for merging into master. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-6255/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:
Warnings Generated during build:Debian 9 amd64 build: Successful with additional warnings:Debian Package lintian failed for Debian 9 amd64 build:
Ubuntu 16.04 i386 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 16.04 i386 build:
Debian 8 amd64 build: Successful with additional warnings:Debian Package lintian failed for Debian 8 amd64 build:
Ubuntu 14.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 14.04 amd64 build:
Ubuntu 16.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 16.04 amd64 build:
Ubuntu 18.04 amd64 build: Successful with additional warnings:Debian Package lintian failed for Ubuntu 18.04 amd64 build:
CLANG Static Analyzer Summary |
Thanks for this great work! |
Summary
This is the last missing piece to full BGP LU support. To this moment, bgpd (and zebra) supported auto label assignment only for prefixes leaked from VRFs to vpn and for MPLS SR prefixes. This adds auto label assignment to other route types in bgpd. The following enhancements have been made:
Components
[bgpd, zebra]