Skip to content
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: Update message received by router is sent back to originator #3044

Closed
wants to merge 1 commit into from

Conversation

@kssoman
Copy link
Contributor

commented Sep 18, 2018

  • The issue is that when processing bgp advertisements in
    subgroup_update_packet() and subgroup_withdraw_packet() the peer from
    which route is learnt is not known and update msg is sent to all
    peers in the update group
  • The command neighbor solo" configures single peer in the
    update group which can help resolve the issue but it will not scale
    for large number of peers
  • The code changes encode peer information in the buffer allocate in
    subgroup_update_packet() and subgroup_withdraw_packet(). The peer information
    is used to determine if the route to be advertised to a peer is already learnt
    from the same peer router. This is removed in bpacket_reformat_for_peer() when
    sending packet to peer router

Signed-off-by: kssoman somanks@vmware.com

Summary

[fill here]

Related Issue

[fill here if applicable]

Components

[bgpd, build, doc, ripd, ospfd, eigrpd, isisd, etc. etc.]

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

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

Verified for 50K routes for the following types :

AFI_IP, SAFI_UNICAST

AFI_IP6 , SAFI_UNICAST

AFI_IP, SAFI_MPLSVPN
upd_logs.txt

@NetDEF-CI

This comment was marked as outdated.

Copy link
Collaborator

commented Sep 18, 2018

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Failed

OpenBSD60 amd64 build: Successful
Fedora24 amd64 build: Successful
FreeBSD11 amd64 build: Successful
NetBSD6 amd64 build: Successful
OmniOS amd64 build: Successful
NetBSD7 amd64 build: Successful
CentOS7 amd64 build: Successful
CentOS6 amd64 build: Successful
FreeBSD10 amd64 build: Successful
FreeBSD9 amd64 build: Successful
Ubuntu1404 amd64 build: Successful
Ubuntu1204 amd64 build: Successful

Ubuntu 16.04 i386: Failed

Package building failed for Ubuntu 16.04 i386:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/U1604I386/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/U1604I386/config.status/config.status

Debian9 amd64 build: Failed

Package building failed for Debian9 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI021BUILD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI021BUILD/config.status/config.status

Ubuntu1604 amd64 build: Failed

Package building failed for Ubuntu1604 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI014BUILD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI014BUILD/config.status/config.status

Ubuntu 18.04 amd64 build: Failed

Package building failed for Ubuntu 18.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/U1804AMD64/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/U1804AMD64/config.status/config.status

Debian8 amd64 build: Failed

Package building failed for Debian8 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI008BLD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5337/artifact/CI008BLD/config.status/config.status

@eqvinox

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

Ubuntu 16.04 build errors:

18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c: In function ‘bpacket_update_peer’:
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c:455:6: error: unused variable ‘i’ [-Werror=unused-variable]
18-Sep-2018 06:39:54	  int i, len;
18-Sep-2018 06:39:54	      ^
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c:454:11: error: unused variable ‘info_len’ [-Werror=unused-variable]
18-Sep-2018 06:39:54	  uint32_t info_len;
18-Sep-2018 06:39:54	           ^
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c: In function ‘subgroup_update_packet’:
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c:1236:6: error: format ‘%zd’ expects argument of type ‘signed size_t’, but argument 4 has type ‘uint32_t’ [-Werror=format=]
18-Sep-2018 06:39:54	      packet_len, num_pfx);
18-Sep-2018 06:39:54	      ^
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c: In function ‘subgroup_withdraw_packet’:
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c:1459:6: error: unknown conversion type character ‘,’ in format [-Werror=format=]
18-Sep-2018 06:39:54	      __func__, num_pfx, unfeasible_len);
18-Sep-2018 06:39:54	      ^
18-Sep-2018 06:39:54	bgpd/bgp_updgrp_packet.c:1459:6: error: too many arguments for format [-Werror=format-extra-args]
18-Sep-2018 06:39:54	cc1: all warnings being treated as errors
18-Sep-2018 06:39:54	Makefile:6090: recipe for target 'bgpd/bgp_updgrp_packet.o' failed

@donaldsharp donaldsharp requested review from donaldsharp and louberger Sep 18, 2018

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

Will fix the compile errors and resubmit PR

@LabN-CI

This comment was marked as outdated.

Copy link

commented Sep 18, 2018

🚧 Basic BGPD CI results: Partial FAILURE, 285 tests failed, has VALGRIND issues

