diff mbox series

[ovs-dev] controller: Delete flows on port delete/add.

Message ID 20241015135305.2140051-1-xsimonar@redhat.com
State Accepted
Headers show
Series [ovs-dev] controller: Delete flows on port delete/add. | 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

Xavier Simonart Oct. 15, 2024, 1:53 p.m. UTC 
When a logical_port is deleted and added back, in two sb transactions
being handled within one ovn-controller loop, some flows belonging to
the deleted pb (such as flows in OFTABLE_CHK_IN_PORT_SEC) were not
properly deleted.

Reported-at: https://issues.redhat.com/browse/FDP-873
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 controller/lflow.c      |  3 +++
 controller/local_data.c | 26 ++++++++++++++++++++++++--
 tests/ovn.at            | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 2 deletions(-)

Comments

Ales Musil Nov. 7, 2024, 8:19 a.m. UTC | #1 
On Tue, Oct 15, 2024 at 3:53 PM Xavier Simonart <xsimonar@redhat.com> wrote:

> When a logical_port is deleted and added back, in two sb transactions
> being handled within one ovn-controller loop, some flows belonging to
> the deleted pb (such as flows in OFTABLE_CHK_IN_PORT_SEC) were not
> properly deleted.
>
> Reported-at: https://issues.redhat.com/browse/FDP-873
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> ---
>  controller/lflow.c      |  3 +++
>  controller/local_data.c | 26 ++++++++++++++++++++++++--
>  tests/ovn.at            | 40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+), 2 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 13c3a0d73..987de3f06 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -2287,6 +2287,9 @@ lflow_handle_flows_for_lport(const struct
> sbrec_port_binding *pb,
>       * port binding'uuid', then this function should handle it properly.
>       */
>      ofctrl_remove_flows(l_ctx_out->flow_table, &pb->header_.uuid);
> +    if (sbrec_port_binding_is_deleted(pb)) {
> +        return true;
> +    }
>
>      if (pb->n_port_security && shash_find(l_ctx_in->binding_lports,
>                                            pb->logical_port)) {
> diff --git a/controller/local_data.c b/controller/local_data.c
> index f889fb76b..9ee5d9171 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -359,15 +359,37 @@ tracked_datapath_lport_add(const struct
> sbrec_port_binding *pb,
>      }
>
>      /* Check if the lport is already present or not.
> -     * If it is already present, then just update the 'pb' field. */
> +     * If it is already present, then check whether it is the same pb.
> +     * We might have two different pb with the same logical_port if it was
> +     * deleted and added back within the same loop.
> +     * If the same pb was already present, just update the 'pb' field.
> +     * Otherwise, add the second pb */
>      struct tracked_lport *lport =
>          shash_find_data(&tracked_dp->lports, pb->logical_port);
>
>      if (!lport) {
>          lport = xmalloc(sizeof *lport);
>          shash_add(&tracked_dp->lports, pb->logical_port, lport);
> +    } else if (pb != lport->pb) {
> +        bool found = false;
> +        /* There is at least another pb with the same logical_port.
> +         * However, our pb might already be shash_added (e.g. pb1
> deleted, pb2
> +         * added, pb2 deleted). This is not really optimal, but this loop
> +         * only runs in a very uncommon race condition (same logical port
> +         * deleted and added within same loop */
> +        struct shash_node *node;
> +        SHASH_FOR_EACH (node, &tracked_dp->lports) {
> +            lport = (struct tracked_lport *) node->data;
> +            if (lport->pb == pb) {
> +                found = true;
> +                break;
> +            }
> +        }
> +        if (!found) {
> +            lport = xmalloc(sizeof *lport);
> +            shash_add(&tracked_dp->lports, pb->logical_port, lport);
> +        }
>      }
> -
>

nit: Unrelated


>      lport->pb = pb;
>      lport->tracked_type = tracked_type;
>  }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d7f01169c..4835612ce 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -39233,3 +39233,43 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Port Deleted and added back])
> +ovn_start
> +
> +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
> +check 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
> +
> +check 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 2001::3"
> +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 2001::3"
> +
> +OVN_POPULATE_ARP
> +wait_for_ports_up
> +check ovn-nbctl --wait=hv sync
> +
> +# Delete sw0-p1
> +sleep_controller hv1
> +check ovn-nbctl --wait=sb lsp-del sw0-p1
> +
> +# Add back sw0-p1 but without any address set.
> +check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1
> +wake_up_controller hv1
> +
> +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Other than that it looks good to me.

Acked-by: Ales Musil <amusil@redhat.com>

