diff mbox series

[ovs-dev,v4,3/6] ovs-vsctl: Add limit to CT zone.

Message ID 20231010141224.638166-4-amusil@redhat.com
State Changes Requested
Delegated to: Ilya Maximets
Headers show
Series Expose CT limit via DB | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ales Musil Oct. 10, 2023, 2:12 p.m. UTC
Add limit to the CT zone DB table with ovs-vsctl
helper methods. The limit has two special values
besides any number, 0 is unlimited and empty limit
is to leave the value untouched in the datapath.

This is preparation step and the value is not yet
propagated to the datapath.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
v4: Rebase on top of current master.
    Address comments from Ilya:
    - Make sure that the NEWS is clear on what has been added.
    - Make the usage of --may-exist and --if-exists more intuitive for the new commands.
    - Some cosmetics.
    Add command and column for default limit.
---
 NEWS                       |   8 ++
 tests/ovs-vsctl.at         |  92 ++++++++++++++++++++
 utilities/ovs-vsctl.8.in   |  45 ++++++++--
 utilities/ovs-vsctl.c      | 171 ++++++++++++++++++++++++++++++++++++-
 vswitchd/vswitch.ovsschema |  14 ++-
 vswitchd/vswitch.xml       |  11 +++
 6 files changed, 331 insertions(+), 10 deletions(-)

Comments

Ilya Maximets Oct. 16, 2023, 7:10 p.m. UTC | #1
On 10/10/23 16:12, Ales Musil wrote:
> Add limit to the CT zone DB table with ovs-vsctl
> helper methods. The limit has two special values
> besides any number, 0 is unlimited and empty limit
> is to leave the value untouched in the datapath.
> 
> This is preparation step and the value is not yet
> propagated to the datapath.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
> v4: Rebase on top of current master.
>     Address comments from Ilya:
>     - Make sure that the NEWS is clear on what has been added.
>     - Make the usage of --may-exist and --if-exists more intuitive for the new commands.
>     - Some cosmetics.
>     Add command and column for default limit.
> ---
>  NEWS                       |   8 ++
>  tests/ovs-vsctl.at         |  92 ++++++++++++++++++++
>  utilities/ovs-vsctl.8.in   |  45 ++++++++--
>  utilities/ovs-vsctl.c      | 171 ++++++++++++++++++++++++++++++++++++-
>  vswitchd/vswitch.ovsschema |  14 ++-
>  vswitchd/vswitch.xml       |  11 +++
>  6 files changed, 331 insertions(+), 10 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index df98e75a0..c0d96b894 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,14 @@ Post-v3.2.0
>     - ovs-dpctl:
>       * Support removal of default CT zone limit via ovs-dpctl, e.g.
>         "ovs-appctl dpctl/ct-del-limits default"
> +   - ovs-vsctl:
> +     * New commands 'add-zone-limit', 'del-zone-limit' and 'list-zone-limit'
> +       to manage the maximum number of connections in conntrack zones via
> +       a new 'limit' column in the 'CT_Zone' database table.
> +     * New command 'set-zone-default-limit' and 'del-zone-default-limit' to
> +       manage the maximum number of connections in conntrack zones that are
> +       not explicitly defined otherwise via new 'ct_zone_default_limit' column
> +       in the 'Datapath' table.

Can we just add a way to specify 'default' instead of 'zone=N'
in the add/del-zone-limit commands?  Two separate commands seem
redundant.

