bgpd: Advertise only routes that are programmed in FIB to bgp peers #3147

kssoman · 2018-10-08T17:09:29Z

Added bgp global configuration command "suppress-fib-pending"
When the command is configured, bgp process sends
ZEBRA_ROUTE_NOTIFY_REQUEST to RIB to register for route notification
from RIB.
The the routes learnt from peer are marked BGP_NODE_FIB_INSTALL_PENDING
When RIB sends route notification and route is installed, the msg is
processed in bgp_zebra_route_notify_owner() which calls
the function group_announce_route() to advertise the route

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 · 2018-10-08T17:10:06Z

Test logs
fib_logs.txt

LabN-CI · 2018-10-08T17:34:38Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/3147 `b3068fa`
Date	10/08/2018
Start	13:11:28
Finish	13:34:32
Run-Time	23:04
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-10-08-13:11:28.txt
Log	autoscript-2018-10-08-13:12:04.log.bz2

For details, please contact louberger

bgpd/bgp_errors.h

qlyoung · 2018-10-08T17:33:08Z

bgpd/bgp_route.c

@@ -2066,10 +2066,13 @@ int subgroup_process_announce_selected(struct update_subgroup *subgrp,
 	struct attr attr;
 	afi_t afi;
 	safi_t safi;
+	struct bgp *bgp = NULL;


No need to set a value here since you set it on line 2075; if that line is ever removed or incorrectly changed then this will suppress a compiler warning that would otherwise catch an uninitialized value

qlyoung · 2018-10-08T17:34:50Z

bgpd/bgp_route.c

@@ -2088,12 +2091,19 @@ int subgroup_process_announce_selected(struct update_subgroup *subgrp,
 	memset(&attr, 0, sizeof(struct attr));
 	/* It's initialized in bgp_announce_check() */

+	/* Do not advertise routes that are not installed in FIB */
+	if (bgp && CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_FIB_PENDING) &&
+			CHECK_FLAG(rn->flags, BGP_NODE_FIB_INSTALL_PENDING))


Indent alignment needs to be fixed here

qlyoung · 2018-10-08T17:35:36Z

bgpd/bgp_updgrp_adv.c

@@ -574,10 +574,13 @@ void subgroup_announce_table(struct update_subgroup *subgrp,
 	afi_t afi;
 	safi_t safi;
 	int addpath_capable;
+	struct bgp *bgp = NULL;


qlyoung · 2018-10-08T17:35:49Z

bgpd/bgp_updgrp_adv.c

+		/* Do not advertise routes that are not installed in FIB */
+		if (bgp &&
+			CHECK_FLAG(bgp->flags, BGP_FLAG_SUPPRESS_FIB_PENDING) &&
+			CHECK_FLAG(rn->flags, BGP_NODE_FIB_INSTALL_PENDING))


qlyoung · 2018-10-08T17:38:24Z

bgpd/bgp_zebra.c

+	if (!bgp) {
+		flog_err(EC_BGP_INVALID_NODE,
+				"%s : bgp instance not found vrf %d",
+				__PRETTY_FUNCTION__, vrf_id);


Indentation needs to be fixed, here and in most other if statements and continued function calls

qlyoung · 2018-10-08T17:40:25Z

bgpd/bgp_zebra.c

+	safi_t safi;
+	struct bgp_node *rn = NULL;
+	struct bgp *bgp = NULL;
+	struct bgp_info *ri = NULL, *new_select = NULL;


rn, bgp and ri should not be set to NULL

qlyoung · 2018-10-08T17:42:51Z

lib/zclient.c

+	if (set)
+		stream_putc(s, 1);
+	else
+		stream_putc(s, 0);


stream_putc(s, !!set)

lib/zclient.h

qlyoung · 2018-10-08T17:44:09Z

lib/zclient.h

@@ -482,6 +484,9 @@ extern void zclient_send_interface_radv_req(struct zclient *zclient,
 extern int zebra_redistribute_send(int command, struct zclient *, afi_t,
 				   int type, unsigned short instance,
 				   vrf_id_t vrf_id);
+/* Send route notify request to zebra */
+extern int  zebra_route_notify_send(int command, struct zclient *zclient,


One space between the type and identifier please.

@kssoman There is still an extra space here before the zebra_route_notify_send :)

donaldsharp · 2018-10-08T19:55:48Z

This needs to be broken up into at least 4 commits:

Change to add afi/safi to ZAPI messages
Addition of new ZAPI message
Usage of new zapi message in bgp

donaldsharp · 2018-10-09T16:02:13Z

default behavior should be to wait for rib/fib install notification.

donaldsharp · 2018-10-09T16:03:20Z

views and when zebra is not used, this is not true.

kssoman · 2018-10-09T16:08:55Z

default behavior should be to wait for rib/fib install notification.

Using this as default behavior will slow down the advertisement process to peers and also there will be some overhead as zebra needs to send notification to bgp, is it ok

rwestphal · 2018-10-09T16:22:07Z

I think this is an important feature (not going to discuss if this should be enabled by default or not).

I'm only wondering if we could merge the two following commands into one:

DEFUN (bgp_network_import_check,                                                                                       
       bgp_network_import_check_cmd,                                                                                   
       "bgp network import-check",                                                                                     
       "BGP specific commands\n"                                                                                       
       "BGP network command\n"                                                                                         
       "Check BGP network route exists in IGP\n")
{                                                                                                                      
        VTY_DECLVAR_CONTEXT(bgp, bgp);                                                                                 
        if (!bgp_flag_check(bgp, BGP_FLAG_IMPORT_CHECK)) {                                                             
                bgp_flag_set(bgp, BGP_FLAG_IMPORT_CHECK);                                                              
                bgp_static_redo_import_check(bgp);                                                                     
        }                                                                                                              
                                                                                                                       
        return CMD_SUCCESS;                                                                                            
}

DEFPY (bgp_suppress_fib_pending,
       bgp_suppress_fib_pending_cmd,
       "bgp suppress-fib-pending",
       BGP_STR
       "Advertise only routes that are programmed in hardware to peers\n")
{
	VTY_DECLVAR_CONTEXT(bgp, bgp);
	bgp_suppress_fib_pending_set(bgp, true);
	return CMD_SUCCESS;
}

My rationale is this: if no bgp network import-check is configured, then the bgp suppress-fib-pending command becomes irrelevant. Maybe a new option like bgp network import-check [suppress-fib-pending] would do the job.

kssoman · 2018-10-09T16:36:16Z

I think this is an important feature (not going to discuss if this should be enabled by default or not).

I'm only wondering if we could merge the two following commands into one:

DEFUN (bgp_network_import_check,                                                                                       
       bgp_network_import_check_cmd,                                                                                   
       "bgp network import-check",                                                                                     
       "BGP specific commands\n"                                                                                       
       "BGP network command\n"                                                                                         
       "Check BGP network route exists in IGP\n")
{                                                                                                                      
        VTY_DECLVAR_CONTEXT(bgp, bgp);                                                                                 
        if (!bgp_flag_check(bgp, BGP_FLAG_IMPORT_CHECK)) {                                                             
                bgp_flag_set(bgp, BGP_FLAG_IMPORT_CHECK);                                                              
                bgp_static_redo_import_check(bgp);                                                                     
        }                                                                                                              
                                                                                                                       
        return CMD_SUCCESS;                                                                                            
}

DEFPY (bgp_suppress_fib_pending,
       bgp_suppress_fib_pending_cmd,
       "bgp suppress-fib-pending",
       BGP_STR
       "Advertise only routes that are programmed in hardware to peers\n")
{
	VTY_DECLVAR_CONTEXT(bgp, bgp);
	bgp_suppress_fib_pending_set(bgp, true);
	return CMD_SUCCESS;
}

My rationale is this: if no bgp network import-check is configured, then the bgp suppress-fib-pending command becomes irrelevant. Maybe a new option like bgp network import-check [suppress-fib-pending] would do the job.

The routes considered for suppress-fib are peer learnt routes

A -------------------- B ---------------------- C

A sends routes to B and B sends to C
If B does not install all routes learnt from router A in the FIB, the routes are currently advertised to C
therefore traffic to these prefixes which are not installed reach router B and then discarded
The fix provides option to suppress advertisement of the routes which are not installed

The import check command is for routes added to BGP table from RIB

rwestphal · 2018-10-09T21:40:53Z

@kssoman indeed, those are two completely different things. Sorry for my confusion ^^

rwestphal · 2018-10-09T21:50:29Z

zebra/rib.h

+
+	/* AFI, SAFI */
+	afi_t afi;
+	safi_t safi;


Unnecessary memory overhead. All routes from a routing table share the same AFI and SAFI values. You should obtain these values from there (rib_table_info_t) when necessary.

lib/zclient.h

bgpd/bgp_route.c

kssoman · 2018-10-13T06:04:31Z

When suppress fib pending is enabled by default, some of the topology tests give error due to timing issues. This is expected since there is delay in route advertisements to peer as it is driven by the FIB install status. When suppress fib is not enabled by default, there are no errors as route are advertised to peer without delay.

Therefore this might require changes to tests or not enabling suppress fib option by default

We have verified that code changes work correctly on local test setup

bgp peers (Part 1) * Code changes to add afi/safi to ZAPI messages Added afi, safi in route entry Signed-off-by: kssoman somanks@vmware.com

bgp peers (Part 2) * Addition of new ZAPI message ZEBRA_ROUTE_NOTIFY_REQUEST used by client to request for route notification from RIB Signed-off-by: kssoman somanks@vmware.com

bgp peers (Part 3) * Added CLI command to enable and disable suppress-fib-pending Signed-off-by: kssoman somanks@vmware.com

bgp peers (Part 4) * Process FIB update in bgp_zebra_route_notify_owner() and call group_announce_route() if route is installed * When bgp update is received for new route set flag BGP_NODE_FIB_INSTALL_PENDING and check the flag when advertising routes to peer Signed-off-by: kssoman somanks@vmware.com

LabN-CI · 2018-10-13T07:34:44Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/3147 `a25f8a3`
Date	10/13/2018
Start	03:11:34
Finish	03:34:37
Run-Time	23:03
Total	1813
Pass	1813
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2018-10-13-03:11:34.txt
Log	autoscript-2018-10-13-03:12:11.log.bz2

For details, please contact louberger

NetDEF-CI · 2018-10-13T08:04:05Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/

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.

CLANG Static Analyzer Summary

Github Pull Request 3147, comparing to Git base SHA 97dc689

New warnings:

Logic error: Dereference of null pointer in bgpd/bgp_route.c, function bgp_update, line 2983
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/artifact/shared/static_analysis/report-0a1adc.html#EndPath
Logic error: Dereference of null pointer in bgpd/bgp_route.c, function bgp_update, line 3539
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/artifact/shared/static_analysis/report-3e1560.html#EndPath
Logic error: Dereference of null pointer in bgpd/bgp_route.c, function bgp_update, line 2988
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/artifact/shared/static_analysis/report-916bbc.html#EndPath
Logic error: Dereference of null pointer in zebra/zebra_rib.c, function rib_install_kernel, line 1182
- See https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/artifact/shared/static_analysis/report-5b5421.html#EndPath

Static Analysis warning summary compared to base:

Fixed warnings: 0
New warnings: 4

4 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-5630/artifact/shared/static_analysis/index.html

issue addressed

kssoman · 2018-10-23T07:33:02Z

code changes to be tested for large scale test scenario (larger number of peers and routes)

eqvinox · 2018-10-23T11:39:50Z

Am I interpreting this correctly that the change is OK and we're just waiting on a single space character?

If that's the case can someone please just remove the space char and hit merge? :D

rwestphal

Please fix the SA warnings.

eqvinox · 2018-10-26T20:26:25Z

Flagged submitter action required due to SA warnings

mjstapp

with the merge of the new dataplane code, the owner notification path in zebra_rib is somewhat different. that'll need to be resolved - please feel free to ping me if you have difficulty figuring out how things are working now. also, you'll see that there's a second path to zapi_msg.c:route_notify_internal() that will need to support the added args.

kssoman · 2018-11-02T06:58:19Z

With the new data plane the call sequence is :

rib_process_dplane_results() -> rib_process_after() -> zsend_route_notify_owner_ctx(ctx, ZAPI_ROUTE_FAIL_INSTALL) -> route_notify_internal() -> Send ZEBRA_ROUTE_NOTIFY_OWNER to client

We are only using the status ZAPI_ROUTE_FAIL_INSTALL and ZAPI_ROUTE_INSTALLED sent by RIB
Therefore looks like the new dataplane changes will not have an impact for this PR

mjstapp · 2018-11-02T13:05:27Z

Therefore looks like the new dataplane changes will not have an impact for this PR

The issue is that the new path to the common zapi message handler does not have the new added arguments - just pointing out that you need to rebase to pick up that second path and ensure it builds.

qlyoung requested changes Oct 8, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

rwestphal reviewed Oct 9, 2018

View reviewed changes

kssoman force-pushed the fib_test branch from b3068fa to 619ad3c Compare October 11, 2018 17:06

This comment has been minimized.

Sign in to view

louberger previously requested changes Oct 12, 2018

View reviewed changes

bgpd/bgp_route.c Show resolved Hide resolved

kssoman force-pushed the fib_test branch from 619ad3c to 9b342a1 Compare October 12, 2018 18:03

This comment has been minimized.

Sign in to view

kssoman force-pushed the fib_test branch from 9b342a1 to 634d2c4 Compare October 13, 2018 04:28

This comment has been minimized.

Sign in to view

kssoman added 4 commits October 12, 2018 23:29

bgpd: Advertise only routes that are programmed in FIB to

a4eaab9

bgp peers (Part 1) * Code changes to add afi/safi to ZAPI messages Added afi, safi in route entry Signed-off-by: kssoman somanks@vmware.com

bgpd: Advertise only routes that are programmed in FIB to

ff6a88e

bgp peers (Part 2) * Addition of new ZAPI message ZEBRA_ROUTE_NOTIFY_REQUEST used by client to request for route notification from RIB Signed-off-by: kssoman somanks@vmware.com

bgpd: Advertise only routes that are programmed in FIB to

79efc21

bgp peers (Part 3) * Added CLI command to enable and disable suppress-fib-pending Signed-off-by: kssoman somanks@vmware.com

kssoman force-pushed the fib_test branch from 634d2c4 to a25f8a3 Compare October 13, 2018 07:02

eqvinox added the review & merge me look at me! label Oct 23, 2018

rwestphal requested changes Oct 24, 2018

View reviewed changes

eqvinox added submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) and removed review & merge me look at me! labels Oct 26, 2018

mjstapp reviewed Oct 31, 2018

View reviewed changes

kssoman closed this Jan 29, 2019

kssoman deleted the fib_test branch January 29, 2019 10:40

bgpd: Advertise only routes that are programmed in FIB to bgp peers #3147

bgpd: Advertise only routes that are programmed in FIB to bgp peers #3147

Conversation

kssoman commented Oct 8, 2018

Summary

Related Issue

Components

kssoman commented Oct 8, 2018

LabN-CI commented Oct 8, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

donaldsharp commented Oct 8, 2018

donaldsharp commented Oct 9, 2018

donaldsharp commented Oct 9, 2018

kssoman commented Oct 9, 2018

rwestphal commented Oct 9, 2018

kssoman commented Oct 9, 2018

rwestphal commented Oct 9, 2018

Choose a reason for hiding this comment

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.

kssoman commented Oct 13, 2018

LabN-CI commented Oct 13, 2018

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Oct 13, 2018

Continuous Integration Result: SUCCESSFUL

CLANG Static Analyzer Summary

New warnings:

Static Analysis warning summary compared to base:

4 Static Analyzer issues remaining.

kssoman commented Oct 23, 2018

eqvinox commented Oct 23, 2018

rwestphal left a comment

Choose a reason for hiding this comment

eqvinox commented Oct 26, 2018

mjstapp left a comment

Choose a reason for hiding this comment

kssoman commented Nov 2, 2018

mjstapp commented Nov 2, 2018