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.

Sign up for GitHub

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

Bmp various fixes #17373

Closed
wants to merge 3 commits into from
+54 −2
Closed

Bmp various fixes #17373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e1f0d2
bgpd: bmp, fix address sanitizer issue
pguibert6WIND Oct 29, 2024
5e41ccd
bgpd: fix warning of compilation when using bgp_trace
pguibert6WIND Nov 8, 2024
454b721
bgpd: fix do re-send post-policy bgp update when not valid
pguibert6WIND Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion bgpd/bgp_bmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "bgpd/bgp_table.h"
#include "bgpd/bgpd.h"
#include "bgpd/bgp_route.h"
#include "bgpd/bgp_nht.h"
#include "bgpd/bgp_attr.h"
#include "bgpd/bgp_dump.h"
#include "bgpd/bgp_errors.h"
Expand Down Expand Up @@ -1708,6 +1709,26 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi,
return 0;
}

static int bmp_nht_path_valid(struct bgp *bgp, struct bgp_path_info *path, bool valid)
{
struct bgp_dest *dest = path->net;
struct bgp_table *table;

if (frrtrace_enabled(frr_bgp, bmp_nht_path_valid)) {
char pfxprint[PREFIX2STR_BUFFER];

prefix2str(&dest->rn->p, pfxprint, sizeof(pfxprint));
frrtrace(4, frr_bgp, bmp_nht_path_valid, bgp, pfxprint, path, valid);
}
if (bgp->peer_self == path->peer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why aren't "redistributed networks relevant for bmp" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redistributed networks are the ones that are added in BGP when doing:

router bgp 65500
address-family ipv4 unicast
redistribute static

because they originate from already selected routes, those routes are always available, and are not impacted by BGP nexthop tracking. If there is a reachability problem, BGP will receive a bgp_redistribute_delete command from the route that disappeared from static route list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh okay i see, you meant "not relevant for next hop tracking" not "for bmp"
i understand it now thx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify, when nht does not work, here is the output on a BMP collector:

{"peer_type": "route distinguisher instance", "policy": "pre-policy", "ipv6": true, "peer_ip": "192:167::3", "peer_distinguisher": "555:1", "peer_asn": 65501, "peer_bgp_id": "192.168.1.3", "timestamp": "2024-11-28 17:05:45.111963", "bmp_log_type": "update", "origin": "IGP", "as_path": "", "afi": 2, "safi": 1, "nxhp_ip": "192:167::3", "nxhp_link-local": "fe80::7caf:c7ff:fe63:2b4d", "ip_prefix": "2001::1125/128", "seq": 32}
{"peer_type": "route distinguisher instance", "policy": "post-policy", "ipv6": true, "peer_ip": "192:167::3", "peer_distinguisher": "555:1", "peer_asn": 65501, "peer_bgp_id": "192.168.1.3", "timestamp": "2024-11-28 17:05:45.111963", "bmp_log_type": "update", "origin": "IGP", "as_path": "", "afi": 2, "safi": 1, "nxhp_ip": "192:167::3", "nxhp_link-local": "fe80::7caf:c7ff:fe63:2b4d", "ip_prefix": "2001::1125/128", "seq": 33}
{"peer_type": "route distinguisher instance", "policy": "pre-policy", "ipv6": true, "peer_ip": "192:167::3", "peer_distinguisher": "555:1", "peer_asn": 65501, "peer_bgp_id": "192.168.1.3", "timestamp": "2024-11-28 17:05:45.111962", "bmp_log_type": "update", "origin": "IGP", "as_path": "", "afi": 2, "safi": 1, "nxhp_ip": "192:167::3", "nxhp_link-local": "fe80::7caf:c7ff:fe63:2b4d", "ip_prefix": "2001::1125/128", "seq": 34}
{"peer_type": "loc-rib instance", "is_filtered": false, "policy": "loc-rib", "peer_distinguisher": "555:1", "peer_asn": 65501, "peer_bgp_id": "192.168.1.1", "timestamp": "2024-11-28 17:05:45.111963", "bmp_log_type": "update", "origin": "IGP", "as_path": "", "afi": 2, "safi": 1, "nxhp_ip": "192:167::3", "nxhp_link-local": "fe80::7caf:c7ff:fe63:2b4d", "ip_prefix": "2001::1125/128", "seq": 35}

As you can see, the pre-policy message for 2001::1125/128 is sent twice.
But I think it is not a big deal, because all the information in the entry tells if it is an addpath entry or not, or if it is an ECMP entry. so we can not make mistakes when reading both messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, in #14847 the adj-rib-in post-policy is moved to the queue of loc-rib since they run lookups on the same structure (the bgp->rib)
because multiple monitoring modes share queues, i added a flag field to the bqe that tells which rib must send a message. this should fix this double message issue too :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i agree, it's not ideal but it's not a huge deal either. it will not confuse a collector

/* self declared networks or redistributed networks are not relevant for bmp */
return 0;

table = bgp_dest_table(dest);

return bmp_process(bgp, table->afi, table->safi, dest, path->peer, !valid);
}

static void bmp_stat_put_u32(struct stream *s, size_t *cnt, uint16_t type,
uint32_t value)
{
Expand Down Expand Up @@ -2018,7 +2039,8 @@ static void bmp_bgp_peer_vrf(struct bmp_bgp_peer *bbpeer, struct bgp *bgp)
memcpy(bbpeer->open_rx, s->data, open_len);

bbpeer->open_tx_len = open_len;
bbpeer->open_tx = bbpeer->open_rx;
bbpeer->open_tx = XMALLOC(MTYPE_BMP_OPEN, open_len);
memcpy(bbpeer->open_tx, s->data, open_len);

stream_free(s);
}
Expand Down Expand Up @@ -3195,6 +3217,7 @@ static int bgp_bmp_module_init(void)
hook_register(peer_status_changed, bmp_peer_status_changed);
hook_register(peer_backward_transition, bmp_peer_backward);
hook_register(bgp_process, bmp_process);
hook_register(bgp_nht_path_update, bmp_nht_path_valid);
hook_register(bgp_inst_config_write, bmp_config_write);
hook_register(bgp_inst_delete, bmp_bgp_del);
hook_register(frr_late_init, bgp_bmp_init);
Expand Down
2 changes: 1 addition & 1 deletion bgpd/bgp_io.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ static int read_ibuf_work(struct peer_connection *connection)
assert(ringbuf_get(ibw, pkt->data, pktsize) == pktsize);
stream_set_endp(pkt, pktsize);

frrtrace(2, frr_bgp, packet_read, connection->peer, pkt);
frrtrace(2, frr_bgp, packet_read, connection, pkt);
frr_with_mutex (&connection->io_mtx) {
stream_fifo_push(connection->ibuf, pkt);
}
Expand Down
6 changes: 6 additions & 0 deletions bgpd/bgp_nht.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ static void unregister_zebra_rnh(struct bgp_nexthop_cache *bnc);
static int make_prefix(int afi, struct bgp_path_info *pi, struct prefix *p);
static void bgp_nht_ifp_initial(struct event *thread);

DEFINE_HOOK(bgp_nht_path_update, (struct bgp *bgp, struct bgp_path_info *pi, bool valid),
(bgp, pi, valid));

static int bgp_isvalid_nexthop(struct bgp_nexthop_cache *bnc)
{
return (bgp_zebra_num_connects() == 0
Expand Down Expand Up @@ -1449,6 +1452,9 @@ void evaluate_paths(struct bgp_nexthop_cache *bnc)
}
}

if (path_valid != bnc_is_valid_nexthop)
hook_call(bgp_nht_path_update, bgp_path, path, bnc_is_valid_nexthop);

bgp_process(bgp_path, dest, path, afi, safi);
}

