-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Addpath - Reuse IDs #3051
Addpath - Reuse IDs #3051
Conversation
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedUbuntu1204 amd64 build: Successful CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5360/artifact/CI010BUILD/config.status/config.status Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build Warnings Generated during build:Checkout code: Successful with additional warnings:CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5360/artifact/CI010BUILD/config.status/config.status Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build
|
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedUbuntu1204 amd64 build: Successful CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5359/artifact/CI010BUILD/config.status/config.status Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build |
39cb5e5
to
478f964
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedUbuntu1204 amd64 build: Successful CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5361/artifact/CI010BUILD/config.status/config.status OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedUbuntu1204 amd64 build: Successful FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5362/artifact/CI010BUILD/config.status/config.status OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build Warnings Generated during build:Checkout code: Successful with additional warnings:FreeBSD9 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD9 amd64 build Ubuntu 16.04 i386: FailedDejaGNU Unittests (make check) failed for Ubuntu 16.04 i386 Ubuntu 18.04 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu 18.04 amd64 build CentOS7 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS7 amd64 build FreeBSD10 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD10 amd64 build Ubuntu1404 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1404 amd64 build Debian8 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian8 amd64 build OmniOS amd64 build: FailedMake failed for OmniOS amd64 build:
OmniOS amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5362/artifact/CI010BUILD/config.status/config.status OpenBSD60 amd64 build: FailedDejaGNU Unittests (make check) failed for OpenBSD60 amd64 build Debian9 amd64 build: FailedDejaGNU Unittests (make check) failed for Debian9 amd64 build NetBSD7 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD7 amd64 build Ubuntu1604 amd64 build: FailedDejaGNU Unittests (make check) failed for Ubuntu1604 amd64 build FreeBSD11 amd64 build: FailedDejaGNU Unittests (make check) failed for FreeBSD11 amd64 build CentOS6 amd64 build: FailedDejaGNU Unittests (make check) failed for CentOS6 amd64 build NetBSD6 amd64 build: FailedDejaGNU Unittests (make check) failed for NetBSD6 amd64 build Fedora24 amd64 build: FailedDejaGNU Unittests (make check) failed for Fedora24 amd64 build
|
This is a small behavioral change and as such it should not be a PR against dev/6.0 it should be in master. Looking at the code now otherwise |
bgpd/bgp_tx_addpath.c
Outdated
{ | ||
int i; | ||
for (i = 0; i < BGP_ADDPATH_MAX; i++) { | ||
if (d->addpath_tx_id != 0) { |
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.
bgpd/bgp_tx_addpath.c:122:10: error: comparison of array 'd->addpath_tx_id' not equal to a null pointer is always true [-Werror,-Wtautological-pointer-compare]
if (d->addpath_tx_id != 0) {
~~~^~~~~~~~~~~~~ ~
1 error generated.
bgpd/bgp_updgrp_adv.c
Outdated
|
||
peer = SUBGRP_PEER(subgrp); | ||
afi = SUBGRP_AFI(subgrp); | ||
safi = SUBGRP_AFI(subgrp); |
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.
Shouldn't this be SUBGRP_SAFI?
Additionally the |
I have more comments but am having issues with the tool complaining at me. I'm going to wait on resubmittal for more detailed notes, One major change needed is flog_err instead of zlog_error |
f325ce9
to
8cab244
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedOpenBSD60 amd64 build: Successful Fedora24 amd64 build: FailedPackage building failed for Fedora24 amd64 build:
Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI015BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedUbuntu1604 amd64 build: Unknown Log <log_snapcraft.txt> Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: Unknown Log <log_snapcraft.txt> Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/U1804AMD64/config.status/config.status CentOS6 amd64 build: FailedPackage building failed for CentOS6 amd64 build:
CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI006BUILD/config.status/config.status CentOS7 amd64 build: FailedPackage building failed for CentOS7 amd64 build:
CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI005BUILD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build:
Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI021BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build:
Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI008BLD/config.status/config.status Ubuntu1404 amd64 build: FailedPackage building failed for Ubuntu1404 amd64 build:
Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI001BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Fedora24 amd64 build: FailedPackage building failed for Fedora24 amd64 build:
Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI015BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedUbuntu1604 amd64 build: Unknown Log <log_snapcraft.txt> Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: Unknown Log <log_snapcraft.txt> Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/U1804AMD64/config.status/config.status CentOS6 amd64 build: FailedPackage building failed for CentOS6 amd64 build:
CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI006BUILD/config.status/config.status CentOS7 amd64 build: FailedPackage building failed for CentOS7 amd64 build:
CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI005BUILD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build:
Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI021BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build:
Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI008BLD/config.status/config.status Ubuntu1404 amd64 build: FailedPackage building failed for Ubuntu1404 amd64 build:
Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5432/artifact/CI001BUILD/config.status/config.status
|
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: FailedFreeBSD11 amd64 build: Successful Fedora24 amd64 build: FailedPackage building failed for Fedora24 amd64 build:
Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI015BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedUbuntu1604 amd64 build: Unknown Log <log_snapcraft.txt> Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/U1804AMD64/config.status/config.status Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: Unknown Log <log_snapcraft.txt> CentOS7 amd64 build: FailedPackage building failed for CentOS7 amd64 build:
CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI005BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build:
Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI008BLD/config.status/config.status CentOS6 amd64 build: FailedPackage building failed for CentOS6 amd64 build:
CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI006BUILD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build:
Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI021BUILD/config.status/config.status Ubuntu1404 amd64 build: FailedPackage building failed for Ubuntu1404 amd64 build:
Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI001BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Fedora24 amd64 build: FailedPackage building failed for Fedora24 amd64 build:
Fedora24 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI015BUILD/config.status/config.status Ubuntu1604 amd64 build: FailedUbuntu1604 amd64 build: Unknown Log <log_snapcraft.txt> Ubuntu 18.04 amd64 build: FailedPackage building failed for Ubuntu 18.04 amd64 build:
Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/U1804AMD64/config.status/config.status Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: Unknown Log <log_snapcraft.txt> CentOS7 amd64 build: FailedPackage building failed for CentOS7 amd64 build:
CentOS7 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI005BUILD/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build:
Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI008BLD/config.status/config.status CentOS6 amd64 build: FailedPackage building failed for CentOS6 amd64 build:
CentOS6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI006BUILD/config.status/config.status Debian9 amd64 build: FailedPackage building failed for Debian9 amd64 build:
Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI021BUILD/config.status/config.status Ubuntu1404 amd64 build: FailedPackage building failed for Ubuntu1404 amd64 build:
Ubuntu1404 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5431/artifact/CI001BUILD/config.status/config.status
|
8cab244
to
68022ec
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-5434/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base4 Static Analyzer issues remaining.See details at |
I'll clean up the final checkpatch warnings while I rebase this to the dev branch. I made some minor code changes to improve readability, so I'll be re-running our internal tests on this as well. |
Are you going to generate a new PR with the above modifications ? |
I had planned on just updating this PR. (Our internal process for contribution has a reference to this PR, but if the history is getting too messy I can jump onto a new one.) |
No. I think it is not Required. Will review once you update this PR with your new changes. |
This comment has been minimized.
This comment has been minimized.
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.
This needs to go into master first and then only after is stable in master submitted against 6.0.
68022ec
to
8a4432d
Compare
💚 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-5478/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
Fixed warnings:
Static Analysis warning summary compared to base:
|
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-5479/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base |
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.
==
I attempted to count the number of configured peers and groups using each addpath strategy per afi/safi. I use the information to avoid doing ID work that isn't needed. However, keeping track of deltas as different events happen got a lot more complicated than I had initially anticipated. It might make sense to go back to iterating all of the peers on event to make sure it stays in sync.
After looking at the code for a bit, I tend to agree it would be simpler just to iterate across the peers rather than trying to keep track. I'm a little concerned that someone is going to mis the "keeping track" bit when working on this code in the future, and cause a mess...
==
I moved a lot of the addpath TX ID selection logic into a separate file. This was helpful to me during development, but might not actually be the way we want to leave things.
I'm actually okay with leaving this, this way. It makes it easier to read, I think.
==
Paths now need to track the TX ID per addpath strategy. For now, this only grows the path structure by 4 bytes, but if more addpath strategies are added in the future, this could end up being unwanted dead weight.
I know @donaldsharp has been trying to reduce the size of the structures here, so I'll defer to him on this point.
==
I'm not sure what should be done with the TX ID in the path JSON output. There is no longer a single TX ID to output, but I'm hesitant to remove a key from a JSON structure. For now I'm just pushing the tx-addpath-all ID, but I'm open to suggestions.
Would it make more sense to change the json to show the TX ID per addpath strategy, as above?
==
The code looks okay to me -- should have a couple of more people look at it, though.
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.
Full BGP RFC compliance check shows no new error introduced and all good.
bgpd/bgp_addpath.c
Outdated
* adjusting the counts, peer sessions will be reset as needed to make the | ||
* change take effect. | ||
*/ | ||
void peer_af_addpath_set_type(struct peer *peer, afi_t afi, safi_t safi, |
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.
Change to bgp_addpath_set_type_for_peer or bgp_addpath_peer_af_set_type? Ok otherwise for me.
@mitch-skiba sorry, I think between all the commenting noone felt responsible to hit the merge button once it was done, but now it has merge conflicts :( Can you rebase this? |
do you have any information on cpu and memory impact? |
Sorry folks I've been traveling for a bit, so I haven't been able to tend to this as well as I would have liked. I think I'd like to change the accounting style for what addpath strategies are in use back to the original count-the-peers style instead of the the delta maintenance. I think the simplicity outweighs saving a small number of cycles while reconfiguring peers. Memory impact to users without addpath enabled will be 4 bytes per path. (This is in raw data size. There will be some interference from sizeof alignment requirements, and beyond that the malloc implementation bin size. On my amd64 build there isn't even a sizeof change. The structure ends up being 112 bytes before an after the change. Still, this could represent a 3.5% increase in structure size for what I presume is the most repeated structure in the system. I had spoken to Nikos about this, and he suggested putting in a pointer to an external structure. I think if we add more addpath strategies that would begin to make sense, so we don't penalize non-addpath users for addpath tracking. (At the moment though, the pointer would be the same size as the two 32 bit values on amd64 systems, so it seems a bit hard to justify moving it out at this point.) If addpath is enabled, we begin to track IDs for it. I wanted an allocator instead of a free running counter. (In our history we have seen a refcount leak count all the way to 2^32 and trigger an assert, so we generally don't trust that to be a big enough number to prevent mishaps.) The allocator on an amd64 box costs about 160K for each million IDs tracked. I don't imagine that is too harsh. Regrettably I don't have a good baseline for CPU utilization. I'm fine if folks want to hold off until it can be characterized in numbers. For now, I have some words though. The two biggest hits I can think of:
Most other actions are fairly inexpensive. The ID allocator was designed with low incremental cost in mind. Shuffling IDs on the paths after best-path calculation re-scans the prefix's path list twice. Since bestpath was just run, this list should be hot in the CPU cache, and there isn't a tremendous amount of actual work going on. (This penalty is only paid when addpath is enabled.) |
needs to be rebased - then I'm okay with merging |
also please fix code format error |
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
This commit introduces lib/id_alloc, which has facilities for both an ID number allocator, and less efficient ID holding pools. The pools are meant to be a temporary holding area for ID numbers meant to be re-used, and are implemented as a linked-list stack. The allocator itself is much more efficient with memory. Based on sizeof values on my 64 bit desktop, the allocator requires around 155 KiB per million IDs tracked. IDs are ultimately tracked in a bit-map split into many "pages." The allocator tracks a list of pages that have free bits, and which sections of each page have free IDs, so there isn't any scanning required to find a free ID. (The library utility ffs, or "Find First Set," is generally a single CPU instruction.) At the moment, totally empty pages will not be freed, so the memory utilization of this allocator will remain at the high water mark. The initial intended use case is for BGP's TX Addpath IDs to be pulled from an allocator that tracks which IDs are in use, rather than a free running counter. The allocator reserves ID #0 as a sentinel value for an invalid ID numbers, and BGP will want ID FRRouting#1 reserved as well. To support this, the allocator allows for IDs to be explicitly reserved, though be aware this is only practical to use with low numbered IDs because the allocator must allocate pages in order. Signed-off-by Mitchell Skiba <mskiba@amazon.com>
The motivation for this patch is to address a concerning behavior of tx-addpath-bestpath-per-AS. Prior to this patch, all paths' TX ID was pre-determined as the path was received from a peer. However, this meant that any time the path selected as best from an AS changed, bgpd had no choice but to withdraw the previous best path, and advertise the new best-path under a new TX ID. This could cause significant network disruption, especially for the subset of prefixes coming from only one AS that were also communicated over a bestpath-per-AS session. The patch's general approach is best illustrated by txaddpath_update_ids. After a bestpath run (required for best-per-AS to know what will and will not be sent as addpaths) ID numbers will be stripped from paths that no longer need to be sent, and held in a pool. Then, paths that will be sent as addpaths and do not already have ID numbers will allocate new ID numbers, pulling first from that pool. Finally, anything left in the pool will be returned to the allocator. In order for this to work, ID numbers had to be split by strategy. The tx-addpath-All strategy would keep every ID number "in use" constantly, preventing IDs from being transferred to different paths. Rather than create two variables for ID, this patch create a more generic array that will easily enable more addpath strategies to be implemented. The previously described ID manipulations will happen per addpath strategy, and will only be run for strategies that are enabled on at least one peer. Finally, the ID numbers are allocated from an allocator that tracks per AFI/SAFI/Addpath Strategy which IDs are in use. Though it would be very improbable, there was the possibility with the free-running counter approach for rollover to cause two paths on the same prefix to get assigned the same TX ID. As remote as the possibility is, we prefer to not leave it to chance. This ID re-use method is not perfect. In some cases you could still get withdraw-then-add behaviors where not strictly necessary. In the case of bestpath-per-AS this requires one AS to advertise a prefix for the first time, then a second AS withdraws that prefix, all within the space of an already pending MRAI timer. In those situations a withdraw-then-add is more forgivable, and fixing it would probably require a much more significant effort, as IDs would need to be moved to ADVs instead of paths. Signed-off-by Mitchell Skiba <mskiba@amazon.com>
8a4432d
to
dcc68b5
Compare
💚 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-5866/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base |
Summary
Lifted from my main commit message:
The motivation for this patch is to address a concerning behavior of
tx-addpath-bestpath-per-AS. Prior to this patch, all paths' TX ID was
pre-determined as the path was received from a peer. However, this meant
that any time the path selected as best from an AS changed, bgpd had no
choice but to withdraw the previous best path, and advertise the new
best-path under a new TX ID. This could cause significant network
disruption, especially for the subset of prefixes coming from only one
AS that were also communicated over a bestpath-per-AS session.
There are a lot of parts of this patch I expect to need some discussion or attention. Some notable areas:
Related Issue
Components
bgpd, lib
Always remember to follow proper coding style etc. as
described in the FRRouting Dev Guide.
http://docs.frrouting.org/projects/dev-guide/en/latest/workflow.html