Claudio Jeker([email protected]) on 2018.09.27 18:12:43 +0200:
> On Thu, Sep 27, 2018 at 04:41:22PM +0200, Claudio Jeker wrote:
> > Some people noticed that routes get stuck or to be more precise that
> > withdraws are not sent to peers in some cases. Until now bgpd did not
> > really track what was announced and what not (there is no real
> > Adj-RIB-Out) and instead tried to figure out if it should or should not
> > send the withdraw. With the increased filtering we try to push this fails
> > more often and the result is rather terrible.
> > Instead now introduce minimal tracking of announced prefixes. This adds a
> > per peer RB tree that tracks what was sent out as UPDATE. This can be
> > considered a minimal Adj-RIB-Out.
>
> As usual once you send it out you realize that something is missing.
> I first only looked at callers of up_generate_updates() but then I double
> checked the sofftreconfig out handler and noticed that we need the same
> magic there too (else the accouting is getting off).
> The following diff does this now as well. Double checked all other
> up_generate() users as well.
this is ok benno@
> :wq Claudio
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.427
> diff -u -p -r1.427 rde.c
> --- rde.c 25 Sep 2018 08:08:38 -0000 1.427
> +++ rde.c 27 Sep 2018 15:49:42 -0000
> @@ -3147,9 +3147,11 @@ rde_softreconfig_out_peer(struct rib_ent
> /* nothing todo */
> if (oa == ACTION_DENY && na == ACTION_ALLOW) {
> /* send update */
> + up_rib_add(peer, re);
> up_generate(peer, &nstate, &addr, pt->prefixlen);
> } else if (oa == ACTION_ALLOW && na == ACTION_DENY) {
> /* send withdraw */
> + up_rib_remove(peer, re);
> up_generate(peer, NULL, &addr, pt->prefixlen);
> } else if (oa == ACTION_ALLOW && na == ACTION_ALLOW) {
> /* send update if anything changed */
> @@ -3197,6 +3199,7 @@ rde_softreconfig_unload_peer(struct rib_
> prefix_nhflags(p));
> if (rde_filter(out_rules_tmp, peer, p, &ostate) != ACTION_DENY) {
> /* send withdraw */
> + up_rib_remove(peer, re);
> up_generate(peer, NULL, &addr, pt->prefixlen);
> }
> rde_filterstate_clean(&ostate);
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.193
> diff -u -p -r1.193 rde.h
> --- rde.h 20 Sep 2018 11:45:59 -0000 1.193
> +++ rde.h 27 Sep 2018 15:48:02 -0000
> @@ -55,6 +55,7 @@ LIST_HEAD(aspath_head, rde_aspath);
> TAILQ_HEAD(aspath_queue, rde_aspath);
> RB_HEAD(uptree_prefix, update_prefix);
> RB_HEAD(uptree_attr, update_attr);
> +RB_HEAD(uptree_rib, update_rib);
>
> struct rib_desc;
> struct rib;
> @@ -70,6 +71,7 @@ struct rde_peer {
> struct bgpd_addr remote_addr;
> struct bgpd_addr local_v4_addr;
> struct bgpd_addr local_v6_addr;
> + struct uptree_rib up_rib;
> struct uptree_prefix up_prefix;
> struct uptree_attr up_attrs;
> struct uplist_attr updates[AID_MAX];
> @@ -566,6 +568,8 @@ int nexthop_compare(struct nexthop *,
> /* rde_update.c */
> void up_init(struct rde_peer *);
> void up_down(struct rde_peer *);
> +int up_rib_remove(struct rde_peer *, struct rib_entry *);
> +void up_rib_add(struct rde_peer *, struct rib_entry *);
> int up_test_update(struct rde_peer *, struct prefix *);
> int up_generate(struct rde_peer *, struct filterstate *,
> struct bgpd_addr *, u_int8_t);
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 rde_update.c
> --- rde_update.c 18 Sep 2018 16:54:01 -0000 1.99
> +++ rde_update.c 27 Sep 2018 15:51:22 -0000
> @@ -53,9 +53,15 @@ struct update_attr {
> u_int16_t mpattr_len;
> };
>
> +struct update_rib {
> + RB_ENTRY(update_rib) entry;
> + struct rib_entry *re;
> +};
> +
> void up_clear(struct uplist_attr *, struct uplist_prefix *);
> int up_prefix_cmp(struct update_prefix *, struct update_prefix *);
> int up_attr_cmp(struct update_attr *, struct update_attr *);
> +int up_rib_cmp(struct update_rib *, struct update_rib *);
> int up_add(struct rde_peer *, struct update_prefix *, struct update_attr *);
>
> RB_PROTOTYPE(uptree_prefix, update_prefix, entry, up_prefix_cmp)
> @@ -64,6 +70,9 @@ RB_GENERATE(uptree_prefix, update_prefix
> RB_PROTOTYPE(uptree_attr, update_attr, entry, up_attr_cmp)
> RB_GENERATE(uptree_attr, update_attr, entry, up_attr_cmp)
>
> +RB_PROTOTYPE(uptree_rib, update_rib, entry, up_rib_cmp)
> +RB_GENERATE(uptree_rib, update_rib, entry, up_rib_cmp)
> +
> SIPHASH_KEY uptree_key;
>
> void
> @@ -77,6 +86,7 @@ up_init(struct rde_peer *peer)
> }
> RB_INIT(&peer->up_prefix);
> RB_INIT(&peer->up_attrs);
> + RB_INIT(&peer->up_rib);
> peer->up_pcnt = 0;
> peer->up_acnt = 0;
> peer->up_nlricnt = 0;
> @@ -110,13 +120,18 @@ up_clear(struct uplist_attr *updates, st
> void
> up_down(struct rde_peer *peer)
> {
> - u_int8_t i;
> + struct update_rib *ur, *nur;
> + u_int8_t i;
>
> for (i = 0; i < AID_MAX; i++)
> up_clear(&peer->updates[i], &peer->withdraws[i]);
>
> RB_INIT(&peer->up_prefix);
> RB_INIT(&peer->up_attrs);
> + RB_FOREACH_SAFE(ur, uptree_rib, &peer->up_rib, nur) {
> + RB_REMOVE(uptree_rib, &peer->up_rib, ur);
> + free(ur);
> + }
>
> peer->up_pcnt = 0;
> peer->up_acnt = 0;
> @@ -195,6 +210,42 @@ up_attr_cmp(struct update_attr *a, struc
> }
>
> int
> +up_rib_cmp(struct update_rib *a, struct update_rib *b)
> +{
> + if (a->re != b->re)
> + return (a->re > b->re ? 1 : -1);
> + return 0;
> +}
> +
> +int
> +up_rib_remove(struct rde_peer *peer, struct rib_entry *re)
> +{
> + struct update_rib *ur, u;
> + u.re = re;
> +
> + ur = RB_FIND(uptree_rib, &peer->up_rib, &u);
> + if (ur != NULL) {
> + RB_REMOVE(uptree_rib, &peer->up_rib, ur);
> + free(ur);
> + return 1;
> + } else
> + return 0;
> +}
> +
> +void
> +up_rib_add(struct rde_peer *peer, struct rib_entry *re)
> +{
> + struct update_rib *ur;
> +
> + if ((ur = calloc(1, sizeof(*ur))) == NULL)
> + fatal("%s", __func__);
> + ur->re = re;
> +
> + if (RB_INSERT(uptree_rib, &peer->up_rib, ur) != NULL)
> + free(ur);
> +}
> +
> +int
> up_add(struct rde_peer *peer, struct update_prefix *p, struct update_attr *a)
> {
> struct update_attr *na = NULL;
> @@ -409,11 +460,11 @@ withdraw:
> if (up_test_update(peer, old) != 1)
> return;
>
> - pt_getaddr(old->re->prefix, &addr);
> - if (rde_filter(rules, peer, old, NULL) == ACTION_DENY)
> + if (!up_rib_remove(peer, old->re))
> return;
>
> /* withdraw prefix */
> + pt_getaddr(old->re->prefix, &addr);
> up_generate(peer, NULL, &addr, old->re->prefix->prefixlen);
> } else {
> switch (up_test_update(peer, new)) {
> @@ -433,8 +484,8 @@ withdraw:
> }
>
> pt_getaddr(new->re->prefix, &addr);
> - up_generate(peer, &state, &addr,
> - new->re->prefix->prefixlen);
> + up_generate(peer, &state, &addr, new->re->prefix->prefixlen);
> + up_rib_add(peer, new->re);
>
> rde_filterstate_clean(&state);
> }
>