>  
>  
>  v3.2.0 - 17 Aug 2023
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index a368bff6e..0d2fa68fb 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -975,6 +975,67 @@ AT_CHECK(
>    [0], [stdout])
>  AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:10, Timeout Policies: system default
>  ])
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=10])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=1 limit=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 1, Limit: 1
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=1 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 1, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=1])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=10 icmp_first=1 icmp_reply=2])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> +Zone:10, Timeout Policies: system default
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=5])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 5
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-default-limit netdev limit=10])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Default limit: 10
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])])
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> +Zone: 10, Limit: 5
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-default-limit netdev])])
> +
>  
>  AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])], [0], [stdout])
>  AT_CHECK([RUN_OVS_VSCTL([list-dp-cap system])], [0], [recirc=true
> @@ -1123,6 +1184,37 @@ AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
>  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([add-zone-limit netdevxx zone=5 limit=1])],
> +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=88888 limit=1])],
> +  [1], [], [ovs-vsctl: zone_id (88888) out of range
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=-1])],
> +  [1], [], [ovs-vsctl: limit (-1) out of range
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])],
> +  [1], [], [ovs-vsctl: zone_id 10 does not have limit
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=1])])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=2])],
> +  [1], [], [ovs-vsctl: zone_id 5 already has limit
> +])
> +
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdevxx limit=1])],
> +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=-1])],
> +  [1], [], [ovs-vsctl: limit (-1) out of range
> +])
> +AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])],
> +  [1], [], [ovs-vsctl: datapath netdev does not have limit
> +])
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=1])])
> +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=2])],
> +  [1], [], [ovs-vsctl: datapath netdev already has limit
> +])
> +
>  AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])], [0], [stdout])
>  AT_CHECK([RUN_OVS_VSCTL([list-dp-cap nosystem])],
>    [1], [], [ovs-vsctl: datapath "nosystem" record not found
> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> index 9e319aa1c..f8a04d707 100644
> --- a/utilities/ovs-vsctl.8.in
> +++ b/utilities/ovs-vsctl.8.in
> @@ -354,7 +354,7 @@ 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.
> +These commands query and modify datapath CT zones, Timeout Policies and Limits.
>  .
>  .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
> @@ -365,20 +365,55 @@ 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 already exists.
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIpolicies\fR for

'a policies' doesn't sound right here.  Should be 'a policy'.

> +\fIzone_id\fR that already exists is an error.

This is not true.  At least, shouldn't be.  An error will be if
there is already timeout policy configured for a specified zone
id, not if zone id exists.  'zone_id' exists is also a strange
way to say that now that existance of the zone record is not
tied to existance of timeout policy in it.

> With \fB\-\-may\-exist\fR,
> +this command updates the \fIpolicies\fR if \fIzone_id\fR already exists.

Is it a behavior change?  The 'add' command did nothing before,
now it updates?

The 'add' is not 'set', so it shoudln't update the exisitng value.

>  .
>  .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
> +Without \fB\-\-if\-exists\fR, attempting to delete a policies for zone that

'a policy', zone can only have one.

>  does not exist is an error.

If the zone doesn't have a timeout policy it will be an error as well.

> With \fB\-\-if\-exists\fR, attempting to
>  delete a zone that does not exist has no effect.

We're not deleting the zone anymore.

All the same things applies to the new commands below.

>  .
>  .IP "\fBlist\-zone\-tp \fIdatapath\fR"
>  Prints the timeout policies of all zones in \fIdatapath\fR.
>  .
> +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-limit \fIdatapath \fBzone=\fIzone_id \fBlimit=\fIzone_limit"
> +Sets a conntrack zone limit with \fIzone_id\fR in
> +\fIdatapath\fR.  The \fIlimit\fR with value \fB0\fR means unlimited.
> +.IP
> +Without \fB\-\-may\-exist\fR, attempting to add a \fIlimit\fR for
> +\fIzone_id\fR that already exists is an error.  With \fB\-\-may\-exist\fR,
> +this command updates the \fIlimit\fR if \fIzone_id\fR already exists.
> +.
> +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR"
> +Delete the limit associated with \fIzone_id\fR from \fIdatapath\fR.
> +.IP
> +Without \fB\-\-if\-exists\fR, attempting to delete a limit for 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\-limit \fIdatapath\fR"
> +Prints the limits of all zones in \fIdatapath\fR.
> +.
> +.IP "[\fB\-\-may\-exist\fR] \fBset\-zone\-default\-limit \fIdatapath \fBlimit=\fIdefault_limit"
> +Sets a conntrack default zone limit in \fIdatapath\fR.
> +The \fIlimit\fR with value \fB0\fR means unlimited.
> +.IP
> +Without \fB\-\-may\-exist\fR, attempting to add a default limit for
> +datapath that already has default limit is an error.
> +With \fB\-\-may\-exist\fR, this command updates the default limit if
> +it is already set for specified datapath.
> +.
> +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-default\-limit \fIdatapath"
> +Delete the default limit associated with \fIdatapath\fR.
> +.IP
> +Without \fB\-\-if\-exists\fR, attempting to delete a default limit for
> +datapath that does not have default limit is an error.
> +With \fB\-\-if\-exists\fR, attempting to delete a default limit that is not
> +set has no effect.
> +.
>  .SS "Datapath Capabilities Command"
>  The command query datapath capabilities.
>  .
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 5e549df00..7e01deaec 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1302,7 +1302,7 @@ cmd_add_zone_tp(struct ctl_context *ctx)
>          ctl_fatal("No timeout policy");
>      }
>  
> -    if (zone && !may_exist) {
> +    if (zone && zone->timeout_policy && !may_exist) {
>          ctl_fatal("zone id %"PRIu64" already exists", zone_id);

This error message is not correct anymore.  Smae for logs in other
commands below.

I didn't read into implementations below too much as they will need
changes for eroror messages and 'add' vs 'set' and removal of the
separate commands for default limits.  But there are a few more
comments for documentation below.

>      }
>  
> @@ -1332,11 +1332,20 @@ cmd_del_zone_tp(struct ctl_context *ctx)
>      }
>  
>      struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> -    if (must_exist && !zone) {
> +    if (must_exist && !(zone && zone->timeout_policy)) {
>          ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
>      }
>  
> -    if (zone) {
> +    if (!zone) {
> +        return;
> +    }
> +
> +    if (zone->limit) {
> +        if (zone->timeout_policy) {
> +            ovsrec_ct_timeout_policy_delete(zone->timeout_policy);
> +        }
> +        ovsrec_ct_zone_set_timeout_policy(zone, NULL);
> +    } else {
>          ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
>      }
>  }
> @@ -1371,12 +1380,156 @@ cmd_list_zone_tp(struct ctl_context *ctx)
>      }
>  }
>  
> +static void
> +cmd_add_zone_limit(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    int64_t zone_id = -1;
> +    int64_t limit = -1;
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +    const char *dp_name = ctx->argv[1];
> +
> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> +    ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit);
> +
> +    if (zone_id < 0 || zone_id > UINT16_MAX) {
> +        ctl_fatal("zone_id (%"PRIi64") out of range", zone_id);
> +    }
> +
> +    if (limit < 0 || limit > UINT32_MAX) {
> +        ctl_fatal("limit (%"PRIi64") out of range", limit);
> +    }
> +
> +    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 (zone && zone->limit && !may_exist) {
> +        ctl_fatal("zone_id %"PRIi64" already has limit", zone_id);
> +    }
> +
> +    if (!zone) {
> +        zone = ovsrec_ct_zone_insert(ctx->txn);
> +        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> +    }
> +
> +    ovsrec_ct_zone_set_limit(zone, &limit, 1);
> +}
> +
> +static void
> +cmd_del_zone_limit(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 && zone->limit)) {
> +        ctl_fatal("zone_id %"PRIi64" does not have limit", zone_id);
> +    }
> +
> +    if (!zone) {
> +        return;
> +    }
> +
> +    if (zone->timeout_policy) {
> +        ovsrec_ct_zone_set_limit(zone, NULL, 0);
> +    } else {
> +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> +    }
> +}
> +
> +static void
> +cmd_list_zone_limit(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]);
> +    }
> +
> +    if (dp->ct_zone_default_limit) {
> +        ds_put_format(&ctx->output, "Default limit: %"PRIu64"\n",
> +                      *dp->ct_zone_default_limit);
> +    }
> +
> +    for (int i = 0; i < dp->n_ct_zones; i++) {
> +        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
> +        if (zone->limit) {
> +            ds_put_format(&ctx->output, "Zone: %"PRIu64", Limit: %"PRIu64"\n",
> +                          dp->key_ct_zones[i], *zone->limit);
> +        }
> +    }
> +}
> +
> +static void
> +cmd_set_zone_default_limit(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +    int64_t limit = -1;
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +    const char *dp_name = ctx->argv[1];
> +
> +    ovs_scan(ctx->argv[2], "limit=%"SCNi64, &limit);
> +
> +    if (limit < 0 || limit > UINT32_MAX) {
> +        ctl_fatal("limit (%"PRIi64") out of range", limit);
> +    }
> +
> +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath %s does not exist", dp_name);
> +    }
> +
> +    if (dp->ct_zone_default_limit && !may_exist) {
> +        ctl_fatal("datapath %s already has limit", dp_name);
> +    }
> +
> +    ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1);
> +}
> +
> +static void
> +cmd_del_zone_default_limit(struct ctl_context *ctx)
> +{
> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> +
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    const char *dp_name = ctx->argv[1];
> +
> +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> +    if (!dp) {
> +        ctl_fatal("datapath %s does not exist", dp_name);
> +    }
> +
> +    if (must_exist && !dp->ct_zone_default_limit) {
> +        ctl_fatal("datapath %s does not have limit", dp_name);
> +    }
> +
> +    ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0);
> +}
> +
>  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_datapath_col_ct_zone_default_limit);
>      ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
> +    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_limit);
>      ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_timeout_policy_col_timeouts);
>  }
>  
> @@ -3159,6 +3312,18 @@ static const struct ctl_command_syntax vsctl_commands[] = {
>      /* Datapath capabilities. */
>      {"list-dp-cap", 1, 1, "", pre_get_dp_cap, cmd_list_dp_cap, NULL, "", RO},
>  
> +    /* CT zone limit. */
> +    {"add-zone-limit", 3, 3, "", pre_get_zone, cmd_add_zone_limit, NULL,
> +     "--may-exist", RW},
> +    {"del-zone-limit", 2, 2, "", pre_get_zone, cmd_del_zone_limit, NULL,
> +     "--if-exists", RW},
> +    {"list-zone-limit", 1, 1, "", pre_get_zone, cmd_list_zone_limit, NULL,
> +     "", RO},
> +    {"set-zone-default-limit", 2, 2, "", pre_get_zone,
> +     cmd_set_zone_default_limit, NULL, "--may-exist", RW},
> +    {"del-zone-default-limit", 1, 1, "", pre_get_zone,
> +     cmd_del_zone_default_limit, NULL, "--if-exists", RW},
> +
>      {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
>  };
>  
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 2d395ff95..e2d5e2e85 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -1,6 +1,6 @@
>  {"name": "Open_vSwitch",
> - "version": "8.4.0",
> - "cksum": "2738838700 27127",
> + "version": "8.5.0",
> + "cksum": "4040946650 27557",
>   "tables": {
>     "Open_vSwitch": {
>       "columns": {
> @@ -670,6 +670,11 @@
>         "capabilities": {
>           "type": {"key": "string", "value": "string",
>                    "min": 0, "max": "unlimited"}},
> +       "ct_zone_default_limit": {
> +          "type": { "key": {"type": "integer",
> +                            "minInteger": 0,
> +                            "maxInteger": 4294967295},
> +                    "min": 0, "max": 1}},
>         "external_ids": {
>           "type": {"key": "string", "value": "string",
>                    "min": 0, "max": "unlimited"}}}},
> @@ -679,6 +684,11 @@
>           "type": {"key": {"type": "uuid",
>                            "refTable": "CT_Timeout_Policy"},
>                    "min": 0, "max": 1}},
> +       "limit": {
> +          "type": { "key": {"type": "integer",
> +                            "minInteger": 0,
> +                            "maxInteger": 4294967295},
> +                    "min": 0, "max": 1}},
>         "external_ids": {
>           "type": {"key": "string", "value": "string",
>                    "min": 0, "max": "unlimited"}}}},
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..84b514e01 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -6417,6 +6417,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>        </column>
>      </group>
>  
> +    <column name="ct_zone_default_limit">
> +        Default connection tracking zone limit that is applied to all zones
> +        that didn't specify the limit explicitly.