Results table
_ _
Result SUCCESS git merge/3044 e3cf2a5
Date 09/18/2018
Start 09:30:45
Finish 12:37:14
Run-Time 03:06:29
Total 1767
Pass 1482
Fail 285
Valgrind-Errors 10
Valgrind-Loss 111
Details vncregress-2018-09-18-09:30:45.txt
Log autoscript-2018-09-18-09:31:25.log.bz2

For details, please contact louberger

@kssoman kssoman force-pushed the kssoman:updgrp branch from e3cf2a5 to 440bfc1 Sep 19, 2018

@NetDEF-CI

This comment was marked as outdated.

Copy link
Collaborator

commented Sep 19, 2018

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Failed

Ubuntu1404 amd64 build: Successful
FreeBSD11 amd64 build: Successful
NetBSD6 amd64 build: Successful
OmniOS amd64 build: Successful
CentOS7 amd64 build: Successful
CentOS6 amd64 build: Successful
OpenBSD60 amd64 build: Successful
FreeBSD10 amd64 build: Successful
NetBSD7 amd64 build: Successful
FreeBSD9 amd64 build: Successful
Fedora24 amd64 build: Successful
Ubuntu1204 amd64 build: Successful

Ubuntu1604 amd64 build: Failed

Package building failed for Ubuntu1604 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI014BUILD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI014BUILD/config.status/config.status

Ubuntu 18.04 amd64 build: Failed

Package building failed for Ubuntu 18.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/U1804AMD64/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu 18.04 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/U1804AMD64/config.status/config.status

Ubuntu 16.04 i386: Failed

Package building failed for Ubuntu 16.04 i386:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/U1604I386/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/U1604I386/config.status/config.status

Debian8 amd64 build: Failed

Package building failed for Debian8 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI008BLD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI008BLD/config.status/config.status

Debian9 amd64 build: Failed

Package building failed for Debian9 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI021BUILD/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Debian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5349/artifact/CI021BUILD/config.status/config.status

@kssoman kssoman force-pushed the kssoman:updgrp branch from 440bfc1 to 6e25ee3 Sep 19, 2018

@NetDEF-CI

This comment was marked as outdated.

Copy link
Collaborator

commented Sep 19, 2018

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5350/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Failed

Ubuntu1604 amd64 build: Successful
Ubuntu1404 amd64 build: Successful
Ubuntu 18.04 amd64 build: Successful
FreeBSD11 amd64 build: Successful
Debian8 amd64 build: Successful
CentOS6 amd64 build: Successful
NetBSD6 amd64 build: Successful
FreeBSD9 amd64 build: Successful
OmniOS amd64 build: Successful
CentOS7 amd64 build: Successful
Debian9 amd64 build: Successful
OpenBSD60 amd64 build: Successful
Ubuntu1204 amd64 build: Successful
FreeBSD10 amd64 build: Successful
NetBSD7 amd64 build: Successful
Fedora24 amd64 build: Successful

Ubuntu 16.04 i386: Failed

Package building failed for Ubuntu 16.04 i386:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5350/artifact/U1604I386/ErrorLog/log_package_build.txt)

dh: Unknown sequence debian/backports/rules (choose from: binary binary-arch binary-indep build build-arch build-indep clean install install-arch install-indep)
# Frr needs /proc to check some BSD vs Linux specific stuff.
# Else it fails with an obscure error message pointing out that
# IPCTL_FORWARDING is an undefined symbol which is not very helpful.
if ! [ -e config.status ]; then \
dh_auto_configure -- \
	--enable-exampledir=/usr/share/doc/frr/examples/ \
	--localstatedir=/var/run/frr \
	--sbindir=/usr/lib/frr \

Ubuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5350/artifact/U1604I386/config.status/config.status

bgpd: Update message received by router is sent back to originator
* The issue is that when processing bgp advertisements in
  subgroup_update_packet() and subgroup_withdraw_packet() the peer from
  which route is learnt is not known and update msg is sent to all
  peers in the update group
* The command neighbor <addr> solo" configures single peer in the
  update group which can help resolve the issue but it will not scale
  for large number of peers
* The code changes encode peer information in the buffer allocate in
  subgroup_update_packet() and subgroup_withdraw_packet(). The peer information
  is used to determine if the route to be advertised to a peer is already learnt
  from the same peer router. This is removed in bpacket_reformat_for_peer() when
  sending packet to peer router

Signed-off-by: kssoman <somanks@vmware.com>

@kssoman kssoman force-pushed the kssoman:updgrp branch from 6e25ee3 to 6731a1e Sep 19, 2018

@LabN-CI

This comment has been minimized.

Copy link

commented Sep 19, 2018

🚧 Basic BGPD CI results: Partial FAILURE, 282 tests failed, has VALGRIND issues

