[ovs-dev,v3] OVN: run local logical flows first in S_ROUTER_OUT_SNAT table
diff mbox series

Message ID 034361495642a40f342187be63c7f54115296d16.1562402015.git.lorenzo.bianconi@redhat.com
State New
Headers show
Series
  • [ovs-dev,v3] OVN: run local logical flows first in S_ROUTER_OUT_SNAT table
Related show

Commit Message

Lorenzo Bianconi July 6, 2019, 10:45 a.m. UTC
Run local logical flows first if the gw router port is scheduled
on the local chassis in order to properly manage snat traffic

Tested-by: Eran Kuris <ekuris@redhat.com>
Acked-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v2:
- fix compilation error
Changes since v1:
- add priority change in ovn-northd.8.xml
---
 ovn/northd/ovn-northd.8.xml | 3 ++-
 ovn/northd/ovn-northd.c     | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Mark Michelson July 8, 2019, 5:53 p.m. UTC | #1
Acked-by: Mark Michelson <mmichels@redhat.com>

On 7/6/19 6:45 AM, Lorenzo Bianconi wrote:
> Run local logical flows first if the gw router port is scheduled
> on the local chassis in order to properly manage snat traffic
> 
> Tested-by: Eran Kuris <ekuris@redhat.com>
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - fix compilation error
> Changes since v1:
> - add priority change in ovn-northd.8.xml
> ---
>   ovn/northd/ovn-northd.8.xml | 3 ++-
>   ovn/northd/ovn-northd.c     | 7 +++++--
>   2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 193aa210f..d2267de0e 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -2428,7 +2428,8 @@ nd_ns {
>           <p>
>             If the NAT rule cannot be handled in a distributed manner, then
>             the flow above is only programmed on the
> -          <code>redirect-chassis</code>.
> +          <code>redirect-chassis</code> increasing flow priority by 128 in
> +          order to be run first
>           </p>
>   
>           <p>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ba2719321..ce382ac89 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -6634,6 +6634,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                                     count_1bits(ntohl(mask)) + 1,
>                                     ds_cstr(&match), ds_cstr(&actions));
>                   } else {
> +                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
> +
>                       /* Distributed router. */
>                       ds_clear(&match);
>                       ds_put_format(&match, "ip && ip4.src == %s"
> @@ -6643,6 +6645,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                       if (!distributed && od->l3redirect_port) {
>                           /* Flows for NAT rules that are centralized are only
>                            * programmed on the "redirect-chassis". */
> +                        priority += 128;
>                           ds_put_format(&match, " && is_chassis_resident(%s)",
>                                         od->l3redirect_port->json_key);
>                       }
> @@ -6657,8 +6660,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                        * nat->logical_ip with the longest mask gets a higher
>                        * priority. */
>                       ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
> -                                  count_1bits(ntohl(mask)) + 1,
> -                                  ds_cstr(&match), ds_cstr(&actions));
> +                                  priority, ds_cstr(&match),
> +                                  ds_cstr(&actions));
>                   }
>               }
>   
>
Ben Pfaff July 8, 2019, 9:17 p.m. UTC | #2
On Sat, Jul 06, 2019 at 12:45:00PM +0200, Lorenzo Bianconi wrote:
> Run local logical flows first if the gw router port is scheduled
> on the local chassis in order to properly manage snat traffic
> 
> Tested-by: Eran Kuris <ekuris@redhat.com>
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - fix compilation error
> Changes since v1:
> - add priority change in ovn-northd.8.xml

Thanks, applied to master.
Darrell Ball July 19, 2019, 5:50 p.m. UTC | #3
The following tests consistently fail for kernel and userspace datapaths

136: ovn -- DNAT and SNAT on distributed router - N/S FAILED (
system-ovn.at:1337)
137: ovn -- DNAT and SNAT on distributed router - E/W FAILED (
system-ovn.at:1510)

after this commit