Expand Down
5 changes: 5 additions & 0 deletions bgpd/bgp_nht.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,9 @@ extern void bgp_nht_ifp_up(struct interface *ifp);
extern void bgp_nht_ifp_down(struct interface *ifp);

extern void bgp_nht_interface_events(struct peer *peer);

/* called when a path becomes valid or invalid, because of nexthop tracking */
DECLARE_HOOK(bgp_nht_path_update, (struct bgp *bgp, struct bgp_path_info *pi, bool valid),
(bgp, pi, valid));

#endif /* _BGP_NHT_H */
18 changes: 18 additions & 0 deletions bgpd/bgp_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,24 @@ TRACEPOINT_EVENT(

TRACEPOINT_LOGLEVEL(frr_bgp, bmp_process, TRACE_DEBUG)

/*
* BMP is hooked for a nexthop tracking event
*/
TRACEPOINT_EVENT(
frr_bgp,
bmp_nht_path_valid,
TP_ARGS(struct bgp *, bgp, char *, pfx, struct bgp_path_info *,
path, bool, valid),
TP_FIELDS(
ctf_string(bgp, bgp->name_pretty)
ctf_string(prefix, pfx)
ctf_string(path, PEER_HOSTNAME(path->peer))
ctf_integer(bool, valid, valid)
)
)

TRACEPOINT_LOGLEVEL(frr_bgp, bmp_nht_path_valid, TRACE_DEBUG)

/*
* bgp_dest_lock/bgp_dest_unlock
*/
Expand Down
Loading