Hi Jacob, Thanks for your review.
Em ter., 11 de ago. de 2026 às 16:33, Jacob Tanenbaum <[email protected]> escreveu: > > > On Fri, Jul 24, 2026 at 12:41 PM Lucas Vargas Dias > <[email protected]> wrote: > >> Until now any change to a logical router's ports fell back to a full >> northd recompute, because lr_changes_can_be_handled() rejected the >> LOGICAL_ROUTER "ports" column and any change to a router port row. >> Enable incremental processing for creation and deletion of regular >> (non-gateway) logical router ports (LRPs), mirroring the router logical >> switch port path added by the preceding commit. >> >> Like a router switch port, an LRP is not self-contained: a full >> recompute assigns a tunnel key and SB Port_Binding in build_ports(), >> populates od->router_ips, and adds a connected route per LRP network. >> The incremental path now performs (and tears down) this work itself: >> lr_port_create()/lr_port_init() mirror the LRP branch of >> join_logical_ports() plus the build_ports() post-processing, and the >> connected routes are added/removed incrementally through the >> routes -> group_ecmp_route -> lflow chain. >> >> The LRP's own flows are regenerated in the lflow port-change handler >> (build_lswitch_and_lrouter_iterate_by_lrp() on op->lflow_ref and >> build_lbnat_lflows_iterate_by_lrp() on op->stateful_lflow_ref), while >> the per-datapath flows (e.g. lr_in_network_id) are regenerated by >> re-tracking the router datapath to the lflow engine. >> >> Dependencies that live outside the LRP's lflow_ref and that this path >> does not keep in sync trigger a fall back to a full recompute >> (lrp_needs_recompute()): distributed gateway ports (gateway_chassis / >> ha_chassis_group), gateway routers, LRP<->LRP peering, NAT, load >> balancers, static or dynamic routing, policies, IPv6 RA, prefix >> delegation, custom route tables, mcast relay, a pre-existing peer >> switch port, or an NB Static_MAC_Binding referencing the port. Updates >> to an existing LRP also fall back to recompute; only create and delete >> are incremental. >> >> While enabling this, fix a latent bug in the group_ecmp route engine: >> unique_routes_remove() matched a route to delete by prefix hash only, so >> deleting one of several connected routes that share a prefix but differ >> in out_port (e.g. the automatic fe80::/64 link-local route present on >> every LRP) could remove the wrong entry. Match the out_port exactly on >> deletion. >> >> Add tests covering incremental create/delete of a router port (including >> the resulting connected-route flows) and the recompute fallbacks for the >> distributed-gateway, dynamic-routing, pre-existing-peer and >> modification cases. >> > > is this bug easy to recreate? is it possible to add a test for it? > > Yes, it's easy to recreate, I'll add in a new version. > >> Assisted-by: Claude Opus 4.8, ClaudeCode >> Signed-off-by: Lucas Vargas Dias <[email protected]> >> --- >> northd/en-group-ecmp-route.c | 29 +- >> northd/en-lflow.c | 7 + >> northd/en-multicast.c | 11 + >> northd/en-northd.c | 66 +++- >> northd/en-sync-sb.c | 2 + >> northd/northd.c | 572 +++++++++++++++++++++++++++++++++-- >> northd/northd.h | 27 +- >> tests/ovn-northd.at | 111 ++++++- >> 8 files changed, 775 insertions(+), 50 deletions(-) >> >> diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c >> index 87dade486..1318aa7c0 100644 >> --- a/northd/en-group-ecmp-route.c >> +++ b/northd/en-group-ecmp-route.c >> @@ -180,11 +180,19 @@ unique_routes_destroy(struct hmap *unique_routes) >> hmap_destroy(unique_routes); >> } >> >> -/* Remove the unique_routes_node from the group, and return the >> parsed_route >> - * pointed by the removed node. */ >> +/* Remove a unique_routes_node from the group, and return the >> parsed_route >> + * pointed by the removed node. >> + * >> + * 'route->prefix'/plen/is_src_route/source/route_table_id are always >> matched. >> + * When 'exact' is true the output port is matched too: several routes >> can >> + * share the same prefix (e.g. the IPv6 link-local fe80::/64 connected >> route >> + * present on every router port) and differ only by their output port, >> so a >> + * deletion must target the specific route. When 'exact' is false any >> route >> + * to the prefix is returned, which is what ECMP-group formation needs >> when a >> + * second next hop for an existing prefix is added. */ >> static const struct parsed_route * >> -unique_routes_remove(struct group_ecmp_datapath *gn, >> - const struct parsed_route *route) >> +unique_routes_remove__(struct group_ecmp_datapath *gn, >> + const struct parsed_route *route, bool exact) >> { >> struct unique_routes_node *ur; >> HMAP_FOR_EACH_WITH_HASH (ur, hmap_node, route->hash, >> &gn->unique_routes) { >> @@ -192,7 +200,8 @@ unique_routes_remove(struct group_ecmp_datapath *gn, >> route->plen == ur->route->plen && >> route->is_src_route == ur->route->is_src_route && >> route->source == ur->route->source && >> - route->route_table_id == ur->route->route_table_id) { >> + route->route_table_id == ur->route->route_table_id && >> + (!exact || route->out_port == ur->route->out_port)) { >> hmap_remove(&gn->unique_routes, &ur->hmap_node); >> const struct parsed_route *existed_route = ur->route; >> free(ur); >> @@ -202,6 +211,13 @@ unique_routes_remove(struct group_ecmp_datapath *gn, >> return NULL; >> } >> >> +static const struct parsed_route * >> +unique_routes_remove(struct group_ecmp_datapath *gn, >> + const struct parsed_route *route) >> +{ >> + return unique_routes_remove__(gn, route, false); >> +} >> + >> static void >> ecmp_groups_add_route(struct ecmp_groups_node *group, >> const struct parsed_route *route) >> @@ -415,7 +431,8 @@ handle_deleted_route(struct group_ecmp_route_data >> *data, >> return false; >> } >> >> - const struct parsed_route *existing = unique_routes_remove(node, pr); >> + const struct parsed_route *existing = unique_routes_remove__(node, >> pr, >> + true); >> if (!existing) { >> /* The route must be part of an ecmp group. */ >> if (pr->source == ROUTE_SOURCE_CONNECTED) { >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 8cb987777..99df5f08f 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -169,6 +169,13 @@ lflow_northd_handler(struct engine_node *node, >> return EN_UNHANDLED; >> } >> >> + if (!lflow_handle_northd_lrp_changes(eng_ctx->ovnsb_idl_txn, >> + &northd_data->trk_data.trk_lrps, >> + &lflow_input, >> + lflow_data->lflow_table)) { >> + return EN_UNHANDLED; >> + } >> + >> if (!lflow_handle_northd_lb_changes( >> eng_ctx->ovnsb_idl_txn, &northd_data->trk_data.trk_lbs, >> &lflow_input, lflow_data->lflow_table)) { >> diff --git a/northd/en-multicast.c b/northd/en-multicast.c >> index 5148d8840..bfe3c4d92 100644 >> --- a/northd/en-multicast.c >> +++ b/northd/en-multicast.c >> @@ -160,6 +160,17 @@ multicast_igmp_northd_handler(struct engine_node >> *node, void *data OVS_UNUSED) >> return EN_UNHANDLED; >> } >> >> + /* A created/deleted logical router port may join or leave the >> router's >> + * multicast groups; recompute this (cheap) node. It does not force >> a >> + * recompute of the lflow node, which consumes multicast_igmp via >> its own >> + * incremental handler. */ >> + struct tracked_ovn_ports *trk_lrps = &northd_data->trk_data.trk_lrps; >> + if (hmapx_count(&trk_lrps->created) || >> + hmapx_count(&trk_lrps->updated) || >> + hmapx_count(&trk_lrps->deleted)) { >> + return EN_UNHANDLED; >> + } >> + >> /* This node uses the below data from the en_northd engine node. >> * - northd_data->lr_datapaths >> * - northd_data->ls_ports >> diff --git a/northd/en-northd.c b/northd/en-northd.c >> index 2bee1a996..bda4c9e80 100644 >> --- a/northd/en-northd.c >> +++ b/northd/en-northd.c >> @@ -199,15 +199,17 @@ northd_nb_logical_router_handler(struct engine_node >> *node, >> { >> struct northd_data *nd = data; >> struct northd_input input_data; >> + const struct engine_context *eng_ctx = engine_get_context(); >> >> northd_get_input_data(node, &input_data); >> >> - if (!northd_handle_lr_changes(&input_data, nd)) { >> + if (!northd_handle_lr_changes(eng_ctx->ovnsb_idl_txn, &input_data, >> nd)) { >> return EN_UNHANDLED; >> } >> >> if (northd_has_lr_nats_in_tracked_data(&nd->trk_data) || >> northd_has_lrouters_in_tracked_data(&nd->trk_data) || >> + northd_has_lrps_in_tracked_data(&nd->trk_data) || >> northd_has_lr_route_in_tracked_data(&nd->trk_data)) { >> return EN_HANDLED_UPDATED; >> } >> @@ -332,23 +334,61 @@ enum engine_input_handler_result >> routes_northd_change_handler(struct engine_node *node, >> void *data OVS_UNUSED) >> { >> + struct routes_data *routes_data = data; >> struct northd_data *northd_data = engine_get_input_data("northd", >> node); >> if (!northd_has_tracked_data(&northd_data->trk_data)) { >> return EN_UNHANDLED; >> } >> >> - /* This node uses the below data from the en_northd engine node. >> - * See (lr_stateful_get_input_data()) >> - * 1. northd_data->lr_datapaths >> - * 2. northd_data->lr_ports >> - * This data gets updated when a logical router or logical >> router port >> - * is created or deleted. >> - * Northd engine node presently falls back to full recompute >> when >> - * this happens and so does this node. >> - * Note: When we add I-P to the created/deleted logical routers >> or >> - * logical router ports, we need to revisit this handler. >> - * >> - */ >> + /* This node uses northd_data->lr_datapaths and >> northd_data->lr_ports. >> + * Creating or deleting a regular logical router port changes the >> set of >> + * directly-connected routes; handle that incrementally. Any other >> change >> + * to this data is either irrelevant to routes (e.g. a portless >> router >> + * create/delete has no connected routes) or already forced a full >> + * recompute by the northd node. */ >> + struct tracked_ovn_ports *trk_lrps = &northd_data->trk_data.trk_lrps; >> + if (hmapx_is_empty(&trk_lrps->created) && >> + hmapx_is_empty(&trk_lrps->updated) && >> + hmapx_is_empty(&trk_lrps->deleted)) { >> + return EN_HANDLED_UNCHANGED; >> + } >> + >> + /* LRP modifications are not incrementally processed by the northd >> node >> + * (they force a recompute), so trk_lrps->updated is always empty >> here. */ >> + ovs_assert(hmapx_is_empty(&trk_lrps->updated)); >> + >> + routes_data->tracked = true; >> + >> + struct hmapx_node *hmapx_node; >> + struct ovn_port *op; >> + >> + /* Deleted LRPs: drop their connected routes. An LRP with N >> networks has >> + * N connected routes that share the LRP's uuid as source hint, so >> loop >> + * until none remains. */ >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->deleted) { >> + op = hmapx_node->data; >> + struct parsed_route *pr; >> + while ((pr = parsed_route_lookup_by_source( >> + ROUTE_SOURCE_CONNECTED, &op->nbrp->header_, >> + &routes_data->parsed_routes))) { >> + hmap_remove(&routes_data->parsed_routes, &pr->key_node); >> + hmapx_add(&routes_data->trk_data.trk_deleted_parsed_route, >> pr); >> + } >> + } >> + >> + /* Created LRPs: add their connected routes. */ >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->created) { >> + op = hmapx_node->data; >> + parsed_routes_add_connected( >> + op->od, op, &routes_data->parsed_routes, >> + &routes_data->trk_data.trk_crupdated_parsed_route); >> + } >> + >> + if >> (!hmapx_is_empty(&routes_data->trk_data.trk_crupdated_parsed_route) || >> + >> !hmapx_is_empty(&routes_data->trk_data.trk_deleted_parsed_route)) { >> + return EN_HANDLED_UPDATED; >> + } >> + >> return EN_HANDLED_UNCHANGED; >> } >> >> diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c >> index db9cf5cf3..b51ece452 100644 >> --- a/northd/en-sync-sb.c >> +++ b/northd/en-sync-sb.c >> @@ -413,6 +413,8 @@ sync_to_sb_pb_northd_handler(struct engine_node >> *node, void *data OVS_UNUSED) >> >> sync_pbs_for_northd_changed_ovn_ports(&nd->trk_data.trk_lsps, >> &lr_stateful_data->table); >> + sync_pbs_for_northd_changed_lrps(&nd->trk_data.trk_lrps, >> + &lr_stateful_data->table); >> return EN_HANDLED_UPDATED; >> } >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 404ab50a9..eca5d6f0e 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4363,6 +4363,27 @@ sync_pbs_for_northd_changed_ovn_ports( >> } >> } >> >> +/* Set the SB Port_Binding options (peer, dynamic-routing, ...) of >> created and >> + * updated logical router ports. Deleted LRPs already had their SB row >> removed >> + * by lr_handle_lrp_changes(). */ >> +void >> +sync_pbs_for_northd_changed_lrps( >> + struct tracked_ovn_ports *trk_lrps, >> + const struct lr_stateful_table *lr_stateful_table) >> +{ >> + struct hmapx_node *hmapx_node; >> + struct ovn_port *op; >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->created) { >> + op = hmapx_node->data; >> + sync_pb_for_lrp(op, lr_stateful_table); >> + } >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->updated) { >> + op = hmapx_node->data; >> + sync_pb_for_lrp(op, lr_stateful_table); >> + } >> +} >> + >> void >> sync_pbs_for_lr_stateful_changes(const struct ovn_datapath *od, >> const struct lr_stateful_table >> *lr_stateful) >> @@ -4652,6 +4673,7 @@ destroy_northd_data_tracked_changes(struct >> northd_data *nd) >> { >> struct northd_tracked_data *trk_changes = &nd->trk_data; >> destroy_tracked_ovn_ports(&trk_changes->trk_lsps); >> + destroy_tracked_ovn_ports(&trk_changes->trk_lrps); >> destroy_tracked_lbs(&trk_changes->trk_lbs); >> hmapx_clear(&trk_changes->trk_nat_lrs); >> hmapx_clear(&trk_changes->trk_lrs_routes); >> @@ -4675,6 +4697,9 @@ init_northd_tracked_data(struct northd_data *nd) >> hmapx_init(&trk_data->trk_lsps.created); >> hmapx_init(&trk_data->trk_lsps.updated); >> hmapx_init(&trk_data->trk_lsps.deleted); >> + hmapx_init(&trk_data->trk_lrps.created); >> + hmapx_init(&trk_data->trk_lrps.updated); >> + hmapx_init(&trk_data->trk_lrps.deleted); >> hmapx_init(&trk_data->trk_lbs.crupdated); >> hmapx_init(&trk_data->trk_lbs.deleted); >> hmapx_init(&trk_data->trk_nat_lrs); >> @@ -4694,6 +4719,9 @@ destroy_northd_tracked_data(struct northd_data *nd) >> hmapx_destroy(&trk_data->trk_switches.deleted); >> hmapx_destroy(&trk_data->trk_lsps.updated); >> hmapx_destroy(&trk_data->trk_lsps.deleted); >> + hmapx_destroy(&trk_data->trk_lrps.created); >> + hmapx_destroy(&trk_data->trk_lrps.updated); >> + hmapx_destroy(&trk_data->trk_lrps.deleted); >> hmapx_destroy(&trk_data->trk_lbs.crupdated); >> hmapx_destroy(&trk_data->trk_lbs.deleted); >> hmapx_destroy(&trk_data->trk_nat_lrs); >> @@ -5091,6 +5119,220 @@ ls_port_reinit(struct ovn_port *op, struct >> ovsdb_idl_txn *ovnsb_txn, >> sbrec_encap_by_ip); >> } >> >> +/* Find the logical router port 'nbrp' among the ports of the logical >> router >> + * datapath 'od'. Mirror of ovn_port_find_in_datapath() for LRPs. */ >> +static struct ovn_port * >> +ovn_lrp_find_in_datapath(struct ovn_datapath *od, >> + const struct nbrec_logical_router_port *nbrp) >> +{ >> + struct ovn_port *op; >> + HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(nbrp->name, 0), >> + &od->ports) { >> + if (op->nbrp == nbrp && !strcmp(op->key, nbrp->name)) { >> + return op; >> + } >> + } >> + return NULL; >> +} >> + >> +/* Find a logical switch port of type "router" that peers with the >> logical >> + * router port 'nbrp' (i.e. its options:router-port names 'nbrp'). */ >> +static struct ovn_port * >> +lrp_find_peer_lsp(const struct hmap *ls_ports, >> + const struct nbrec_logical_router_port *nbrp) >> +{ >> + struct ovn_port *op; >> + HMAP_FOR_EACH (op, key_node, ls_ports) { >> + if (op->nbsp && lsp_is_router(op->nbsp)) { >> + const char *rp = smap_get(&op->nbsp->options, "router-port"); >> + if (rp && !strcmp(rp, nbrp->name)) { >> + return op; >> + } >> + } >> + } >> + return NULL; >> +} >> + >> +/* Symmetric counterpart of router_lsp_needs_recompute(): a logical >> router port >> + * (LRP) is not self-contained either. A full recompute wires its peer >> + * relationship (to a "router" LSP) and generates flows owned by >> lflow_refs >> + * other than the port's own. Return true when the LRP pulls in a >> dependency >> + * that this incremental path does not keep in sync, so the caller falls >> back >> + * to a full recompute. */ >> +static bool >> +lrp_needs_recompute(struct ovn_datapath *od, >> + const struct nbrec_logical_router_port *nbrp, >> + const struct hmap *ls_ports, >> + const struct nbrec_static_mac_binding_table >> *nb_smb_table) >> +{ >> + /* A Static_MAC_Binding referencing this port is synced to the SB by >> + * build_static_mac_binding_table(), which only runs on a full northd >> + * recompute; creating/deleting the port incrementally would leave >> the SB >> + * Static_MAC_Binding stale. These are rare, so fall back to >> recompute. */ >> + const struct nbrec_static_mac_binding *nb_smb; >> + NBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH (nb_smb, nb_smb_table) { >> + if (!strcmp(nb_smb->logical_port, nbrp->name)) { >> + return true; >> + } >> + } >> + >> + /* Distributed gateway ports / cr-ports (gateway_chassis, >> ha_chassis_group) >> + * pull in chassisredirect handling not supported here. */ >> + if (nbrp->n_gateway_chassis || nbrp->ha_chassis_group) { >> + return true; >> + } >> + >> + /* LRP-to-LRP peering, disabled ports, prefix delegation, >> redirect-type and >> + * per-port route tables are not supported. */ >> + if (nbrp->peer || !lrport_is_enabled(nbrp) || >> + smap_get_bool(&nbrp->options, "prefix_delegation", false) || >> + smap_get(&nbrp->options, "redirect-type") || >> + smap_get(&nbrp->options, "route_table")) { >> + return true; >> + } >> + >> + /* IPv6 RA flows are owned by the port's lflow_ref but also flip >> + * datapath-level state; keep it simple and fall back. */ >> + if (!smap_is_empty(&nbrp->ipv6_ra_configs)) { >> + return true; >> + } >> + >> + /* Gateway-router / distributed-gateway complexity on this router. */ >> + if (od->is_gw_router || smap_get(&od->nbr->options, "chassis") || >> + !vector_is_empty(&od->l3dgw_ports)) { >> + return true; >> + } >> + >> + /* NAT, static routes, route policies, LBs and dynamic routing on >> this >> + * router pull in stateful/routable/advertised-route/policy >> dependencies >> + * that live outside the port's lflow_ref and are not tracked here. >> */ >> + const struct nbrec_logical_router *nbr = od->nbr; >> + if (nbr->n_nat || nbr->n_static_routes || nbr->n_policies || >> + nbr->n_load_balancer || nbr->n_load_balancer_group) { >> + return true; >> + } >> + if (od->dynamic_routing || >> + od->dynamic_routing_redistribute != DRRM_NONE) { >> + return true; >> + } >> + >> + /* mcast relay flips od->mcast_info and the peer switch's >> flood_relay. */ >> + if (od->mcast_info.rtr.relay) { >> + return true; >> + } >> + >> + /* Wiring an already-present peer "router" LSP from the LRP side is >> not >> + * supported; fall back to recompute. The common ordering creates >> the LRP >> + * first (no peer yet), so the peer LSP is wired later by the >> incremental >> + * LSP path (ls_router_port_wire_peer()). */ >> + if (lrp_find_peer_lsp(ls_ports, nbrp)) { >> + return true; >> + } >> + >> + return false; >> +} >> + >> +/* Initialize a newly created ovn_port for a regular (non-gateway) >> logical >> + * router port, mirroring the LRP path of join_logical_ports_lrp() plus >> the >> + * tunnel-key and SB port-binding steps of build_ports(). Ownership of >> + * '*lrp_networks' is transferred to 'op'. 'op->nbrp' and 'op->key' >> must be >> + * set. Returns false (leaving cleanup to the caller) on failure. */ >> +static bool >> +lr_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn, >> + struct ovn_datapath *od, const struct sbrec_port_binding >> *sb, >> + struct lport_addresses *lrp_networks, >> + struct ovsdb_idl_index *sbrec_chassis_by_name, >> + struct ovsdb_idl_index *sbrec_chassis_by_hostname, >> + struct ovsdb_idl_index *sbrec_encap_by_ip) >> +{ >> + op->od = od; >> + op->lrp_networks = *lrp_networks; >> + op->prefix_delegation = smap_get_bool(&op->nbrp->options, >> + "prefix_delegation", false); >> + op->dynamic_routing_redistribute = >> + parse_dynamic_routing_redistribute(&op->nbrp->options, >> + >> od->dynamic_routing_redistribute, >> + op->nbrp->name); >> + >> + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { >> + sset_add(&op->od->router_ips, >> op->lrp_networks.ipv4_addrs[j].addr_s); >> + } >> + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) { >> + /* Exclude the LLA. */ >> + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) { >> + sset_add(&op->od->router_ips, >> + op->lrp_networks.ipv6_addrs[j].addr_s); >> + } >> + } >> + >> + /* Assign explicitly requested tunnel ids first. */ >> + if (!ovn_port_assign_requested_tnl_id(op)) { >> + return false; >> + } >> + /* Keep a nonconflicting tunnel ID that is already assigned. */ >> + if (sb && !op->tunnel_key) { >> + ovn_port_add_tnlid(op, sb->tunnel_key); >> + } >> + /* Assign a new tunnel id if needed. */ >> + if (!ovn_port_allocate_key(op)) { >> + return false; >> + } >> + /* Create the SB port binding, if needed. */ >> + if (sb) { >> + op->sb = sb; >> + } else { >> + op->sb = sbrec_port_binding_insert(ovnsb_txn); >> + sbrec_port_binding_set_logical_port(op->sb, op->key); >> + } >> + /* A regular LRP takes the non-gateway "patch" branch of >> + * ovn_port_update_sbrec(), so the ha-chassis-group / mirror / >> queue-id >> + * arguments are unused and passing NULL is safe. The SB options >> are set >> + * later by sync_pb_for_lrp(). */ >> + ovn_port_update_sbrec(ovnsb_txn, sbrec_chassis_by_name, >> + sbrec_chassis_by_hostname, NULL, NULL, >> + sbrec_encap_by_ip, op, NULL, NULL); >> + return true; >> +} >> + >> +/* Create an ovn_port for a regular logical router port and insert it >> into >> + * 'lr_ports' and 'od->ports'. Returns NULL (after cleaning up) on >> failure. */ >> +static struct ovn_port * >> +lr_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *lr_ports, >> + const char *key, >> + const struct nbrec_logical_router_port *nbrp, >> + struct ovn_datapath *od, struct lport_addresses >> *lrp_networks, >> + struct ovsdb_idl_index *sbrec_chassis_by_name, >> + struct ovsdb_idl_index *sbrec_chassis_by_hostname, >> + struct ovsdb_idl_index *sbrec_encap_by_ip) >> +{ >> + struct ovn_port *op = ovn_port_create(lr_ports, key, NULL, nbrp, >> NULL); >> + hmap_insert(&od->ports, &op->dp_node, hmap_node_hash(&op->key_node)); >> + if (!lr_port_init(op, ovnsb_txn, od, NULL, lrp_networks, >> + sbrec_chassis_by_name, sbrec_chassis_by_hostname, >> + sbrec_encap_by_ip)) { >> + ovn_port_destroy(lr_ports, op); >> + return NULL; >> + } >> + ipam_add_lrp_port_addresses(op); >> + return op; >> +} >> + >> +/* Remove the router-port IPs of 'op' from od->router_ips on deletion. */ >> +static void >> +lr_port_remove_router_ips(struct ovn_port *op) >> +{ >> + for (size_t j = 0; j < op->lrp_networks.n_ipv4_addrs; j++) { >> + sset_find_and_delete(&op->od->router_ips, >> + op->lrp_networks.ipv4_addrs[j].addr_s); >> + } >> + for (size_t j = 0; j < op->lrp_networks.n_ipv6_addrs; j++) { >> + if (!in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)) { >> + sset_find_and_delete(&op->od->router_ips, >> + op->lrp_networks.ipv6_addrs[j].addr_s); >> + } >> + } >> +} >> + >> /* Returns true if the logical switch has changes which can be >> * incrementally handled. >> * Presently supports i-p for the below changes: >> @@ -5834,6 +6076,7 @@ lr_changes_can_be_handled(const struct >> nbrec_logical_router *lr) >> if (col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER >> || col == NBREC_LOGICAL_ROUTER_COL_LOAD_BALANCER_GROUP >> || col == NBREC_LOGICAL_ROUTER_COL_NAT >> + || col == NBREC_LOGICAL_ROUTER_COL_PORTS >> || col == NBREC_LOGICAL_ROUTER_COL_STATIC_ROUTES) { >> continue; >> } >> @@ -5841,14 +6084,12 @@ lr_changes_can_be_handled(const struct >> nbrec_logical_router *lr) >> } >> } >> >> + /* Note: changes to the referenced logical router port rows are >> classified >> + * per-port in lr_handle_lrp_changes() (created ports are handled; >> modified >> + * ports fall back to a full recompute). */ >> + >> /* Check if the referenced rows are changed. >> XXX: Need a better OVSDB IDL interface for this check. */ >> - for (size_t i = 0; i < lr->n_ports; i++) { >> - if (nbrec_logical_router_port_row_get_seqno(lr->ports[i], >> - OVSDB_IDL_CHANGE_MODIFY) > 0) { >> - return false; >> - } >> - } >> if (lr->copp && nbrec_copp_row_get_seqno(lr->copp, >> OVSDB_IDL_CHANGE_MODIFY) > 0) { >> return false; >> @@ -5906,6 +6147,131 @@ is_lr_static_routes_changed(const struct >> nbrec_logical_router *nbr) >> || is_lr_static_routes_seqno_changed(nbr); >> } >> >> +/* Handles logical router port changes of a changed (created, updated or >> + * deleted) logical router 'changed_lr' with datapath 'od'. Regular >> + * (non-gateway) LRPs are created and deleted incrementally; anything >> that >> + * pulls in dependencies not tracked here (see lrp_needs_recompute()) or >> a >> + * modification of an existing LRP falls back to a full recompute. >> + * >> + * Returns false if any port can't be incrementally handled. */ >> +static bool >> +lr_handle_lrp_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, >> + const struct nbrec_logical_router *changed_lr, >> + const struct northd_input *ni, struct northd_data >> *nd, >> + struct ovn_datapath *od, >> + struct tracked_ovn_ports *trk_lrps) >> +{ >> + bool lr_deleted = nbrec_logical_router_is_deleted(changed_lr); >> + bool lr_ports_changed = lr_deleted; >> + if (!nbrec_logical_router_is_updated(changed_lr, >> + >> NBREC_LOGICAL_ROUTER_COL_PORTS)) { >> + for (size_t i = 0; i < changed_lr->n_ports; i++) { >> + if (nbrec_logical_router_port_row_get_seqno( >> + changed_lr->ports[i], OVSDB_IDL_CHANGE_MODIFY) > 0 || >> + !ovn_lrp_find_in_datapath(od, changed_lr->ports[i])) { >> + lr_ports_changed = true; >> + break; >> + } >> + } >> + } else { >> + lr_ports_changed = true; >> + } >> + >> + if (!lr_ports_changed) { >> + return true; >> + } >> + >> + struct ovn_port *op; >> + HMAP_FOR_EACH (op, dp_node, &od->ports) { >> + op->visited = false; >> + } >> + >> + /* Create newly added LRPs. Modifications fall back to recompute. */ >> + if (!lr_deleted) { >> + for (size_t j = 0; j < changed_lr->n_ports; j++) { >> + struct nbrec_logical_router_port *new_nbrp = >> changed_lr->ports[j]; >> + op = ovn_lrp_find_in_datapath(od, new_nbrp); >> + >> + if (!op) { >> + if (lrp_needs_recompute(od, new_nbrp, &nd->ls_ports, >> + >> ni->nbrec_static_mac_binding_table)) { >> + goto fail; >> + } >> + struct lport_addresses lrp_networks; >> + if (!extract_lrp_networks(new_nbrp, &lrp_networks)) { >> + goto fail; >> + } >> + if (!lrp_networks.n_ipv4_addrs && >> + !lrp_networks.n_ipv6_addrs) { >> + /* join_logical_ports_lrp() skips such ports. */ >> + destroy_lport_addresses(&lrp_networks); >> + goto fail; >> + } >> + op = lr_port_create(ovnsb_idl_txn, &nd->lr_ports, >> + new_nbrp->name, new_nbrp, od, >> + &lrp_networks, >> + ni->sbrec_chassis_by_name, >> + ni->sbrec_chassis_by_hostname, >> + ni->sbrec_encap_by_ip); >> + if (!op) { >> + goto fail; >> + } >> + add_op_to_northd_tracked_ports(&trk_lrps->created, op); >> + /* Datapath-level router flows that iterate the router's >> ports >> + * (e.g. build_lrouter_network_id_flows()) live on >> + * od->datapath_lflows; retrack the router so the lflow >> engine >> + * regenerates them. */ >> + hmapx_add(&nd->trk_data.trk_routers.crupdated, od); >> + } else if (nbrec_logical_router_port_row_get_seqno( >> + new_nbrp, OVSDB_IDL_CHANGE_MODIFY) > 0) { >> + /* Re-initializing an LRP (re-wiring its peer, networks, >> ...) >> + * is not supported yet. */ >> + goto fail; >> + } >> + op->visited = true; >> + } >> + } >> + >> + /* Delete removed LRPs. */ >> + bool lrp_deleted = false; >> + HMAP_FOR_EACH_SAFE (op, dp_node, &od->ports) { >> + if (op->visited || !op->nbrp) { >> + continue; >> + } >> + /* cr-ports are synthetic and never appear in nbr->ports; a >> router >> + * with one is a distributed gateway router which is out of >> scope. */ >> + if (is_cr_port(op) || >> + lrp_needs_recompute(od, op->nbrp, &nd->ls_ports, >> + ni->nbrec_static_mac_binding_table)) { >> + goto fail; >> + } >> + lr_port_remove_router_ips(op); >> + add_op_to_northd_tracked_ports(&trk_lrps->deleted, op); >> + hmap_remove(&nd->lr_ports, &op->key_node); >> + hmap_remove(&od->ports, &op->dp_node); >> + sbrec_port_binding_delete(op->sb); >> + /* Regenerate the router's datapath-level flows (see the create >> + * path above). */ >> + hmapx_add(&nd->trk_data.trk_routers.crupdated, od); >> + lrp_deleted = true; >> + } >> + >> + /* Purge SB MAC_Bindings learned on the just-deleted LRPs. A full >> + * recompute does this in build_ports() via cleanup_mac_bindings(); >> the >> + * incremental path must do it too, otherwise MAC_Bindings >> referencing a >> + * deleted logical router port are leaked in the southbound DB. */ >> + if (lrp_deleted) { >> + cleanup_mac_bindings(ni->sbrec_mac_binding_table, >> + &nd->lr_datapaths.datapaths, &nd->lr_ports); >> + } >> + >> + return true; >> + >> +fail: >> + destroy_tracked_ovn_ports(trk_lrps); >> + return false; >> +} >> + >> /* Return true if changes are handled incrementally, false otherwise. >> * >> * Note: Changes to load balancer and load balancer groups associated >> with >> @@ -5913,7 +6279,8 @@ is_lr_static_routes_changed(const struct >> nbrec_logical_router *nbr) >> * handler - northd_handle_lb_data_changes(). >> * */ >> bool >> -northd_handle_lr_changes(const struct northd_input *ni, >> +northd_handle_lr_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, >> + const struct northd_input *ni, >> struct northd_data *nd) >> { >> const struct nbrec_logical_router *changed_lr; >> @@ -5929,9 +6296,9 @@ northd_handle_lr_changes(const struct northd_input >> *ni, >> const struct ovn_synced_logical_router *synced = node->data; >> const struct nbrec_logical_router *new_lr = synced->nb; >> >> - /* If the logical router is create with the below columns set, >> + /* If the logical router is created with the below columns set, >> * then we can't handle it in the incremental processor goto >> fail. */ >> - if (new_lr->copp || (new_lr->n_ports > 0)) { >> + if (new_lr->copp) { >> goto fail; >> } >> if (sparse_array_get(&nd->lr_datapaths.dps, synced->sdp->index)) >> { >> @@ -5950,6 +6317,12 @@ northd_handle_lr_changes(const struct northd_input >> *ni, >> od->nbr->name); >> hmapx_add(&nd->trk_data.trk_nat_lrs,od); >> hmapx_add(&nd->trk_data.trk_routers.crupdated, od); >> + >> + /* Create any logical router ports the new router already has. */ >> + if (!lr_handle_lrp_changes(ovnsb_idl_txn, new_lr, ni, nd, od, >> + &nd->trk_data.trk_lrps)) { >> + goto fail; >> + } >> } >> >> HMAPX_FOR_EACH (node, &ni->synced_lrs->updated) { >> @@ -5957,11 +6330,27 @@ northd_handle_lr_changes(const struct >> northd_input *ni, >> changed_lr = synced->nb; >> >> /* Presently only able to handle load balancer, >> - * load balancer group changes and NAT changes. */ >> + * load balancer group changes, NAT changes and (regular) logical >> + * router port creation/deletion. */ >> if (!lr_changes_can_be_handled(changed_lr)) { >> goto fail; >> } >> >> + struct ovn_datapath *lrp_od = ovn_datapath_find_( >> + &nd->lr_datapaths.datapaths, >> + &changed_lr->header_.uuid); >> + if (!lrp_od) { >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, >> 1); >> + VLOG_WARN_RL(&rl, "Internal error: a tracked updated LR " >> + "doesn't exist in lr_datapaths: "UUID_FMT, >> + UUID_ARGS(&changed_lr->header_.uuid)); >> + goto fail; >> + } >> + if (!lr_handle_lrp_changes(ovnsb_idl_txn, changed_lr, ni, nd, >> lrp_od, >> + &nd->trk_data.trk_lrps)) { >> + goto fail; >> + } >> + >> if (is_lr_nats_changed(changed_lr)) { >> struct ovn_datapath *od = ovn_datapath_find_( >> &nd->lr_datapaths.datapaths, >> @@ -6036,6 +6425,10 @@ northd_handle_lr_changes(const struct northd_input >> *ni, >> if (!hmapx_is_empty(&nd->trk_data.trk_lrs_routes)) { >> nd->trk_data.type |= NORTHD_TRACKED_LR_ROUTES; >> } >> + if (!hmapx_is_empty(&nd->trk_data.trk_lrps.created) || >> + !hmapx_is_empty(&nd->trk_data.trk_lrps.deleted)) { >> + nd->trk_data.type |= NORTHD_TRACKED_LR_PORTS; >> + } >> if (!hmapx_is_empty(&nd->trk_data.trk_routers.crupdated) || >> !hmapx_is_empty(&nd->trk_data.trk_routers.deleted)) { >> nd->trk_data.type |= NORTHD_TRACKED_ROUTERS; >> @@ -13074,29 +13467,41 @@ parsed_routes_add_static(const struct >> ovn_datapath *od, >> return pr; >> } >> >> -static void >> +/* Add the directly-connected routes for the logical router port 'op' to >> + * 'routes'. When 'trk_crupdated' is non-NULL each newly created >> parsed_route >> + * is added to it (used by the incremental routes handler). */ >> +void >> parsed_routes_add_connected(const struct ovn_datapath *od, >> const struct ovn_port *op, >> - struct hmap *routes) >> + struct hmap *routes, >> + struct hmapx *trk_crupdated) >> { >> for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { >> const struct ipv4_netaddr *addr = >> &op->lrp_networks.ipv4_addrs[i]; >> struct in6_addr prefix; >> >> in6_addr_set_mapped_ipv4(&prefix, addr->network); >> - parsed_route_add(od, NULL, &prefix, addr->plen, >> + struct parsed_route *pr = parsed_route_add( >> + od, NULL, &prefix, addr->plen, >> false, addr->addr_s, op, 0, false, false, >> false, NULL, ROUTE_SOURCE_CONNECTED, >> true, &op->nbrp->header_, NULL, routes); >> + if (trk_crupdated && pr) { >> + hmapx_add(trk_crupdated, pr); >> + } >> } >> >> for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { >> const struct ipv6_netaddr *addr = >> &op->lrp_networks.ipv6_addrs[i]; >> >> - parsed_route_add(od, NULL, &addr->network, addr->plen, false, >> + struct parsed_route *pr = parsed_route_add( >> + od, NULL, &addr->network, addr->plen, false, >> addr->addr_s, op, 0, false, false, false, >> NULL, ROUTE_SOURCE_CONNECTED, true, >> &op->nbrp->header_, NULL, routes); >> + if (trk_crupdated && pr) { >> + hmapx_add(trk_crupdated, pr); >> + } >> } >> } >> >> @@ -13114,7 +13519,7 @@ build_parsed_routes(const struct ovn_datapath >> *od, const struct hmap *lr_ports, >> >> const struct ovn_port *op; >> HMAP_FOR_EACH (op, dp_node, &od->ports) { >> - parsed_routes_add_connected(od, op, routes); >> + parsed_routes_add_connected(od, op, routes, NULL); >> } >> } >> >> @@ -15707,11 +16112,22 @@ static void >> build_route_flows_for_lrouter( >> struct ovn_datapath *od, struct lflow_table *lflows, >> const struct group_ecmp_route_data *route_data, >> - struct simap *route_tables, const struct sset *bfd_ports) >> + struct simap *route_tables, const struct sset *bfd_ports, >> + bool skip_data_route_flows) >> { >> ovs_assert(od->nbr); >> + /* The default route drop flows are owned by od->datapath_lflows. */ >> build_default_route_flows_for_lrouter(od, lflows, route_tables); >> >> + /* The per-route flows are owned by the group_ecmp_route node's >> + * lflow_ref, which is managed incrementally by >> + * lflow_group_ecmp_route_change_handler(). Skip them when >> regenerating a >> + * router's datapath flows incrementally to avoid double-adding them >> to >> + * that ref. */ >> + if (skip_data_route_flows) { >> + return; >> + } >> + >> const struct group_ecmp_datapath *datapath_node = >> group_ecmp_datapath_lookup(route_data, od); >> if (!datapath_node) { >> @@ -20327,6 +20743,14 @@ struct lswitch_flow_build_info { >> struct hmap *route_policies; >> struct simap *route_tables; >> const struct sbrec_acl_id_table *sbrec_acl_id_table; >> + >> + /* When true, build_lswitch_and_lrouter_iterate_by_lr() skips the >> logical >> + * router route flows. Those flows are owned by the group_ecmp_route >> + * node's lflow_ref, which is managed incrementally by >> + * lflow_group_ecmp_route_change_handler(); regenerating them here >> (e.g. >> + * when a router is retracked because its ports changed) would >> double-add >> + * them to that ref and corrupt its refcounts. */ >> + bool skip_route_flows; >> }; >> >> /* Helper function to combine all lflow generation which is iterated by >> @@ -20387,7 +20811,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct >> ovn_datapath *od, >> od->datapath_lflows); >> build_route_flows_for_lrouter(od, lsi->lflows, >> lsi->route_data, lsi->route_tables, >> - lsi->bfd_ports); >> + lsi->bfd_ports, lsi->skip_route_flows); >> build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, >> od->datapath_lflows); >> build_ingress_policy_flows_for_lrouter(od, lsi->lflows, >> lsi->lr_ports, >> @@ -21077,12 +21501,18 @@ lflow_handle_northd_lr_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> } >> >> struct lswitch_flow_build_info lsi = { >> + .ls_ports = lflow_input->ls_ports, >> + .lr_ports = lflow_input->lr_ports, >> .lr_datapaths = lflow_input->lr_datapaths, >> .lr_stateful_table = lflow_input->lr_stateful_table, >> + .meter_groups = lflow_input->meter_groups, >> + .bfd_ports = lflow_input->bfd_ports, >> + .features = lflow_input->features, >> .lflows = lflows, >> .route_data = lflow_input->route_data, >> .route_tables = lflow_input->route_tables, >> .route_policies = lflow_input->route_policies, >> + .skip_route_flows = true, >> .match = DS_EMPTY_INITIALIZER, >> .actions = DS_EMPTY_INITIALIZER, >> }; >> @@ -21286,6 +21716,114 @@ lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> return handled; >> } >> >> +/* Regenerate the logical flows for created and deleted logical router >> ports >> + * tracked in 'trk_lrps'. Mirrors lflow_handle_northd_port_changes() >> but for >> + * LRPs: their flows live on op->lflow_ref (per-port routing/ARP/... >> flows via >> + * build_lswitch_and_lrouter_iterate_by_lrp()) and op->stateful_lflow_ref >> + * (LB/NAT flows via build_lbnat_lflows_iterate_by_lrp()). */ >> +bool >> +lflow_handle_northd_lrp_changes(struct ovsdb_idl_txn *ovnsb_txn, >> + struct tracked_ovn_ports *trk_lrps, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows) >> +{ >> + struct hmapx_node *hmapx_node; >> + struct ovn_port *op; >> + >> + struct lswitch_flow_build_info lsi = { >> + .ls_ports = lflow_input->ls_ports, >> + .lr_ports = lflow_input->lr_ports, >> + .lr_datapaths = lflow_input->lr_datapaths, >> + .lr_stateful_table = lflow_input->lr_stateful_table, >> + .meter_groups = lflow_input->meter_groups, >> + .bfd_ports = lflow_input->bfd_ports, >> + .lflows = lflows, >> + .match = DS_EMPTY_INITIALIZER, >> + .actions = DS_EMPTY_INITIALIZER, >> + }; >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->deleted) { >> + op = hmapx_node->data; >> + ovs_assert(op->nbrp); >> + bool handled = lflow_ref_resync_flows( >> + op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + if (handled) { >> + handled = lflow_ref_resync_flows( >> + op->stateful_lflow_ref, lflows, ovnsb_txn, >> lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + } >> + if (!handled) { >> + goto out; >> + } >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->updated) { >> > > trk_lrps->updated should always be empty, should we enforce the invariant > here too? It is enforced in routes_northd_change_handler() with > ovs_assert(hmapx_is_empty(&trk_lrps->updated)) but I think we should in > this function to catch future breakage. At the very least, add a comment > I agree, actually nothing adds to trk_lrps->update, it makes sense to add ovs_assert. Regards, Lucas > + op = hmapx_node->data; >> + ovs_assert(op->nbrp); >> + lflow_ref_unlink_lflows(op->lflow_ref); >> + build_lswitch_and_lrouter_iterate_by_lrp(op, &lsi); >> + bool handled = lflow_ref_sync_lflows( >> + op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + if (handled) { >> + lflow_ref_unlink_lflows(op->stateful_lflow_ref); >> + build_lbnat_lflows_iterate_by_lrp( >> + op, lflow_input->lr_stateful_table, >> + lflow_input->meter_groups, lflow_input->bfd_ports, >> + &lsi.match, &lsi.actions, lflows); >> + handled = lflow_ref_sync_lflows( >> + op->stateful_lflow_ref, lflows, ovnsb_txn, >> lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + } >> + if (!handled) { >> + goto out; >> + } >> + } >> + >> + HMAPX_FOR_EACH (hmapx_node, &trk_lrps->created) { >> + op = hmapx_node->data; >> + ovs_assert(op->nbrp); >> + build_lswitch_and_lrouter_iterate_by_lrp(op, &lsi); >> + bool handled = lflow_ref_sync_lflows( >> + op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + if (handled) { >> + build_lbnat_lflows_iterate_by_lrp( >> + op, lflow_input->lr_stateful_table, >> + lflow_input->meter_groups, lflow_input->bfd_ports, >> + &lsi.match, &lsi.actions, lflows); >> + handled = lflow_ref_sync_lflows( >> + op->stateful_lflow_ref, lflows, ovnsb_txn, >> lflow_input->dps, >> + lflow_input->ovn_internal_version_changed, >> + lflow_input->sbrec_logical_flow_table, >> + lflow_input->sbrec_logical_dp_group_table); >> + } >> + if (!handled) { >> + goto out; >> + } >> + } >> + >> + ds_destroy(&lsi.match); >> + ds_destroy(&lsi.actions); >> + return true; >> + >> +out: >> + ds_destroy(&lsi.match); >> + ds_destroy(&lsi.actions); >> + return false; >> +} >> + >> bool >> lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, >> struct tracked_lbs *trk_lbs, >> diff --git a/northd/northd.h b/northd/northd.h >> index d27f519d6..61546bdd2 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -160,6 +160,7 @@ enum northd_tracked_data_type { >> NORTHD_TRACKED_SWITCHES = (1 << 5), >> NORTHD_TRACKED_ROUTERS = (1 << 6), >> NORTHD_TRACKED_LR_ROUTES = (1 << 7), >> + NORTHD_TRACKED_LR_PORTS = (1 << 8), >> }; >> >> /* Track what's changed in the northd engine node. >> @@ -171,6 +172,10 @@ struct northd_tracked_data { >> struct tracked_dps trk_switches; >> struct tracked_dps trk_routers; >> struct tracked_ovn_ports trk_lsps; >> + >> + /* Tracked created/updated/deleted logical router ports. >> + * hmapx node data is 'struct ovn_port *'. */ >> + struct tracked_ovn_ports trk_lrps; >> struct tracked_lbs trk_lbs; >> >> /* Tracked logical routers whose NATs have changed. >> @@ -914,6 +919,11 @@ struct parsed_route *parsed_routes_add_static( >> struct hmap *routes, struct simap *route_tables, >> struct hmap *bfd_active_connections); >> >> +void parsed_routes_add_connected(const struct ovn_datapath *od, >> + const struct ovn_port *op, >> + struct hmap *routes, >> + struct hmapx *trk_crupdated); >> + >> struct svc_monitors_map_data { >> const struct hmap *local_svc_monitors_map; >> const struct hmap *ic_learned_svc_monitors_map; >> @@ -939,7 +949,8 @@ void ovnsb_db_run(struct ovsdb_idl_txn *ovnsb_txn, >> bool northd_handle_ls_changes(struct ovsdb_idl_txn *, >> const struct northd_input *, >> struct northd_data *); >> -bool northd_handle_lr_changes(const struct northd_input *, >> +bool northd_handle_lr_changes(struct ovsdb_idl_txn *, >> + const struct northd_input *, >> struct northd_data *); >> bool northd_handle_pgs_acl_changes(const struct northd_input *ni, >> struct northd_data *nd); >> @@ -994,6 +1005,10 @@ bool lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> struct tracked_ovn_ports *, >> struct lflow_input *, >> struct lflow_table *lflows); >> +bool lflow_handle_northd_lrp_changes(struct ovsdb_idl_txn *ovnsb_txn, >> + struct tracked_ovn_ports *, >> + struct lflow_input *, >> + struct lflow_table *lflows); >> bool lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, >> struct tracked_lbs *, >> struct lflow_input *, >> @@ -1045,6 +1060,10 @@ void sync_pbs_for_northd_changed_ovn_ports( >> struct tracked_ovn_ports *, >> const struct lr_stateful_table *); >> >> +void sync_pbs_for_northd_changed_lrps( >> + struct tracked_ovn_ports *, >> + const struct lr_stateful_table *); >> + >> void sync_pbs_for_lr_stateful_changes( >> const struct ovn_datapath *od, >> const struct lr_stateful_table *lr_stateful); >> @@ -1066,6 +1085,12 @@ northd_has_lsps_in_tracked_data(struct >> northd_tracked_data *trk_nd_changes) >> return trk_nd_changes->type & NORTHD_TRACKED_PORTS; >> } >> >> +static inline bool >> +northd_has_lrps_in_tracked_data(struct northd_tracked_data >> *trk_nd_changes) >> +{ >> + return trk_nd_changes->type & NORTHD_TRACKED_LR_PORTS; >> +} >> + >> static inline bool >> northd_has_lr_nats_in_tracked_data(struct northd_tracked_data >> *trk_nd_changes) >> { >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 13df2b712..9a0ed2f01 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -12411,6 +12411,95 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE >> OVN_CLEANUP_NORTHD >> AT_CLEANUP >> >> +AT_SETUP([Logical router port incremental processing]) >> +AT_KEYWORDS([incremental processing]) >> +ovn_start >> + >> +check ovn-nbctl ls-add sw0 >> +check ovn-nbctl --wait=sb lr-add lr0 >> + >> +# Adding a regular router port to a bare router (no peer switch port >> yet) is >> +# incrementally processed. >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 >> 10.0.0.1/24 >> +check_engine_compute northd incremental >> +check_engine_compute lflow incremental >> + >> +# The router port's SB Port_Binding was created. >> +AT_CHECK([ovn-sbctl get port_binding lr0-sw0 type], [0], [dnl >> +patch >> +]) >> +# The directly-connected route flow is present on the router. >> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep -c 'ip4.dst == 10.0.0.0/24'], >> [0], >> + [1 >> +]) >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Adding a second regular router port is also incremental. >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 >> 20.0.0.1/24 >> +check_engine_compute northd incremental >> +check_engine_compute lflow incremental >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Deleting a router port is incrementally processed. >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lrp-del lr0-sw1 >> +check_engine_compute northd incremental >> +check_engine_compute lflow incremental >> +AT_CHECK([ovn-sbctl find port_binding logical_port=lr0-sw1], [0], []) >> +AT_CHECK([ovn-sbctl dump-flows lr0 | grep -c 'ip4.dst == 20.0.0.0/24'], >> [1], >> + [0 >> +]) >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +OVN_CLEANUP_NORTHD >> +AT_CLEANUP >> + >> +AT_SETUP([Logical router port incremental processing fallback cases]) >> +AT_KEYWORDS([incremental processing]) >> +ovn_start >> + >> +check ovn-sbctl chassis-add gw1 geneve 127.0.0.1 >> + >> +# Adding a distributed gateway port falls back to recompute (cr-port). >> +check ovn-nbctl --wait=sb lr-add lr0 >> +check ovn-nbctl ls-add public >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 >> 172.168.0.100/24 >> +check ovn-nbctl --wait=sb lrp-set-gateway-chassis lr0-public gw1 20 >> +check_engine_compute northd recompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Adding a router port to a dynamic-routing router falls back to >> recompute. >> +check ovn-nbctl --wait=sb lr-add lr1 \ >> + -- set Logical_Router lr1 options:dynamic-routing=true >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lrp-add lr1 lr1-sw0 00:00:00:00:ff:11 >> 10.1.0.1/24 >> +check_engine_compute northd recompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Adding a router port whose peer "router" LSP already exists falls back >> to >> +# recompute (wiring the peer from the LRP side is not supported). >> +check ovn-nbctl --wait=sb lr-add lr2 >> +check ovn-nbctl ls-add sw2 >> +check ovn-nbctl lsp-add sw2 sw2-lr2 >> +check ovn-nbctl lsp-set-type sw2-lr2 router >> +check ovn-nbctl --wait=sb lsp-set-options sw2-lr2 router-port=lr2-sw2 >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lrp-add lr2 lr2-sw2 00:00:00:00:ff:21 >> 10.2.0.1/24 >> +check_engine_compute northd recompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Modifying an existing router port falls back to recompute. >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb set logical_router_port lr2-sw2 options:foo=bar >> +check_engine_compute northd recompute >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +OVN_CLEANUP_NORTHD >> +AT_CLEANUP >> + >> OVN_FOR_EACH_NORTHD_NO_HV([ >> AT_SETUP([SB Port binding incremental processing]) >> ovn_start >> @@ -12480,13 +12569,14 @@ check ovn-nbctl --wait=sb lsp-set-options e1 >> foo=bar >> check_recompute_counter 1 1 >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> -# Test lrp >> +# Test lrp. Adding a regular router port to a bare router with no peer >> switch >> +# port is incrementally processed. >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl --wait=sb lrp-add lr0 lrp 00:00:02:01:02:03 10.0.0.1/24 >> -check_recompute_counter <http://10.0.0.1/24-check_recompute_counter> 1 1 >> +check_recompute_counter 0 0 >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> -# Set some options on 'lrp'. northd should only recompute once. >> +# Set some options on 'lrp'. A router-port modification falls back to >> recompute. >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl --wait=sb lrp-set-options lrp route_table=rtb-1 >> check_recompute_counter 1 1 >> @@ -13846,18 +13936,13 @@ check_engine_stats lflow norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> check ovn-nbctl --wait=sb lr-add lr0 >> -# Adding a logical router port should result in recompute >> +# Adding a regular logical router port to a bare router (no peer switch >> port >> +# yet) is incrementally processed. >> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> check ovn-nbctl --wait=sb lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 >> 10.0.0.1/24 >> -# for northd engine there will be both recompute and compute >> -# first it will be recompute to handle lr0-sw0 and then a compute >> -# for the SB port binding change. >> -check_engine_stats northd recompute compute >> -check_engine_stats lr_nat recompute nocompute >> -check_engine_stats lr_stateful recompute nocompute >> -check_engine_stats sync_to_sb_pb recompute nocompute >> -check_engine_stats sync_to_sb_lb recompute nocompute >> -check_engine_stats lflow recompute nocompute >> +check_engine_compute northd incremental >> +check_engine_compute lflow incremental >> +check_engine_stats sync_to_sb_pb norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> check ovn-nbctl lsp-add sw0 sw0-lr0 >> -- >> 2.43.0 >> >> >> -- >> >> >> >> >> _'Esta mensagem é direcionada apenas para os endereços constantes no >> cabeçalho inicial. Se você não está listado nos endereços constantes no >> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa >> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas >> estão >> imediatamente anuladas e proibidas'._ >> >> >> * **'Apesar do Magazine Luiza tomar >> todas as precauções razoáveis para assegurar que nenhum vírus esteja >> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade >> por >> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* >> >> >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
