diff mbox series

[ovs-dev,ovn] ovn-controller: Clear flows not associated with db rows in physical flow change handler.

Message ID 20200727165702.2769313-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev,ovn] ovn-controller: Clear flows not associated with db rows in physical flow change handler. | expand

Commit Message

Numan Siddique July 27, 2020, 4:57 p.m. UTC
From: Numan Siddique <numans@ovn.org>

The commit in the Fixes tag while handling changes for OVS interface changes and ct zone
changes, called physical_run() without clearing the flow table. This works ok for existing
flows in the flow table which are associated with ovsdb rows. physical_run() ensures that
such flows are deleted by calling ofctrl_delete_flows() by adding them back again.

But flows not associated with ovsdb rows (whose OF flow cookie is set to 0) are not cleared
at all until a full recompute is triggered. Particularly flows in table 33 which set
various conntrack zone registers still remain. Suppose a lport is claimed again and if
the ct-zone id for this lport changes, then the old flow for this lport still remains and
this causes the packet to enter the conntrack with a  wrong zone id.

Such flows are stored indexed with the uuid 'hc_uuid' in  the flow table and it is easy
to clear them. This patch clears such flows before calling physical_run() to fix this issue.

A more accurate fix would be to store the logical and physical flows in separate flow tables
and clear all the flows in the  physical flow table before calling physical_run().
This patch is good enough for now until we implement separate flow tables.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861042
Fixes: a3005f0dc777("ovn-controller: I-P for ct zone and OVS interface changes in flow output stage.")
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/ovn-controller.c |   1 +
 controller/physical.c       |   8 +++
 controller/physical.h       |   1 +
 tests/ovn.at                | 105 ++++++++++++++++++++++++++++++++++++
 tests/system-ovn.at         |  89 ++++++++++++++++++++++++++++++
 5 files changed, 204 insertions(+)

Comments

Mark Michelson July 27, 2020, 6:15 p.m. UTC | #1
Acked-by: Mark Michelson <mmichels@redhat.com>

There's one small comment in your test. You can just fix it when you 
merge, though.

