From 825d98347d8993fb2f7debefdaa5eabbceec996c Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 7 Oct 2018 20:47:42 -0400 Subject: [PATCH 1/3] bgpd: Add ability to dump the bgp peerhash The bgp->peerhash is a secretive bit of data that we use to quickly lookup data about peers. Unfortunately since we had not way to look at it, we had no way of knowing if it had gotten in or out of sync. Signed-off-by: Donald Sharp --- bgpd/bgp_route.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/bgpd/bgp_route.c b/bgpd/bgp_route.c index 715393b91d..49c547c5bf 100644 --- a/bgpd/bgp_route.c +++ b/bgpd/bgp_route.c @@ -11285,6 +11285,36 @@ DEFUN (clear_ip_bgp_dampening_address_mask, NULL, 0); } +static void show_bgp_peerhash_entry(struct hash_backet *backet, void *arg) +{ + struct vty *vty = arg; + struct peer *peer = backet->data; + char buf[SU_ADDRSTRLEN]; + + vty_out(vty, "\tPeer: %s %s\n", peer->host, + sockunion2str(&peer->su, buf, sizeof(buf))); +} + +DEFUN (show_bgp_peerhash, + show_bgp_peerhash_cmd, + "show bgp peerhash", + SHOW_STR + BGP_STR + "Display information about the BGP peerhash\n") +{ + struct list *instances = bm->bgp; + struct listnode *node; + struct bgp *bgp; + + for (ALL_LIST_ELEMENTS_RO(instances, node, bgp)) { + vty_out(vty, "BGP: %s\n", bgp->name); + hash_iterate(bgp->peerhash, show_bgp_peerhash_entry, + vty); + } + + return CMD_SUCCESS; +} + /* also used for encap safi */ static void bgp_config_write_network_vpn(struct vty *vty, struct bgp *bgp, afi_t afi, safi_t safi) @@ -11679,6 +11709,7 @@ void bgp_route_init(void) /* show bgp ipv4 flowspec detailed */ install_element(VIEW_NODE, &show_ip_bgp_flowspec_routes_detailed_cmd); + install_element(VIEW_NODE, &show_bgp_peerhash_cmd); } void bgp_route_finish(void) From cc4d4ce8229d6107599eb42d4f646eddbea5ddc7 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 7 Oct 2018 19:54:07 -0400 Subject: [PATCH 2/3] bgpd: Cleanup peer->su handling Cleanup calls where we were passing in the su for peer creation a tiny bit. Creating a peer from the cli will always have a conf_if *or* a su but not both. While a doppelganger will have both. Signed-off-by: Donald Sharp --- bgpd/bgp_network.c | 1 - bgpd/bgp_vty.c | 2 +- bgpd/bgpd.c | 2 ++ 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_network.c b/bgpd/bgp_network.c index d56fdd75ce..191b1641b2 100644 --- a/bgpd/bgp_network.c +++ b/bgpd/bgp_network.c @@ -406,7 +406,6 @@ static int bgp_accept(struct thread *thread) peer = peer_create(&su, peer1->conf_if, peer1->bgp, peer1->local_as, peer1->as, peer1->as_type, 0, 0, NULL); - peer->su = su; hash_release(peer->bgp->peerhash, peer); hash_get(peer->bgp->peerhash, peer, hash_alloc_intern); diff --git a/bgpd/bgp_vty.c b/bgpd/bgp_vty.c index 58f23fd2f4..f617c9f997 100644 --- a/bgpd/bgp_vty.c +++ b/bgpd/bgp_vty.c @@ -2835,7 +2835,7 @@ static int peer_conf_interface_get(struct vty *vty, const char *conf_if, peer = peer_lookup_by_conf_if(bgp, conf_if); if (peer) { if (as_str) - ret = peer_remote_as(bgp, &su, conf_if, &as, as_type, + ret = peer_remote_as(bgp, NULL, conf_if, &as, as_type, afi, safi); } else { if (bgp_flag_check(bgp, BGP_FLAG_NO_DEFAULT_IPV4) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 0300485e91..ebaa0a3e5f 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1439,6 +1439,8 @@ struct peer *peer_create(union sockunion *su, const char *conf_if, if (peer->host) XFREE(MTYPE_BGP_PEER_HOST, peer->host); peer->host = XSTRDUP(MTYPE_BGP_PEER_HOST, conf_if); + if (su) + peer->su = *su; } else if (su) { peer->su = *su; sockunion2str(su, buf, SU_ADDRSTRLEN); From 19bd3dffc19eb0a4fb13ec6da987c6bd00a1f727 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Sun, 7 Oct 2018 20:34:31 -0400 Subject: [PATCH 3/3] bgpd: Do a bit better job of tracking the bgp->peerhash When we add/remove peers we need to do a bit better job of tracking them in the bgp->peerhash. 1) When we have the doppelganger take over, make sure the winner is the one represented in the peerhash. 2) When creating the doppelganger, leave the current one in place instead of blindly replacing it. Signed-off-by: Donald Sharp --- bgpd/bgp_fsm.c | 9 +++++++++ bgpd/bgpd.c | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/bgpd/bgp_fsm.c b/bgpd/bgp_fsm.c index 65b8b5bd2d..d17426e3ff 100644 --- a/bgpd/bgp_fsm.c +++ b/bgpd/bgp_fsm.c @@ -1677,6 +1677,15 @@ static int bgp_establish(struct peer *peer) peer_delete(peer->doppelganger); } + /* + * If we are replacing the old peer for a doppelganger + * then switch it around in the bgp->peerhash + * the doppelgangers su and this peer's su are the same + * so the hash_release is the same for either. + */ + hash_release(peer->bgp->peerhash, peer); + hash_get(peer->bgp->peerhash, peer, hash_alloc_intern); + bgp_bfd_register_peer(peer); return ret; } diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index ebaa0a3e5f..ed065c55fa 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -1422,7 +1422,15 @@ void bgp_recalculate_all_bestpaths(struct bgp *bgp) } } -/* Create new BGP peer. */ +/* + * Create new BGP peer. + * + * conf_if and su are mutually exclusive if configuring from the cli. + * If we are handing a doppelganger, then we *must* pass in both + * the original peer's su and conf_if, so that we can appropriately + * track the bgp->peerhash( ie we don't want to remove the current + * one from the config ). + */ struct peer *peer_create(union sockunion *su, const char *conf_if, struct bgp *bgp, as_t local_as, as_t remote_as, int as_type, afi_t afi, safi_t safi, @@ -1435,12 +1443,13 @@ struct peer *peer_create(union sockunion *su, const char *conf_if, peer = peer_new(bgp); if (conf_if) { peer->conf_if = XSTRDUP(MTYPE_PEER_CONF_IF, conf_if); - bgp_peer_conf_if_to_su_update(peer); + if (su) + peer->su = *su; + else + bgp_peer_conf_if_to_su_update(peer); if (peer->host) XFREE(MTYPE_BGP_PEER_HOST, peer->host); peer->host = XSTRDUP(MTYPE_BGP_PEER_HOST, conf_if); - if (su) - peer->su = *su; } else if (su) { peer->su = *su; sockunion2str(su, buf, SU_ADDRSTRLEN);