[ovs-dev,v5,2/9] ovs-vsctl: Add conntrack zone commands.
diff mbox series

Message ID 1567030469-120137-3-git-send-email-yihung.wei@gmail.com
State New
Headers show
Series
  • Support zone-based conntrack timeout policy
Related show

Commit Message

Yi-Hung Wei Aug. 28, 2019, 10:14 p.m. UTC
From: William Tu <u9012063@gmail.com>

The patch adds commands creating/deleting/listing conntrack zone
timeout policies:
  $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ...

Signed-off-by: William Tu <u9012063@gmail.com>
---
 tests/ovs-vsctl.at       |  34 +++++++-
 utilities/ovs-vsctl.8.in |  26 ++++++
 utilities/ovs-vsctl.c    | 202 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 256 insertions(+), 6 deletions(-)

Comments

Justin Pettit Sept. 12, 2019, 9:14 p.m. UTC | #1
> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> 
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 7c09df79bd29..5b9883ae1c3d 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -353,6 +353,32 @@ list.
> Prints the name of the bridge that contains \fIiface\fR on standard
> output.
> .
> +.SS "Conntrack Zone Commands"
> ...
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> +already exists is an error.  With \fB\-\-may\-exist\fR,
> +this command does nothing if \fIzone_id\fR is already created\fR.

I don't think you need that final \fR.

I also made a few minor language tweaks in the icremental.

> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 4948137efe8c..76a708bd5069 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> 
> +static struct ovsrec_ct_timeout_policy *
> +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> +{


I think it's clearer if we switch "argv" to "tps", since the argument should only be timeout policies.

> +static void
> +cmd_list_zone_tp(struct ctl_context *ctx)
> +{
> ...
> +        for (int j = 0; j < tp->n_timeouts; j++) {
> +            if (j == tp->n_timeouts - 1) {
> +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> +            } else {
> +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> +            }

I think this can be done without the duplicated code with a ds_chomp() and ds_put_char().  Let me know if you agree with the incremental patch at the bottom.

> static void
> cmd_add_br(struct ctl_context *ctx)
> {
> @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax vsctl_commands[] = {
>     /* Switch commands. */
>     {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", RW},
> 
> +    /* Zone and CT Timeout Policy commands. */
> +    {"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
> +     "--may-exist", RW},

Is there a reason not to make the minimum arguments 3 instead of 2?  It seems like it's required in the code.

Is there a reason you limited this to 18 arguments and not use INT_MAX?

Let me know if you agree with the incremental.

Thanks,

--Justin


-=-=-=-=-=-=-=-=-=-=-

diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 5b9883ae1c3d..14a8aa4a48ac 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -361,13 +361,13 @@ Creates a conntrack zone timeout policy with \fIzone_id\fR in
 \fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
 pairs, separated by spaces.  For example, \fBicmp_first=30
 icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
-packet and a 60-second policy for ICMP reply packet.  See the
+packet and a 60-second policy for ICMP reply packets.  See the
 \fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
 supported keys.
 .IP
 Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
 already exists is an error.  With \fB\-\-may\-exist\fR,
-this command does nothing if \fIzone_id\fR is already created\fR.
+this command does nothing if \fIzone_id\fR already exists.
 .
 .IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
 Delete the timeout policy associated with \fIzone_id\fR from \fIdatapath\fR.
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 76a708bd5069..43f64d4711f9 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1180,7 +1180,7 @@ find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
 }
 
 static struct ovsrec_ct_timeout_policy *
-create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
+create_timeout_policy(struct ctl_context *ctx, char **tps, int n_tps)
 {
     const struct ovsrec_ct_timeout_policy_table *tp_table;
     const struct ovsrec_ct_timeout_policy *row;
@@ -1193,7 +1193,7 @@ create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
 
     /* Parse timeout arguments. */
     for (int i = 0; i < n_tps; i++) {
-        policies[i] = xstrdup(argv[i]);
+        policies[i] = xstrdup(tps[i]);
 
         char *key, *value;
         char *policy = policies[i];
@@ -1320,14 +1320,11 @@ cmd_list_zone_tp(struct ctl_context *ctx)
         struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
 
         for (int j = 0; j < tp->n_timeouts; j++) {
-            if (j == tp->n_timeouts - 1) {
-                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
+            ds_put_format(&ctx->output, "%s=%"PRIu64" ",
                           tp->key_timeouts[j], tp->value_timeouts[j]);
-            } else {
-                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
-                          tp->key_timeouts[j], tp->value_timeouts[j]);
-            }
         }
+        ds_chomp(&ctx->output, ' ');
+        ds_put_char(&ctx->output, '\n');
     }
 }
 
@@ -3084,7 +3081,7 @@ static const struct ctl_command_syntax vsctl_commands[] = {
     {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", RW},
 
     /* Zone and CT Timeout Policy commands. */
-    {"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
+    {"add-zone-tp", 3, INT_MAX, "", pre_get_zone, cmd_add_zone_tp, NULL,
      "--may-exist", RW},
     {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
      "--if-exists", RW},
William Tu Sept. 13, 2019, 5:41 p.m. UTC | #2
Hi Justin,

Thanks for your feedbacks.


On Thu, Sep 12, 2019 at 02:14:56PM -0700, Justin Pettit wrote:
> 
> > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
> > 
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..5b9883ae1c3d 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,32 @@ list.
> > Prints the name of the bridge that contains \fIiface\fR on standard
> > output.
> > .
> > +.SS "Conntrack Zone Commands"
> > ...
> > +Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
> > +already exists is an error.  With \fB\-\-may\-exist\fR,
> > +this command does nothing if \fIzone_id\fR is already created\fR.
> 
> I don't think you need that final \fR.
> 
> I also made a few minor language tweaks in the icremental.
> 
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 4948137efe8c..76a708bd5069 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > 
> > +static struct ovsrec_ct_timeout_policy *
> > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> > +{
> 
> 
> I think it's clearer if we switch "argv" to "tps", since the argument should only be timeout policies.

OK

> 
> > +static void
> > +cmd_list_zone_tp(struct ctl_context *ctx)
> > +{
> > ...
> > +        for (int j = 0; j < tp->n_timeouts; j++) {
> > +            if (j == tp->n_timeouts - 1) {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            } else {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            }
> 
> I think this can be done without the duplicated code with a ds_chomp() and ds_put_char().  Let me know if you agree with the incremental patch at the bottom.

OK, looks much better!

> 
> > static void
> > cmd_add_br(struct ctl_context *ctx)
> > {
> > @@ -2896,6 +3083,13 @@ static const struct ctl_command_syntax vsctl_commands[] = {
> >     /* Switch commands. */
> >     {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", RW},
> > 
> > +    /* Zone and CT Timeout Policy commands. */
> > +    {"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
> > +     "--may-exist", RW},
> 
> Is there a reason not to make the minimum arguments 3 instead of 2?  It seems like it's required in the code.

OK, using 3 makes sure at least one policies is added.

> 
> Is there a reason you limited this to 18 arguments and not use INT_MAX?

I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
(dp_name, zone_id), so total is 18.
I think using INT_MAX is fine, because at db schema, the value type is set.

I will merge your diff and send next version.

Thanks
William

> 
> Let me know if you agree with the incremental.
> 
> Thanks,
> 
> --Justin
> 
>
Justin Pettit Sept. 13, 2019, 5:48 p.m. UTC | #3
> On Sep 13, 2019, at 10:41 AM, William Tu <u9012063@gmail.com> wrote:
> 
>> Is there a reason you limited this to 18 arguments and not use INT_MAX?
> 
> I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
> (dp_name, zone_id), so total is 18.
> I think using INT_MAX is fine, because at db schema, the value type is set.

That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do that enforcement.  It's nice to decouple the configuration tool from the binary as much as possible in case people run different versions.

> I will merge your diff and send next version.

Sounds good.  I'm hopeful that we'll be able to get this version merged with minimal changes, though.

Thanks,

--Justin
William Tu Sept. 13, 2019, 6:06 p.m. UTC | #4
On Fri, Sep 13, 2019 at 10:48 AM Justin Pettit <jpettit@ovn.org> wrote:
>
>
> > On Sep 13, 2019, at 10:41 AM, William Tu <u9012063@gmail.com> wrote:
> >
> >> Is there a reason you limited this to 18 arguments and not use INT_MAX?
> >
> > I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
> > (dp_name, zone_id), so total is 18.
> > I think using INT_MAX is fine, because at db schema, the value type is set.
>
> That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do that enforcement.  It's nice to decouple the configuration tool from the binary as much as possible in case people run different versions.
>
> > I will merge your diff and send next version.
>
> Sounds good.  I'm hopeful that we'll be able to get this version merged with minimal changes, though.
>
> Thanks,

Hi Justin,

I applied your diff and it's working fine.
I don't have any other change, do you want me to send another
patch or you want to directly apply?

Thanks
William
Justin Pettit Sept. 13, 2019, 9:21 p.m. UTC | #5
> On Sep 13, 2019, at 11:06 AM, William Tu <u9012063@gmail.com> wrote:
> 
> On Fri, Sep 13, 2019 at 10:48 AM Justin Pettit <jpettit@ovn.org> wrote:
>> 
>> 
>>> On Sep 13, 2019, at 10:41 AM, William Tu <u9012063@gmail.com> wrote:
>>> 
>>>> Is there a reason you limited this to 18 arguments and not use INT_MAX?
>>> 
>>> I use 18 because at most we have 11 tcp, 3 udp, 2 icmp, total of 16 and plus
>>> (dp_name, zone_id), so total is 18.
>>> I think using INT_MAX is fine, because at db schema, the value type is set.
>> 
>> That's a fair argument.  However, I'd suggest that we let ovs-vswitchd do that enforcement.  It's nice to decouple the configuration tool from the binary as much as possible in case people run different versions.
>> 
>>> I will merge your diff and send next version.
>> 
>> Sounds good.  I'm hopeful that we'll be able to get this version merged with minimal changes, though.
>> 
>> Thanks,
> 
> Hi Justin,
> 
> I applied your diff and it's working fine.
> I don't have any other change, do you want me to send another
> patch or you want to directly apply?

I'm hoping that I can just apply your patch with the changes directly.

--Justin

Patch
diff mbox series

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index 46fa3c5b1a33..df15fb6901a0 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -805,6 +805,20 @@  AT_CHECK(
   [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567"'])])
 AT_CHECK(
   [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
+
+AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1 icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1 icmp_first=1 icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout Policies: icmp_first=1 icmp_reply=2
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout Policies: icmp_first=1 icmp_reply=2
+Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
+])
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
 
@@ -890,10 +904,10 @@  AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=-1])],
 AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
   [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid range 0 to 4095 (inclusive)
 ])
-AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
+AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
   [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the allowed values ([in-band, out-of-band])
 ]])
-AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
+AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
   [1], [], [ovs-vsctl: cannot specify key to set for non-map column connection_mode
 ])
 AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
@@ -929,6 +943,22 @@  AT_CHECK([RUN_OVS_VSCTL([remove bridge br1 flood-vlans true])],
 AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
   [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge
 ])
+
+AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 -- set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1 icmp_reply=2])],
+  [1], [], [ovs-vsctl: datapath netdevxx does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])],
+  [1], [], [ovs-vsctl: zone id 2 already exists
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
+  [1], [], [ovs-vsctl: zone id 11 does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
+])
 OVS_VSCTL_CLEANUP
 AT_CLEANUP
 
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 7c09df79bd29..5b9883ae1c3d 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -353,6 +353,32 @@  list.
 Prints the name of the bridge that contains \fIiface\fR on standard
 output.
 .
+.SS "Conntrack Zone Commands"
+These commands query and modify datapath CT zones and Timeout Policies.
+.
+.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
+Creates a conntrack zone timeout policy with \fIzone_id\fR in
+\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
+pairs, separated by spaces.  For example, \fBicmp_first=30
+icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
+packet and a 60-second policy for ICMP reply packet.  See the
+\fBCT_Timeout_Policy\fR table in \fBovs-vswitchd.conf.db\fR(5) for the
+supported keys.
+.IP
+Without \fB\-\-may\-exist\fR, attempting to add a \fIzone_id\fR that
+already exists is an error.  With \fB\-\-may\-exist\fR,
+this command does nothing if \fIzone_id\fR is already created\fR.
+.
+.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
+Delete the timeout policy associated with \fIzone_id\fR from \fIdatapath\fR.
+.IP
+Without \fB\-\-if\-exists\fR, attempting to delete a zone that
+does not exist is an error.  With \fB\-\-if\-exists\fR, attempting to
+delete a zone that does not exist has no effect.
+.
+.IP "\fBlist\-zone\-tp \fIdatapath\fR"
+Prints the timeout policies of all zones in \fIdatapath\fR.
+.
 .SS "OpenFlow Controller Connectivity"
 .
 \fBovs\-vswitchd\fR can perform all configured bridging and switching
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 4948137efe8c..76a708bd5069 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -32,14 +32,18 @@ 
 #include "command-line.h"
 #include "compiler.h"
 #include "dirs.h"
-#include "openvswitch/dynamic-string.h"
 #include "fatal-signal.h"
 #include "hash.h"
+#include "openvswitch/dynamic-string.h"
 #include "openvswitch/json.h"
+#include "openvswitch/ofp-parse.h"
+#include "openvswitch/poll-loop.h"
+#include "openvswitch/vconn.h"
+#include "openvswitch/vlog.h"
 #include "ovsdb-data.h"
 #include "ovsdb-idl.h"
-#include "openvswitch/poll-loop.h"
 #include "process.h"
+#include "simap.h"
 #include "stream.h"
 #include "stream-ssl.h"
 #include "smap.h"
@@ -49,8 +53,6 @@ 
 #include "table.h"
 #include "timeval.h"
 #include "util.h"
-#include "openvswitch/vconn.h"
-#include "openvswitch/vlog.h"
 
 VLOG_DEFINE_THIS_MODULE(vsctl);
 
@@ -1153,6 +1155,191 @@  cmd_emer_reset(struct ctl_context *ctx)
     vsctl_context_invalidate_cache(ctx);
 }
 
+static struct ovsrec_datapath *
+find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name)
+{
+    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
+
+    for (int i = 0; i < ovs->n_datapaths; i++) {
+        if (!strcmp(ovs->key_datapaths[i], dp_name)) {
+            return ovs->value_datapaths[i];
+        }
+    }
+    return NULL;
+}
+
+static struct ovsrec_ct_zone *
+find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
+{
+    for (int i = 0; i < dp->n_ct_zones; i++) {
+        if (dp->key_ct_zones[i] == zone_id) {
+            return dp->value_ct_zones[i];
+        }
+    }
+    return NULL;
+}
+
+static struct ovsrec_ct_timeout_policy *
+create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
+{
+    const struct ovsrec_ct_timeout_policy_table *tp_table;
+    const struct ovsrec_ct_timeout_policy *row;
+    struct ovsrec_ct_timeout_policy *tp = NULL;
+    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
+
+    char **policies = xzalloc(sizeof *policies * n_tps);
+    const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
+    int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
+
+    /* Parse timeout arguments. */
+    for (int i = 0; i < n_tps; i++) {
+        policies[i] = xstrdup(argv[i]);
+
+        char *key, *value;
+        char *policy = policies[i];
+        if (!ofputil_parse_key_value(&policy, &key, &value)) {
+            goto done;
+        }
+        key_timeouts[i] = key;
+        value_timeouts[i] = atoi(value);
+        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
+    }
+
+done:
+    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
+    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
+        struct simap s = SIMAP_INITIALIZER(&s);
+
+        /* Convert to simap. */
+        for (int i = 0; i < row->n_timeouts; i++) {
+            simap_put(&s, row->key_timeouts[i], row->value_timeouts[i]);
+        }
+
+        if (simap_equal(&s, &new_tp)) {
+            tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row);
+            simap_destroy(&s);
+            break;
+        }
+        simap_destroy(&s);
+    }
+
+    if (!tp) {
+        tp = ovsrec_ct_timeout_policy_insert(ctx->txn);
+        ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts,
+                                              (const int64_t *)value_timeouts,
+                                              n_tps);
+    }
+
+    for (int i = 0; i < n_tps; i++) {
+        free(policies[i]);
+    }
+    free(policies);
+    simap_destroy(&new_tp);
+    free(key_timeouts);
+    free(value_timeouts);
+    return tp;
+}
+
+static void
+cmd_add_zone_tp(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+    struct ovsrec_ct_timeout_policy *tp;
+    int64_t zone_id;
+
+    const char *dp_name = ctx->argv[1];
+    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
+    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
+    if (!dp) {
+        ctl_fatal("datapath %s does not exist", dp_name);
+    }
+
+    int n_tps = ctx->argc - 3;
+    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
+
+    if (n_tps <= 0) {
+        ctl_fatal("No timeout policy");
+    }
+
+    if (zone && !may_exist) {
+        ctl_fatal("zone id %"PRIu64" already exists", zone_id);
+    }
+
+    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
+    if (zone) {
+        ovsrec_ct_zone_set_timeout_policy(zone, tp);
+    } else {
+        zone = ovsrec_ct_zone_insert(ctx->txn);
+        ovsrec_ct_zone_set_timeout_policy(zone, tp);
+        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
+    }
+}
+
+static void
+cmd_del_zone_tp(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+    int64_t zone_id;
+
+    bool must_exist = !shash_find(&ctx->options, "--if-exists");
+    const char *dp_name = ctx->argv[1];
+    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
+
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
+    if (!dp) {
+        ctl_fatal("datapath %s does not exist", dp_name);
+    }
+
+    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
+    if (must_exist && !zone) {
+        ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
+    }
+
+    if (zone) {
+        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
+    }
+}
+
+static void
+cmd_list_zone_tp(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, ctx->argv[1]);
+    if (!dp) {
+        ctl_fatal("datapath: %s record not found", ctx->argv[1]);
+    }
+
+    for (int i = 0; i < dp->n_ct_zones; i++) {
+        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
+        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
+                      dp->key_ct_zones[i]);
+
+        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
+
+        for (int j = 0; j < tp->n_timeouts; j++) {
+            if (j == tp->n_timeouts - 1) {
+                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
+                          tp->key_timeouts[j], tp->value_timeouts[j]);
+            } else {
+                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
+                          tp->key_timeouts[j], tp->value_timeouts[j]);
+            }
+        }
+    }
+}
+
+static void
+pre_get_zone(struct ctl_context *ctx)
+{
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_datapath_col_ct_zones);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_timeout_policy_col_timeouts);
+}
+
 static void
 cmd_add_br(struct ctl_context *ctx)
 {
@@ -2896,6 +3083,13 @@  static const struct ctl_command_syntax vsctl_commands[] = {
     /* Switch commands. */
     {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL, "", RW},
 
+    /* Zone and CT Timeout Policy commands. */
+    {"add-zone-tp", 2, 18, "", pre_get_zone, cmd_add_zone_tp, NULL,
+     "--may-exist", RW},
+    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
+     "--if-exists", RW},
+    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, NULL, "", RO},
+
     {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
 };