On 8/15/25 7:55 AM, Dumitru Ceara wrote:
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 <[email protected]>
Reported-at: https://issues.redhat.com/browse/FDP-757
Signed-off-by: Jacob Tanenbaum <[email protected]>
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.
These same restrictions exist in current logical router I-P. Logical
routers can only be handled incrementally if the load balancer, load
balancer group, or nat values change. Any other column changing results
in a recompute. See lr_changes_can_be_handled() in northd/northd.c and
you can see the basis for this if statement. Although, I actually don't
know why existing LR I-P is so restrictive in the first place.
There may be some leeway we can give for new routers that we can't for
existing routers, specifically because we don't have any existing state
for the router or its settings.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