On 7/27/20 12:57 PM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> The commit in the Fixes tag while handling changes for OVS interface changes and ct zone
> changes, called physical_run() without clearing the flow table. This works ok for existing
> flows in the flow table which are associated with ovsdb rows. physical_run() ensures that
> such flows are deleted by calling ofctrl_delete_flows() by adding them back again.
> 
> But flows not associated with ovsdb rows (whose OF flow cookie is set to 0) are not cleared
> at all until a full recompute is triggered. Particularly flows in table 33 which set
> various conntrack zone registers still remain. Suppose a lport is claimed again and if
> the ct-zone id for this lport changes, then the old flow for this lport still remains and
> this causes the packet to enter the conntrack with a  wrong zone id.
> 
> Such flows are stored indexed with the uuid 'hc_uuid' in  the flow table and it is easy
> to clear them. This patch clears such flows before calling physical_run() to fix this issue.
> 
> A more accurate fix would be to store the logical and physical flows in separate flow tables
> and clear all the flows in the  physical flow table before calling physical_run().
> This patch is good enough for now until we implement separate flow tables.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861042
> Fixes: a3005f0dc777("ovn-controller: I-P for ct zone and OVS interface changes in flow output stage.")
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   controller/ovn-controller.c |   1 +
>   controller/physical.c       |   8 +++
>   controller/physical.h       |   1 +
>   tests/ovn.at                | 105 ++++++++++++++++++++++++++++++++++++
>   tests/system-ovn.at         |  89 ++++++++++++++++++++++++++++++
>   5 files changed, 204 insertions(+)
> 
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e355007b88..5ca32ac433 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1994,6 +1994,7 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
>   
>       if (pfc_data->recompute_physical_flows) {
>           /* This indicates that we need to recompute the physical flows. */
> +        physical_clear_unassoc_flows_with_db(&fo->flow_table);
>           physical_run(&p_ctx, &fo->flow_table);
>           return true;
>       }
> diff --git a/controller/physical.c b/controller/physical.c
> index 6d7d8e93bc..535c777307 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1851,3 +1851,11 @@ get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t *ofport)
>       *ofport = tun->ofport;
>       return true;
>   }
> +
> +void
> +physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table)
> +{
> +    if (hc_uuid) {
> +        ofctrl_remove_flows(flow_table, hc_uuid);
> +    }
> +}
> diff --git a/controller/physical.h b/controller/physical.h
> index 9ca34436a5..feab41df4c 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -61,6 +61,7 @@ struct physical_ctx {
>   void physical_register_ovs_idl(struct ovsdb_idl *);
>   void physical_run(struct physical_ctx *,
>                     struct ovn_desired_flow_table *);
> +void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *);
>   void physical_handle_port_binding_changes(struct physical_ctx *,
>                                             struct ovn_desired_flow_table *);
>   void physical_handle_mc_group_changes(struct physical_ctx *,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 7c9e14e2ea..48ab4e0655 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -20806,3 +20806,108 @@ AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"])
>   
>   OVN_CLEANUP([hv1], [hv2])
>   AT_CLEANUP
> +
> +
> +# When a lport is released on a chassis, ovn-controller was
> +# not clearing some of the flowss in the table 33.
> +# Make sure that those flows are cleared properly.
> +AT_SETUP([ovn -- Test clear flows in physical tables])
> +AT_KEYWORDS([lb])

Misleading keywords.

> +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-nbctl ls-add sw0
> +
> +ovn-nbctl lsp-add sw0 sw0-p1
> +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> +
> +ovn-nbctl lsp-add sw0 sw0-p2
> +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> +
> +as hv1
> +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
> +
> +as hv1
> +ovs-vsctl -- add-port br-int hv1-vif2 -- \
> +    set interface hv1-vif2 external-ids:iface-id=sw0-p2 \
> +    options:tx_pcap=hv1/vif2-tx.pcap \
> +    options:rxq_pcap=hv1/vif2-rx.pcap \
> +    ofport-request=2
> +
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup])
> +
> +sw0_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list datapath_binding sw0)
> +p1_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding sw0-p1)
> +p2_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding sw0-p2)
> +
> +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> +AT_CHECK([test ! -z $p1_zoneid])
> +
> +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g')
> +AT_CHECK([test ! -z $p2_zoneid])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\
> +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\
> +reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
> +
> +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
> +
> +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> +AT_CHECK([test -z $p1_zoneid])
> +
> +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> +
> +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> +AT_CHECK([test ! -z $p1_zoneid])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
> +
> +ovs-vsctl del-port hv1-vif2
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown])
> +
> +AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0])
> +
> +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g')
> +AT_CHECK([test -z $p2_zoneid])
> +
> +ovn-nbctl lsp-del sw0-p1
> +
> +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
> +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
> +
> +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> +AT_CHECK([test ! -z $p1_zoneid])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index eddc530f97..bca2601267 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -4467,6 +4467,95 @@ NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0])
>   # Send the packet to VIP.
>   NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0])
>   
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +
> +AT_CLEANUP
> +
> +# When a lport is released on a chassis, ovn-controller was
> +# not clearing some of the flowss in the table 33 leading
> +# to packet drops if ct() is hit.
> +# Make sure that those flows are cleared properly.
> +AT_SETUP([ovn --Test packet drops due to incorrect flows in physical table 33])
> +AT_KEYWORDS([lb])
> +
> +ovn_start
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +
> +# Set external-ids in br-int needed for ovn-controller
> +ovs-vsctl \
> +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> +        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> +        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
> +
> +# Start ovn-controller
> +start_daemon ovn-controller
> +
> +ovn-nbctl ls-add sw0
> +ovn-nbctl lsp-add sw0 sw0-p1-f
> +ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3"
> +
> +ovn-nbctl lsp-add sw0 sw0-p2-f
> +ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4"
> +
> +ovn-nbctl lsp-add sw0 sw0-p3-f
> +ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5"
> +
> +# Add ACL with allow-ralated so that conntrack is hit.
> +
> +ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related
> +ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related
> +
> +ADD_NAMESPACES(sw0-p1-f)
> +ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24", "10:54:00:00:00:03", \
> +         "10.0.0.1")
> +
> +ADD_NAMESPACES(sw0-p2-f)
> +ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24", "10:54:00:00:00:04", \
> +         "10.0.0.1")
> +
> +ADD_NAMESPACES(sw0-p3-f)
> +ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24", "10:54:00:00:00:05", \
> +         "10.0.0.1")
> +
> +# Send ping from sw0-p1-f to sw0-p3-f
> +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \
> +[0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id
> +ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id
> +
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown])
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown])
> +
> +ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f
> +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup])
> +
> +# Send ping from sw0-p1-f to sw0-p3-f again and it should work.
> +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \
> +[0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +
>   OVS_APP_EXIT_AND_WAIT([ovn-controller])
>   
>   as ovn-sb
>
Numan Siddique July 27, 2020, 7:30 p.m. UTC | #2
On Mon, Jul 27, 2020 at 11:45 PM Mark Michelson <mmichels@redhat.com> wrote:

> Acked-by: Mark Michelson <mmichels@redhat.com>
>
> There's one small comment in your test. You can just fix it when you
> merge, though.
>

Thanks for the review. I addressed your comment and applied this patch to
master and branch-20.06

Numan


>
> On 7/27/20 12:57 PM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > The commit in the Fixes tag while handling changes for OVS interface
> changes and ct zone
> > changes, called physical_run() without clearing the flow table. This
> works ok for existing
> > flows in the flow table which are associated with ovsdb rows.
> physical_run() ensures that
> > such flows are deleted by calling ofctrl_delete_flows() by adding them
> back again.
> >
> > But flows not associated with ovsdb rows (whose OF flow cookie is set to
> 0) are not cleared
> > at all until a full recompute is triggered. Particularly flows in table
> 33 which set
> > various conntrack zone registers still remain. Suppose a lport is
> claimed again and if
> > the ct-zone id for this lport changes, then the old flow for this lport
> still remains and
> > this causes the packet to enter the conntrack with a  wrong zone id.
> >
> > Such flows are stored indexed with the uuid 'hc_uuid' in  the flow table
> and it is easy
> > to clear them. This patch clears such flows before calling
> physical_run() to fix this issue.
> >
> > A more accurate fix would be to store the logical and physical flows in
> separate flow tables
> > and clear all the flows in the  physical flow table before calling
> physical_run().
> > This patch is good enough for now until we implement separate flow
> tables.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1861042
> > Fixes: a3005f0dc777("ovn-controller: I-P for ct zone and OVS interface
> changes in flow output stage.")
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   controller/ovn-controller.c |   1 +
> >   controller/physical.c       |   8 +++
> >   controller/physical.h       |   1 +
> >   tests/ovn.at                | 105 ++++++++++++++++++++++++++++++++++++
> >   tests/system-ovn.at         |  89 ++++++++++++++++++++++++++++++
> >   5 files changed, 204 insertions(+)
> >
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index e355007b88..5ca32ac433 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1994,6 +1994,7 @@ flow_output_physical_flow_changes_handler(struct
> engine_node *node, void *data)
> >
> >       if (pfc_data->recompute_physical_flows) {
> >           /* This indicates that we need to recompute the physical
> flows. */
> > +        physical_clear_unassoc_flows_with_db(&fo->flow_table);
> >           physical_run(&p_ctx, &fo->flow_table);
> >           return true;
> >       }
> > diff --git a/controller/physical.c b/controller/physical.c
> > index 6d7d8e93bc..535c777307 100644
> > --- a/controller/physical.c
> > +++ b/controller/physical.c
> > @@ -1851,3 +1851,11 @@ get_tunnel_ofport(const char *chassis_name, char
> *encap_ip, ofp_port_t *ofport)
> >       *ofport = tun->ofport;
> >       return true;
> >   }
> > +
> > +void
> > +physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table
> *flow_table)
> > +{
> > +    if (hc_uuid) {
> > +        ofctrl_remove_flows(flow_table, hc_uuid);
> > +    }
> > +}
> > diff --git a/controller/physical.h b/controller/physical.h
> > index 9ca34436a5..feab41df4c 100644
> > --- a/controller/physical.h
> > +++ b/controller/physical.h
> > @@ -61,6 +61,7 @@ struct physical_ctx {
> >   void physical_register_ovs_idl(struct ovsdb_idl *);
> >   void physical_run(struct physical_ctx *,
> >                     struct ovn_desired_flow_table *);
> > +void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table
> *);
> >   void physical_handle_port_binding_changes(struct physical_ctx *,
> >                                             struct
> ovn_desired_flow_table *);
> >   void physical_handle_mc_group_changes(struct physical_ctx *,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 7c9e14e2ea..48ab4e0655 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -20806,3 +20806,108 @@ AT_CHECK([test "$hv2_offlows" =
> "$hv2_offlows_mon"])
> >
> >   OVN_CLEANUP([hv1], [hv2])
> >   AT_CLEANUP
> > +
> > +
> > +# When a lport is released on a chassis, ovn-controller was
> > +# not clearing some of the flowss in the table 33.
> > +# Make sure that those flows are cleared properly.
> > +AT_SETUP([ovn -- Test clear flows in physical tables])
> > +AT_KEYWORDS([lb])
>
> Misleading keywords.
>
> > +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-nbctl ls-add sw0
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p1
> > +ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p2
> > +ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> > +
> > +as hv1
> > +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
> > +
> > +as hv1
> > +ovs-vsctl -- add-port br-int hv1-vif2 -- \
> > +    set interface hv1-vif2 external-ids:iface-id=sw0-p2 \
> > +    options:tx_pcap=hv1/vif2-tx.pcap \
> > +    options:rxq_pcap=hv1/vif2-rx.pcap \
> > +    ofport-request=2
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup])
> > +
> > +sw0_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list
> datapath_binding sw0)
> > +p1_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding
> sw0-p1)
> > +p2_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding
> sw0-p2)
> > +
> > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> > +AT_CHECK([test ! -z $p1_zoneid])
> > +
> > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p2 | sed 's/"//g')
> > +AT_CHECK([test ! -z $p2_zoneid])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l)
> -eq 1])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw1_dpkey},\
> > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw1_dpkey},\
> > +reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l)
> -eq 1])
> > +
> > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
> > +
> > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> > +AT_CHECK([test -z $p1_zoneid])
> > +
> > +ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
> > +
> > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> > +AT_CHECK([test ! -z $p1_zoneid])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l)
> -eq 1])
> > +
> > +ovs-vsctl del-port hv1-vif2
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown])
> > +
> > +AT_CHECK([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0])
> > +
> > +p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p2 | sed 's/"//g')
> > +AT_CHECK([test -z $p2_zoneid])
> > +
> > +ovn-nbctl lsp-del sw0-p1
> > +
> > +OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int
> table=33,metadata=${sw0_dpkey},\
> > +reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
> > +
> > +p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int
> external_ids:ct-zone-sw0-p1 | sed 's/"//g')
> > +AT_CHECK([test ! -z $p1_zoneid])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index eddc530f97..bca2601267 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -4467,6 +4467,95 @@ NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80],
> [0])
> >   # Send the packet to VIP.
> >   NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0])
> >
> > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > +
> > +as ovn-sb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as ovn-nb
> > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > +
> > +as northd
> > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > +
> > +as
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> > +/connection dropped.*/d"])
> > +
> > +AT_CLEANUP
> > +
> > +# When a lport is released on a chassis, ovn-controller was
> > +# not clearing some of the flowss in the table 33 leading
> > +# to packet drops if ct() is hit.
> > +# Make sure that those flows are cleared properly.
> > +AT_SETUP([ovn --Test packet drops due to incorrect flows in physical
> table 33])
> > +AT_KEYWORDS([lb])
> > +
> > +ovn_start
> > +
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +ADD_BR([br-int])
> > +
> > +# Set external-ids in br-int needed for ovn-controller
> > +ovs-vsctl \
> > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> > +        -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> > +        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> > +        -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true
> > +
> > +# Start ovn-controller
> > +start_daemon ovn-controller
> > +
> > +ovn-nbctl ls-add sw0
> > +ovn-nbctl lsp-add sw0 sw0-p1-f
> > +ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3"
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p2-f
> > +ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4"
> > +
> > +ovn-nbctl lsp-add sw0 sw0-p3-f
> > +ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5"
> > +
> > +# Add ACL with allow-ralated so that conntrack is hit.
> > +
> > +ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related
> > +ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related
> > +
> > +ADD_NAMESPACES(sw0-p1-f)
> > +ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24",
> "10:54:00:00:00:03", \
> > +         "10.0.0.1")
> > +
> > +ADD_NAMESPACES(sw0-p2-f)
> > +ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24",
> "10:54:00:00:00:04", \
> > +         "10.0.0.1")
> > +
> > +ADD_NAMESPACES(sw0-p3-f)
> > +ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24",
> "10:54:00:00:00:05", \
> > +         "10.0.0.1")
> > +
> > +# Send ping from sw0-p1-f to sw0-p3-f
> > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 |
> FORMAT_PING], \
> > +[0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +
> > +ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id
> > +ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown])
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown])
> > +
> > +ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f
> > +OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup])
> > +
> > +# Send ping from sw0-p1-f to sw0-p3-f again and it should work.
> > +NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 |
> FORMAT_PING], \
> > +[0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +
> > +
> >   OVS_APP_EXIT_AND_WAIT([ovn-controller])
> >
> >   as ovn-sb
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
diff mbox series

Patch

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index e355007b88..5ca32ac433 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1994,6 +1994,7 @@  flow_output_physical_flow_changes_handler(struct engine_node *node, void *data)
 
     if (pfc_data->recompute_physical_flows) {
         /* This indicates that we need to recompute the physical flows. */
+        physical_clear_unassoc_flows_with_db(&fo->flow_table);
         physical_run(&p_ctx, &fo->flow_table);
         return true;
     }
diff --git a/controller/physical.c b/controller/physical.c
index 6d7d8e93bc..535c777307 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1851,3 +1851,11 @@  get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t *ofport)
     *ofport = tun->ofport;
     return true;
 }