Please, use the <ref table="" column="" /> to refererence the exact place
where the per-zone value is specified.

> +        If the limit is unspecified
> +        the datapath configuration is left intact.

I'm not sure this is correct thing to say.  We should mention that the
datapath configuration is left intact only if both the default and the
per-zone limit are not specified.

> +        The value 0 means unlimited.
> +    </column>
> +
>      <group title="Common Columns">
>        The overall purpose of these columns is described under <code>Common
>        Columns</code> at the beginning of this document.
> @@ -6433,6 +6439,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>        is not specified, it defaults to the timeout policy in the system.
>      </column>
>  
> +    <column name="limit">
> +      Connection tracking limit for this zone. If the limit is unspecified
> +      the datapath configuration is left intact.

The same here.  We may just say that if not specified than the defualt
value will be used referencing the table and the column where default
value is stored.

> +      The value 0 means unlimited.
> +    </column>
> +
>      <group title="Common Columns">
>        The overall purpose of these columns is described under <code>Common
>        Columns</code> at the beginning of this document.
Ales Musil Oct. 18, 2023, 7:56 a.m. UTC | #2
On Mon, Oct 16, 2023 at 9:09 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 10/10/23 16:12, Ales Musil wrote:
> > Add limit to the CT zone DB table with ovs-vsctl
> > helper methods. The limit has two special values
> > besides any number, 0 is unlimited and empty limit
> > is to leave the value untouched in the datapath.
> >
> > This is preparation step and the value is not yet
> > propagated to the datapath.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
> > v4: Rebase on top of current master.
> >     Address comments from Ilya:
> >     - Make sure that the NEWS is clear on what has been added.
> >     - Make the usage of --may-exist and --if-exists more intuitive for
> the new commands.
> >     - Some cosmetics.
> >     Add command and column for default limit.
> > ---
> >  NEWS                       |   8 ++
> >  tests/ovs-vsctl.at         |  92 ++++++++++++++++++++
> >  utilities/ovs-vsctl.8.in   |  45 ++++++++--
> >  utilities/ovs-vsctl.c      | 171 ++++++++++++++++++++++++++++++++++++-
> >  vswitchd/vswitch.ovsschema |  14 ++-
> >  vswitchd/vswitch.xml       |  11 +++
> >  6 files changed, 331 insertions(+), 10 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index df98e75a0..c0d96b894 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -9,6 +9,14 @@ Post-v3.2.0
> >     - ovs-dpctl:
> >       * Support removal of default CT zone limit via ovs-dpctl, e.g.
> >         "ovs-appctl dpctl/ct-del-limits default"
> > +   - ovs-vsctl:
> > +     * New commands 'add-zone-limit', 'del-zone-limit' and
> 'list-zone-limit'
> > +       to manage the maximum number of connections in conntrack zones
> via
> > +       a new 'limit' column in the 'CT_Zone' database table.
> > +     * New command 'set-zone-default-limit' and
> 'del-zone-default-limit' to
> > +       manage the maximum number of connections in conntrack zones that
> are
> > +       not explicitly defined otherwise via new 'ct_zone_default_limit'
> column
> > +       in the 'Datapath' table.
>
> Can we just add a way to specify 'default' instead of 'zone=N'
> in the add/del-zone-limit commands?  Two separate commands seem
> redundant.
>
> >
> >
> >  v3.2.0 - 17 Aug 2023
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index a368bff6e..0d2fa68fb 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> > @@ -975,6 +975,67 @@ AT_CHECK(
> >    [0], [stdout])
> >  AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:10, Timeout
> Policies: system default
> >  ])
> > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=10])])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=1 limit=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Zone: 1, Limit: 1
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=1
> limit=5])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Zone: 1, Limit: 5
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Zone: 10, Limit: 5
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=10 icmp_first=1
> icmp_reply=2])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> > +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> > +Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Zone: 10, Limit: 5
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
> > +Zone:10, Timeout Policies: system default
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=5])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Default limit: 5
> > +Zone: 10, Limit: 5
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-default-limit netdev
> limit=10])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Default limit: 10
> > +Zone: 10, Limit: 5
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])])
> > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
> > +Zone: 10, Limit: 5
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-default-limit netdev])])
> > +
> >
> >  AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0
> 'capabilities={recirc=true}' -- set Open_vSwitch .
> datapaths:"system"=@m])], [0], [stdout])
> >  AT_CHECK([RUN_OVS_VSCTL([list-dp-cap system])], [0], [recirc=true
> > @@ -1123,6 +1184,37 @@ AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev
> zone=11])],
> >  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([add-zone-limit netdevxx zone=5 limit=1])],
> > +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=88888 limit=1])],
> > +  [1], [], [ovs-vsctl: zone_id (88888) out of range
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=-1])],
> > +  [1], [], [ovs-vsctl: limit (-1) out of range
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])],
> > +  [1], [], [ovs-vsctl: zone_id 10 does not have limit
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=2])],
> > +  [1], [], [ovs-vsctl: zone_id 5 already has limit
> > +])
> > +
> > +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdevxx limit=1])],
> > +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=-1])],
> > +  [1], [], [ovs-vsctl: limit (-1) out of range
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])],
> > +  [1], [], [ovs-vsctl: datapath netdev does not have limit
> > +])
> > +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=1])])
> > +AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=2])],
> > +  [1], [], [ovs-vsctl: datapath netdev already has limit
> > +])
> > +
> >  AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0
> 'capabilities={recirc=true}' -- set Open_vSwitch .
> datapaths:"system"=@m])], [0], [stdout])
> >  AT_CHECK([RUN_OVS_VSCTL([list-dp-cap nosystem])],
> >    [1], [], [ovs-vsctl: datapath "nosystem" record not found
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 9e319aa1c..f8a04d707 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -354,7 +354,7 @@ 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.
> > +These commands query and modify datapath CT zones, Timeout Policies and
> Limits.
> >  .
> >  .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
> > @@ -365,20 +365,55 @@ 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 already exists.
> > +Without \fB\-\-may\-exist\fR, attempting to add a \fIpolicies\fR for
>
> 'a policies' doesn't sound right here.  Should be 'a policy'.
>
> > +\fIzone_id\fR that already exists is an error.
>
> This is not true.  At least, shouldn't be.  An error will be if
> there is already timeout policy configured for a specified zone
> id, not if zone id exists.  'zone_id' exists is also a strange
> way to say that now that existance of the zone record is not
> tied to existance of timeout policy in it.
>
> > With \fB\-\-may\-exist\fR,
> > +this command updates the \fIpolicies\fR if \fIzone_id\fR already exists.
>
> Is it a behavior change?  The 'add' command did nothing before,
> now it updates?
>
> The 'add' is not 'set', so it shoudln't update the exisitng value.
>
> >  .
> >  .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
> > +Without \fB\-\-if\-exists\fR, attempting to delete a policies for zone
> that
>
> 'a policy', zone can only have one.
>
> >  does not exist is an error.
>
> If the zone doesn't have a timeout policy it will be an error as well.
>
> > With \fB\-\-if\-exists\fR, attempting to
> >  delete a zone that does not exist has no effect.
>
> We're not deleting the zone anymore.
>
> All the same things applies to the new commands below.
>
> >  .
> >  .IP "\fBlist\-zone\-tp \fIdatapath\fR"
> >  Prints the timeout policies of all zones in \fIdatapath\fR.
> >  .
> > +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-limit \fIdatapath
> \fBzone=\fIzone_id \fBlimit=\fIzone_limit"
> > +Sets a conntrack zone limit with \fIzone_id\fR in
> > +\fIdatapath\fR.  The \fIlimit\fR with value \fB0\fR means unlimited.
> > +.IP
> > +Without \fB\-\-may\-exist\fR, attempting to add a \fIlimit\fR for
> > +\fIzone_id\fR that already exists is an error.  With
> \fB\-\-may\-exist\fR,
> > +this command updates the \fIlimit\fR if \fIzone_id\fR already exists.
> > +.
> > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath
> \fBzone=\fIzone_id\fR"
> > +Delete the limit associated with \fIzone_id\fR from \fIdatapath\fR.
> > +.IP
> > +Without \fB\-\-if\-exists\fR, attempting to delete a limit for 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\-limit \fIdatapath\fR"
> > +Prints the limits of all zones in \fIdatapath\fR.
> > +.
> > +.IP "[\fB\-\-may\-exist\fR] \fBset\-zone\-default\-limit \fIdatapath
> \fBlimit=\fIdefault_limit"
> > +Sets a conntrack default zone limit in \fIdatapath\fR.
> > +The \fIlimit\fR with value \fB0\fR means unlimited.
> > +.IP
> > +Without \fB\-\-may\-exist\fR, attempting to add a default limit for
> > +datapath that already has default limit is an error.
> > +With \fB\-\-may\-exist\fR, this command updates the default limit if
> > +it is already set for specified datapath.
> > +.
> > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-default\-limit \fIdatapath"
> > +Delete the default limit associated with \fIdatapath\fR.
> > +.IP
> > +Without \fB\-\-if\-exists\fR, attempting to delete a default limit for
> > +datapath that does not have default limit is an error.
> > +With \fB\-\-if\-exists\fR, attempting to delete a default limit that is
> not
> > +set has no effect.
> > +.
> >  .SS "Datapath Capabilities Command"
> >  The command query datapath capabilities.
> >  .
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 5e549df00..7e01deaec 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > @@ -1302,7 +1302,7 @@ cmd_add_zone_tp(struct ctl_context *ctx)
> >          ctl_fatal("No timeout policy");
> >      }
> >
> > -    if (zone && !may_exist) {
> > +    if (zone && zone->timeout_policy && !may_exist) {
> >          ctl_fatal("zone id %"PRIu64" already exists", zone_id);
>
> This error message is not correct anymore.  Smae for logs in other
> commands below.
>
> I didn't read into implementations below too much as they will need
> changes for eroror messages and 'add' vs 'set' and removal of the
> separate commands for default limits.  But there are a few more
> comments for documentation below.
>
> >      }
> >
> > @@ -1332,11 +1332,20 @@ cmd_del_zone_tp(struct ctl_context *ctx)
> >      }
> >
> >      struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > -    if (must_exist && !zone) {
> > +    if (must_exist && !(zone && zone->timeout_policy)) {
> >          ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
> >      }
> >
> > -    if (zone) {
> > +    if (!zone) {
> > +        return;
> > +    }
> > +
> > +    if (zone->limit) {
> > +        if (zone->timeout_policy) {
> > +            ovsrec_ct_timeout_policy_delete(zone->timeout_policy);
> > +        }
> > +        ovsrec_ct_zone_set_timeout_policy(zone, NULL);
> > +    } else {
> >          ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> >      }
> >  }
> > @@ -1371,12 +1380,156 @@ cmd_list_zone_tp(struct ctl_context *ctx)
> >      }
> >  }
> >
> > +static void
> > +cmd_add_zone_limit(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    int64_t zone_id = -1;
> > +    int64_t limit = -1;
> > +
> > +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +    const char *dp_name = ctx->argv[1];
> > +
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +    ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit);
> > +
> > +    if (zone_id < 0 || zone_id > UINT16_MAX) {
> > +        ctl_fatal("zone_id (%"PRIi64") out of range", zone_id);
> > +    }
> > +
> > +    if (limit < 0 || limit > UINT32_MAX) {
> > +        ctl_fatal("limit (%"PRIi64") out of range", limit);
> > +    }
> > +
> > +    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 (zone && zone->limit && !may_exist) {
> > +        ctl_fatal("zone_id %"PRIi64" already has limit", zone_id);
> > +    }
> > +
> > +    if (!zone) {
> > +        zone = ovsrec_ct_zone_insert(ctx->txn);
> > +        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> > +    }
> > +
> > +    ovsrec_ct_zone_set_limit(zone, &limit, 1);
> > +}
> > +
> > +static void
> > +cmd_del_zone_limit(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 && zone->limit)) {
> > +        ctl_fatal("zone_id %"PRIi64" does not have limit", zone_id);
> > +    }
> > +
> > +    if (!zone) {
> > +        return;
> > +    }
> > +
> > +    if (zone->timeout_policy) {
> > +        ovsrec_ct_zone_set_limit(zone, NULL, 0);
> > +    } else {
> > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > +    }
> > +}
> > +
> > +static void
> > +cmd_list_zone_limit(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]);
> > +    }
> > +
> > +    if (dp->ct_zone_default_limit) {
> > +        ds_put_format(&ctx->output, "Default limit: %"PRIu64"\n",
> > +                      *dp->ct_zone_default_limit);
> > +    }
> > +
> > +    for (int i = 0; i < dp->n_ct_zones; i++) {
> > +        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
> > +        if (zone->limit) {
> > +            ds_put_format(&ctx->output, "Zone: %"PRIu64", Limit:
> %"PRIu64"\n",
> > +                          dp->key_ct_zones[i], *zone->limit);
> > +        }
> > +    }
> > +}
> > +
> > +static void
> > +cmd_set_zone_default_limit(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    int64_t limit = -1;
> > +
> > +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +    const char *dp_name = ctx->argv[1];
> > +
> > +    ovs_scan(ctx->argv[2], "limit=%"SCNi64, &limit);
> > +
> > +    if (limit < 0 || limit > UINT32_MAX) {
> > +        ctl_fatal("limit (%"PRIi64") out of range", limit);
> > +    }
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath %s does not exist", dp_name);
> > +    }
> > +
> > +    if (dp->ct_zone_default_limit && !may_exist) {
> > +        ctl_fatal("datapath %s already has limit", dp_name);
> > +    }
> > +
> > +    ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1);
> > +}
> > +
> > +static void
> > +cmd_del_zone_default_limit(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +
> > +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > +    const char *dp_name = ctx->argv[1];
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath %s does not exist", dp_name);
> > +    }
> > +
> > +    if (must_exist && !dp->ct_zone_default_limit) {
> > +        ctl_fatal("datapath %s does not have limit", dp_name);
> > +    }
> > +
> > +    ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0);
> > +}
> > +
> >  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_datapath_col_ct_zone_default_limit);
> >      ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
> > +    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_limit);
> >      ovsdb_idl_add_column(ctx->idl,
> &ovsrec_ct_timeout_policy_col_timeouts);
> >  }
> >
> > @@ -3159,6 +3312,18 @@ static const struct ctl_command_syntax
> vsctl_commands[] = {
> >      /* Datapath capabilities. */
> >      {"list-dp-cap", 1, 1, "", pre_get_dp_cap, cmd_list_dp_cap, NULL,
> "", RO},
> >
> > +    /* CT zone limit. */
> > +    {"add-zone-limit", 3, 3, "", pre_get_zone, cmd_add_zone_limit, NULL,
> > +     "--may-exist", RW},
> > +    {"del-zone-limit", 2, 2, "", pre_get_zone, cmd_del_zone_limit, NULL,
> > +     "--if-exists", RW},
> > +    {"list-zone-limit", 1, 1, "", pre_get_zone, cmd_list_zone_limit,
> NULL,
> > +     "", RO},
> > +    {"set-zone-default-limit", 2, 2, "", pre_get_zone,
> > +     cmd_set_zone_default_limit, NULL, "--may-exist", RW},
> > +    {"del-zone-default-limit", 1, 1, "", pre_get_zone,
> > +     cmd_del_zone_default_limit, NULL, "--if-exists", RW},
> > +
> >      {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
> >  };
> >
> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index 2d395ff95..e2d5e2e85 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> > @@ -1,6 +1,6 @@
> >  {"name": "Open_vSwitch",
> > - "version": "8.4.0",
> > - "cksum": "2738838700 27127",
> > + "version": "8.5.0",
> > + "cksum": "4040946650 27557",
> >   "tables": {
> >     "Open_vSwitch": {
> >       "columns": {
> > @@ -670,6 +670,11 @@
> >         "capabilities": {
> >           "type": {"key": "string", "value": "string",
> >                    "min": 0, "max": "unlimited"}},
> > +       "ct_zone_default_limit": {
> > +          "type": { "key": {"type": "integer",
> > +                            "minInteger": 0,
> > +                            "maxInteger": 4294967295},
> > +                    "min": 0, "max": 1}},
> >         "external_ids": {
> >           "type": {"key": "string", "value": "string",
> >                    "min": 0, "max": "unlimited"}}}},
> > @@ -679,6 +684,11 @@
> >           "type": {"key": {"type": "uuid",
> >                            "refTable": "CT_Timeout_Policy"},
> >                    "min": 0, "max": 1}},
> > +       "limit": {
> > +          "type": { "key": {"type": "integer",
> > +                            "minInteger": 0,
> > +                            "maxInteger": 4294967295},
> > +                    "min": 0, "max": 1}},
> >         "external_ids": {
> >           "type": {"key": "string", "value": "string",
> >                    "min": 0, "max": "unlimited"}}}},
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index cfcde34ff..84b514e01 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -6417,6 +6417,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> >        </column>
> >      </group>
> >
> > +    <column name="ct_zone_default_limit">
> > +        Default connection tracking zone limit that is applied to all
> zones
> > +        that didn't specify the limit explicitly.
>
> Please, use the <ref table="" column="" /> to refererence the exact place
> where the per-zone value is specified.
>
> > +        If the limit is unspecified
> > +        the datapath configuration is left intact.
>
> I'm not sure this is correct thing to say.  We should mention that the
> datapath configuration is left intact only if both the default and the
> per-zone limit are not specified.
>
> > +        The value 0 means unlimited.
> > +    </column>
> > +
> >      <group title="Common Columns">
> >        The overall purpose of these columns is described under
> <code>Common
> >        Columns</code> at the beginning of this document.
> > @@ -6433,6 +6439,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> >        is not specified, it defaults to the timeout policy in the system.
> >      </column>
> >
> > +    <column name="limit">
> > +      Connection tracking limit for this zone. If the limit is
> unspecified
> > +      the datapath configuration is left intact.
>
> The same here.  We may just say that if not specified than the defualt
> value will be used referencing the table and the column where default
> value is stored.
>
> > +      The value 0 means unlimited.
> > +    </column>
> > +
> >      <group title="Common Columns">
> >        The overall purpose of these columns is described under
> <code>Common
> >        Columns</code> at the beginning of this document.
>
>
Thank you for the review, all should be addressed in v5.

