On Thu, Nov 7, 2024 at 3:20 AM Ales Musil <[email protected]> wrote:
>
> On Tue, Oct 15, 2024 at 3:53 PM Xavier Simonart <[email protected]> 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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
> Other than that it looks good to me.
>
> Acked-by: Ales Musil <[email protected]>
>
> Thanks,
> Ales
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> [email protected]
> <https://red.ht/sig>
> _______________________________________________
> 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