Results table
_ _
Result SUCCESS git merge/3044 440bfc1
Date 09/19/2018
Start 07:40:43
Finish 10:44:06
Run-Time 03:03:23
Total 1767
Pass 1485
Fail 282
Valgrind-Errors 9
Valgrind-Loss 113
Details vncregress-2018-09-19-07:40:43.txt
Log autoscript-2018-09-19-07:41:27.log.bz2

For details, please contact louberger

@NetDEF-CI

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5352/

This is a comment from an EXPERIMENTAL automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source and apply patch from patchwork: Successful

Building Stage: Successful

Basic Tests: Failed

IPv4 ldp protocol on Ubuntu 16.04: Successful
Ubuntu 14.04 deb pkg check: Successful
Addresssanitizer topotest: Successful
Debian 8 deb pkg check: Successful
IPv4 protocols on Ubuntu 14.04: Successful
Fedora 24 rpm pkg check: Successful
Debian 9 deb pkg check: Successful
CentOS 7 rpm pkg check: Successful
Static analyzer (clang): Successful
Ubuntu 12.04 deb pkg check: Successful
Ubuntu 16.04 deb pkg check: Successful
IPv6 protocols on Ubuntu 14.04: Successful
CentOS 6 rpm pkg check: Successful

Topology tests on Ubuntu 16.04 amd64: Failed

Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOU1604-5352/test

Topology Tests failed for Topology tests on Ubuntu 16.04 amd64:

2018-09-19 08:06:05,915 ERROR: ******************************************************************************
2018-09-19 08:06:05,915 ERROR: Test Target Summary                                                  Pass Fail
2018-09-19 08:06:05,916 ERROR: ******************************************************************************
2018-09-19 08:06:05,916 ERROR: FILE: scripts/adjacencies.py
2018-09-19 08:06:05,916 ERROR: FILE: scripts/add_routes.py
2018-09-19 08:06:05,916 ERROR: FILE: scripts/check_routes.py
2018-09-19 08:06:05,916 ERROR: 47   ce1    Local and remote routes +10.45 secs                      0    1
2018-09-19 08:06:05,916 ERROR: 49   ce3    Local and remote routes +10.45 secs                      0    1
2018-09-19 08:06:05,916 ERROR: See /tmp/topotests/bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/output.log for details of errors
2018-09-19 08:06:05,920 ERROR: assert failed at "bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/test_check_routes": 2 tests failed

see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5352/artifact/TOPOU1604/ErrorLog/log_topotests.txt

Topotest tests on Ubuntu 16.04 i386: Failed

Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPOI386-5352/test

Topology Tests failed for Topotest tests on Ubuntu 16.04 i386:

2018-09-19 08:05:31,201 ERROR: ******************************************************************************
2018-09-19 08:05:31,201 ERROR: Test Target Summary                                                  Pass Fail
2018-09-19 08:05:31,201 ERROR: ******************************************************************************
2018-09-19 08:05:31,201 ERROR: FILE: scripts/adjacencies.py
2018-09-19 08:05:31,201 ERROR: FILE: scripts/add_routes.py
2018-09-19 08:05:31,202 ERROR: FILE: scripts/check_routes.py
2018-09-19 08:05:31,202 ERROR: 47   ce1    Local and remote routes +10.37 secs                      0    1
2018-09-19 08:05:31,202 ERROR: 49   ce3    Local and remote routes +10.38 secs                      0    1
2018-09-19 08:05:31,202 ERROR: See /tmp/topotests/bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/output.log for details of errors
2018-09-19 08:05:31,204 ERROR: assert failed at "bgp_l3vpn_to_bgp_direct.test_bgp_l3vpn_to_bgp_direct/test_check_routes": 2 tests failed
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument
RTNETLINK answers: Invalid argument

see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5352/artifact/TOPOI386/ErrorLog/log_topotests.txt

Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5352/artifact/TOPOU1604/MemoryLeaks/
Topology Tests memory analysis: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5352/artifact/TOPOI386/MemoryLeaks/

CLANG Static Analyzer Summary

  • Github Pull Request 3044, comparing to Git base SHA dc790ba

No Changes in Static Analysis warnings compared to base

@LabN-CI

This comment has been minimized.

Copy link

commented Sep 19, 2018

🚧 Basic BGPD CI results: Partial FAILURE, 284 tests failed, has VALGRIND issues

Results table
_ _
Result SUCCESS git merge/3044 6731a1e
Date 09/19/2018
Start 11:14:16
Finish 14:18:53
Run-Time 03:04:37
Total 1767
Pass 1483
Fail 284
Valgrind-Errors 10
Valgrind-Loss 111
Details vncregress-2018-09-19-11:14:16.txt
Log autoscript-2018-09-19-11:14:54.log.bz2

