BgpRib removeRouteGetDelta cannot remove the _logicalArrivalTime entries of backup routes

[heroinedd]

Hi, I'm reading and running the source code of batfish recently. And found a possible bug in the removeRouteGetDelta(R route) function of the BgpRib<R>. That is, if the route to be removed is a backup route of the BgpRib<R>, the entry of the route in _logicalArrivalTime cannot be removed.

In BgpRib<R>:

328  @Override
329  public @Nonnull RibDelta<R> removeRouteGetDelta(R route) {
330    RibDelta<R> delta = actionRouteGetDelta(route, super::removeRouteGetDelta);
331    if (!delta.isEmpty()) {
332      delta.getPrefixes().forEach(this::selectBestPath);
333      if (_tieBreaker == BgpTieBreaker.ARRIVAL_ORDER) {
334        for (RouteAdvertisement<R> a : delta.getActions()) {
335          if (a.isWithdrawn()) {
336            _logicalArrivalTime.remove(a.getRoute());
337          }
338        }
339      }
340    }
341    return delta;
342  }

The first line of this function shows it calls the super function, i.e., AbstractRib<R>.removeRouteGetDelta(R route), which returns the delta of removing the route from the underlying RibTree:

  public @Nonnull RibDelta<R> removeRouteGetDelta(R route) {
    // Remove the backup route first, then remove route from rib
    removeBackupRoute(route);
    RibDelta<R> delta = _tree.removeRouteGetDelta(route, Reason.WITHDRAW);
    if (!delta.isEmpty()) {
      // A change to routes has been made
      _allRoutes = null;
    }
    return delta;
  }

However, since the route is not a best route, _tree.removeRouteGetDelta(route, Reason.WITHDRAW) returns an empty delta (since _root does not contain route):

  @Nonnull
  RibDelta<R> removeRouteGetDelta(R route, Reason reason) {
    assert reason != Reason.ADD : "cannot remove a route with reason ADD";

    Prefix network = route.getNetwork();
    boolean removed = _root.remove(network, route);
    if (!removed) {
      return RibDelta.empty();
    }
    ......
  }

Thus, the if blocks (i.e., line 331-340) in BgpRib.mergeRouteGetDelta is not executed, and the entry of route in the _logicalArrivalTime cannot be removed (i.e., line 336 is not executed).

[dhalperi]

Dumb Q: does it work to just remove route at L330?

[heroinedd]

Dumb Q: does it work to just remove route at L330?

Yes, I think the simplest solution is to add _logicalArrivalTime.remove(route) after L330.