+
+void
+physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *flow_table)
+{
+    if (hc_uuid) {
+        ofctrl_remove_flows(flow_table, hc_uuid);
+    }
+}
diff --git a/controller/physical.h b/controller/physical.h
index 9ca34436a5..feab41df4c 100644
--- a/controller/physical.h
+++ b/controller/physical.h
@@ -61,6 +61,7 @@  struct physical_ctx {
 void physical_register_ovs_idl(struct ovsdb_idl *);
 void physical_run(struct physical_ctx *,
                   struct ovn_desired_flow_table *);
+void physical_clear_unassoc_flows_with_db(struct ovn_desired_flow_table *);
 void physical_handle_port_binding_changes(struct physical_ctx *,
                                           struct ovn_desired_flow_table *);
 void physical_handle_mc_group_changes(struct physical_ctx *,
diff --git a/tests/ovn.at b/tests/ovn.at
index 7c9e14e2ea..48ab4e0655 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -20806,3 +20806,108 @@  AT_CHECK([test "$hv2_offlows" = "$hv2_offlows_mon"])
 
 OVN_CLEANUP([hv1], [hv2])
 AT_CLEANUP
+
+
+# When a lport is released on a chassis, ovn-controller was
+# not clearing some of the flowss in the table 33.
+# Make sure that those flows are cleared properly.
+AT_SETUP([ovn -- Test clear flows in physical tables])
+AT_KEYWORDS([lb])
+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-nbctl ls-add sw0
+
+ovn-nbctl lsp-add sw0 sw0-p1
+ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
+ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3"
+
+ovn-nbctl lsp-add sw0 sw0-p2
+ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
+ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
+
+as hv1
+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
+
+as hv1
+ovs-vsctl -- add-port br-int hv1-vif2 -- \
+    set interface hv1-vif2 external-ids:iface-id=sw0-p2 \
+    options:tx_pcap=hv1/vif2-tx.pcap \
+    options:rxq_pcap=hv1/vif2-rx.pcap \
+    ofport-request=2
+
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xup])
+
+sw0_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list datapath_binding sw0)
+p1_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding sw0-p1)
+p2_dpkey=$(ovn-sbctl  --bare --columns tunnel_key list port_binding sw0-p2)
+
+p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
+AT_CHECK([test ! -z $p1_zoneid])
+
+p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g')
+AT_CHECK([test ! -z $p2_zoneid])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\
+reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 1])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw1_dpkey},\
+reg15=0x${p2_dpkey} | grep "load:0x${p2_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
+
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=foo
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xdown])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
+
+p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
+AT_CHECK([test -z $p1_zoneid])
+
+ovs-vsctl set interface hv1-vif1 external_ids:iface-id=sw0-p1
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p1) = xup])
+
+p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
+AT_CHECK([test ! -z $p1_zoneid])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 1])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep "load:0x${p1_zoneid}->NXM_NX_REG13" | wc -l) -eq 1])
+
+ovs-vsctl del-port hv1-vif2
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2) = xdown])
+
+AT_CHECK([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p2_dpkey} | grep REG13 | wc -l) -eq 0])
+
+p2_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p2 | sed 's/"//g')
+AT_CHECK([test -z $p2_zoneid])
+
+ovn-nbctl lsp-del sw0-p1
+
+OVS_WAIT_UNTIL([test $(ovs-ofctl dump-flows br-int table=33,metadata=${sw0_dpkey},\
+reg15=0x${p1_dpkey} | grep REG13 | wc -l) -eq 0])
+
+p1_zoneid=$(as hv1 ovs-vsctl get bridge br-int external_ids:ct-zone-sw0-p1 | sed 's/"//g')
+AT_CHECK([test ! -z $p1_zoneid])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index eddc530f97..bca2601267 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -4467,6 +4467,95 @@  NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0])
 # Send the packet to VIP.
 NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0])
 
