> On Sep 17, Jacob Tanenbaum via dev 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.
>
> Hi Jacob,
>
> I think the patch is almost fine, just some comments inline.
>
> Regards,
> Lorenzo
>
> >
> > Reported-by: Dumitru Ceara <[email protected]>
> > Reported-at: https://issues.redhat.com/browse/FDP-757
> > Signed-off-by: Jacob Tanenbaum <[email protected]>
> >
> > 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..0d5fc7b5f 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);
>
> I think here you need to destroy the 'deleted' routers otherwise it is a leak,
> agree? I think you just nee to run destroy_tracked_deleted_dps(), something
> like:
>
> destroy_tracked_deleted_dps(&trk_data->trk_routers);
> hmapx_destroy(&trk_data->trk_routers.deleted);
actually I was wrong here, we do not need to run destroy_tracked_deleted_dps()
since we always run node->clear_tracked_data(node->data) before
node->cleanup(node->data) in engine_cleanup(). Please ignore this comment.
Regards,
Lorenzo
>
> > }
> >
> > /* Check if a changed LSP can be handled incrementally within the I-P
> > engine
> > @@ -4931,13 +4936,39 @@ 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);
> > +
> > + if (smap_get(&od->nbr->options, "chassis")) {
> > + od->is_gw_router = true;
> > + }
>
> nit: I would prefer
>
> 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 +4996,49 @@ 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) {
> > +
>
> nit: remove new-line here.
>
> > + 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);
>
> I guess here you are missing to 'clear' nd->lr_datapaths.dps vector:
>
> 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
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