commit a6ee09882283426553e1a475e8b396af9bb378d0

Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Date:   Sat Jul 6 12:45:00 2019 +0200



    OVN: run local logical flows first in S_ROUTER_OUT_SNAT table



    Run local logical flows first if the gw router port is scheduled

    on the local chassis in order to properly manage snat traffic



    Tested-by: Eran Kuris <ekuris@redhat.com>

    Acked-by: Numan Siddique <nusiddiq@redhat.com>

    Acked-by: Mark Michelson <mmichels@redhat.com>

    Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

    Signed-off-by: Ben Pfaff blp@ovn.org




*136: ovn -- DNAT and SNAT on distributed router - N/S FAILED
(system-ovn.at:1337 <http://system-ovn.at:1337>)*



Missing conntrack entry:



@@ -1,2 +1 @@

-icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>



*137: ovn -- DNAT and SNAT on distributed router - E/W FAILED
(system-ovn.at:1510 <http://system-ovn.at:1510>)*



Missing conntrack entries:



+++
/home/dball/openvswitch/ovs/_gcc/tests/system-kmod-testsuite.dir/at-groups/137/stdout
2019-07-19
10:14:39.821883399 -0700

@@ -1,3 +1 @@

-icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>

-icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>




On Sat, Jul 6, 2019 at 3:48 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
wrote:

> Run local logical flows first if the gw router port is scheduled
> on the local chassis in order to properly manage snat traffic
>
> Tested-by: Eran Kuris <ekuris@redhat.com>
> Acked-by: Numan Siddique <nusiddiq@redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - fix compilation error
> Changes since v1:
> - add priority change in ovn-northd.8.xml
> ---
>  ovn/northd/ovn-northd.8.xml | 3 ++-
>  ovn/northd/ovn-northd.c     | 7 +++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 193aa210f..d2267de0e 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -2428,7 +2428,8 @@ nd_ns {
>          <p>
>            If the NAT rule cannot be handled in a distributed manner, then
>            the flow above is only programmed on the
> -          <code>redirect-chassis</code>.
> +          <code>redirect-chassis</code> increasing flow priority by 128 in
> +          order to be run first
>          </p>
>
>          <p>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index ba2719321..ce382ac89 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -6634,6 +6634,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                                    count_1bits(ntohl(mask)) + 1,
>                                    ds_cstr(&match), ds_cstr(&actions));
>                  } else {
> +                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
> +
>                      /* Distributed router. */
>                      ds_clear(&match);
>                      ds_put_format(&match, "ip && ip4.src == %s"
> @@ -6643,6 +6645,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                      if (!distributed && od->l3redirect_port) {
>                          /* Flows for NAT rules that are centralized are
> only
>                           * programmed on the "redirect-chassis". */
> +                        priority += 128;
>                          ds_put_format(&match, " &&
> is_chassis_resident(%s)",
>                                        od->l3redirect_port->json_key);
>                      }
> @@ -6657,8 +6660,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
>                       * nat->logical_ip with the longest mask gets a higher
>                       * priority. */
>                      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
> -                                  count_1bits(ntohl(mask)) + 1,
> -                                  ds_cstr(&match), ds_cstr(&actions));
> +                                  priority, ds_cstr(&match),
> +                                  ds_cstr(&actions));
>                  }
>              }
>
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Lorenzo Bianconi July 20, 2019, 11:07 a.m. UTC | #4
> The following tests consistently fail for kernel and userspace datapaths
> 
> 136: ovn -- DNAT and SNAT on distributed router - N/S FAILED (
> system-ovn.at:1337)
> 137: ovn -- DNAT and SNAT on distributed router - E/W FAILED (
> system-ovn.at:1510)
> 
> after this commit
> 
> commit a6ee09882283426553e1a475e8b396af9bb378d0
> 
> Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Date:   Sat Jul 6 12:45:00 2019 +0200
> 
> 
> 
>     OVN: run local logical flows first in S_ROUTER_OUT_SNAT table
> 
> 
> 
>     Run local logical flows first if the gw router port is scheduled
> 
>     on the local chassis in order to properly manage snat traffic
> 
> 
> 
>     Tested-by: Eran Kuris <ekuris@redhat.com>
> 
>     Acked-by: Numan Siddique <nusiddiq@redhat.com>
> 
>     Acked-by: Mark Michelson <mmichels@redhat.com>
> 
>     Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
>     Signed-off-by: Ben Pfaff blp@ovn.org
> 
> 
> 
> 
> *136: ovn -- DNAT and SNAT on distributed router - N/S FAILED
> (system-ovn.at:1337 <http://system-ovn.at:1337>)*

Hi Darrell,

thx a lot for the report. I guess this is a particular case since both the gw
router port and the FIPs are on the same hypervisor. Could you please try the
following patch:

diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 10fbd2649..a42cdf826 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -1334,11 +1334,13 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
 ])
 
 # We verify that SNAT indeed happened via 'dump-conntrack' command.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
 sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
-icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
 ])
 
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
 # South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
 # from 172.16.1.1
 NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
@@ -1347,7 +1349,7 @@ NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
 ])
 
 # We verify that SNAT indeed happened via 'dump-conntrack' command.
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.2) | \
 sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
 icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
 ])
@@ -1507,12 +1509,14 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
 
 # Check conntrack entries.  First SNAT of 'foo1' address happens.
 # Then DNAT of 'bar1' address happens (listed first below).
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.3) | \
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
 sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
-icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
-icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
+icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
 ])
 
+AT_CHECK([ovs-appctl dpctl/flush-conntrack])
+
 # East-West NAT: 'foo2' pings 'bar1' using 172.16.1.4.
 NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
 [0], [dnl

Moreover I got some issues since I am running fedora 30 and I have not
installed python2. Should we do something like?

diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
index 9d5f3bf41..a411e3d89 100644
--- a/tests/system-userspace-macros.at
+++ b/tests/system-userspace-macros.at
@@ -65,7 +65,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
 # Perform requirements checks for running conntrack tests.
 #
 m4_define([CHECK_CONNTRACK],
-    [AT_SKIP_IF([test $HAVE_PYTHON2 = no])]
+    [AT_SKIP_IF([test $HAVE_PYTHON = no])]
 )
 
 # CHECK_CONNTRACK_ALG()

Regards,
Lorenzo

> 
> 
> 
> Missing conntrack entry:
> 
> 
> 
> @@ -1,2 +1 @@
> 
> -icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
> 
> 
> 
> *137: ovn -- DNAT and SNAT on distributed router - E/W FAILED
> (system-ovn.at:1510 <http://system-ovn.at:1510>)*
> 
> 
> 
> Missing conntrack entries:
> 
> 
> 
> +++
> /home/dball/openvswitch/ovs/_gcc/tests/system-kmod-testsuite.dir/at-groups/137/stdout
> 2019-07-19
> 10:14:39.821883399 -0700
> 
> @@ -1,3 +1 @@
> 
> -icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
> 
> -icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
> 
> 
> 
> 
> On Sat, Jul 6, 2019 at 3:48 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> wrote:
> 
> > Run local logical flows first if the gw router port is scheduled
> > on the local chassis in order to properly manage snat traffic
> >
> > Tested-by: Eran Kuris <ekuris@redhat.com>
> > Acked-by: Numan Siddique <nusiddiq@redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> > Changes since v2:
> > - fix compilation error
> > Changes since v1:
> > - add priority change in ovn-northd.8.xml
> > ---
> >  ovn/northd/ovn-northd.8.xml | 3 ++-
> >  ovn/northd/ovn-northd.c     | 7 +++++--
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > index 193aa210f..d2267de0e 100644
> > --- a/ovn/northd/ovn-northd.8.xml
> > +++ b/ovn/northd/ovn-northd.8.xml
> > @@ -2428,7 +2428,8 @@ nd_ns {
> >          <p>
> >            If the NAT rule cannot be handled in a distributed manner, then
> >            the flow above is only programmed on the
> > -          <code>redirect-chassis</code>.
> > +          <code>redirect-chassis</code> increasing flow priority by 128 in
> > +          order to be run first
> >          </p>
> >
> >          <p>
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index ba2719321..ce382ac89 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -6634,6 +6634,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> >                                    count_1bits(ntohl(mask)) + 1,
> >                                    ds_cstr(&match), ds_cstr(&actions));
> >                  } else {
> > +                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
> > +
> >                      /* Distributed router. */
> >                      ds_clear(&match);
> >                      ds_put_format(&match, "ip && ip4.src == %s"
> > @@ -6643,6 +6645,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> >                      if (!distributed && od->l3redirect_port) {
> >                          /* Flows for NAT rules that are centralized are
> > only
> >                           * programmed on the "redirect-chassis". */
> > +                        priority += 128;
> >                          ds_put_format(&match, " &&
> > is_chassis_resident(%s)",
> >                                        od->l3redirect_port->json_key);
> >                      }
> > @@ -6657,8 +6660,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
> > hmap *ports,
> >                       * nat->logical_ip with the longest mask gets a higher
> >                       * priority. */
> >                      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
> > -                                  count_1bits(ntohl(mask)) + 1,
> > -                                  ds_cstr(&match), ds_cstr(&actions));
> > +                                  priority, ds_cstr(&match),
> > +                                  ds_cstr(&actions));
> >                  }
> >              }
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
Darrell Ball July 20, 2019, 7 p.m. UTC | #5
Thanks for the quick turnaround

couple comments inline

On Sat, Jul 20, 2019 at 4:07 AM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> wrote:

> > The following tests consistently fail for kernel and userspace datapaths
> >
> > 136: ovn -- DNAT and SNAT on distributed router - N/S FAILED (
> > system-ovn.at:1337)
> > 137: ovn -- DNAT and SNAT on distributed router - E/W FAILED (
> > system-ovn.at:1510)
> >
> > after this commit
> >
> > commit a6ee09882283426553e1a475e8b396af9bb378d0
> >
> > Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> > Date:   Sat Jul 6 12:45:00 2019 +0200
> >
> >
> >
> >     OVN: run local logical flows first in S_ROUTER_OUT_SNAT table
> >
> >
> >
> >     Run local logical flows first if the gw router port is scheduled
> >
> >     on the local chassis in order to properly manage snat traffic
> >
> >
> >
> >     Tested-by: Eran Kuris <ekuris@redhat.com>
> >
> >     Acked-by: Numan Siddique <nusiddiq@redhat.com>
> >
> >     Acked-by: Mark Michelson <mmichels@redhat.com>
> >
> >     Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> >
> >     Signed-off-by: Ben Pfaff blp@ovn.org
> >
> >
> >
> >
> > *136: ovn -- DNAT and SNAT on distributed router - N/S FAILED
> > (system-ovn.at:1337 <http://system-ovn.at:1337>)*
>
> Hi Darrell,
>
> thx a lot for the report. I guess this is a particular case since both the
> gw
> router port and the FIPs are on the same hypervisor. Could you please try
> the
> following patch:
>
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 10fbd2649..a42cdf826 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -1334,11 +1334,13 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.2 | FORMAT_PING], \
>  ])
>
>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
> -icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
>
> +icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>  ])
>
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
>  # South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
>  # from 172.16.1.1
>  NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 |
> FORMAT_PING], \
> @@ -1347,7 +1349,7 @@ NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.2 | FORMAT_PING], \
>  ])


>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.2) | \
>

Above change to 'FORMAT_CT(172.16.1.2)' will not be needed.