+OVS_APP_EXIT_AND_WAIT([ovn-controller])
+
+as ovn-sb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as ovn-nb
+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+
+as northd
+OVS_APP_EXIT_AND_WAIT([ovn-northd])
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+
+AT_CLEANUP
+
+# When a lport is released on a chassis, ovn-controller was
+# not clearing some of the flowss in the table 33 leading
+# to packet drops if ct() is hit.
+# Make sure that those flows are cleared properly.
+AT_SETUP([ovn --Test packet drops due to incorrect flows in physical table 33])
+AT_KEYWORDS([lb])
+
+ovn_start
+
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+# Set external-ids in br-int needed for ovn-controller
+ovs-vsctl \
+        -- set Open_vSwitch . external-ids:system-id=hv1 \
+        -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+        -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+        -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+        -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+# Start ovn-controller
+start_daemon ovn-controller
+
+ovn-nbctl ls-add sw0
+ovn-nbctl lsp-add sw0 sw0-p1-f
+ovn-nbctl lsp-set-addresses sw0-p1-f "10:54:00:00:00:03 10.0.0.3"
+
+ovn-nbctl lsp-add sw0 sw0-p2-f
+ovn-nbctl lsp-set-addresses sw0-p2-f "10:54:00:00:00:04 10.0.0.4"
+
+ovn-nbctl lsp-add sw0 sw0-p3-f
+ovn-nbctl lsp-set-addresses sw0-p3-f "10:54:00:00:00:05 10.0.0.5"
+
+# Add ACL with allow-ralated so that conntrack is hit.
+
+ovn-nbctl acl-add sw0 from-lport 1002 "ip" allow-related
+ovn-nbctl acl-add sw0 to-lport 1002 "ip" allow-related
+
+ADD_NAMESPACES(sw0-p1-f)
+ADD_VETH(sw0-p1-f, sw0-p1-f, br-int, "10.0.0.3/24", "10:54:00:00:00:03", \
+         "10.0.0.1")
+
+ADD_NAMESPACES(sw0-p2-f)
+ADD_VETH(sw0-p2-f, sw0-p2-f, br-int, "10.0.0.4/24", "10:54:00:00:00:04", \
+         "10.0.0.1")
+
+ADD_NAMESPACES(sw0-p3-f)
+ADD_VETH(sw0-p3-f, sw0-p3-f, br-int, "10.0.0.5/24", "10:54:00:00:00:05", \
+         "10.0.0.1")
+
+# Send ping from sw0-p1-f to sw0-p3-f
+NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+ovs-vsctl remove interface ovs-sw0-p2-f external_ids iface-id
+ovs-vsctl remove interface ovs-sw0-p3-f external_ids iface-id
+
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p2-f) = xdown])
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xdown])
+
+ovs-vsctl set interface ovs-sw0-p3-f external_ids:iface-id=sw0-p3-f
+OVS_WAIT_UNTIL([test x$(ovn-nbctl lsp-get-up sw0-p3-f) = xup])
+
+# Send ping from sw0-p1-f to sw0-p3-f again and it should work.
+NS_CHECK_EXEC([sw0-p1-f], [ping -q -c 3 -i 0.3 -w 2 10.0.0.5 | FORMAT_PING], \
+[0], [dnl
+3 packets transmitted, 3 received, 0% packet loss, time 0ms
+])
+
+
 OVS_APP_EXIT_AND_WAIT([ovn-controller])
 
 as ovn-sb