Skip to content

Graceful transition to/from BFD-based networks - #174

Merged
rcgoodfellow merged 3 commits into
mainfrom
fix-172
Feb 22, 2024
Merged

rcgoodfellow merged 3 commits into
mainfrom
fix-172

Conversation

@rcgoodfellow

@rcgoodfellow rcgoodfellow commented Feb 21, 2024 •

Copy link
Copy Markdown
Collaborator

This change makes it possible for operators of Oxide racks to transition to/from BFD without downtime.

In traditional BFD/static-routing setups, when a BFD session associated with the nexthop of a route enters the down state, that route is removed from the data plane routing tables. This means there is no graceful way to light up a BFD session, since we have to pick one end of the session to start first, and it will detect that the other side is down and start removing all the routes associated with the corresponding nexthop.

There are a few solutions to this problem. For example, the approach taken by FRR is to separate enabling BFD's upper half that implements the protocol, from the lower half that modifies the data plane routing tables. This works well for bootstrapping BFD without loss of comms if things are done carefully.

What about the reverse problem, e.g., graceful transition off of BFD? With the separation of upper and lower halves, one could carefully disable the lower halves of BFD implementations on one half of a network bisection for planned maintenance. This can definitely be done - but it's one of those scary operations that should probably be done inside a maintenance window where downtime due to a mishap would be acceptable. And this only works for planned events, if some part of a BFD network stops speaking BFD, there is going to be a loss of comms.

This PR opts for something that is hopefully a bit more robust, is fully automatic, and does not rely on careful, tedious, potentially error-prone, and certainly anxiety-driving maintenance operations.

The primary change here is that we view BFD session state as an indicator of nexthop viability instead of a necessary condition. The basic logic is this. If there are multiple nexthops for a given prefix then:

  • If some nexthops have BFD sessions that are up and some do not, only add the ones that are up to the data plane RIB.
  • If all nexthops have BFD sessions that are up, add them all to the data plane RIB.
  • If all nexthops have BFD sessions that are down, add them all to the data plane RIB.

The last bullet may seem a bit odd at first glance, but it's the key to achieving the goals here. If we have no viable nexthops according to BFD, then there is really no harm in trying to use any of the equally bad (according to BFD) options in front of us. This allows for graceful BFD startup and shutdown with no operator involvement required. It also handles BFD weather from neighbors gracefully.

This was always the intent of the BFD implementation. But the first cut at it fell short in two key areas.

  • The full sync operation in mg_lower was just cutting out any routes that had their priority dropped to zero due to a BFD session going down. We had implemented the logic above on incremental mg_lower updates, but not for the full sync.
  • The data plane routing tables on the rack switches are not the only place that matters here. We have a tunneled overlay/underlay network on the rack that adds and withdraws tunnel endpoints according to external routes coming and going (see RFD 404 for more detail). Previously, if a set of nexthops for a rack switch became unavailable according to BFD, the corresponding tunnel routes would be withdrawn from the underlay network. This is not consistent with the three-part logic above. There is also an additional wrinkle here though. Each server sled operating off tunnel routing information is getting that information from at least two routers. The compute sleds are manipulating OPTE's virt-to-boundary routing tables and need to be able to perform the 3-part logic above. But in order to do that, they need an indicator of whether BFD thinks the route is viable or not. We don't really want DDM to be BFD aware, so we've abstracted BFD's take on nexthop viability into a new metric field added to tunnel route advertisements. In this initial commit the metric is 100 for routes with a BFD session that is up, or for routes that have no BFD session configured, and is 0 for routes that have BFD configured where the session is down. More sophisticated schemes may come into play later, such as ranking prefixes based on the number of viable nexthops for static routing, or, bringing the BGP metric into play when we connect BFD and BGP. The DDM lower half for OPTE then performs a set calculation according to the three-way logic above when setting up the boundary routing tables in OPTE.

This has been tested and is behaving as expected in a4x2. In particular, graceful startup works, and combinatorially taking down sessions behaves as expected. The rack can continue to communicate with the outside world independent of the number of BFD sessions that are actually up (assuming the upstream routers are actually viable).

The new metric field in the TunnelRoute message has a #[serde(default)], so this should be backward compatible, defaulting to the zero-valued metric.

@rcgoodfellow rcgoodfellow changed the title compute effective route set on full sync Graceful transition to/from BFD-based networks Feb 22, 2024
@rcgoodfellow
rcgoodfellow marked this pull request as ready for review February 22, 2024 03:25

@FelixMcFelix FelixMcFelix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Ry, the PR comment does a fantastic job of explaining the higher-level intent here. I'm in agreement that it makes sense, and I quite like the design of effective_route_set/EffectiveTunnelRouteSet. I have one or two questions wrt. MUpdate and some orphan functions.

Comment thread ddm/src/db.rs
pub boundary_addr: Ipv6Addr,
pub vni: u32,
#[serde(default)]
pub metric: u64,

@FelixMcFelix FelixMcFelix Feb 22, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The #[serde(default)] here will ensure that newer DDMv2(.1?) speakers have no issues receiving tunnel routes from neighbours, and from what I know of serde_json any peers will ignore the metric field which I can see is necessary for MUpdate. Good!

Are there any implications for the case during MUpdate where one scrimlet is marking all imported TunnelOrigins as having metric: 0? I'm thinking it should be fine, just possibly degraded? Thinking it out:

  • This switch is the sole origin for (prefix) => no change, we continue to advertise as there is no other equivalent TunnelOrigin with higher metric. Others ignore metric field.
  • Other switch is the sole origin for (prefix) => no change, no other equivalent TunnelOrigin with higher metric.
  • Both switches originate => the DDMv2.1 scrimlet's own nexthop is locally preferred, all other sidecars/scrimlet use both.

Is that about right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that is more or less how things will shake out. Since everyone pre-v2.1 is advertising a metric of zero implicitly, the only way priorities can start to influence routing behavior in the network is by both upgrading to 2.1 and having active BFD sessions that are up. So I think we're OK there. Downgrading from v2.1 to v2.0 could introduce deprioritization of preferred routes and decay to the current behavior, but I don't think there's much we can do about that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, glad to hear it!

Comment thread rdb/src/db.rs
Comment on lines +568 to +573
let mut result = Vec::new();
for xs in sets.values() {
for x in xs.values() {
result.push(*x);
}
}

@FelixMcFelix FelixMcFelix Feb 22, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tried to find an iter-y functional alternative, but it strikes me as being uglier:

sets.values()
    .flat_map(|v| v.values().iter().copied())
    .collect()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I find that harder to read and understand what the intent is.

Comment thread ddm/src/db.rs Outdated
Comment thread ddm/src/db.rs Outdated
Comment thread ddm/src/db.rs Outdated
Comment thread ddm/src/db.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bfd: no neighbors case

2 participants