On 8/4/25 10:22 PM, Jacob Tanenbaum wrote:
> This patch handles the logical router creation incrementally in the
> northd engine node. This patch covers the depended node lr-nat but
> further dependant nodes will be handled in upcoming patches.
>
> This is built on Mark Michelson's patch series located here
>
> https://patchwork.ozlabs.org/project/ovn/list/?series=465828
>
> this is required because the datapath refactor was required in order to
> accomplish incremental processing of the logical routers
>
> also built with the first two patches of this series by Lorenzo Bianconi
> located https://patchwork.ozlabs.org/project/ovn/list/?series=466579.
> He brought in some changes required to dynamically allocate the sizes of
> the datapaths array.
>
> Reported-by: Dumitru Ceara <dce...@redhat.com>
> Reported-at: https://issues.redhat.com/browse/FDP-757
> Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com>
>
Hi Jacob,
I know you're working on rebasing this but I wanted to share a comment
that I think needs to be addressed as a minimal prerequisite for getting
this series merged.
> @@ -5097,13 +5099,37 @@ northd_handle_lr_changes(const struct northd_input
> *ni,
> struct northd_data *nd)
> {
> const struct nbrec_logical_router *changed_lr;
> + const struct nbrec_logical_router *new_lr;
>
> - if (!hmapx_is_empty(&ni->synced_lrs->new) ||
> - !hmapx_is_empty(&ni->synced_lrs->deleted)) {
> + if (!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;
> + new_lr = synced->nb;
> +
> + if (new_lr->n_ports > 0
> + || new_lr->n_policies > 0
> + || new_lr->n_static_routes > 0
> + || !lrouter_is_enabled(new_lr)
> + || !smap_is_empty(&new_lr->options)
> + || !smap_is_empty(&new_lr->external_ids)) {
> + goto fail;
> + }
This is extremely restrictive, it essentially translates to CMS
inserting a transaction that's the exact equivalent of "ovn-nbctl lr-add
lr".
In practice CMS almost never do that. The transaction that creates the
router normally also adds ports to it (otherwise the router is useless)
and almost always adds some routes and/or options. Probably also
external IDs.
So, while this would maybe pass on simple tests like "lr-add lr" in
practice it won't provide any benefit to CMS.
I think the next revision should try to handle more common cases than v2
does.
Which also makes me think that, unfortunately, this might have to slip
to the next development cycle.
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
