On Tue, Sep 23, 2025 at 8:29 AM Lorenzo Bianconi via dev < [email protected]> wrote:
> > When a router is created or destroyed add the ability to the en-northd > > engine node to compute the ovn_datapath for the router instead of > > recalculating the whole database. > > > > Just like updating a router there are limitations including no router > > ports and nothing in the COPP table. Differently from updating a logical > > router during creation flags can be set because since there are no ports > > there is no way setting those flags will effect other objects. > > > > Reported-by: Dumitru Ceara <[email protected]> > > Reported-at: https://issues.redhat.com/browse/FDP-757 > > Signed-off-by: Jacob Tanenbaum <[email protected]> > > Acked-by: Lorenzo Bianconi <[email protected]> > > > > > ---- > > > > v4: cleared the vector entry for deleted routers > > changed the smap_get for chassis options for new routers > > > > diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c > > index 5a7b857f4..0583e34a5 100644 > > --- a/northd/en-lr-nat.c > > +++ b/northd/en-lr-nat.c > > @@ -125,6 +125,10 @@ lr_nat_northd_handler(struct engine_node *node, > void *data_) > > return EN_UNHANDLED; > > } > > > > + if (northd_has_lrouters_in_tracked_data(&northd_data->trk_data)) { > > + return EN_UNHANDLED; > > + } > > + > > if (!northd_has_lr_nats_in_tracked_data(&northd_data->trk_data)) { > > return EN_HANDLED_UNCHANGED; > > } > > diff --git a/northd/en-northd.c b/northd/en-northd.c > > index 56b25d271..4006d91c8 100644 > > --- a/northd/en-northd.c > > +++ b/northd/en-northd.c > > @@ -208,7 +208,8 @@ northd_nb_logical_router_handler(struct engine_node > *node, > > return EN_UNHANDLED; > > } > > > > - if (northd_has_lr_nats_in_tracked_data(&nd->trk_data)) { > > + if (northd_has_lr_nats_in_tracked_data(&nd->trk_data) || > > + northd_has_lrouters_in_tracked_data(&nd->trk_data)) { > > return EN_HANDLED_UPDATED; > > } > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 8b5413ef3..eaecb424b 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -4051,6 +4051,7 @@ destroy_northd_data_tracked_changes(struct > northd_data *nd) > > hmapx_clear(&trk_changes->ls_with_changed_acls); > > hmapx_clear(&trk_changes->ls_with_changed_ipam); > > destroy_tracked_dps(&trk_changes->trk_switches); > > + destroy_tracked_dps(&trk_changes->trk_routers); > > trk_changes->type = NORTHD_TRACKED_NONE; > > } > > > > @@ -4059,6 +4060,8 @@ init_northd_tracked_data(struct northd_data *nd) > > { > > struct northd_tracked_data *trk_data = &nd->trk_data; > > trk_data->type = NORTHD_TRACKED_NONE; > > + hmapx_init(&trk_data->trk_routers.crupdated); > > + hmapx_init(&trk_data->trk_routers.deleted); > > hmapx_init(&trk_data->trk_switches.crupdated); > > hmapx_init(&trk_data->trk_switches.deleted); > > hmapx_init(&trk_data->trk_lsps.created); > > @@ -4089,6 +4092,8 @@ destroy_northd_tracked_data(struct northd_data *nd) > > hmapx_destroy(&trk_data->ls_with_changed_lbs); > > hmapx_destroy(&trk_data->ls_with_changed_acls); > > hmapx_destroy(&trk_data->ls_with_changed_ipam); > > + hmapx_destroy(&trk_data->trk_routers.crupdated); > > + hmapx_destroy(&trk_data->trk_routers.deleted); > > } > > > > /* Check if a changed LSP can be handled incrementally within the I-P > engine > > @@ -4931,13 +4936,37 @@ northd_handle_lr_changes(const struct > northd_input *ni, > > { > > const struct nbrec_logical_router *changed_lr; > > > > - if (!hmapx_is_empty(&ni->synced_lrs->new) || > > - !hmapx_is_empty(&ni->synced_lrs->deleted) || > > - hmapx_is_empty(&ni->synced_lrs->updated)) { > > + if (hmapx_is_empty(&ni->synced_lrs->new) && > > + hmapx_is_empty(&ni->synced_lrs->updated) && > > + hmapx_is_empty(&ni->synced_lrs->deleted)) { > > goto fail; > > } > > > > struct hmapx_node *node; > > + HMAPX_FOR_EACH (node, &ni->synced_lrs->new) { > > + 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, > > + * then we can't handle it in the incremental processor goto > fail. */ > > + if (new_lr->copp || (new_lr->n_ports > 0)) { > > + goto fail; > > + } > > + struct ovn_datapath *od = ovn_datapath_create( > > + &nd->lr_datapaths.datapaths, &new_lr->header_.uuid, > > + NULL, new_lr, synced->sdp); > > + ods_assign_array_index(&nd->lr_datapaths, od); > > + > > + od->is_gw_router = !!smap_get(&od->nbr->options, "chassis"); > > + od->dynamic_routing = smap_get_bool(&od->nbr->options, > > + "dynamic-routing", false); > > + od->dynamic_routing_redistribute = > > + parse_dynamic_routing_redistribute(&od->nbr->options, > DRRM_NONE, > > + od->nbr->name); > > + hmapx_add(&nd->trk_data.trk_nat_lrs,od); > > + hmapx_add(&nd->trk_data.trk_routers.crupdated, od); > > + } > > + > > HMAPX_FOR_EACH (node, &ni->synced_lrs->updated) { > > const struct ovn_synced_logical_router *synced = node->data; > > changed_lr = synced->nb; > > @@ -4965,9 +4994,50 @@ northd_handle_lr_changes(const struct > northd_input *ni, > > } > > } > > > > + HMAPX_FOR_EACH (node, &ni->synced_lrs->deleted) { > > + const struct ovn_synced_logical_router *synced = node->data; > > + const struct nbrec_logical_router *deleted_lr = synced->nb; > > + > > + struct ovn_datapath *od = ovn_datapath_find_( > > + &nd->lr_datapaths.datapaths, > > + &deleted_lr->header_.uuid); > > + if (!od) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 1); > > + VLOG_WARN_RL(&rl, "Internal error: a tracked deleted LR > doesn't " > > + "exist in lr_datapaths: "UUID_FMT, > > + UUID_ARGS(&deleted_lr->header_.uuid)); > > + goto fail; > > + } > > + > > + if (deleted_lr->copp || > > + deleted_lr->n_ports > 0 || > > + deleted_lr->n_policies > 0 || > > + deleted_lr->n_static_routes > 0) { > > + goto fail; > > + } > > + /* Since there are no ports the lr_group should be empty. If > > + * a logical router is deleted before the db gets > > + * recalculated nothing will create the lr_group. */ > > + if (od->lr_group) { > > + free(od->lr_group->router_dps); > > + free(od->lr_group); > > + } > > + > > + hmap_remove(&nd->lr_datapaths.datapaths, &od->key_node); > > + vector_get(&nd->lr_datapaths.dps, > > + od->index, struct ovn_datapath *) = NULL; > > + dynamic_bitmap_set0(&nd->lr_datapaths.dps_index_map, od->index); > > + > > + hmapx_add(&nd->trk_data.trk_routers.deleted, od); > > + } > > + > > if (!hmapx_is_empty(&nd->trk_data.trk_nat_lrs)) { > > nd->trk_data.type |= NORTHD_TRACKED_LR_NATS; > > } > > + 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; > > + } > > > > return true; > > fail: > > diff --git a/northd/northd.h b/northd/northd.h > > index ca35eb91e..6836831d7 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -155,6 +155,7 @@ enum northd_tracked_data_type { > > NORTHD_TRACKED_LS_LBS = (1 << 3), > > NORTHD_TRACKED_LS_ACLS = (1 << 4), > > NORTHD_TRACKED_SWITCHES = (1 << 5), > > + NORTHD_TRACKED_ROUTERS = (1 << 6), > > }; > > > > /* Track what's changed in the northd engine node. > > @@ -164,6 +165,7 @@ struct northd_tracked_data { > > /* Indicates the type of data tracked. One or all of > NORTHD_TRACKED_*. */ > > enum northd_tracked_data_type type; > > struct tracked_dps trk_switches; > > + struct tracked_dps trk_routers; > > struct tracked_ovn_ports trk_lsps; > > struct tracked_lbs trk_lbs; > > > > @@ -1014,6 +1016,13 @@ northd_has_lswitches_in_tracked_data( > > return trk_nd_changes->type & NORTHD_TRACKED_SWITCHES; > > } > > > > +static inline bool > > +northd_has_lrouters_in_tracked_data( > > + struct northd_tracked_data *trk_nd_changes) > > +{ > > + return trk_nd_changes->type & NORTHD_TRACKED_ROUTERS; > > +} > > + > > /* Returns 'true' if the IPv4 'addr' is on the same subnet with one of > the > > * IPs configured on the router port. > > */ > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index c9e998129..2d62a2399 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -12520,14 +12520,26 @@ check ovn-nbctl --wait=sb lsp-add sw0 sw0p1 -- > lsp-set-addresses sw0p1 "00:00:20 > > > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > check ovn-nbctl --wait=sb lr-add lr0 > > -check_engine_stats northd recompute nocompute > > +check_engine_stats northd norecompute 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 sync_to_sb_lb norecompute compute > > +check_engine_stats lflow recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > + > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > +check ovn-nbctl --wait=sb lr-del lr0 > > + > > +check_engine_stats northd norecompute 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 norecompute compute > > check_engine_stats lflow recompute nocompute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > > > +check ovn-nbctl --wait=sb lr-add lr0 > > # Adding a logical router port should result in recompute > > 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 > > -- > > 2.51.0 > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Looks good to me: Acked-by: Ales Musil <[email protected]> Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
