Hi Daniel Thanks a lot for the comments.
On Mon, Nov 23, 2015 at 6:50 PM, Daniel Walton <[email protected]> wrote: > > > On Sun, Nov 22, 2015 at 12:13 PM, Balaji Gurudoss <[email protected]> > wrote: > >> This patch set introduces the support for BGP table version. >> BGP table version is generally used to denote the instability in the >> network. >> Table versions are incremented when any changes in the best path >> happens. The initial version of table version is set to one by default. >> >> I have tested the changes in my local setup against three Cisco boxes >> connected in GNS3 setup in few scenarios and fairly works. >> >> Comments/Suggestions most welcome. >> >> Signed-off-by: Balaji Gurudoss <[email protected]> >> --- >> bgpd/bgp_advertise.c | 2 ++ >> bgpd/bgp_fsm.c | 2 ++ >> bgpd/bgp_mplsvpn.c | 8 ++++---- >> bgpd/bgp_route.c | 24 +++++++++++++++++------- >> bgpd/bgp_vty.c | 6 +++++- >> bgpd/bgpd.c | 1 + >> bgpd/bgpd.h | 4 ++++ >> 7 files changed, 35 insertions(+), 12 deletions(-) >> >> diff --git a/bgpd/bgp_advertise.c b/bgpd/bgp_advertise.c >> index be9b480..38f5029 100644 >> --- a/bgpd/bgp_advertise.c >> +++ b/bgpd/bgp_advertise.c >> @@ -264,6 +264,7 @@ bgp_adj_out_set (struct bgp_node *rn, struct peer >> *peer, struct prefix *p, >> bgp_advertise_add (adv->baa, adv); >> >> FIFO_ADD (&peer->sync[afi][safi]->update, &adv->fifo); >> + peer->table_version++; >> } >> >> void >> @@ -299,6 +300,7 @@ bgp_adj_out_unset (struct bgp_node *rn, struct peer >> *peer, struct prefix *p, >> /* Add to synchronization entry for withdraw announcement. */ >> FIFO_ADD (&peer->sync[afi][safi]->withdraw, &adv->fifo); >> >> + peer->table_version++; >> /* Schedule packet write. */ >> BGP_WRITE_ON (peer->t_write, bgp_write, peer->fd); >> } >> > > How the table_version in maintained for the peer structure looks a little > odd. Typically if a peer's tblver is 57 you know that you have converged > that peer for all prefixes whose tblver is <= 57. What you have above is > just a counter for how many times something has been added/removed from the > peer's adj_out. > > > You could instead set the peer's tblver based on the max between the > peer's current tblver and the tblver of the prefix being advertised. > I am sorry i didn't get your point. > > > >> diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c >> index 9ac3335..93752de 100644 >> --- a/bgpd/bgp_fsm.c >> +++ b/bgpd/bgp_fsm.c >> @@ -578,6 +578,7 @@ bgp_stop (struct peer *peer) >> } >> >> peer->update_time = 0; >> + peer->table_version = 0; >> >> /* Until we are sure that there is no problem about prefix count >> this should be commented out.*/ >> @@ -830,6 +831,7 @@ bgp_establish (struct peer *peer) >> >> /* Increment established count. */ >> peer->established++; >> + peer->table_version++; >> bgp_fsm_change_status (peer, Established); >> >> /* bgp log-neighbor-changes of neighbor Up */ >> diff --git a/bgpd/bgp_mplsvpn.c b/bgpd/bgp_mplsvpn.c >> index 8a1ed70..05b7745 100644 >> --- a/bgpd/bgp_mplsvpn.c >> +++ b/bgpd/bgp_mplsvpn.c >> @@ -343,8 +343,8 @@ show_adj_route_vpn (struct vty *vty, struct peer >> *peer, struct prefix_rd *prd) >> { >> if (header) >> { >> - vty_out (vty, "BGP table version is 0, local router >> ID is %s%s", >> - inet_ntoa (bgp->router_id), VTY_NEWLINE); >> + vty_out (vty, "BGP table version is %d, local router >> ID is %s%s", >> + bgp->table_version, inet_ntoa >> (bgp->router_id), VTY_NEWLINE); >> vty_out (vty, "Status codes: s suppressed, d damped, >> h history, * valid, > best, i - internal%s", >> VTY_NEWLINE); >> vty_out (vty, "Origin codes: i - IGP, e - EGP, ? - >> incomplete%s%s", >> @@ -449,8 +449,8 @@ bgp_show_mpls_vpn (struct vty *vty, struct prefix_rd >> *prd, enum bgp_show_type ty >> vty_out (vty, v4_header_tag, VTY_NEWLINE); >> else >> { >> - vty_out (vty, "BGP table version is 0, local >> router ID is %s%s", >> - inet_ntoa (bgp->router_id), VTY_NEWLINE); >> + vty_out (vty, "BGP table version is %d, local >> router ID is %s%s", >> + bgp->table_version, inet_ntoa >> (bgp->router_id), VTY_NEWLINE); >> vty_out (vty, "Status codes: s suppressed, d >> damped, h history, * valid, > best, i - internal%s", >> VTY_NEWLINE); >> vty_out (vty, "Origin codes: i - IGP, e - EGP, ? >> - incomplete%s%s", >> diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c >> index 316fa5a..8753c02 100644 >> --- a/bgpd/bgp_route.c >> +++ b/bgpd/bgp_route.c >> @@ -1515,7 +1515,6 @@ bgp_process_announce_selected (struct peer *peer, >> struct bgp_info *selected, >> bgp_adj_out_unset (rn, peer, p, afi, safi); >> break; >> } >> - >> return 0; >> } >> >> @@ -1625,7 +1624,11 @@ bgp_process_main (struct work_queue *wq, void >> *data) >> } >> >> if (old_select) >> - bgp_info_unset_flag (rn, old_select, BGP_INFO_SELECTED); >> + { >> + bgp_info_unset_flag (rn, old_select, BGP_INFO_SELECTED); >> + bgp->table_version++; >> + old_select->peer->table_version++; >> + } >> if (new_select) >> { >> bgp_info_set_flag (rn, new_select, BGP_INFO_SELECTED); >> @@ -1633,6 +1636,11 @@ bgp_process_main (struct work_queue *wq, void >> *data) >> UNSET_FLAG (new_select->flags, BGP_INFO_MULTIPATH_CHG); >> } >> >> + if (new_select && CHECK_FLAG (new_select->flags, BGP_INFO_SELECTED)) >> + { >> + bgp->table_version++; >> + new_select->peer->table_version++; >> + } >> > > Why not just bump the table_versions in the "if (new_select)" that is > right after the "if (old_select)" up above? > The table version actually gets incremented when the changes happen for any of the best paths available. Hence i check if the flag is set before i update the version. > > Just a heads up, we have a similar patch in the queue, it is part of the > work we did on update-groups. Will defer to Donald on when that will be > posted for review. I'm not saying one is better than the other but if > Donald plans on posting it soon it might be worth holding off and taking a > look at that and then we can figure out how best to move forward. > Yeah sure we could see how to take this forward. I tested this patch in few scenarios mentioned in the following link http://www.networkworld.com/article/2221953/cisco-subnet/understanding-the-bgp-table-version---part-1--introduction-to-bgp-table-version.html http://www.networkworld.com/article/2221973/cisco-subnet/understanding-the-bgp-table-version---part-2--more-introduction-to-bgp-table-version.html Thanks, - Balaji > > Daniel > >
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
