Skip to content

Commit

Permalink
wedge_agent config: static route deletes do not take effect
Browse files Browse the repository at this point in the history 
Summary:
While working on D6649378, we noticed a bug in the implementation of
applyConfig().  If a static route is deleted in the config, upon a warm
boot, this deletion is not taking effect.  In other words, when wedge_agent
comes up, the deleted static route is still in the route table.

Upon investigation, the bug is the way we apply static config.  We rely on
"prevCfg" to tell us what we had applied before in order to delete stale
routes.  However, in the case of warm boot there is no prevCfg, and we
cannot reconcile stale config.

The solution is to effectively "sync" all static routes, the way we do it
for BGP. I.e., delete all existing routes and add all new routes.

Differential Revision: D8219728

fbshipit-source-id: bc92e8c263541e55cef7b16111a7e3402da86eda
  • Loading branch information
@facebook-github-bot
Allwyn Carvalho authored and facebook-github-bot committed Jul 20, 2018
1 parent 4c5af38 commit 9e321df
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 105 deletions.
90 changes: 81 additions & 9 deletions fboss/agent/ApplyThriftConfig.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,7 @@ class ThriftConfigApplier {
bool updateDhcpOverrides(Vlan* vlan, const cfg::Vlan* config);
std::shared_ptr<InterfaceMap> updateInterfaces();
std::shared_ptr<RouteTableMap> updateInterfaceRoutes();
std::shared_ptr<RouteTableMap> updateStaticRoutes(
const std::shared_ptr<RouteTableMap>& curRoutingTables);
std::shared_ptr<RouteTableMap> syncStaticRoutes();
shared_ptr<Interface> createInterface(const cfg::Interface* config,
const Interface::Addresses& addrs);
shared_ptr<Interface> updateInterface(const shared_ptr<Interface>& orig,
Expand Down Expand Up @@ -306,8 +305,7 @@ shared_ptr<SwitchState> ThriftConfigApplier::run() {
newState->resetRouteTables(newTables);
changed = true;
}
auto newerTables = updateStaticRoutes(newTables ? newTables :
orig_->getRouteTables());
auto newerTables = syncStaticRoutes();
if (newerTables) {
newState->resetRouteTables(std::move(newerTables));
changed = true;
Expand Down Expand Up @@ -1319,11 +1317,85 @@ shared_ptr<RouteTableMap> ThriftConfigApplier::updateInterfaceRoutes() {
return updater.updateDone();
}


std::shared_ptr<RouteTableMap> ThriftConfigApplier::updateStaticRoutes(
const std::shared_ptr<RouteTableMap>& curRoutingTables) {
RouteUpdater updater(curRoutingTables);
updater.updateStaticRoutes(*cfg_, *prevCfg_);
/**
* syncStaticRoutes:
*
* A long note about why we "sync" static routes from config file.
* To set the stage, we come here in one of two ways:
* (a) Switch is coming up after a warm or cold boot and is loading config
* (b) "reloadConfig" has been issued from thrift API
*
* In both cases, there may already exist static routes in our SwitchState.
* (In the case of warm boot, we read and reload our state from the warm
* boot file.)
*
* The intent of this function is that after we applyUpdate(), the static
* routes in our new SwitchState will be exactly what is in the new config,
* no more no less. Note that this means that any static routes added using
* addUnicastRoute() API will be removed after we reloadConfig, or when we
* come up after restart. This is by design.
*
* There are two ways to do the above. One way would be to go through the
* existing static routes in SwitchState and "reconcile" with that in
* config. I.e., add, delete, modify, or leave unchanged, as necessary.
*
* The second approach, the one we adopt here, not very intuitive but a lot
* cleaner to code, is to simply delete all static routes in current state,
* and to add back static routes from config file. This works because the
* "delete" in this step does not take immediate effect. It is only the
* state delta, after all processing is done, that is sent to the hardware
* switch.
*
* As a side note, there is a third (incorrect) approach that was tried, but
* does not work. The old approach was to compute the delta between old and
* new config files, and to only apply that delta. This would work for
* "reloadConfig", but does not work when the switch restarts, because we do
* not save the old config. In particular, it does not work in the case of
* "delete", i.e., the new config does not have an entry that was there in
* the old config, because there is no old config to compare with.
*/
std::shared_ptr<RouteTableMap> ThriftConfigApplier::syncStaticRoutes() {
RouteUpdater updater(orig_->getRouteTables());
auto staticClientId = StdClientIds2ClientID(StdClientIds::STATIC_ROUTE);
auto staticAdminDistance = AdminDistance::STATIC_ROUTE;
updater.removeAllRoutesForClient(RouterID(0), staticClientId);
if (cfg_->__isset.staticRoutesToNull) {
for (const auto& route : cfg_->staticRoutesToNull) {
auto prefix = folly::IPAddress::createNetwork(route.prefix);
updater.addRoute(RouterID(route.routerID), prefix.first, prefix.second,
staticClientId,
RouteNextHopEntry(RouteForwardAction::DROP,
staticAdminDistance));
}
}
if (cfg_->__isset.staticRoutesToCPU) {
for (const auto& route : cfg_->staticRoutesToCPU) {
auto prefix = folly::IPAddress::createNetwork(route.prefix);
updater.addRoute(RouterID(route.routerID), prefix.first, prefix.second,
staticClientId,
RouteNextHopEntry(RouteForwardAction::TO_CPU,
staticAdminDistance));
}
}
if (cfg_->__isset.staticRoutesWithNhops) {
for (const auto& route : cfg_->staticRoutesWithNhops) {
auto prefix = folly::IPAddress::createNetwork(route.prefix);
RouteNextHopSet nhops;
// NOTE: Static routes use the default UCMP weight so that they can be
// compatible with UCMP, i.e., so that we can do ucmp where the next
// hops resolve to a static route. If we define recursive static
// routes, that may lead to unexpected behavior where some interface
// gets more traffic. If necessary, in the future, we can make it
// possible to configure strictly ECMP static routes
for (auto& nhopStr : route.nexthops) {
nhops.emplace(UnresolvedNextHop(folly::IPAddress(nhopStr),
UCMP_DEFAULT_WEIGHT));
}
updater.addRoute(RouterID(route.routerID), prefix.first, prefix.second,
staticClientId, RouteNextHopEntry(std::move(nhops),
staticAdminDistance));
}
}
return updater.updateDone();
}

Expand Down
4 changes: 2 additions & 2 deletions fboss/agent/SwSwitch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,11 +449,11 @@ void SwSwitch::init(std::unique_ptr<TunManager> tunMgr, SwitchFlags flags) {
// route
RouteUpdater updater(initialStateDesired->getRouteTables());
updater.addRoute(RouterID(0), folly::IPAddressV4("0.0.0.0"), 0,
StdClientIds2ClientID(StdClientIds::STATIC_ROUTE),
StdClientIds2ClientID(StdClientIds::STATIC_INTERNAL),
RouteNextHopEntry(RouteForwardAction::DROP,
AdminDistance::MAX_ADMIN_DISTANCE));
updater.addRoute(RouterID(0), folly::IPAddressV6("::"), 0,
StdClientIds2ClientID(StdClientIds::STATIC_ROUTE),
StdClientIds2ClientID(StdClientIds::STATIC_INTERNAL),
RouteNextHopEntry(RouteForwardAction::DROP,
AdminDistance::MAX_ADMIN_DISTANCE));
auto newRt = updater.updateDone();
Expand Down
1 change: 1 addition & 0 deletions fboss/agent/if/ctrl.thrift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ enum StdClientIds {
INTERFACE_ROUTE = 2,
LINKLOCAL_ROUTE = 3,
NETLINK_LISTENER = 100,
STATIC_INTERNAL = 700,
OPENR = 786,
}

Expand Down
88 changes: 0 additions & 88 deletions fboss/agent/state/RouteUpdater.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,94 +654,6 @@ void RouteUpdater::addInterfaceAndLinkLocalRoutes(
}
}

template<typename StaticRouteType>
void RouteUpdater::staticRouteDelHelper(
const std::vector<StaticRouteType>& oldRoutes,
const flat_map<RouterID, flat_set<CIDRNetwork>>& newRoutes) {
for (const auto& oldRoute : oldRoutes) {
RouterID rid(oldRoute.routerID);
auto network = IPAddress::createNetwork(oldRoute.prefix);
auto itr = newRoutes.find(rid);
if (itr == newRoutes.end() || itr->second.find(network) ==
itr->second.end()) {
delRoute(rid, network.first, network.second,
StdClientIds2ClientID(StdClientIds::STATIC_ROUTE));
XLOG(DBG1) << "Unconfigured static route : " << network.first << "/"
<< (int)network.second;
}
}
}

void RouteUpdater::updateStaticRoutes(const cfg::SwitchConfig& curCfg,
const cfg::SwitchConfig& prevCfg) {
flat_map<RouterID, flat_set<CIDRNetwork>> newCfgVrf2StaticPfxs;
auto processStaticRoutesNoNhops = [&](
const std::vector<cfg::StaticRouteNoNextHops>& routes,
RouteForwardAction action) {
for (const auto& route : routes) {
RouterID rid(route.routerID) ;
auto network = IPAddress::createNetwork(route.prefix);
if (newCfgVrf2StaticPfxs[rid].find(network)
!= newCfgVrf2StaticPfxs[rid].end()) {
throw FbossError("Prefix : ", network.first, "/",
(int)network.second, " in multiple static routes");
}
addRoute(rid, network.first, network.second,
StdClientIds2ClientID(StdClientIds::STATIC_ROUTE),
RouteNextHopEntry(action, AdminDistance::STATIC_ROUTE));
// Note down prefix for comparing with old static routes
newCfgVrf2StaticPfxs[rid].emplace(network);
}
};


if (curCfg.__isset.staticRoutesToNull) {
processStaticRoutesNoNhops(curCfg.staticRoutesToNull, DROP);
}
if (curCfg.__isset.staticRoutesToCPU) {
processStaticRoutesNoNhops(curCfg.staticRoutesToCPU, TO_CPU);
}

if (curCfg.__isset.staticRoutesWithNhops) {
for (const auto& route : curCfg.staticRoutesWithNhops) {
RouterID rid(route.routerID) ;
auto network = IPAddress::createNetwork(route.prefix);
RouteNextHopSet nhops;
// NOTE: Static routes use the default UCMP weight so that they
// can be compatible with UCMP. (i.e., so that we can do ucmp where
// the next hops resolve to a static route). If we define recursive
// static routes, that may lead to unexpected behavior where some
// interface gets more traffic. If necessary, in the future, we can make
// it possible to configure strictly ECMP static routes.
for (auto& nhopStr : route.nexthops) {
nhops.emplace(
UnresolvedNextHop(folly::IPAddress(nhopStr), UCMP_DEFAULT_WEIGHT));
}
addRoute(rid, network.first, network.second,
StdClientIds2ClientID(StdClientIds::STATIC_ROUTE),
RouteNextHopEntry(
std::move(nhops), AdminDistance::STATIC_ROUTE));
// Note down prefix for comparing with old static routes
newCfgVrf2StaticPfxs[rid].emplace(network);
}
}
// Now blow away any static routes that are not in the config
// Ideally after this we should replay the routes from lower
// precedence route announcers (e.g. BGP) for deleted prefixes.
// The static route may have overridden a existing protocol
// route and in that case rather than blowing away the route we
// should just replace it - t8910011
if (prevCfg.__isset.staticRoutesWithNhops) {
staticRouteDelHelper(prevCfg.staticRoutesWithNhops, newCfgVrf2StaticPfxs);
}
if (prevCfg.__isset.staticRoutesToCPU) {
staticRouteDelHelper(prevCfg.staticRoutesToCPU, newCfgVrf2StaticPfxs);
}
if (prevCfg.__isset.staticRoutesToNull) {
staticRouteDelHelper(prevCfg.staticRoutesToNull, newCfgVrf2StaticPfxs);
}
}

template<typename RibT>
bool RouteUpdater::dedupRoutes(const RibT* oldRib, RibT* newRib) {
bool isSame = true;
Expand Down
6 changes: 0 additions & 6 deletions fboss/agent/state/RouteUpdater.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,8 @@ class RouteUpdater {
void addLinkLocalRoutes(RouterID id);
void delLinkLocalRoutes(RouterID id);

void updateStaticRoutes(const cfg::SwitchConfig& curCfg,
const cfg::SwitchConfig& prevCfg);

private:
template<typename StaticRouteType>
void staticRouteDelHelper(const std::vector<StaticRouteType>& oldRoutes,
const boost::container::flat_map<RouterID,
boost::container::flat_set<folly::CIDRNetwork>>& newRoutes);
// Forbidden copy constructor and assignment operator
RouteUpdater(RouteUpdater const &) = delete;
RouteUpdater& operator=(RouteUpdater const &) = delete;
Expand Down

0 comments on commit 9e321df

Please sign in to comment.