On 2 June 2016 at 14:19, Ben Pfaff <b...@ovn.org> wrote:
> On Thu, May 19, 2016 at 01:02:33PM -0700, Gurucharan Shetty wrote:
> > OVS NAT currently cannot do snat and dnat in the same zone.
> > So we need two zones per gateway router.
> >
> > Signed-off-by: Gurucharan Shetty <g...@ovn.org>
>
> We're running out of registers quickly, but we're also using a full
> 32-bit register when we only need 16 bits, so there's considerable room
> to economize later if necessary.
>
I agree. I will work on atleast combining the 2 registers being used here
into one.
>
> Please update ovn-architecture(7) to mention the new connection tracking
> zones (the existing one is already mentioned).
>
I will add the following incremental.
diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index 13acaf5..553c2e5 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -669,12 +669,22 @@
</p>
</dd>
- <dt>conntrack zone field</dt>
+ <dt>conntrack zone field for logical ports</dt>
<dd>
- A field that denotes the connection tracking zone. The value only
- has local significance and is not meaningful between chassis.
- This is initialized to 0 at the beginning of the logical ingress
- pipeline. OVN stores this in Nicira extension register number 5.
+ A field that denotes the connection tracking zone for logical ports.
+ The value only has local significance and is not meaningful between
+ chassis. This is initialized to 0 at the beginning of the logical
+ ingress pipeline. OVN stores this in Nicira extension register
number 5.
+ </dd>
+
+ <dt>conntrack zone fields for Gateway router</dt>
+ <dd>
+ Fields that denote the connection tracking zones for Gateway routers.
+ These values only have local significance (only on chassis that have
+ Gateway routers instantiated) and is not meaningful between
+ chassis. OVN stores the zone information for DNATting in Nicira
+ extension register number 3 and zone information for SNATing in
Nicira
+ extension register number 4.
</dd>
>
> There are a couple of instances of
> + char *dnat = xasprintf(UUID_FMT"_%s",
> +
> UUID_ARGS(&binding->datapath->header_.uuid),
> + "dnat");
> + char *snat = xasprintf(UUID_FMT"_%s",
> +
> UUID_ARGS(&binding->datapath->header_.uuid),
> + "snat");
> or similar. Do you think it would be worth having a helper function (or
> two) so that they are harder to get out-of-sync?
>
How about something like the following?
diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
index 3b55e6c..356a94b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -40,6 +40,7 @@
#include "openvswitch/vconn.h"
#include "openvswitch/vlog.h"
#include "ovn/lib/ovn-sb-idl.h"
+#include "ovn/lib/ovn-util.h"
#include "patch.h"
#include "physical.h"
#include "pinctrl.h"
@@ -273,12 +274,8 @@ update_ct_zones(struct sset *lports, struct hmap
*patched_datapaths,
continue;
}
- char *dnat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&pd->port_binding->datapath->header_.uuid),
- "dnat");
- char *snat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&pd->port_binding->datapath->header_.uuid),
- "snat");
+ char *dnat = alloc_nat_zone_key(pd->port_binding, "dnat");
+ char *snat = alloc_nat_zone_key(pd->port_binding, "snat");
sset_add(&all_users, dnat);
sset_add(&all_users, snat);
free(dnat);
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index b6e752c..85528e0 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -24,6 +24,7 @@
#include "openvswitch/vlog.h"
#include "ovn-controller.h"
#include "ovn/lib/ovn-sb-idl.h"
+#include "ovn/lib/ovn-util.h"
#include "physical.h"
#include "shash.h"
#include "simap.h"
@@ -368,12 +369,8 @@ physical_run(struct controller_ctx *ctx, enum
mf_field_id mff_ovn_ge
}
int zone_id_dnat, zone_id_snat;
- char *dnat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&binding->datapath->header_.uuid),
- "dnat");
- char *snat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&binding->datapath->header_.uuid),
- "snat");
+ char *dnat = alloc_nat_zone_key(binding, "dnat");
+ char *snat = alloc_nat_zone_key(binding, "snat");
zone_id_dnat = simap_get(ct_zones, dnat);
if (zone_id_dnat) {
put_load(zone_id_dnat, MFF_LOG_DNAT_ZONE, 0, 32, &ofpacts);
diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
index abdc247..5a1dcb6 100644
--- a/ovn/lib/ovn-util.c
+++ b/ovn/lib/ovn-util.c
@@ -15,6 +15,7 @@
#include <config.h>
#include "ovn-util.h"
#include "openvswitch/vlog.h"
+#include "ovn/lib/ovn-sb-idl.h"
VLOG_DEFINE_THIS_MODULE(ovn_util);
@@ -102,3 +103,15 @@ extract_lport_addresses(char *address, struct
lport_addresses *laddr
return true;
}
+
+/* Allocates a key for NAT conntrack zone allocation for a provided
+ * 'port_binding' record and a 'type'.
+ *
+ * It is the caller's responsibility to free the allocated memory. */
+char *
+alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
+ const char *type)
+{
+ return xasprintf(UUID_FMT"_%s",
+ UUID_ARGS(&port_binding->datapath->header_.uuid),
type);
+}
diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
index 242984a..f475e6f 100644
--- a/ovn/lib/ovn-util.h
+++ b/ovn/lib/ovn-util.h
@@ -18,6 +18,8 @@
#include "lib/packets.h"
+struct sbrec_port_binding;
+
struct ipv4_netaddr {
ovs_be32 addr;
unsigned int plen;
@@ -40,5 +42,7 @@ bool
extract_lport_addresses(char *address, struct lport_addresses *laddrs,
bool store_ipv6);
-
+char *
+alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
+ const char *type);
>
> Acked-by: Ben Pfaff <b...@ovn.org>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
