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. > 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? 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. Daniel
_______________________________________________ Quagga-dev mailing list [email protected] https://lists.quagga.net/mailman/listinfo/quagga-dev