Thanks,
Ales
Numan Siddique Nov. 14, 2024, 10:54 p.m. UTC | #2 
On Thu, Nov 7, 2024 at 3:20 AM Ales Musil <amusil@redhat.com> wrote:
>
> On Tue, Oct 15, 2024 at 3:53 PM Xavier Simonart <xsimonar@redhat.com> wrote:
>
> > When a logical_port is deleted and added back, in two sb transactions
> > being handled within one ovn-controller loop, some flows belonging to
> > the deleted pb (such as flows in OFTABLE_CHK_IN_PORT_SEC) were not
> > properly deleted.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-873
> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> > ---
> >  controller/lflow.c      |  3 +++
> >  controller/local_data.c | 26 ++++++++++++++++++++++++--
> >  tests/ovn.at            | 40 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 13c3a0d73..987de3f06 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -2287,6 +2287,9 @@ lflow_handle_flows_for_lport(const struct
> > sbrec_port_binding *pb,
> >       * port binding'uuid', then this function should handle it properly.
> >       */
> >      ofctrl_remove_flows(l_ctx_out->flow_table, &pb->header_.uuid);
> > +    if (sbrec_port_binding_is_deleted(pb)) {

It doesn't seem right to check if 'pb' is deleted or not in this handler.

So I did the below changes and applied this patch to main.


----
diff --git a/controller/lflow.c b/controller/lflow.c
index 764200f357..6c49484f14 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -2269,7 +2269,8 @@ lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *dp,
 bool
 lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
                              struct lflow_ctx_in *l_ctx_in,
-                             struct lflow_ctx_out *l_ctx_out)
+                             struct lflow_ctx_out *l_ctx_out,
+                             bool deleted)
 {
     bool changed;

@@ -2290,7 +2291,7 @@ lflow_handle_flows_for_lport(const struct
sbrec_port_binding *pb,
      * port binding'uuid', then this function should handle it properly.
      */
     ofctrl_remove_flows(l_ctx_out->flow_table, &pb->header_.uuid);
-    if (sbrec_port_binding_is_deleted(pb)) {
+    if (deleted) {
         return true;
     }

diff --git a/controller/lflow.h b/controller/lflow.h
index 58a12ee0fe..206328f9ec 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -190,7 +190,8 @@ bool lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *,
                                   struct lflow_ctx_out *);
 bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
                                   struct lflow_ctx_in *,
-                                  struct lflow_ctx_out *);
+                                  struct lflow_ctx_out *,
+                                  bool deleted);
 bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
                                     struct lflow_ctx_out *);
 bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 41db47fac8..187f600b1f 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4271,8 +4271,9 @@ lflow_output_runtime_data_handler(struct
engine_node *node,
         struct shash_node *shash_node;
         SHASH_FOR_EACH (shash_node, &tdp->lports) {
             struct tracked_lport *lport = shash_node->data;
-            if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
-                                                &l_ctx_out)) {
+            if (!lflow_handle_flows_for_lport(
+                    lport->pb, &l_ctx_in, &l_ctx_out,
+                    lport->tracked_type == TRACKED_RESOURCE_REMOVED)) {
                 return false;
             }
         }

----------------------


Thanks
Numan


