On Tue, Aug 8, 2023 at 1:15 PM Han Zhou <[email protected]> wrote:
>
> On Thu, Aug 3, 2023 at 12:29 PM <[email protected]> wrote:
> >
> > From: Numan Siddique <[email protected]>
> >
> > When a logical switch has only router ports and if a new VIF port is
> > added, both northd engine and lflow engine handle this change
> > incrementally, but it misses out on adding a few logical flows
> > where we have checks like :
> > if (od->n_router_ports != od->nbs->n_ports) {
> > ds_put_format(&actions, "clone {outport = %s; output; }; "
> > "outport = \""MC_FLOOD_L2"\"; output;",
> > patch_op->json_key);
> > ....
> > } else {
> > ds_put_format(&actions, "outport = %s; output;",
> patch_op->json_key);
> > }
> >
> > The same issue is seen when a VIF port is deleted and after which the
> > logical switch has only router ports.
> >
> > This patch fixes this issue by falling back to full recompute of northd
> > engine node. It is possible to handle these changes incrementally
> > in northd engine node but fall back to full recompute in lflow engine
> > node. But this patch goes for a simpler fix. This can be optimized
> > later if required.
> >
>
> Good catch! This reminds me about a ddlog performance problem for this
> scenario:
> https://mail.openvswitch.org/pipermail/ovs-dev/2021-September/388184.html
> +1 for the quick fix. Thanks a lot for identifying such corner cases.
> I'd like to work on I-P for such a scenario because it may be quite common
> in environments like ovn-k8s when the first LSP is added to a LS or the
> last LSP is deleted from it.
> Please also find some minor comments below.
>
> With those addressed:
> Acked-by: Han Zhou <[email protected]>
Thanks. I addressed your comments and applied the patch to the main branch.
I added a separate test case as you suggested.
Numan
>
> > Fixes: b337750e45be ("northd: Incremental processing of VIF changes in
> 'northd' node.")
> > CC: Han Zhou <[email protected]>
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
> > northd/northd.c | 10 ++++++++++
> > tests/ovn-northd.at | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index b9605862e..462fa83ca 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -5120,6 +5120,11 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > op->visited = false;
> > }
> >
> > + bool only_rports = (od->n_router_ports ==
> hmap_count(&od->ports));
> > + if (only_rports) {
> > + goto fail;
> > + }
>
> Would be better to also check if the n_router_ports == 0 then no need to
> fallback to recompute.
> Would be better to add a comment here to explain why we are doing this
> check.
>
> > +
> > /* Compare the individual ports in the old and new Logical
> Switches */
> > for (size_t j = 0; j < changed_ls->n_ports; ++j) {
> > struct nbrec_logical_switch_port *new_nbsp =
> changed_ls->ports[j];
> > @@ -5201,6 +5206,11 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > }
> > }
> >
> > + only_rports = (od->n_router_ports == hmap_count(&od->ports));
> > + if (only_rports) {
> > + goto fail_clean_deleted;
> > + }
>
> Same as above. (comments may be different, because we are checking if the
> last LSP was just got deleted)
>
> > +
> > if (!ovs_list_is_empty(&ls_change->added_ports) ||
> > !ovs_list_is_empty(&ls_change->updated_ports) ||
> > !ovs_list_is_empty(&ls_change->deleted_ports)) {
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 3e06f14c9..912aa5431 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9589,6 +9589,34 @@ check_recompute_counter 0 0
> >
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
>
> nit: Better to add a comment to describe the purpose of the tests below
> because they are quite different from the above basic tests. (or even
> better if just move to a separate test case)
>
> > +check ovn-nbctl --wait=hv ls-del ls0
> > +check ovn-nbctl ls-add ls0
> > +check ovn-nbctl --wait=sb lr-add lr0
> > +ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:ff:01 192.168.0.1/24
> > +ovn-nbctl lsp-add ls0 ls0-lr0
> > +ovn-nbctl lsp-set-type ls0-lr0 router
> > +ovn-nbctl lsp-set-addresses ls0-lr0 router
> > +check ovn-nbctl --wait=sb lsp-set-options ls0-lr0 router-port=lr0-ls0
> > +
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +ovn-nbctl lb-add lb0 192.168.0.10:80 10.0.0.10:8080
> > +check ovn-nbctl --wait=sb ls-lb-add ls0 lb0
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +# Add a lsp. northd and lflow engine should recompute since this is
> > +# the first lsp added after the router ports.
> > +check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1
> "aa:aa:aa:00:00:01 192.168.0.11"
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> nit: after asserting that the recompute is performed (the coutner 1 1),
> there is no need to CHECK_NO_CHANGE_AFTER_RECOMPUTE.
>
> > +
> > +# Delete the lsp. northd and lflow engine should recompute.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl lsp-del lsp0-1
> > +check_recompute_counter 1 1
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> > --
> > 2.40.1
> >
> _______________________________________________
> 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