For details, please contact louberger

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

There was issue raised earlier : #1681 which was closed specifying that it is normal behavior.

In a scenario where there a router has large number of peers (100) and large number of routes (100000) and the router sends these routes to all peers. These peers send back same updates to routes due to update group and the router will become busy processing these messages and then drop all routes with error "denied due to same AS in the as path".
Also network bandwidth will be wasted for sending the unwanted messages

Just like in the case of ORF (outbound route filter) where peers exchange prefix filter info to avoid sending unwanted routes, same principle will probably apply in this case also

Therefore it will be preferable not to send updates to peer for the routes learnt from same peer
On the sending side additional processing will be required to filter routes based on peer but however this should not have significant impact on sender side.

Please provide your suggestions

@eqvinox eqvinox requested a review from riw777 Sep 25, 2018

@louberger
Copy link
Member

left a comment

The current code uses the same updates to all peers in a peer group. This was done, as I understand it, to improve scaling number of peers. I certainly agree where the reflection of update information to the peer that originated it is not always desirable, and is never needed in the case where there is a single peer in a peer group. given that, with the exception of the single peer case, this is a deployment specific trade off I think the different behaviours need to be governed by configuration. In other words, this PR should not change behavior of peer groups with >1 peer without a config change.

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

For single peer, there is command "neighbor solo" but this is frr specific command and creates seperate update group for each peer.

For the case where there are many peers, creating seperate update group per neighbor is not efficient and scalable as this will result in parsing best routes and preparing update msg for as many times as the number of peers

In the proposed fix, the update message is prepared only once for all peers in same update group,
and peer info is encoded in the message and while sending to peer, this info is removed
Therefore only buffer manipulation is done while sending to peer.

If we use the current behavior without change and there are large number of peers and routes as mentioned earlier, the receiving routes will get many copies of same message and the tcp/ip layer and bgpd process will become busy processing these messages only to drop after processing. Therefore although on sending side little extra processing will be required to filter routes per neighbor but it will help on receiving side.
In case where many protocols are running on the router it will be preferable to prevent bgp from consuming large cpu time as it will help other processes on the system

@louberger

This comment has been minimized.

Copy link
Member

commented Sep 27, 2018

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

We can add a option in the global bgp configuration to disable the reflection of update messages back to same peer, this can be used in the scenario where the bgp process causes issues on the router due to large number of reflected messages processing

@louberger

This comment has been minimized.

Copy link
Member

commented Sep 28, 2018

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2018

The solo option will not be modified as it is special command for having single peer for each update group

Only global command will be added for preventing reflection that can be used in required scenario as mentioned earlier

@nikos-github

This comment has been minimized.

Copy link

commented Sep 28, 2018

We can add a option in the global bgp configuration to disable the reflection of update messages back to same peer, this can be used in the scenario where the bgp process causes issues on the router due to large number of reflected messages processing

@kssoman Do you have data to support this assertion? If the router can't handle prefixes being reflected back, it is likely the issue is elsewhere because the processing of those messages is minimal in the majority of bgp implementations.

@nikos-github
Copy link

left a comment

If the router can't handle prefixes being reflected back, it is likely the issue is elsewhere because the processing of those messages is minimal in the majority of bgp implementations. If the router can't handle those updates, then it sounds like it will die if a refresh is performed. The bandwidth bandwidth argument doesn't hold btw. This functionality should only be enabled via a configuration option. Current behavior should always be the default.

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

The current behavior will not be modified and a configuration parameter will be added to prevent reflection

We will get back with the details of any issues found in the current implementation for large scale test cases

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Observed in large scale test setup (router peering to many peers (9) and advertising 1000000 routes
the advertising router receives 14077 reflected messages and function bgp_receive_update() uses
19940879 microsecs

@eqvinox

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Agreement is to have this configurable and the previous behaviour be the default.

@riw777
Copy link
Member

left a comment

Definitely needs to be configurable before committing.

@kssoman kssoman closed this Jan 29, 2019

@kssoman kssoman deleted the kssoman:updgrp branch Jan 29, 2019

@rodnymolina

This comment has been minimized.

Copy link

commented Feb 8, 2019

@kssoman Are you still planning to merge this one? I was under the impression that you would extend this PR with a global config-knob, and then you would push it. I'm interested in having this PR merged as i agree with your explanation above.

@kssoman

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2019

There are some technical issues to be discussed with the community which influence the fix
When these are resolved, we will test all scenarios and then consider merging the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.