> > +        return true;
> > +    }
> >
> >      if (pb->n_port_security && shash_find(l_ctx_in->binding_lports,
> >                                            pb->logical_port)) {
> > diff --git a/controller/local_data.c b/controller/local_data.c
> > index f889fb76b..9ee5d9171 100644
> > --- a/controller/local_data.c
> > +++ b/controller/local_data.c
> > @@ -359,15 +359,37 @@ tracked_datapath_lport_add(const struct
> > sbrec_port_binding *pb,
> >      }
> >
> >      /* Check if the lport is already present or not.
> > -     * If it is already present, then just update the 'pb' field. */
> > +     * If it is already present, then check whether it is the same pb.
> > +     * We might have two different pb with the same logical_port if it was
> > +     * deleted and added back within the same loop.
> > +     * If the same pb was already present, just update the 'pb' field.
> > +     * Otherwise, add the second pb */
> >      struct tracked_lport *lport =
> >          shash_find_data(&tracked_dp->lports, pb->logical_port);
> >
> >      if (!lport) {
> >          lport = xmalloc(sizeof *lport);
> >          shash_add(&tracked_dp->lports, pb->logical_port, lport);
> > +    } else if (pb != lport->pb) {
> > +        bool found = false;
> > +        /* There is at least another pb with the same logical_port.
> > +         * However, our pb might already be shash_added (e.g. pb1
> > deleted, pb2
> > +         * added, pb2 deleted). This is not really optimal, but this loop
> > +         * only runs in a very uncommon race condition (same logical port
> > +         * deleted and added within same loop */
> > +        struct shash_node *node;
> > +        SHASH_FOR_EACH (node, &tracked_dp->lports) {
> > +            lport = (struct tracked_lport *) node->data;
> > +            if (lport->pb == pb) {
> > +                found = true;
> > +                break;
> > +            }
> > +        }
> > +        if (!found) {
> > +            lport = xmalloc(sizeof *lport);
> > +            shash_add(&tracked_dp->lports, pb->logical_port, lport);
> > +        }
> >      }
> > -
> >
>
> nit: Unrelated
>
>
> >      lport->pb = pb;
> >      lport->tracked_type = tracked_type;
> >  }
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index d7f01169c..4835612ce 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -39233,3 +39233,43 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> >  OVN_CLEANUP([hv1])
> >  AT_CLEANUP
> >  ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([Port Deleted and added back])
> > +ovn_start
> > +
> > +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
> > +check 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
> > +
> > +check 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 2001::3"
> > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 2001::3"
> > +
> > +OVN_POPULATE_ARP
> > +wait_for_ports_up
> > +check ovn-nbctl --wait=hv sync
> > +
> > +# Delete sw0-p1
> > +sleep_controller hv1
> > +check ovn-nbctl --wait=sb lsp-del sw0-p1
> > +
> > +# Add back sw0-p1 but without any address set.
> > +check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1
> > +wake_up_controller hv1
> > +
> > +CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
> > --
> > 2.31.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Other than that it looks good to me.
>
> Acked-by: Ales Musil <amusil@redhat.com>
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amusil@redhat.com
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/controller/lflow.c b/controller/lflow.c
index 13c3a0d73..987de3f06 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -2287,6 +2287,9 @@  lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
      * port binding'uuid', then this function should handle it properly.
      */
     ofctrl_remove_flows(l_ctx_out->flow_table, &pb->header_.uuid);
+    if (sbrec_port_binding_is_deleted(pb)) {
+        return true;
+    }
 
     if (pb->n_port_security && shash_find(l_ctx_in->binding_lports,
                                           pb->logical_port)) {
diff --git a/controller/local_data.c b/controller/local_data.c
index f889fb76b..9ee5d9171 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -359,15 +359,37 @@  tracked_datapath_lport_add(const struct sbrec_port_binding *pb,
     }
 
     /* Check if the lport is already present or not.
-     * If it is already present, then just update the 'pb' field. */
+     * If it is already present, then check whether it is the same pb.
+     * We might have two different pb with the same logical_port if it was
+     * deleted and added back within the same loop.
+     * If the same pb was already present, just update the 'pb' field.
+     * Otherwise, add the second pb */
     struct tracked_lport *lport =
         shash_find_data(&tracked_dp->lports, pb->logical_port);
 
     if (!lport) {
         lport = xmalloc(sizeof *lport);
         shash_add(&tracked_dp->lports, pb->logical_port, lport);
+    } else if (pb != lport->pb) {
+        bool found = false;
+        /* There is at least another pb with the same logical_port.
+         * However, our pb might already be shash_added (e.g. pb1 deleted, pb2
+         * added, pb2 deleted). This is not really optimal, but this loop
+         * only runs in a very uncommon race condition (same logical port
+         * deleted and added within same loop */
+        struct shash_node *node;
+        SHASH_FOR_EACH (node, &tracked_dp->lports) {
+            lport = (struct tracked_lport *) node->data;
+            if (lport->pb == pb) {
+                found = true;
+                break;
+            }
+        }
+        if (!found) {
+            lport = xmalloc(sizeof *lport);
+            shash_add(&tracked_dp->lports, pb->logical_port, lport);
+        }
     }
-
     lport->pb = pb;
     lport->tracked_type = tracked_type;
 }
diff --git a/tests/ovn.at b/tests/ovn.at
index d7f01169c..4835612ce 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -39233,3 +39233,43 @@  OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Port Deleted and added back])
+ovn_start
+
+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
+check 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
+
+check 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 2001::3"
+check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 2001::3"
+
+OVN_POPULATE_ARP
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+# Delete sw0-p1
+sleep_controller hv1
+check ovn-nbctl --wait=sb lsp-del sw0-p1
+
+# Add back sw0-p1 but without any address set.
+check ovn-nbctl --wait=sb lsp-add sw0 sw0-p1
+wake_up_controller hv1
+
+CHECK_FLOWS_AFTER_RECOMPUTE([hv1], [hv1])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])