On 7/25/24 09:20, 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 | 28 +++++++++++++++++
> tests/ovn.at | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 112 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 5b50ea191..0db0ba006 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4525,6 +4525,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> && (od->n_router_ports == hmap_count(&od->ports)));
>
> struct ovn_port *op;
> + struct ovs_list exist_virtual_ports;
> + ovs_list_init(&exist_virtual_ports);
> HMAP_FOR_EACH (op, dp_node, &od->ports) {
> op->visited = false;
> }
> @@ -4588,6 +4590,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> delete_fdb_entry(ni->sbrec_fdb_by_dp_and_port,
> od->tunnel_key,
> old_tunnel_key);
> }
> + } else if (!strcmp(op->nbsp->type, "virtual")) {
> + ovs_list_push_back(&exist_virtual_ports, &op->list);
> }
> op->visited = true;
> }
> @@ -4628,6 +4632,30 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> }
> }
>
> + /*
> + * Update old virtual ports that have new created 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 (!ovs_list_is_empty(&exist_virtual_ports)) {
There is no real need for this check, the LIST_FOR_EACH will do practically
the same check and end early.
> + struct hmapx_node *hmapx_node;
> + struct ovn_port *new_op;
> + LIST_FOR_EACH (op, list, &exist_virtual_ports) {
> + ovs_list_remove(&op->list);
May use LIST_FOR_EACH_POP.
> + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) {
> + new_op = hmapx_node->data;
> + if (strstr(smap_get_def(&op->nbsp->options,
> + "virtual-parents", ""), new_op->nbsp->name)) {
We can save some time by looking up the "virtual-parents" once before
the HMAPX loop. If there is no such key (smap_get returns NULL), then
we can continue to the next virtual port, if it does, then we can run
the loop and use the value for comparison without looking it up in the
smap each time.
A faster strategy maybe to create an smap of names of all the created
ports. Then, for each virtual parent we could look it up in that smap
instead of iterating over all the created ports. This will require
iteration over virtual parents with strtok_r, but it should be cheap.
This will also save us from a potential problem where name of one port
is a substring of a name of another port, where strstr will not be
reliable.
> + add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
> + /* Can stop the loop cause we have at lest one new
> parent
> + * created, no need to check the rest.
> + */
> + break;
> + }
> + }
> + }
> + }
> +
> return true;
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