Thanks,
Ales
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index df98e75a0..c0d96b894 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,14 @@  Post-v3.2.0
    - ovs-dpctl:
      * Support removal of default CT zone limit via ovs-dpctl, e.g.
        "ovs-appctl dpctl/ct-del-limits default"
+   - ovs-vsctl:
+     * New commands 'add-zone-limit', 'del-zone-limit' and 'list-zone-limit'
+       to manage the maximum number of connections in conntrack zones via
+       a new 'limit' column in the 'CT_Zone' database table.
+     * New command 'set-zone-default-limit' and 'del-zone-default-limit' to
+       manage the maximum number of connections in conntrack zones that are
+       not explicitly defined otherwise via new 'ct_zone_default_limit' column
+       in the 'Datapath' table.
 
 
 v3.2.0 - 17 Aug 2023
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a368bff6e..0d2fa68fb 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -975,6 +975,67 @@  AT_CHECK(
   [0], [stdout])
 AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:10, Timeout Policies: system default
 ])
+AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=10])])
+
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=1 limit=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 1, Limit: 1
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-limit netdev zone=1 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 1, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=1])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 10, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=10 icmp_first=1 icmp_reply=2])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
+])
+
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: icmp_first=1 icmp_reply=2
+])
+
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=10 limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=10])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 10, Limit: 5
+])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [dnl
+Zone:10, Timeout Policies: system default
+])
+
+AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=5])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Default limit: 5
+Zone: 10, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--may-exist set-zone-default-limit netdev limit=10])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Default limit: 10
+Zone: 10, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])])
+AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl
+Zone: 10, Limit: 5
+])
+
+AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-default-limit netdev])])
+
 
 AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])], [0], [stdout])
 AT_CHECK([RUN_OVS_VSCTL([list-dp-cap system])], [0], [recirc=true
@@ -1123,6 +1184,37 @@  AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
 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([add-zone-limit netdevxx zone=5 limit=1])],
+  [1], [], [ovs-vsctl: datapath netdevxx does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=88888 limit=1])],
+  [1], [], [ovs-vsctl: zone_id (88888) out of range
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=-1])],
+  [1], [], [ovs-vsctl: limit (-1) out of range
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev zone=10])],
+  [1], [], [ovs-vsctl: zone_id 10 does not have limit
+])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=1])])
+AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=2])],
+  [1], [], [ovs-vsctl: zone_id 5 already has limit
+])
+
+AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdevxx limit=1])],
+  [1], [], [ovs-vsctl: datapath netdevxx does not exist
+])
+AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=-1])],
+  [1], [], [ovs-vsctl: limit (-1) out of range
+])
+AT_CHECK([RUN_OVS_VSCTL([del-zone-default-limit netdev])],
+  [1], [], [ovs-vsctl: datapath netdev does not have limit
+])
+AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=1])])
+AT_CHECK([RUN_OVS_VSCTL([set-zone-default-limit netdev limit=2])],
+  [1], [], [ovs-vsctl: datapath netdev already has limit
+])
+
 AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 'capabilities={recirc=true}' -- set Open_vSwitch . datapaths:"system"=@m])], [0], [stdout])
 AT_CHECK([RUN_OVS_VSCTL([list-dp-cap nosystem])],
   [1], [], [ovs-vsctl: datapath "nosystem" record not found
diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
index 9e319aa1c..f8a04d707 100644
--- a/utilities/ovs-vsctl.8.in
+++ b/utilities/ovs-vsctl.8.in
@@ -354,7 +354,7 @@  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.
+These commands query and modify datapath CT zones, Timeout Policies and Limits.
 .
 .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
@@ -365,20 +365,55 @@  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 already exists.
+Without \fB\-\-may\-exist\fR, attempting to add a \fIpolicies\fR for
+\fIzone_id\fR that already exists is an error.  With \fB\-\-may\-exist\fR,
+this command updates the \fIpolicies\fR 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.
 .IP
-Without \fB\-\-if\-exists\fR, attempting to delete a zone that
+Without \fB\-\-if\-exists\fR, attempting to delete a policies for 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.
 .
+.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-limit \fIdatapath \fBzone=\fIzone_id \fBlimit=\fIzone_limit"
+Sets a conntrack zone limit with \fIzone_id\fR in
+\fIdatapath\fR.  The \fIlimit\fR with value \fB0\fR means unlimited.
+.IP
+Without \fB\-\-may\-exist\fR, attempting to add a \fIlimit\fR for
+\fIzone_id\fR that already exists is an error.  With \fB\-\-may\-exist\fR,
+this command updates the \fIlimit\fR if \fIzone_id\fR already exists.
+.
+.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR"
+Delete the limit associated with \fIzone_id\fR from \fIdatapath\fR.
+.IP
+Without \fB\-\-if\-exists\fR, attempting to delete a limit for 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\-limit \fIdatapath\fR"
+Prints the limits of all zones in \fIdatapath\fR.
+.
+.IP "[\fB\-\-may\-exist\fR] \fBset\-zone\-default\-limit \fIdatapath \fBlimit=\fIdefault_limit"
+Sets a conntrack default zone limit in \fIdatapath\fR.
+The \fIlimit\fR with value \fB0\fR means unlimited.
+.IP
+Without \fB\-\-may\-exist\fR, attempting to add a default limit for
+datapath that already has default limit is an error.
+With \fB\-\-may\-exist\fR, this command updates the default limit if
+it is already set for specified datapath.
+.
+.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-default\-limit \fIdatapath"
+Delete the default limit associated with \fIdatapath\fR.
+.IP
+Without \fB\-\-if\-exists\fR, attempting to delete a default limit for
+datapath that does not have default limit is an error.
+With \fB\-\-if\-exists\fR, attempting to delete a default limit that is not
+set has no effect.
+.
 .SS "Datapath Capabilities Command"
 The command query datapath capabilities.
 .
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 5e549df00..7e01deaec 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1302,7 +1302,7 @@  cmd_add_zone_tp(struct ctl_context *ctx)
         ctl_fatal("No timeout policy");
     }
 
-    if (zone && !may_exist) {
+    if (zone && zone->timeout_policy && !may_exist) {
         ctl_fatal("zone id %"PRIu64" already exists", zone_id);
     }
 
@@ -1332,11 +1332,20 @@  cmd_del_zone_tp(struct ctl_context *ctx)
     }
 
     struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
-    if (must_exist && !zone) {
+    if (must_exist && !(zone && zone->timeout_policy)) {
         ctl_fatal("zone id %"PRIu64" does not exist", zone_id);
     }
 
-    if (zone) {
+    if (!zone) {
+        return;
+    }
+
+    if (zone->limit) {
+        if (zone->timeout_policy) {
+            ovsrec_ct_timeout_policy_delete(zone->timeout_policy);
+        }
+        ovsrec_ct_zone_set_timeout_policy(zone, NULL);
+    } else {
         ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
     }
 }
@@ -1371,12 +1380,156 @@  cmd_list_zone_tp(struct ctl_context *ctx)
     }
 }
 