>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
>  icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>  ])
> @@ -1507,12 +1509,14 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2
> 172.16.1.4 | FORMAT_PING], \
>
>  # Check conntrack entries.  First SNAT of 'foo1' address happens.
>  # Then DNAT of 'bar1' address happens (listed first below).
> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.3) | \
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>
> -icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>
> -icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>
> +icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>
> +icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>  ])
>
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> +
>  # East-West NAT: 'foo2' pings 'bar1' using 172.16.1.4.
>  NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 |
> FORMAT_PING], \
>  [0], [dnl
>
> Moreover I got some issues since I am running fedora 30 and I have not
> installed python2. Should we do something like?
>
> diff --git a/tests/system-userspace-macros.at b/tests/
> system-userspace-macros.at
> index 9d5f3bf41..a411e3d89 100644
> --- a/tests/system-userspace-macros.at
> +++ b/tests/system-userspace-macros.at
> @@ -65,7 +65,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
>  # Perform requirements checks for running conntrack tests.
>  #
>  m4_define([CHECK_CONNTRACK],
> -    [AT_SKIP_IF([test $HAVE_PYTHON2 = no])]
> +    [AT_SKIP_IF([test $HAVE_PYTHON = no])]
>

It is fine, but I think you also want to make the same change to
system-kmod-macros.at <http://system-userspace-macros.at/>


>  )
>
>  # CHECK_CONNTRACK_ALG()
>
> Regards,
> Lorenzo
>
> >
> >
> >
> > Missing conntrack entry:
> >
> >
> >
> > @@ -1,2 +1 @@
> >
> >
> -icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
> >
> >
> >
> > *137: ovn -- DNAT and SNAT on distributed router - E/W FAILED
> > (system-ovn.at:1510 <http://system-ovn.at:1510>)*
> >
> >
> >
> > Missing conntrack entries:
> >
> >
> >
> > +++
> >
> /home/dball/openvswitch/ovs/_gcc/tests/system-kmod-testsuite.dir/at-groups/137/stdout
> > 2019-07-19
> > 10:14:39.821883399 -0700
> >
> > @@ -1,3 +1 @@
> >
> >
> -icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
> >
> >
> -icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
> >
> >
> >
> >
> > On Sat, Jul 6, 2019 at 3:48 AM Lorenzo Bianconi <
> lorenzo.bianconi@redhat.com>
> > wrote:
> >
> > > Run local logical flows first if the gw router port is scheduled
> > > on the local chassis in order to properly manage snat traffic
> > >
> > > Tested-by: Eran Kuris <ekuris@redhat.com>
> > > Acked-by: Numan Siddique <nusiddiq@redhat.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > > ---
> > > Changes since v2:
> > > - fix compilation error
> > > Changes since v1:
> > > - add priority change in ovn-northd.8.xml
> > > ---
> > >  ovn/northd/ovn-northd.8.xml | 3 ++-
> > >  ovn/northd/ovn-northd.c     | 7 +++++--
> > >  2 files changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> > > index 193aa210f..d2267de0e 100644
> > > --- a/ovn/northd/ovn-northd.8.xml
> > > +++ b/ovn/northd/ovn-northd.8.xml
> > > @@ -2428,7 +2428,8 @@ nd_ns {
> > >          <p>
> > >            If the NAT rule cannot be handled in a distributed manner,
> then
> > >            the flow above is only programmed on the
> > > -          <code>redirect-chassis</code>.
> > > +          <code>redirect-chassis</code> increasing flow priority by
> 128 in
> > > +          order to be run first
> > >          </p>
> > >
> > >          <p>
> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > > index ba2719321..ce382ac89 100644
> > > --- a/ovn/northd/ovn-northd.c
> > > +++ b/ovn/northd/ovn-northd.c
> > > @@ -6634,6 +6634,8 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >                                    count_1bits(ntohl(mask)) + 1,
> > >                                    ds_cstr(&match), ds_cstr(&actions));
> > >                  } else {
> > > +                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
> > > +
> > >                      /* Distributed router. */
> > >                      ds_clear(&match);
> > >                      ds_put_format(&match, "ip && ip4.src == %s"
> > > @@ -6643,6 +6645,7 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >                      if (!distributed && od->l3redirect_port) {
> > >                          /* Flows for NAT rules that are centralized
> are
> > > only
> > >                           * programmed on the "redirect-chassis". */
> > > +                        priority += 128;
> > >                          ds_put_format(&match, " &&
> > > is_chassis_resident(%s)",
> > >                                        od->l3redirect_port->json_key);
> > >                      }
> > > @@ -6657,8 +6660,8 @@ build_lrouter_flows(struct hmap *datapaths,
> struct
> > > hmap *ports,
> > >                       * nat->logical_ip with the longest mask gets a
> higher
> > >                       * priority. */
> > >                      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
> > > -                                  count_1bits(ntohl(mask)) + 1,
> > > -                                  ds_cstr(&match), ds_cstr(&actions));
> > > +                                  priority, ds_cstr(&match),
> > > +                                  ds_cstr(&actions));
> > >                  }
> > >              }
> > >
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>
Lorenzo Bianconi July 20, 2019, 8:39 p.m. UTC | #6
>
> Thanks for the quick turnaround
>
> couple comments inline
>
> On Sat, Jul 20, 2019 at 4:07 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote:
>>
>> > The following tests consistently fail for kernel and userspace datapaths
>> >
>> > 136: ovn -- DNAT and SNAT on distributed router - N/S FAILED (
>> > system-ovn.at:1337)
>> > 137: ovn -- DNAT and SNAT on distributed router - E/W FAILED (
>> > system-ovn.at:1510)
>> >
>> > after this commit
>> >
>> > commit a6ee09882283426553e1a475e8b396af9bb378d0
>> >
>> > Author: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> >
>> > Date:   Sat Jul 6 12:45:00 2019 +0200
>> >
>> >
>> >
>> >     OVN: run local logical flows first in S_ROUTER_OUT_SNAT table
>> >
>> >
>> >
>> >     Run local logical flows first if the gw router port is scheduled
>> >
>> >     on the local chassis in order to properly manage snat traffic
>> >
>> >
>> >
>> >     Tested-by: Eran Kuris <ekuris@redhat.com>
>> >
>> >     Acked-by: Numan Siddique <nusiddiq@redhat.com>
>> >
>> >     Acked-by: Mark Michelson <mmichels@redhat.com>
>> >
>> >     Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> >
>> >     Signed-off-by: Ben Pfaff blp@ovn.org
>> >
>> >
>> >
>> >
>> > *136: ovn -- DNAT and SNAT on distributed router - N/S FAILED
>> > (system-ovn.at:1337 <http://system-ovn.at:1337>)*
>>
>> Hi Darrell,
>>
>> thx a lot for the report. I guess this is a particular case since both the gw
>> router port and the FIPs are on the same hypervisor. Could you please try the
>> following patch:
>>
>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
>> index 10fbd2649..a42cdf826 100644
>> --- a/tests/system-ovn.at
>> +++ b/tests/system-ovn.at
>> @@ -1334,11 +1334,13 @@ NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
>>  ])
>>
>>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
>> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
>>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>> -icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
>> +icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>  ])
>>
>> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
>> +
>>  # South-North SNAT: 'bar1' pings 'alice1'. But 'alice1' receives traffic
>>  # from 172.16.1.1
>>  NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
>> @@ -1347,7 +1349,7 @@ NS_CHECK_EXEC([bar1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.2 | FORMAT_PING], \
>>  ])
>>
>>
>>  # We verify that SNAT indeed happened via 'dump-conntrack' command.
>> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.1) | \
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.2) | \
>
>
> Above change to 'FORMAT_CT(172.16.1.2)' will not be needed.
>

