On 8/15/25 5:02 PM, Mark Michelson wrote: > 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 <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. >
Hi Mark, > 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. > Sure but the concern I have with the approach in this patch is that in practice we don't gain any performance (while adding quite some code for it). I tried to summarize here, it's true, from ovn-kubernetes perspective, the minimum set of columns we should be able to incrementally process in order to achieve some performance gain in actual deployments: https://issues.redhat.com/browse/FDP-757?focusedId=27801378&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-27801378 > 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. Yes, it seems like the way forward indeed, but I don't think we can realistically get this done before branching for 25.09. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
