northd: Mark BFD sessions down when no chassis can run them.

northd/en-northd.c

@@ -566,6 +566,30 @@ bfd_sync_routes_change_handler(struct engine_node *node,
     return EN_HANDLED_UNCHANGED;
 }
 
+enum engine_input_handler_result
+bfd_sync_sb_port_binding_change_handler(struct engine_node *node, void *data)
+{
+    struct bfd_sync_data *bfd_sync_data = data;
+    struct northd_data *northd_data = engine_get_input_data("northd", node);
+    const struct sbrec_port_binding_table *sbrec_port_binding_table =
+        EN_OVSDB_GET(engine_get_input("SB_port_binding", node));
+
+    /* bfd_table_sync() derives the chassis that runs a BFD session (and
+     * with it the NB/SB BFD "status" and the SB BFD "chassis_name") from
+     * the Port_Binding of the session's logical port and of its
+     * chassisredirect twin.  If a chassis binding of such a port changed
+     * (e.g. the chassis running the session went away and the binding
+     * was released), fall back to recompute; all other Port_Binding
+     * changes are irrelevant to BFD. */
+    if (!bfd_sync_handle_sb_port_binding_changes(sbrec_port_binding_table,
+                                                 &northd_data->lr_ports,
+                                                 &bfd_sync_data->bfd_ports)) {
+        return EN_UNHANDLED;
+    }
+
+    return EN_HANDLED_UNCHANGED;
+}
+
 enum engine_node_state
 en_bfd_sync_run(struct engine_node *node, void *data)
 {

northd/en-northd.h

@@ -58,6 +58,9 @@ bfd_sync_northd_change_handler(struct engine_node *node,
 enum engine_input_handler_result
 bfd_sync_routes_change_handler(struct engine_node *node,
                                void *data OVS_UNUSED);
+enum engine_input_handler_result
+bfd_sync_sb_port_binding_change_handler(struct engine_node *node,
+                                        void *data);
 
 enum engine_node_state en_bfd_sync_run(struct engine_node *node, void *data);
 void en_bfd_sync_cleanup(void *data OVS_UNUSED);

northd/inc-proc-northd.c

@@ -345,6 +345,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
     engine_add_input(&en_bfd_sync, &en_routes, bfd_sync_routes_change_handler);
     engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
     engine_add_input(&en_bfd_sync, &en_northd, bfd_sync_northd_change_handler);
+    engine_add_input(&en_bfd_sync, &en_sb_port_binding,
+                     bfd_sync_sb_port_binding_change_handler);
 
     engine_add_input(&en_ecmp_nexthop, &en_global_config, NULL);
     engine_add_input(&en_ecmp_nexthop, &en_routes, NULL);

northd/northd.c

@@ -11955,6 +11955,23 @@ bfd_get_connection_status(const struct nbrec_bfd *nb_bt,
     return bfd_rp ? bfd_rp->status : bfd_sr->status;
 }
 
+/* Returns the chassis that runs the BFD session for the logical router
+ * port 'op', mirroring the ownership rule implemented by bfd_monitor_run()
+ * in controller/pinctrl.c: ovn-controller runs the session if the
+ * chassisredirect twin of 'op' is bound to the chassis or if 'op' itself
+ * (an "l3gateway" port) is bound to the chassis.  Returns NULL if there is
+ * no currently bound BFD owner. */
+static const struct sbrec_chassis *
+bfd_get_session_chassis(const struct ovn_port *op)
+{
+    if (op->cr_port && op->cr_port->sb) {
+        return op->cr_port->sb->chassis;
+    }
+
+    return (op->sb && !strcmp(op->sb->type, "l3gateway"))
+           ? op->sb->chassis : NULL;
+}
+
 void
 bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
                const struct nbrec_bfd_table *nbrec_bfd_table,
@@ -11996,10 +12013,24 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
             continue;
         }
 
+        const struct sbrec_chassis *chassis = bfd_get_session_chassis(op);
+
         nbrec_bfd_set_status(nb_bt,
                              bfd_get_connection_status(nb_bt,
                                                        rp_bfd_connections,
                                                        sr_bfd_connections));
+
+        if (!chassis && strcmp(nb_bt->status, "admin_down") &&
+            strcmp(nb_bt->status, "down")) {
+            /* No chassis currently owns the BFD session for this port, e.g.
+             * the chassis that was running it disappeared and no other
+             * chassis took the port over.  ovn-controller is the only writer of
+             * the SB BFD status, so mark the session down from here;
+             * otherwise a stale "up" status would keep BFD-monitored
+             * static routes pointing at a dead next hop. */
+            nbrec_bfd_set_status(nb_bt, "down");
+        }
+
         if (!bfd_e->sb_bt) {
             int udp_src = bfd_get_unused_port(bfd_src_ports);
             if (udp_src < 0) {
@@ -12013,8 +12044,8 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
             sbrec_bfd_set_disc(sb_bt, 1 + random_uint32());
             sbrec_bfd_set_src_port(sb_bt, udp_src);
             sbrec_bfd_set_status(sb_bt, nb_bt->status);
-            if (op->sb->chassis) {
-                sbrec_bfd_set_chassis_name(sb_bt, op->sb->chassis->name);
+            if (chassis) {
+                sbrec_bfd_set_chassis_name(sb_bt, chassis->name);
             }
 
             int min_tx = nb_bt->n_min_tx ? nb_bt->min_tx[0] : BFD_DEF_MINTX;
@@ -12025,7 +12056,14 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
                                               : BFD_DEF_DETECT_MULT;
             sbrec_bfd_set_detect_mult(sb_bt, d_mult);
         } else {
-            if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
+            if (!chassis && strcmp(nb_bt->status, "admin_down")) {
+                /* NB status has already been set to "down" above; make
+                 * the SB status follow it instead of syncing the stale
+                 * SB status back into the NB. */
+                if (strcmp(bfd_e->sb_bt->status, "down")) {
+                    sbrec_bfd_set_status(bfd_e->sb_bt, "down");
+                }
+            } else if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
                 if (!strcmp(nb_bt->status, "admin_down") ||
                     !strcmp(bfd_e->sb_bt->status, "admin_down")) {
                     sbrec_bfd_set_status(bfd_e->sb_bt, nb_bt->status);
@@ -12035,10 +12073,10 @@ bfd_table_sync(struct ovsdb_idl_txn *ovnsb_txn,
             }
 
             build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
-            if (op->sb->chassis && !strcmp(op->sb->chassis->name,
-                                           bfd_e->sb_bt->chassis_name)) {
-                sbrec_bfd_set_chassis_name(bfd_e->sb_bt,
-                                           op->sb->chassis->name);
+
+            const char *chassis_name = chassis ? chassis->name : "";
+            if (strcmp(chassis_name, bfd_e->sb_bt->chassis_name)) {
+                sbrec_bfd_set_chassis_name(bfd_e->sb_bt, chassis_name);
             }
         }
 
@@ -12089,6 +12127,50 @@ build_bfd_map(const struct nbrec_bfd_table *nbrec_bfd_table,
     }
 }
 
+/* Returns false if a tracked SB Port_Binding change modified the chassis
+ * a BFD session runs on, i.e. the "chassis" column of a Port_Binding
+ * whose logical port (or, for a chassisredirect port, whose distributed
+ * port) has a BFD session changed.  bfd_table_sync() then needs to run
+ * again to reevaluate the session ownership: e.g. the chassis running
+ * the session went away and the binding was released, in which case the
+ * NB/SB BFD status must be marked "down".  Returns true if none of the
+ * tracked changes are relevant to BFD. */
+bool
+bfd_sync_handle_sb_port_binding_changes(
+    const struct sbrec_port_binding_table *sbrec_port_binding_table,
+    const struct hmap *lr_ports, const struct sset *bfd_ports)
+{
+    const struct sbrec_port_binding *pb;
+    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
+                                               sbrec_port_binding_table) {
+        if (sbrec_port_binding_is_new(pb) ||
+            sbrec_port_binding_is_deleted(pb)) {
+            continue;
+        }
+
+        if (!sbrec_port_binding_is_updated(pb,
+                                           SBREC_PORT_BINDING_COL_CHASSIS)) {
+            continue;
+        }
+
+        const struct ovn_port *op = ovn_port_find(lr_ports,
+                                                  pb->logical_port);
+        if (!op) {
+            continue;
+        }
+
+        if (op->primary_port) {
+            op = op->primary_port;
+        }
+
+        if (bfd_is_port_running(bfd_ports, op->key)) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 void
 build_ic_learned_svc_monitors_map(
     struct hmap *ic_learned_svc_monitors_map,

northd/northd.h

@@ -1030,6 +1030,9 @@ void bfd_table_sync(struct ovsdb_idl_txn *, const struct nbrec_bfd_table *,
                     struct sset *);
 void build_bfd_map(const struct nbrec_bfd_table *,
                    const struct sbrec_bfd_table *, struct hmap *);
+bool bfd_sync_handle_sb_port_binding_changes(
+    const struct sbrec_port_binding_table *,
+    const struct hmap *lr_ports, const struct sset *bfd_ports);
 
 void build_ic_learned_svc_monitors_map(
     struct hmap *ic_learned_svc_monitors_map,

ovn-nb.xml

@@ -6704,6 +6704,13 @@ or
             </li>
           </ul>
         </p>
+        <p>
+          The status is normally reported by the <code>ovn-controller</code>
+          running the BFD session.  If no chassis currently owns the session
+          for the logical port (e.g. the gateway-port binding was released),
+          <code>ovn-northd</code> sets the status of a session that is not
+          <code>admin_down</code> to <code>down</code>.
+        </p>
       </column>
     </group>
   </table>

ovn-sb.xml

@@ -5411,7 +5411,9 @@ tcp.flags = RST;
       </column>
 
       <column name="chassis_name">
-        The name of the chassis where the logical port is bound.
+        The name of the chassis running the BFD session.  For a
+        distributed gateway port this is the chassis its chassisredirect
+        port is bound to.  Empty when there is no currently bound owner.
       </column>
 
       <column name="options">

tests/ovn-inc-proc-graph-dump.at

@@ -164,6 +164,7 @@ digraph "Incremental-Processing-Engine" {
 	routes -> bfd_sync [[label="bfd_sync_routes_change_handler"]];
 	route_policies -> bfd_sync [[label=""]];
 	northd -> bfd_sync [[label="bfd_sync_northd_change_handler"]];
+	SB_port_binding -> bfd_sync [[label="bfd_sync_sb_port_binding_change_handler"]];
 	SB_learned_route [[style=filled, shape=box, fillcolor=white, label="SB_learned_route"]];
 	learned_route_sync [[style=filled, shape=box, fillcolor=white, label="learned_route_sync"]];
 	SB_learned_route -> learned_route_sync [[label="learned_route_sync_sb_learned_route_change_handler"]];

tests/ovn-northd.at

@@ -4419,8 +4419,15 @@ wait_row_count bfd 5
 
 # Simulate BFD up in Southbound for an automatically created entry.
 # This entry is referenced so the state in the Northbound should also
-# become "up".
+# become "up".  A BFD session can only be up if some chassis runs it, so
+# turn r0-sw2 into a gateway port and bind its chassisredirect port
+# first, like ovn-controller would do.
 wait_column down nb:bfd status logical_port=r0-sw2
+check ovn-sbctl chassis-add hv1 geneve 127.0.0.1
+check ovn-nbctl --wait=sb lrp-set-gateway-chassis r0-sw2 hv1 10
+hv1_uuid=$(fetch_column chassis _uuid name=hv1)
+check ovn-sbctl set port_binding cr-r0-sw2 chassis=$hv1_uuid
+wait_column hv1 bfd chassis_name logical_port=r0-sw2
 bfd2_uuid=$(fetch_column bfd _uuid logical_port=r0-sw2)
 check ovn-sbctl set bfd $bfd2_uuid status=up
 wait_column up nb:bfd status logical_port=r0-sw2
@@ -4468,6 +4475,122 @@ OVN_CLEANUP_NORTHD
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([BFD sessions marked down when no chassis can run them])
+AT_KEYWORDS([northd-bfd])
+ovn_start
+
+check ovn-sbctl chassis-add hv1 geneve 127.0.0.1
+
+check ovn-nbctl lr-add r0
+check ovn-nbctl lrp-add r0 r0-public 00:00:20:20:12:13 172.16.0.1/24
+check ovn-nbctl ls-add public
+check ovn-nbctl lsp-add-router-port public public-r0 r0-public
+check ovn-nbctl --wait=sb lrp-set-gateway-chassis r0-public hv1 10
+
+bfd_uuid=$(ovn-nbctl create bfd logical_port=r0-public dst_ip=172.16.0.50)
+check test -n "$bfd_uuid"
+check ovn-nbctl --bfd=$bfd_uuid lr-route-add r0 100.0.0.0/8 172.16.0.50
+
+# The gateway port is not bound to any chassis yet, so no chassis runs
+# the BFD session: both NB and SB status must be "down".
+wait_column down nb:bfd status logical_port=r0-public
+wait_column down bfd status logical_port=r0-public
+check_column "" bfd chassis_name logical_port=r0-public
+
+# Simulate hv1 claiming the chassisredirect port and the BFD session
+# coming up, like ovn-controller would do.
+hv1_uuid=$(fetch_column chassis _uuid name=hv1)
+check ovn-sbctl set port_binding cr-r0-public chassis=$hv1_uuid
+wait_column hv1 bfd chassis_name logical_port=r0-public
+
+sb_bfd_uuid=$(fetch_column bfd _uuid logical_port=r0-public)
+check ovn-sbctl set bfd $sb_bfd_uuid status=up
+wait_column up nb:bfd status logical_port=r0-public
+
+# The BFD monitored route is programmed while the session is up.
+OVS_WAIT_UNTIL([ovn-sbctl dump-flows r0 > r0flows dnl
+&& grep -q "ip4.dst == 100.0.0.0/8" r0flows])
+
+# Release the chassisredirect binding while leaving the Chassis row intact.
+# This specifically exercises the incremental Port_Binding.chassis input.
+# The session must be marked "down" in both databases and the BFD monitored
+# route must be withdrawn.
+check ovn-sbctl clear port_binding cr-r0-public chassis
+wait_column down nb:bfd status logical_port=r0-public
+wait_column down bfd status logical_port=r0-public
+check_column "" bfd chassis_name logical_port=r0-public
+OVS_WAIT_UNTIL([ovn-sbctl dump-flows r0 > r0flows dnl
+&& ! grep -q "ip4.dst == 100.0.0.0/8" r0flows])
+
+# Give the session an owner again: northd must track the owning chassis
+# but must not flip the status to "up" on its own; that is up to
+# ovn-controller's BFD negotiation.
+check ovn-sbctl set port_binding cr-r0-public chassis=$hv1_uuid
+wait_column hv1 bfd chassis_name logical_port=r0-public
+check ovn-nbctl --wait=sb sync
+check_column down nb:bfd status logical_port=r0-public
+check_column down bfd status logical_port=r0-public
+
+# Once "ovn-controller" reports the session up again, the status must
+# propagate to the Northbound as usual.
+check ovn-sbctl set bfd $sb_bfd_uuid status=up
+wait_column up nb:bfd status logical_port=r0-public
+
+# An "admin_down" session must be left untouched when it loses its
+# owner.
+route_uuid=$(fetch_column nb:logical_router_static_route _uuid \
+             ip_prefix="100.0.0.0/8")
+check ovn-nbctl clear logical_router_static_route $route_uuid bfd
+wait_column admin_down nb:bfd status logical_port=r0-public
+wait_column admin_down bfd status logical_port=r0-public
+check ovn-sbctl clear port_binding cr-r0-public chassis
+check ovn-nbctl --wait=sb sync
+check_column admin_down nb:bfd status logical_port=r0-public
+check_column admin_down bfd status logical_port=r0-public
+check_column "" bfd chassis_name logical_port=r0-public
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([BFD ownership on an l3gateway port])
+AT_KEYWORDS([northd-bfd])
+ovn_start
+
+check ovn-sbctl chassis-add hv1 geneve 127.0.0.1
+hv1_uuid=$(fetch_column chassis _uuid name=hv1)
+
+check ovn-nbctl lr-add r0
+check ovn-nbctl set logical_router r0 options:chassis=hv1
+check ovn-nbctl lrp-add r0 r0-public 00:00:20:20:12:13 172.16.0.1/24
+check ovn-nbctl ls-add public
+check ovn-nbctl lsp-add-router-port public public-r0 r0-public
+
+bfd_uuid=$(ovn-nbctl create bfd logical_port=r0-public dst_ip=172.16.0.50)
+check test -n "$bfd_uuid"
+check ovn-nbctl --bfd=$bfd_uuid lr-route-add r0 100.0.0.0/8 172.16.0.50
+
+wait_column l3gateway port_binding type logical_port=r0-public
+check ovn-sbctl set port_binding r0-public chassis=$hv1_uuid
+wait_column hv1 bfd chassis_name logical_port=r0-public
+
+sb_bfd_uuid=$(fetch_column bfd _uuid logical_port=r0-public)
+check ovn-sbctl set bfd $sb_bfd_uuid status=up
+wait_column up nb:bfd status logical_port=r0-public
+
+# The direct l3gateway binding is the owner.  Releasing it must invalidate
+# the stale status through the same incremental Port_Binding input.
+check ovn-sbctl clear port_binding r0-public chassis
+wait_column down nb:bfd status logical_port=r0-public
+wait_column down bfd status logical_port=r0-public
+check_column "" bfd chassis_name logical_port=r0-public
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD_NO_HV([
 AT_SETUP([ovn -- check CoPP config])
 AT_KEYWORDS([northd-CoPP])