diff --git a/northd/en-northd.c b/northd/en-northd.c index 480dc61cad..da57d4d74b 100644 --- a/northd/en-northd.c +++ b/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) { diff --git a/northd/en-northd.h b/northd/en-northd.h index c626310089..d528d19f76 100644 --- a/northd/en-northd.h +++ b/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); diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index d2f28666ce..cda3efd850 100644 --- a/northd/inc-proc-northd.c +++ b/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); diff --git a/northd/northd.c b/northd/northd.c index 88e3ece884..707b9d972f 100644 --- a/northd/northd.c +++ b/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, diff --git a/northd/northd.h b/northd/northd.h index 2e3a9e00da..6625491299 100644 --- a/northd/northd.h +++ b/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, diff --git a/ovn-nb.xml b/ovn-nb.xml index c741a3b327..4b3b6494af 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -6704,6 +6704,13 @@ or

+

+ The status is normally reported by the ovn-controller + running the BFD session. If no chassis currently owns the session + for the logical port (e.g. the gateway-port binding was released), + ovn-northd sets the status of a session that is not + admin_down to down. +

diff --git a/ovn-sb.xml b/ovn-sb.xml index c72a206783..319b699b6b 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -5411,7 +5411,9 @@ tcp.flags = RST; - 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. diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at index 44bf5689e8..f748a21c9d 100644 --- a/tests/ovn-inc-proc-graph-dump.at +++ b/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"]]; diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6d191c1a0b..a914b1c523 100644 --- a/tests/ovn-northd.at +++ b/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])