[ovs-dev] ovn-controller: Fix conntrack zone in gateway routers.
diff mbox

Message ID 1482149537-20962-1-git-send-email-guru@ovn.org
State Accepted
Headers show

Commit Message

Guru Shetty Dec. 19, 2016, 12:12 p.m. UTC
The gateway router was using the ct_next action to
reassemble packets.  But ct_next action by default would
use the zone allocated for a logical port and in case of
gateway routers that value was zero.  This would make
the flow use the default zone of zero.  This had some
unintended consequences as the zone used to track packets
and the zone used to eventually commit it (DNAT zone)
was different.  As a result, a packet would never have ct.est set.

With this commit, when ct_next action is used in a gateway
router, we use the DNAT zone.  This is similar to the
strategy used in commit c2e954a117a8 (ovn-controller: Datapath
based conntrack zone for load-balancing.)

Signed-off-by: Gurucharan Shetty <guru@ovn.org>
---
 ovn/lib/actions.c | 3 ++-
 ovn/ovn-sb.xml    | 8 +++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Ben Pfaff Dec. 20, 2016, 5:26 a.m. UTC | #1
On Mon, Dec 19, 2016 at 04:12:17AM -0800, Gurucharan Shetty wrote:
> The gateway router was using the ct_next action to
> reassemble packets.  But ct_next action by default would
> use the zone allocated for a logical port and in case of
> gateway routers that value was zero.  This would make
> the flow use the default zone of zero.  This had some
> unintended consequences as the zone used to track packets
> and the zone used to eventually commit it (DNAT zone)
> was different.  As a result, a packet would never have ct.est set.
> 
> With this commit, when ct_next action is used in a gateway
> router, we use the DNAT zone.  This is similar to the
> strategy used in commit c2e954a117a8 (ovn-controller: Datapath
> based conntrack zone for load-balancing.)
> 
> Signed-off-by: Gurucharan Shetty <guru@ovn.org>

Seems reasonable.

> -    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> +    ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
> +                            : mf_from_id(MFF_LOG_DNAT_ZONE);

You could put the ?: inside the mf_from_id().

Acked-by: Ben Pfaff <blp@ovn.org>
Guru Shetty Dec. 20, 2016, 4:21 p.m. UTC | #2
On 19 December 2016 at 21:26, Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Dec 19, 2016 at 04:12:17AM -0800, Gurucharan Shetty wrote:
> > The gateway router was using the ct_next action to
> > reassemble packets.  But ct_next action by default would
> > use the zone allocated for a logical port and in case of
> > gateway routers that value was zero.  This would make
> > the flow use the default zone of zero.  This had some
> > unintended consequences as the zone used to track packets
> > and the zone used to eventually commit it (DNAT zone)
> > was different.  As a result, a packet would never have ct.est set.
> >
> > With this commit, when ct_next action is used in a gateway
> > router, we use the DNAT zone.  This is similar to the
> > strategy used in commit c2e954a117a8 (ovn-controller: Datapath
> > based conntrack zone for load-balancing.)
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>
> Seems reasonable.
>
> > -    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> > +    ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
> > +                            : mf_from_id(MFF_LOG_DNAT_ZONE);
>
> You could put the ?: inside the mf_from_id().
>
> There were a few other places using the above style. So I let it be (I
hope there was no inherent advantage with one style over the other here?)


> Acked-by: Ben Pfaff <blp@ovn.org>
>

Thanks, I applied this!
Ben Pfaff Dec. 20, 2016, 10:45 p.m. UTC | #3
On Tue, Dec 20, 2016 at 08:21:28AM -0800, Guru Shetty wrote:
> On 19 December 2016 at 21:26, Ben Pfaff <blp@ovn.org> wrote:
> > > -    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> > > +    ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
> > > +                            : mf_from_id(MFF_LOG_DNAT_ZONE);
> >
> > You could put the ?: inside the mf_from_id().
> >
> There were a few other places using the above style. So I let it be (I
> hope there was no inherent advantage with one style over the other here?)

The two forms are only syntactically different.

Patch
diff mbox

diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index fa8f175..686ecc5 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -551,7 +551,8 @@  encode_CT_NEXT(const struct ovnact_next *next,
 {
     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
     ct->recirc_table = ep->first_ptable + next->ltable;
-    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+    ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
+                            : mf_from_id(MFF_LOG_DNAT_ZONE);
     ct->zone_src.ofs = 0;
     ct->zone_src.n_bits = 16;
     ofpact_finish(ofpacts, &ct->ofpact);
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 65191ed..6daa8aa 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1037,9 +1037,11 @@ 
             As a side effect, IP fragments will be reassembled for matching.
             If a fragmented packet is output, then it will be sent with any
             overlapping fragments squashed.  The connection tracking state is
-            scoped by the logical port, so overlapping addresses may be used.
-            To allow traffic related to the matched flow, execute
-            <code>ct_commit</code>.
+            scoped by the logical port when the action is used in a flow for
+            a logical switch, so overlapping addresses may be used.  To allow
+            traffic related to the matched flow, execute <code>ct_commit
+            </code>.  Connection tracking state is scoped by the logical
+            topology when the action is used in a flow for a router.
           </p>
 
           <p>