ack

>>
>>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>>  icmp,orig=(src=192.168.2.2,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>  ])
>> @@ -1507,12 +1509,14 @@ NS_CHECK_EXEC([foo1], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
>>
>>  # Check conntrack entries.  First SNAT of 'foo1' address happens.
>>  # Then DNAT of 'bar1' address happens (listed first below).
>> -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.3) | \
>> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.1.4) | \
>>  sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl
>> -icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>> -icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>> +icmp,orig=(src=172.16.1.1,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>> +icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.1,id=<cleared>,type=0,code=0),zone=<cleared>
>>  ])
>>
>> +AT_CHECK([ovs-appctl dpctl/flush-conntrack])
>> +
>>  # East-West NAT: 'foo2' pings 'bar1' using 172.16.1.4.
>>  NS_CHECK_EXEC([foo2], [ping -q -c 3 -i 0.3 -w 2 172.16.1.4 | FORMAT_PING], \
>>  [0], [dnl
>>
>> Moreover I got some issues since I am running fedora 30 and I have not
>> installed python2. Should we do something like?
>>
>> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at
>> index 9d5f3bf41..a411e3d89 100644
>> --- a/tests/system-userspace-macros.at
>> +++ b/tests/system-userspace-macros.at
>> @@ -65,7 +65,7 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
>>  # Perform requirements checks for running conntrack tests.
>>  #
>>  m4_define([CHECK_CONNTRACK],
>> -    [AT_SKIP_IF([test $HAVE_PYTHON2 = no])]
>> +    [AT_SKIP_IF([test $HAVE_PYTHON = no])]
>
>
> It is fine, but I think you also want to make the same change to system-kmod-macros.at
>

ack. I will post two separated patches. Next week I am in PTO so I
will post them the week after.

Regards,
Lorenzo

>>
>>  )
>>
>>  # CHECK_CONNTRACK_ALG()
>>
>> Regards,
>> Lorenzo
>>
>> >
>> >
>> >
>> > Missing conntrack entry:
>> >
>> >
>> >
>> > @@ -1,2 +1 @@
>> >
>> > -icmp,orig=(src=192.168.1.3,dst=172.16.1.2,id=<cleared>,type=8,code=0),reply=(src=172.16.1.2,dst=172.16.1.4,id=<cleared>,type=0,code=0),zone=<cleared>
>> >
>> >
>> >
>> > *137: ovn -- DNAT and SNAT on distributed router - E/W FAILED
>> > (system-ovn.at:1510 <http://system-ovn.at:1510>)*
>> >
>> >
>> >
>> > Missing conntrack entries:
>> >
>> >
>> >
>> > +++
>> > /home/dball/openvswitch/ovs/_gcc/tests/system-kmod-testsuite.dir/at-groups/137/stdout
>> > 2019-07-19
>> > 10:14:39.821883399 -0700
>> >
>> > @@ -1,3 +1 @@
>> >
>> > -icmp,orig=(src=172.16.1.3,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=192.168.2.2,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>> >
>> > -icmp,orig=(src=192.168.1.2,dst=172.16.1.4,id=<cleared>,type=8,code=0),reply=(src=172.16.1.4,dst=172.16.1.3,id=<cleared>,type=0,code=0),zone=<cleared>
>> >
>> >
>> >
>> >
>> > On Sat, Jul 6, 2019 at 3:48 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> > wrote:
>> >
>> > > Run local logical flows first if the gw router port is scheduled
>> > > on the local chassis in order to properly manage snat traffic
>> > >
>> > > Tested-by: Eran Kuris <ekuris@redhat.com>
>> > > Acked-by: Numan Siddique <nusiddiq@redhat.com>
>> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
>> > > ---
>> > > Changes since v2:
>> > > - fix compilation error
>> > > Changes since v1:
>> > > - add priority change in ovn-northd.8.xml
>> > > ---
>> > >  ovn/northd/ovn-northd.8.xml | 3 ++-
>> > >  ovn/northd/ovn-northd.c     | 7 +++++--
>> > >  2 files changed, 7 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
>> > > index 193aa210f..d2267de0e 100644
>> > > --- a/ovn/northd/ovn-northd.8.xml
>> > > +++ b/ovn/northd/ovn-northd.8.xml
>> > > @@ -2428,7 +2428,8 @@ nd_ns {
>> > >          <p>
>> > >            If the NAT rule cannot be handled in a distributed manner, then
>> > >            the flow above is only programmed on the
>> > > -          <code>redirect-chassis</code>.
>> > > +          <code>redirect-chassis</code> increasing flow priority by 128 in
>> > > +          order to be run first
>> > >          </p>
>> > >
>> > >          <p>
>> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
>> > > index ba2719321..ce382ac89 100644
>> > > --- a/ovn/northd/ovn-northd.c
>> > > +++ b/ovn/northd/ovn-northd.c
>> > > @@ -6634,6 +6634,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> > > hmap *ports,
>> > >                                    count_1bits(ntohl(mask)) + 1,
>> > >                                    ds_cstr(&match), ds_cstr(&actions));
>> > >                  } else {
>> > > +                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
>> > > +
>> > >                      /* Distributed router. */
>> > >                      ds_clear(&match);
>> > >                      ds_put_format(&match, "ip && ip4.src == %s"
>> > > @@ -6643,6 +6645,7 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> > > hmap *ports,
>> > >                      if (!distributed && od->l3redirect_port) {
>> > >                          /* Flows for NAT rules that are centralized are
>> > > only
>> > >                           * programmed on the "redirect-chassis". */
>> > > +                        priority += 128;
>> > >                          ds_put_format(&match, " &&
>> > > is_chassis_resident(%s)",
>> > >                                        od->l3redirect_port->json_key);
>> > >                      }
>> > > @@ -6657,8 +6660,8 @@ build_lrouter_flows(struct hmap *datapaths, struct
>> > > hmap *ports,
>> > >                       * nat->logical_ip with the longest mask gets a higher
>> > >                       * priority. */
>> > >                      ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
>> > > -                                  count_1bits(ntohl(mask)) + 1,
>> > > -                                  ds_cstr(&match), ds_cstr(&actions));
>> > > +                                  priority, ds_cstr(&match),
>> > > +                                  ds_cstr(&actions));
>> > >                  }
>> > >              }
>> > >
>> > > --
>> > > 2.21.0
>> > >
>> > > _______________________________________________
>> > > dev mailing list
>> > > dev@openvswitch.org
>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > >

Patch
diff mbox series

diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 193aa210f..d2267de0e 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -2428,7 +2428,8 @@  nd_ns {
         <p>
           If the NAT rule cannot be handled in a distributed manner, then
           the flow above is only programmed on the
-          <code>redirect-chassis</code>.
+          <code>redirect-chassis</code> increasing flow priority by 128 in
+          order to be run first
         </p>
 
         <p>
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index ba2719321..ce382ac89 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -6634,6 +6634,8 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                                   count_1bits(ntohl(mask)) + 1,
                                   ds_cstr(&match), ds_cstr(&actions));
                 } else {
+                    uint16_t priority = count_1bits(ntohl(mask)) + 1;
+
                     /* Distributed router. */
                     ds_clear(&match);
                     ds_put_format(&match, "ip && ip4.src == %s"
@@ -6643,6 +6645,7 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                     if (!distributed && od->l3redirect_port) {
                         /* Flows for NAT rules that are centralized are only
                          * programmed on the "redirect-chassis". */
+                        priority += 128;
                         ds_put_format(&match, " && is_chassis_resident(%s)",
                                       od->l3redirect_port->json_key);
                     }
@@ -6657,8 +6660,8 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                      * nat->logical_ip with the longest mask gets a higher
                      * priority. */
                     ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT,
-                                  count_1bits(ntohl(mask)) + 1,
-                                  ds_cstr(&match), ds_cstr(&actions));
+                                  priority, ds_cstr(&match),
+                                  ds_cstr(&actions));
                 }
             }