+static void
+cmd_add_zone_limit(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+    int64_t zone_id = -1;
+    int64_t limit = -1;
+
+    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+    const char *dp_name = ctx->argv[1];
+
+    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
+    ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit);
+
+    if (zone_id < 0 || zone_id > UINT16_MAX) {
+        ctl_fatal("zone_id (%"PRIi64") out of range", zone_id);
+    }
+
+    if (limit < 0 || limit > UINT32_MAX) {
+        ctl_fatal("limit (%"PRIi64") out of range", limit);
+    }
+
+    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 (zone && zone->limit && !may_exist) {
+        ctl_fatal("zone_id %"PRIi64" already has limit", zone_id);
+    }
+
+    if (!zone) {
+        zone = ovsrec_ct_zone_insert(ctx->txn);
+        ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
+    }
+
+    ovsrec_ct_zone_set_limit(zone, &limit, 1);
+}
+
+static void
+cmd_del_zone_limit(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 && zone->limit)) {
+        ctl_fatal("zone_id %"PRIi64" does not have limit", zone_id);
+    }
+
+    if (!zone) {
+        return;
+    }
+
+    if (zone->timeout_policy) {
+        ovsrec_ct_zone_set_limit(zone, NULL, 0);
+    } else {
+        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
+    }
+}
+
+static void
+cmd_list_zone_limit(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]);
+    }
+
+    if (dp->ct_zone_default_limit) {
+        ds_put_format(&ctx->output, "Default limit: %"PRIu64"\n",
+                      *dp->ct_zone_default_limit);
+    }
+
+    for (int i = 0; i < dp->n_ct_zones; i++) {
+        struct ovsrec_ct_zone *zone = dp->value_ct_zones[i];
+        if (zone->limit) {
+            ds_put_format(&ctx->output, "Zone: %"PRIu64", Limit: %"PRIu64"\n",
+                          dp->key_ct_zones[i], *zone->limit);
+        }
+    }
+}
+
+static void
+cmd_set_zone_default_limit(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+    int64_t limit = -1;
+
+    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
+    const char *dp_name = ctx->argv[1];
+
+    ovs_scan(ctx->argv[2], "limit=%"SCNi64, &limit);
+
+    if (limit < 0 || limit > UINT32_MAX) {
+        ctl_fatal("limit (%"PRIi64") out of range", limit);
+    }
+
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
+    if (!dp) {
+        ctl_fatal("datapath %s does not exist", dp_name);
+    }
+
+    if (dp->ct_zone_default_limit && !may_exist) {
+        ctl_fatal("datapath %s already has limit", dp_name);
+    }
+
+    ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1);
+}
+
+static void
+cmd_del_zone_default_limit(struct ctl_context *ctx)
+{
+    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
+
+    bool must_exist = !shash_find(&ctx->options, "--if-exists");
+    const char *dp_name = ctx->argv[1];
+
+    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
+    if (!dp) {
+        ctl_fatal("datapath %s does not exist", dp_name);
+    }
+
+    if (must_exist && !dp->ct_zone_default_limit) {
+        ctl_fatal("datapath %s does not have limit", dp_name);
+    }
+
+    ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0);
+}
+
 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_datapath_col_ct_zone_default_limit);
     ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_timeout_policy);
