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

[Ben Pfaff]

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  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  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  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());
> > > > > > +ds_destroy();
> > > > > > +
> > > > > > +/* Trace the actions in the next table. */
> > > > > > +trace__(dp, _flow, ct_nat->ltable, pipeline, >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(), "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(), "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, 

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

[Mickey Spiegel]

One minor nit and one real comment below.

On Tue, May 2, 2017 at 11:07 AM, Ben Pfaff  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  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  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());
> > > > > +ds_destroy();
> > > > > +
> > > > > +/* Trace the actions in the next table. */
> > > > > +trace__(dp, _flow, ct_nat->ltable, pipeline, >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(), "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(), "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
> +++ 

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

[Ben Pfaff]

On Mon, May 01, 2017 at 05:50:57PM -0700, Mickey Spiegel wrote:
> On Mon, May 1, 2017 at 5:12 PM, Ben Pfaff  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  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());
> > > > +ds_destroy();
> > > > +
> > > > +/* Trace the actions in the next table. */
> > > > +trace__(dp, _flow, ct_nat->ltable, pipeline, >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(), "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(), "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;
   

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

[Mickey Spiegel]

On Mon, May 1, 2017 at 5:12 PM, Ben Pfaff  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  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());
> > > +ds_destroy();
> > > +
> > > +/* Trace the actions in the next table. */
> > > +trace__(dp, _flow, ct_nat->ltable, pipeline, >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(), "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(), "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.

Mickey


> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

On Mon, May 01, 2017 at 03:39:32PM -0700, Mickey Spiegel wrote:
> On Sun, Apr 30, 2017 at 4:22 PM, Ben Pfaff  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());
> > +ds_destroy();
> > +
> > +/* Trace the actions in the next table. */
> > +trace__(dp, _flow, ct_nat->ltable, pipeline, >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(), "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(), "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?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Ben Pfaff]

Without this support, ovn-trace is not very useful with OpenStack, which
uses connection tracking extensively.

Signed-off-by: Ben Pfaff 
---
 ovn/utilities/ovn-trace.8.xml | 50 +++
 ovn/utilities/ovn-trace.c | 43 ++---
 2 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/ovn/utilities/ovn-trace.8.xml b/ovn/utilities/ovn-trace.8.xml
index 8bb329bfbd71..b2d46ac3d50b 100644
--- a/ovn/utilities/ovn-trace.8.xml
+++ b/ovn/utilities/ovn-trace.8.xml
@@ -166,6 +166,56 @@
 output;
   
 
+  Stateful Actions
+
+  
+Some OVN logical actions use or update state that is not available in the
+southbound database.  ovn-trace handles these actions as
+described below:
+  
+
+  
+ct_next
+
+  By default ovn-trace treats flows as ``tracked'' and
+  ``established.''  The --ct option overrides this behavior;
+  refer to its description for more information.
+
+
+ct_commit
+
+  This action is treated as a no-op.
+
+
+ct_dnat
+ct_snat
+
+  
+When one of these action is used without arguments, to ``un-NAT'' a
+packet, ovn-trace assumes that no NAT state was available
+and treats it as a no-op.
+  
+
+  
+With an argument, ovn-trace sets the IP destination or
+source, as appropriate, to the given address. It also sets
+ct.dnat or ct.snat to 1 to indicate that NAT
+has taken place.
+  
+
+
+ct_lb
+
+  Not yet implemented; currently implemented as a no-op.
+
+
+put_arp
+put_nd
+
+  This action is treated as a no-op.
+
+  
+
   Summary Output
 
   
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 647c8a51cd0d..4346fb39268a 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -1510,6 +1510,42 @@ execute_ct_next(const struct ovnact_ct_next *ct_next,
 }
 
 static void
+execute_ct_nat(const struct ovnact_ct_nat *ct_nat,
+   const struct ovntrace_datapath *dp, struct flow *uflow,
+   enum ovnact_pipeline pipeline, struct ovs_list *super)
+{
+bool is_dst = ct_nat->ovnact.type == OVNACT_CT_DNAT;
+const char *direction = is_dst ? "dst" : "src";
+
+/* Make a sub-node for attaching the next table,
+ * and figure out the changes if any. */
+struct flow ct_flow = *uflow;
+struct ds s = DS_EMPTY_INITIALIZER;
+ds_put_format(, "ct_%cnat", direction[0]);
+if (ct_nat->ip) {
+ds_put_format(, "(ip4.%s="IP_FMT")", direction, IP_ARGS(ct_nat->ip));
+ovs_be32 *ip = is_dst ? _flow.nw_dst : _flow.nw_src;
+*ip = ct_nat->ip;
+
+uint8_t state = is_dst ? CS_DST_NAT : CS_SRC_NAT;
+ct_flow.ct_state |= state;
+} else {
+ds_put_format(, " /* assuming no un-%cnat entry, so no change */",
+  direction[0]);
+}
+struct ovntrace_node *node = ovntrace_node_append(
+super, OVNTRACE_NODE_TRANSFORMATION, "%s", ds_cstr());
+ds_destroy();
+
+/* Trace the actions in the next table. */
+trace__(dp, _flow, ct_nat->ltable, pipeline, >subs);
+
+/* Upon return, we will trace the actions following the ct action in the
+ * original table.  The pipeline forked, so we're using the original
+ * flow, not ct_flow. */
+}
+
+static void
 trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
   const struct ovntrace_datapath *dp, struct flow *uflow,
   uint8_t table_id, enum ovnact_pipeline pipeline,
@@ -1573,10 +1609,11 @@ trace_actions(const struct ovnact *ovnacts, size_t 
ovnacts_len,
 break;
 
 case OVNACT_CT_DNAT:
+execute_ct_nat(ovnact_get_CT_DNAT(a), dp, uflow, pipeline, super);
+break;
+
 case OVNACT_CT_SNAT:
-ovntrace_node_append(super, OVNTRACE_NODE_ERROR,
- "*** ct_dnat and ct_snat actions "
- "not implemented");
+execute_ct_nat(ovnact_get_CT_SNAT(a), dp, uflow, pipeline, super);
 break;
 
 case OVNACT_CT_LB:
-- 
2.10.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev