On 7/24/24 11:56, Mohammad Heib wrote:
> Northd will not update/add flows for a virtual port if their
> parent ports were created/recreated after creating this virtual port.
>
> This change will fix the above issue by triggering an update for a
> virtual port if their parent port has been created.
>
> Rreported-at: https://issues.redhat.com/browse/FDP-710
> Signed-off-by: Mohammad Heib <[email protected]>
> ---
> northd/northd.c | 19 ++++++++++-
> tests/ovn.at | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 102 insertions(+), 1 deletion(-)
>
Hi, Mohammad. Thanks for the patch!
See some comments inline.
> diff --git a/northd/northd.c b/northd/northd.c
> index 5b50ea191..39c19b07f 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4524,7 +4524,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> bool ls_had_only_router_ports = (od->n_router_ports
> && (od->n_router_ports == hmap_count(&od->ports)));
>
> - struct ovn_port *op;
> + struct ovn_port *op, *op_v;
> HMAP_FOR_EACH (op, dp_node, &od->ports) {
> op->visited = false;
> }
> @@ -4547,6 +4547,23 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> goto fail;
> }
> add_op_to_northd_tracked_ports(&trk_lsps->created, op);
> +
> + /*
> + * Update old virtual ports that had this VIF as parent port
> + * this code handles cases where the virtual port was created
> + * before the parent port or when the parent port was recreated.
> + */
> + if (!strcmp(new_nbsp->type, "")) {
> + for (size_t i = 0; i < changed_ls->n_ports; i++) {
This seems inefficient and may potentially tank performance at scale.
There should be a way to find all these ports without introducing
quadratic complexity.
On option might be to iterate once, if trk_lsps->created is not empty
after the loop, find all the parent ports and check if they are in
trk_lsps->created.
Not sure if that's the most efficient way, but seems better than
quadratic. What do you think?
> + op_v = ovn_port_find_in_datapath(od,
> changed_ls->ports[i]);
> + if (op_v && !strcmp(op_v->nbsp->type, "virtual") &&
> + strstr(smap_get_def(&op_v->nbsp->options,
> + "virtual-parents", ""), new_nbsp->name)) {
> + add_op_to_northd_tracked_ports(&trk_lsps->updated,
> + op_v);
Is there a chance that these ports end up in both updated and created?
Can this cause issues later?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