+    ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_zone_col_limit);
     ovsdb_idl_add_column(ctx->idl, &ovsrec_ct_timeout_policy_col_timeouts);
 }
 
@@ -3159,6 +3312,18 @@  static const struct ctl_command_syntax vsctl_commands[] = {
     /* Datapath capabilities. */
     {"list-dp-cap", 1, 1, "", pre_get_dp_cap, cmd_list_dp_cap, NULL, "", RO},
 
+    /* CT zone limit. */
+    {"add-zone-limit", 3, 3, "", pre_get_zone, cmd_add_zone_limit, NULL,
+     "--may-exist", RW},
+    {"del-zone-limit", 2, 2, "", pre_get_zone, cmd_del_zone_limit, NULL,
+     "--if-exists", RW},
+    {"list-zone-limit", 1, 1, "", pre_get_zone, cmd_list_zone_limit, NULL,
+     "", RO},
+    {"set-zone-default-limit", 2, 2, "", pre_get_zone,
+     cmd_set_zone_default_limit, NULL, "--may-exist", RW},
+    {"del-zone-default-limit", 1, 1, "", pre_get_zone,
+     cmd_del_zone_default_limit, NULL, "--if-exists", RW},
+
     {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
 };
 
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 2d395ff95..e2d5e2e85 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@ 
 {"name": "Open_vSwitch",
- "version": "8.4.0",
- "cksum": "2738838700 27127",
+ "version": "8.5.0",
+ "cksum": "4040946650 27557",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -670,6 +670,11 @@ 
        "capabilities": {
          "type": {"key": "string", "value": "string",
                   "min": 0, "max": "unlimited"}},
+       "ct_zone_default_limit": {
+          "type": { "key": {"type": "integer",
+                            "minInteger": 0,
+                            "maxInteger": 4294967295},
+                    "min": 0, "max": 1}},
        "external_ids": {
          "type": {"key": "string", "value": "string",
                   "min": 0, "max": "unlimited"}}}},
