[ovs-dev] [PATCH 23/27] ovn-trace: Add some basic tracing for ct_snat and ct_dnat actions.

Ben Pfaff blp at ovn.org
Wed May 3 15:43:05 UTC 2017


On Tue, May 02, 2017 at 11:33:27AM -0700, Mickey Spiegel wrote:
> One minor nit and one real comment below.
> 
> On Tue, May 2, 2017 at 11:07 AM, Ben Pfaff <blp at ovn.org> wrote:
> 
> > On Mon, May 01, 2017 at 05:50:57PM -0700, Mickey Spiegel wrote:
> > > On Mon, May 1, 2017 at 5:12 PM, Ben Pfaff <blp at ovn.org> wrote:
> > >
> > > > On Mon, May 01, 2017 at 03:39:32PM -0700, Mickey Spiegel wrote:
> > > > > On Sun, Apr 30, 2017 at 4:22 PM, Ben Pfaff <blp at ovn.org> wrote:
> > > > >
> > > > > > Without this support, ovn-trace is not very useful with OpenStack,
> > > > which
> > > > > > uses connection tracking extensively.
> > > > > >
> > > > >
> > > > > I scanned the patch set briefly, not what I would call a full review
> > but
> > > > > quick sanity checking. The only issue that I saw is described inline
> > > > below.
> > > >
> > > > Thanks!
> > > >
> > > > > > +    struct ovntrace_node *node = ovntrace_node_append(
> > > > > > +        super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr(&s));
> > > > > > +    ds_destroy(&s);
> > > > > > +
> > > > > > +    /* Trace the actions in the next table. */
> > > > > > +    trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs);
> > > > > >
> > > > >
> > > > > Since OpenStack uses NAT on distributed routers, moving on to the
> > next
> > > > > table is the right thing to do.
> > > > >
> > > > > However, in case gateway routers are used, ct_snat without an IP
> > address
> > > > > does not do recirc.
> > > > > Lines 832 to 842 of ovn/lib/actions.c:
> > > > >
> > > > >     } else if (snat && ep->is_gateway_router) {
> > > > >         /* For performance reasons, we try to prevent additional
> > > > >          * recirculations.  ct_snat which is used in a gateway router
> > > > >          * does not need a recirculation.  ct_snat(IP) does need a
> > > > >          * recirculation.  ct_snat in a distributed router needs
> > > > >          * recirculation regardless of whether an IP address is
> > > > >          * specified.
> > > > >          * XXX Should we consider a method to let the actions specify
> > > > >          * whether an action needs recirculation if there are more
> > use
> > > > >          * cases?. */
> > > > >         ct->recirc_table = NX_CT_RECIRC_NONE;
> > > > >
> > > > > Lines 4548, 4549 of ovn/northd/ovn-northd.c for a gateway router:
> > > > >
> > > > >                     ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT, 90,
> > > > >                                   ds_cstr(&match), "ct_snat; next;");
> > > > >
> > > > > The corresponding lines 4565, 4566 of ovn/northd/ovn-northd.c for a
> > > > > distributed router:
> > > > >
> > > > >                     ovn_lflow_add(lflows, od, S_ROUTER_IN_UNSNAT,
> > 100,
> > > > >                                   ds_cstr(&match), "ct_snat;");
> > > > >
> > > > > I think with this code you would be seeing double on a gateway
> > router,
> > > > > since both "ct_snat" and "next" would trace the actions in the next
> > > > table.
> > > >
> > > > Oh, that's a good point.
> > > >
> > > > From lflow.c, a given router is a gateway router if its datapath is
> > > > present on the local hypervisor and it has a local L3 gateway:
> > > >
> > > >     static bool
> > > >     is_gateway_router(const struct sbrec_datapath_binding *ldp,
> > > >                       const struct hmap *local_datapaths)
> > > >     {
> > > >         struct local_datapath *ld =
> > > >             get_local_datapath(local_datapaths, ldp->tunnel_key);
> > > >         return ld ? ld->has_local_l3gateway : false;
> > > >     }
> > > >
> > > > Therefore, this is another bit of context that ovn-trace would need to
> > > > be provided via command-line options.  I guess it would have to be
> > > > something like "--gateway-router no,yes" to indicate, for example, that
> > > > the first snat is not for a gateway router and that the second one is
> > > > (or whatever).  And I'd tend to assume that the default is "no" since
> > > > that makes the OpenStack case work OK.  Mickey and Guru, does this
> > > > concept and syntax make sense?  If not, can you suggest a way?
> > > >
> > >
> > > Two ways to figure out if a router is a gateway router or not:
> > >
> > > 1. If you have access to nb, if the logical router has options:chassis
> > then
> > > it is a gateway router.
> > > 2. From sb, while processing read_ports in ovn/utilities/ovn-trace.c, any
> > > ports with type "l3gateway" on a datapath representing a router indicate
> > > that the router is a gateway router. That is more or less what
> > > ovn-controller does in "add_local_datapath" in ovn/controller/binding.c
> > to
> > > set "has_local_l3gateway", which ends up triggering no recirc in
> > > ovn/lib/actions.c.
> > >
> > > The next question is whether a specific gateway router should be treated
> > as
> > > local. Since ovn-trace has no knowledge of topology and hypervisors, it
> > > seems like the consistent approach would be to treat all gateway routers
> > as
> > > local for the purposes of ovn-trace.
> >
> > OK, how about folding in something like this?
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
> > index 4346fb39268a..638c21317f85 100644
> > --- a/ovn/utilities/ovn-trace.c
> > +++ b/ovn/utilities/ovn-trace.c
> > @@ -346,6 +346,8 @@ struct ovntrace_datapath {
> >      size_t n_flows, allocated_flows;
> >
> >      struct hmap mac_bindings;   /* Contains "struct
> > ovntrace_mac_binding"s. */
> > +
> > +    bool has_local_l3gateway;
> >  };
> >
> >  struct ovntrace_port {
> > @@ -570,6 +572,9 @@ read_ports(void)
> >                      port->peer->peer = port;
> >                  }
> >              }
> > +        } else if (!strcmp(sbpb->type, "l3gateway")) {
> > +            /* Treat all gateways are local for our purposes. */
> >
> 
> Minor nit: Treat all gateways as local for our purposes.
> 
> 
> > +            dp->has_local_l3gateway = true;
> >          }
> >      }
> >
> > @@ -1515,6 +1520,10 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
> >                 enum ovnact_pipeline pipeline, struct ovs_list *super)
> >  {
> >      bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT;
> > +    if (!is_dst && dp->has_local_l3gateway) {
> > +        /* ct_snat has no visible effect in a gateway router. */
> > +        return;
> > +    }
> >
> 
> If an IP address is specified with ct_snat, then you want to replace the
> address, and even on a gateway router it does trigger recirc.
> 
> Instead of this change, near the bottom, something like the following:
> 
>     if (is_dst || ct_nat->ip || !dp->has_local_l3gateway) {
>         /* Trace the actions in the next table, except for ct_snat
>          * with no IP on a gateway router since there is no recirc
>          * in that case. */
>         trace__(dp, &ct_flow, ct_nat->ltable, pipeline, &node->subs);
>     }

Thanks for pointing out those problems.

I recognize that this did the wrong thing for ct_snat(IP) for a gateway
router.  But I think we want to skip most of the function in the
"ct_snat;" case for a gateway router, because if I understand correctly
there can never be a visible change to the flow, or other noticeable
behavior, in that case.

So, I folded this in:

diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 65623e1beb41..860fd4b26be0 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -573,7 +573,7 @@ read_ports(void)
                 }
             }
         } else if (!strcmp(sbpb->type, "l3gateway")) {
-            /* Treat all gateways are local for our purposes. */
+            /* Treat all gateways as local for our purposes. */
             dp->has_local_l3gateway = true;
         }
     }
@@ -1532,8 +1532,8 @@ execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
                enum ovnact_pipeline pipeline, struct ovs_list *super)
 {
     bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT;
-    if (!is_dst && dp->has_local_l3gateway) {
-        /* ct_snat has no visible effect in a gateway router. */
+    if (!is_dst && dp->has_local_l3gateway && !ct_nat->ip) {
+        /* "ct_snat;" has no visible effect in a gateway router. */
         return;
     }
     const char *direction = is_dst ? "dst" : "src";


More information about the dev mailing list