diff mbox series

[ovs-dev,v2] northd: Update virtual port when updating parent ports.

Message ID 20240725072049.3884358-1-mheib@redhat.com
State New
Headers show
Series [ovs-dev,v2] northd: Update virtual port when updating parent ports. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Mohammad Heib July 25, 2024, 7:20 a.m. UTC 
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 <mheib@redhat.com>
---
 northd/northd.c | 28 +++++++++++++++++
 tests/ovn.at    | 84 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

Comments

Ilya Maximets Aug. 28, 2024, 12:29 a.m. UTC | #1 
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 <mheib@redhat.com>
> ---
>  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.
diff mbox series

Patch

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)) {
+         struct hmapx_node *hmapx_node;
+         struct ovn_port *new_op;
+         LIST_FOR_EACH (op, list, &exist_virtual_ports) {
+             ovs_list_remove(&op->list);
+             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)) {
+                     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;
 
 fail:
diff --git a/tests/ovn.at b/tests/ovn.at
index 2ced7c0b2..83fba4f11 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22439,6 +22439,90 @@  OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([virtual port - parents port re-create])
+AT_KEYWORDS([virtual ports])
+ovn_start
+send_garp() {
+    local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
+    local request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+    as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request
+}
+
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovn-appctl vlog/set dbg
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+ovs-appctl -t ovn-controller vlog/set dbg
+
+ovn-nbctl ls-add sw0
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 10.0.0.10 1000::3"
+
+check ovn-nbctl lsp-add sw0 sw0-vir
+check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
+check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
+check ovn-nbctl lsp-set-type sw0-vir virtual
+check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
+check ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1
+
+wait_for_ports_up
+ovn-nbctl --wait=hv sync
+hv1_ch_uuid=`ovn-sbctl --bare --columns _uuid find chassis name="hv1"`
+
+# From sw0-p1 send GARP for 10.0.0.10. hv1 should claim sw0-vir
+# and sw0-p1 should be its virtual_parent.
+eth_src=505400000003
+eth_dst=ffffffffffff
+spa=$(ip_to_hex 10 0 0 10)
+tpa=$(ip_to_hex 10 0 0 10)
+send_garp 1 1 $eth_src $eth_dst $spa $tpa
+
+OVS_WAIT_UNTIL([test 1 = `cat hv1/ovn-controller.log | grep "pinctrl received  packet-in" | \
+grep opcode=BIND_VPORT | grep OF_Table_ID=$(ovn-debug lflow-stage-to-oftable ls_in_arp_rsp) | wc -l`])
+wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid
+check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1
+wait_for_ports_up sw0-vir
+
+# Delete parent port
+check ovn-nbctl lsp-del sw0-p1
+ovn-nbctl --wait=hv sync
+
+# Re-add parent port
+check ovn-nbctl lsp-add sw0 sw0-p1
+check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 10.0.0.10 1000::3"
+
+wait_for_ports_up
+# From sw0-p1 send GARP for 10.0.0.10. hv1 should claim sw0-vir
+# and sw0-p1 should be its virtual_parent.
+eth_src=505400000003
+eth_dst=ffffffffffff
+spa=$(ip_to_hex 10 0 0 10)
+tpa=$(ip_to_hex 10 0 0 10)
+send_garp 1 1 $eth_src $eth_dst $spa $tpa
+
+OVS_WAIT_UNTIL([test 2 = `cat hv1/ovn-controller.log | grep "pinctrl received  packet-in" | \
+grep opcode=BIND_VPORT | grep OF_Table_ID=$(ovn-debug lflow-stage-to-oftable ls_in_arp_rsp) | wc -l`])
+
+wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid
+check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1
+wait_for_ports_up sw0-vir
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
 OVN_FOR_EACH_NORTHD([
 # Run ovn-nbctl in daemon mode, change to a backup database and verify that
 # an insert operation is not allowed.