@@ -679,6 +684,11 @@ 
          "type": {"key": {"type": "uuid",
                           "refTable": "CT_Timeout_Policy"},
                   "min": 0, "max": 1}},
+       "limit": {
+          "type": { "key": {"type": "integer",
+                            "minInteger": 0,
+                            "maxInteger": 4294967295},
+                    "min": 0, "max": 1}},
        "external_ids": {
          "type": {"key": "string", "value": "string",
                   "min": 0, "max": "unlimited"}}}},
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cfcde34ff..84b514e01 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -6417,6 +6417,12 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
       </column>
     </group>
 
+    <column name="ct_zone_default_limit">
+        Default connection tracking zone limit that is applied to all zones
+        that didn't specify the limit explicitly. If the limit is unspecified
+        the datapath configuration is left intact. The value 0 means unlimited.
+    </column>
+
     <group title="Common Columns">
       The overall purpose of these columns is described under <code>Common
       Columns</code> at the beginning of this document.
@@ -6433,6 +6439,11 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
       is not specified, it defaults to the timeout policy in the system.
     </column>
 
+    <column name="limit">
+      Connection tracking limit for this zone. If the limit is unspecified
+      the datapath configuration is left intact. The value 0 means unlimited.
+    </column>
+
     <group title="Common Columns">
       The overall purpose of these columns is described under <code>Common
       Columns</code> at the beginning of this document.