diff mbox series

[ovs-dev] vswitchd, ofproto-dpif: Add support for CT limit

Message ID 20230810124827.172030-1-amusil@redhat.com
State Changes Requested
Headers show
Series [ovs-dev] vswitchd, ofproto-dpif: Add support for CT limit | 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 Aug. 10, 2023, 12:48 p.m. UTC
Add support for setting CT zone limit via ovs-vswitchd
database CT_Zone entry. The limit is propagated
into corresponding datapath. In order to keep
backward compatibility the dpctl/ct-set-limits command
can overwrite the database settings.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 ofproto/ofproto-dpif.c     | 42 +++++++++++++++++++++++++++++++
 ofproto/ofproto-dpif.h     |  5 ++++
 ofproto/ofproto-provider.h |  8 ++++++
 ofproto/ofproto.c          | 23 +++++++++++++++++
 ofproto/ofproto.h          |  3 +++
 tests/system-traffic.at    | 51 +++++++++++++++++++++++++++++++++++++-
 vswitchd/bridge.c          | 10 ++++++++
 vswitchd/vswitch.ovsschema |  8 ++++--
 vswitchd/vswitch.xml       |  5 ++++
 9 files changed, 152 insertions(+), 3 deletions(-)

Comments

Simon Horman Aug. 16, 2023, 2:03 p.m. UTC | #1
On Thu, Aug 10, 2023 at 02:48:27PM +0200, Ales Musil wrote:
> Add support for setting CT zone limit via ovs-vswitchd
> database CT_Zone entry. The limit is propagated
> into corresponding datapath. In order to keep
> backward compatibility the dpctl/ct-set-limits command
> can overwrite the database settings.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>
Ilya Maximets Sept. 8, 2023, 10:34 p.m. UTC | #2
On 8/10/23 14:48, Ales Musil wrote:
> Add support for setting CT zone limit via ovs-vswitchd
> database CT_Zone entry. The limit is propagated
> into corresponding datapath.

Thanks for tha patch!  I didn't try it, but see some comments inline.

> In order to keep
> backward compatibility the dpctl/ct-set-limits command
> can overwrite the database settings.

This doesn't seem like a good option.  If anything, the behavior
should be the opposite, i.e. if the value is set in the database,
it should overwrite whatever user have set before via appctl.
We can treat a zero as a 'whatever currently is set' value, and
all other values set in the database has to be enforced.  Or the
column in the database may be optional, in this case we may treat
an empty column as not configured and zero as unlimited.

The dpctl/ct-set-limits may warn or fail in case of conflicting data.
We also need a NEWS record about the new functionality.

> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>  ofproto/ofproto-dpif.c     | 42 +++++++++++++++++++++++++++++++
>  ofproto/ofproto-dpif.h     |  5 ++++
>  ofproto/ofproto-provider.h |  8 ++++++
>  ofproto/ofproto.c          | 23 +++++++++++++++++
>  ofproto/ofproto.h          |  3 +++
>  tests/system-traffic.at    | 51 +++++++++++++++++++++++++++++++++++++-
>  vswitchd/bridge.c          | 10 ++++++++
>  vswitchd/vswitch.ovsschema |  8 ++++--
>  vswitchd/vswitch.xml       |  5 ++++
>  9 files changed, 152 insertions(+), 3 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index e22ca757a..0c2818a5a 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5522,6 +5522,8 @@ ct_zone_config_init(struct dpif_backer *backer)
>      cmap_init(&backer->ct_zones);
>      hmap_init(&backer->ct_tps);
>      ovs_list_init(&backer->ct_tp_kill_list);
> +    ovs_list_init(&backer->ct_zone_limits_to_add);
> +    ovs_list_init(&backer->ct_zone_limits_to_del);
>      clear_existing_ct_timeout_policies(backer);
>  }
>  
> @@ -5545,6 +5547,8 @@ ct_zone_config_uninit(struct dpif_backer *backer)
>      id_pool_destroy(backer->tp_ids);
>      cmap_destroy(&backer->ct_zones);
>      hmap_destroy(&backer->ct_tps);
> +    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> +    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
>  }
>  
>  static void
> @@ -5625,6 +5629,42 @@ ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
>      }
>  }
>  
> +static void
> +ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
> +                           uint32_t limit)
> +{
> +    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> +                                                 datapath_type);
> +    if (!backer) {
> +        return;
> +    }
> +
> +    struct ovs_list *queue = limit ?
> +            &backer->ct_zone_limits_to_add : &backer->ct_zone_limits_to_del;
> +
> +    ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
> +}
> +
> +static void
> +ct_zone_limits_commit(const char *datapath_type)
> +{
> +    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> +                                                 datapath_type);
> +    if (!backer) {
> +        return;
> +    }
> +
> +    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> +        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add);
> +        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> +    }
> +
> +    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> +        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
> +        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> +    }
> +}
> +
>  static void
>  get_datapath_cap(const char *datapath_type, struct smap *cap)
>  {
> @@ -6914,4 +6954,6 @@ const struct ofproto_class ofproto_dpif_class = {
>      ct_flush,                   /* ct_flush */
>      ct_set_zone_timeout_policy,
>      ct_del_zone_timeout_policy,
> +    ct_zone_limit_queue_update,
> +    ct_zone_limits_commit

Please, keep the trailing comma.

>  };
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d8e0cd37a..b863dd6fc 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -284,6 +284,11 @@ struct dpif_backer {
>                                                  feature than 'bt_support'. */
>  
>      struct atomic_count tnl_count;
> +
> +    struct ovs_list ct_zone_limits_to_add;  /* CT zone limits queued for
> +                                             * addition into datapath. */
> +    struct ovs_list ct_zone_limits_to_del;  /* CT zone limt queued for
> +                                             * deletion from datapath. */
>  };
>  
>  /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 143ded690..99e4017c3 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1921,6 +1921,14 @@ struct ofproto_class {
>      /* Deletes the timeout policy associated with 'zone' in datapath type
>       * 'dp_type'. */
>      void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
> +
> +    /* Queues the CT zone limit update. In order for this change to take
> +     * effect it needs to be commited. */

Typo.

> +    void (*ct_zone_limit_queue_update)(const char *dp_type, uint16_t zone,
> +                                       uint32_t limit);
> +
> +    /* Commits the queued CT zone limit updates to datapath. */
> +    void (*ct_zone_limits_commit)(const char *dp_type);
>  };
>  
>  extern const struct ofproto_class ofproto_dpif_class;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index dbf4958bc..c293d97ac 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -1026,6 +1026,29 @@ ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
>  
>  }
>  
> +void
> +ofproto_ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
> +                                   uint32_t limit)
> +{
> +    datapath_type = ofproto_normalize_type(datapath_type);
> +    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
> +
> +    if (class && class->ct_zone_limit_queue_update) {
> +        class->ct_zone_limit_queue_update(datapath_type, zone_id, limit);
> +    }
> +}
> +
> +void
> +ofproto_ct_zone_limits_commit(const char *datapath_type)
> +{
> +    datapath_type = ofproto_normalize_type(datapath_type);
> +    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
> +
> +    if (class && class->ct_zone_limits_commit) {
> +        class->ct_zone_limits_commit(datapath_type);
> +    }
> +}
> +
>  
>  /* Spanning Tree Protocol (STP) configuration. */
>  
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 8efdb20a0..740afba12 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -384,6 +384,9 @@ void ofproto_ct_set_zone_timeout_policy(const char *datapath_type,
>                                          struct simap *timeout_policy);
>  void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
>                                          uint16_t zone);
> +void ofproto_ct_zone_limit_queue_update(const char *datapath_type,
> +                                        uint16_t zone_id, uint32_t limit);
> +void ofproto_ct_zone_limits_commit(const char *datapath_type);
>  void ofproto_get_datapath_cap(const char *datapath_type,
>                                struct smap *dp_cap);
>  
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 945037ec0..6bf2ca938 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -5177,9 +5177,58 @@ udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.
>  udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
>  ])
>  
> +dnl Test limit set via database

Period.

> +VSCTL_ADD_DATAPATH_TABLE()
> +
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0])
> +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0,3])
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +])
> +
> +dp_uuid=$(ovs-vsctl get open . datapaths:$DP_TYPE)
> +ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid ct_zones:"0"=@m
> +ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid ct_zones:"3"=@m

This is not a user-friendly inteerface.  We should have a separate
command like we have for timeout policies.  Also, both types of the
database update should be tested in ovs-vsctl.at.

> +
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +zone=3,limit=3,count=0
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000200080000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000300080000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000400080000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000500080000 actions=resubmit(,0)"])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000600080000 actions=resubmit(,0)"])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.3," | sort ], [0], [dnl
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=2),reply=(src=10.1.1.4,dst=10.1.1.3,sport=2,dport=1),zone=3
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.1.1.3,sport=3,dport=1),zone=3
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=3,count=0
> +zone=3,limit=3,count=3
> +])
> +
> +# Overwrite the zone limit via command

Period.  And 'dnl' - strange to mix different comment types within
a single test.

> +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5])
> +
> +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> +default limit=10
> +zone=0,limit=5,count=0

This should not happen while the database has a different value.

> +zone=3,limit=3,count=3
> +])
> +
>  OVS_TRAFFIC_VSWITCHD_STOP(["dnl
>  /could not create datapath/d
> -/(Cannot allocate memory) on packet/d"])
> +/(Cannot allocate memory) on packet/d
> +/failed to .* timeout policy/d"])

Please move the '"])' to a separate line.  Same idea as with trailing commas.

>  AT_CLEANUP
>  
>  AT_SETUP([FTP - no conntrack])
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index e9110c1d8..b62e07f98 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -157,6 +157,7 @@ struct aa_mapping {
>  /* Internal representation of conntrack zone configuration table in OVSDB. */
>  struct ct_zone {
>      uint16_t zone_id;
> +    uint32_t limit;             /* Limit of allowed entries. */

s/Limit/Number/

>      struct simap tp;            /* A map from timeout policy attribute to
>                                   * timeout value. */
>      struct hmap_node node;      /* Node in 'struct datapath' 'ct_zones'
> @@ -756,6 +757,12 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
>              ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
>                                                 &ct_zone->tp);
>          }
> +
> +        if (ct_zone->limit != zone_cfg->limit) {
> +            ofproto_ct_zone_limit_queue_update(dp->type, zone_id,
> +                                               zone_cfg->limit);
> +        }
> +        ct_zone->limit = zone_cfg->limit;
>          ct_zone->last_used = idl_seqno;
>      }
>  
> @@ -763,9 +770,12 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
>      HMAP_FOR_EACH_SAFE (ct_zone, node, &dp->ct_zones) {
>          if (ct_zone->last_used != idl_seqno) {
>              ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
> +            ofproto_ct_zone_limit_queue_update(dp->type, ct_zone->zone_id, 0);
>              ct_zone_remove_and_destroy(dp, ct_zone);
>          }
>      }
> +
> +    ofproto_ct_zone_limits_commit(dp->type);
>  }
>  
>  static void
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 2d395ff95..3505ba238 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.4.1",

.z is for aesthetic changes only.  New columns should increase .y.

> + "cksum": "3443162273 27294",
>   "tables": {
>     "Open_vSwitch": {
>       "columns": {
> @@ -679,6 +679,10 @@
>           "type": {"key": {"type": "uuid",
>                            "refTable": "CT_Timeout_Policy"},
>                    "min": 0, "max": 1}},
> +       "limit": {
> +          "type": { "key": {"type": "integer",
> +                            "minInteger": 0,
> +                            "maxInteger": 4294967295}}},
>         "external_ids": {
>           "type": {"key": "string", "value": "string",
>                    "min": 0, "max": "unlimited"}}}},
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index cfcde34ff..ae6524320 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -6428,6 +6428,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>    <table name="CT_Zone">
>      Connection tracking zone configuration
>  
> +    <column name="limit">
> +      Connection tracking limit for this zone. If a limit is zero, it
> +      defaults to the limit in the system.
> +    </column>
> +
>      <column name="timeout_policy">
>        Connection tracking timeout policy for this zone. If a timeout policy
>        is not specified, it defaults to the timeout policy in the system.
Ales Musil Sept. 11, 2023, 6:36 a.m. UTC | #3
On Sat, Sep 9, 2023 at 12:33 AM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 8/10/23 14:48, Ales Musil wrote:
> > Add support for setting CT zone limit via ovs-vswitchd
> > database CT_Zone entry. The limit is propagated
> > into corresponding datapath.
>

Hi Ilya,


>
> Thanks for tha patch!  I didn't try it, but see some comments inline.
>

thank you for the review.


>
> > In order to keep
> > backward compatibility the dpctl/ct-set-limits command
> > can overwrite the database settings.
>
> This doesn't seem like a good option.  If anything, the behavior
> should be the opposite, i.e. if the value is set in the database,
> it should overwrite whatever user have set before via appctl.
> We can treat a zero as a 'whatever currently is set' value, and
> all other values set in the database has to be enforced.  Or the
> column in the database may be optional, in this case we may treat
> an empty column as not configured and zero as unlimited.
>

In this version 0 is default system behavior I guess for backward
compatibility it would make sense to leave the setting untouched for 0.


>
> The dpctl/ct-set-limits may warn or fail in case of conflicting data.
> We also need a NEWS record about the new functionality.
>

Fail would be better IMO if we go with the database value being enforced
unless 0. WDYT?


>
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
> >  ofproto/ofproto-dpif.c     | 42 +++++++++++++++++++++++++++++++
> >  ofproto/ofproto-dpif.h     |  5 ++++
> >  ofproto/ofproto-provider.h |  8 ++++++
> >  ofproto/ofproto.c          | 23 +++++++++++++++++
> >  ofproto/ofproto.h          |  3 +++
> >  tests/system-traffic.at    | 51 +++++++++++++++++++++++++++++++++++++-
> >  vswitchd/bridge.c          | 10 ++++++++
> >  vswitchd/vswitch.ovsschema |  8 ++++--
> >  vswitchd/vswitch.xml       |  5 ++++
> >  9 files changed, 152 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index e22ca757a..0c2818a5a 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5522,6 +5522,8 @@ ct_zone_config_init(struct dpif_backer *backer)
> >      cmap_init(&backer->ct_zones);
> >      hmap_init(&backer->ct_tps);
> >      ovs_list_init(&backer->ct_tp_kill_list);
> > +    ovs_list_init(&backer->ct_zone_limits_to_add);
> > +    ovs_list_init(&backer->ct_zone_limits_to_del);
> >      clear_existing_ct_timeout_policies(backer);
> >  }
> >
> > @@ -5545,6 +5547,8 @@ ct_zone_config_uninit(struct dpif_backer *backer)
> >      id_pool_destroy(backer->tp_ids);
> >      cmap_destroy(&backer->ct_zones);
> >      hmap_destroy(&backer->ct_tps);
> > +    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> > +    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> >  }
> >
> >  static void
> > @@ -5625,6 +5629,42 @@ ct_del_zone_timeout_policy(const char
> *datapath_type, uint16_t zone_id)
> >      }
> >  }
> >
> > +static void
> > +ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
> > +                           uint32_t limit)
> > +{
> > +    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > +                                                 datapath_type);
> > +    if (!backer) {
> > +        return;
> > +    }
> > +
> > +    struct ovs_list *queue = limit ?
> > +            &backer->ct_zone_limits_to_add :
> &backer->ct_zone_limits_to_del;
> > +
> > +    ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
> > +}
> > +
> > +static void
> > +ct_zone_limits_commit(const char *datapath_type)
> > +{
> > +    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > +                                                 datapath_type);
> > +    if (!backer) {
> > +        return;
> > +    }
> > +
> > +    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
> > +        ct_dpif_set_limits(backer->dpif, NULL,
> &backer->ct_zone_limits_to_add);
> > +        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
> > +    }
> > +
> > +    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
> > +        ct_dpif_del_limits(backer->dpif,
> &backer->ct_zone_limits_to_del);
> > +        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
> > +    }
> > +}
> > +
> >  static void
> >  get_datapath_cap(const char *datapath_type, struct smap *cap)
> >  {
> > @@ -6914,4 +6954,6 @@ const struct ofproto_class ofproto_dpif_class = {
> >      ct_flush,                   /* ct_flush */
> >      ct_set_zone_timeout_policy,
> >      ct_del_zone_timeout_policy,
> > +    ct_zone_limit_queue_update,
> > +    ct_zone_limits_commit
>
> Please, keep the trailing comma.
>
> >  };
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index d8e0cd37a..b863dd6fc 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -284,6 +284,11 @@ struct dpif_backer {
> >                                                  feature than
> 'bt_support'. */
> >
> >      struct atomic_count tnl_count;
> > +
> > +    struct ovs_list ct_zone_limits_to_add;  /* CT zone limits queued for
> > +                                             * addition into datapath.
> */
> > +    struct ovs_list ct_zone_limits_to_del;  /* CT zone limt queued for
> > +                                             * deletion from datapath.
> */
> >  };
> >
> >  /* All existing ofproto_backer instances, indexed by ofproto->up.type.
> */
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 143ded690..99e4017c3 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -1921,6 +1921,14 @@ struct ofproto_class {
> >      /* Deletes the timeout policy associated with 'zone' in datapath
> type
> >       * 'dp_type'. */
> >      void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t
> zone);
> > +
> > +    /* Queues the CT zone limit update. In order for this change to take
> > +     * effect it needs to be commited. */
>
> Typo.
>
> > +    void (*ct_zone_limit_queue_update)(const char *dp_type, uint16_t
> zone,
> > +                                       uint32_t limit);
> > +
> > +    /* Commits the queued CT zone limit updates to datapath. */
> > +    void (*ct_zone_limits_commit)(const char *dp_type);
> >  };
> >
> >  extern const struct ofproto_class ofproto_dpif_class;
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index dbf4958bc..c293d97ac 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1026,6 +1026,29 @@ ofproto_ct_del_zone_timeout_policy(const char
> *datapath_type, uint16_t zone_id)
> >
> >  }
> >
> > +void
> > +ofproto_ct_zone_limit_queue_update(const char *datapath_type, uint16_t
> zone_id,
> > +                                   uint32_t limit)
> > +{
> > +    datapath_type = ofproto_normalize_type(datapath_type);
> > +    const struct ofproto_class *class =
> ofproto_class_find__(datapath_type);
> > +
> > +    if (class && class->ct_zone_limit_queue_update) {
> > +        class->ct_zone_limit_queue_update(datapath_type, zone_id,
> limit);
> > +    }
> > +}
> > +
> > +void
> > +ofproto_ct_zone_limits_commit(const char *datapath_type)
> > +{
> > +    datapath_type = ofproto_normalize_type(datapath_type);
> > +    const struct ofproto_class *class =
> ofproto_class_find__(datapath_type);
> > +
> > +    if (class && class->ct_zone_limits_commit) {
> > +        class->ct_zone_limits_commit(datapath_type);
> > +    }
> > +}
> > +
> >
> >  /* Spanning Tree Protocol (STP) configuration. */
> >
> > diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> > index 8efdb20a0..740afba12 100644
> > --- a/ofproto/ofproto.h
> > +++ b/ofproto/ofproto.h
> > @@ -384,6 +384,9 @@ void ofproto_ct_set_zone_timeout_policy(const char
> *datapath_type,
> >                                          struct simap *timeout_policy);
> >  void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
> >                                          uint16_t zone);
> > +void ofproto_ct_zone_limit_queue_update(const char *datapath_type,
> > +                                        uint16_t zone_id, uint32_t
> limit);
> > +void ofproto_ct_zone_limits_commit(const char *datapath_type);
> >  void ofproto_get_datapath_cap(const char *datapath_type,
> >                                struct smap *dp_cap);
> >
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 945037ec0..6bf2ca938 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -5177,9 +5177,58 @@
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.
> >
> udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> >  ])
> >
> > +dnl Test limit set via database
>
> Period.
>
> > +VSCTL_ADD_DATAPATH_TABLE()
> > +
> > +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0])
> > +AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0,3])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +])
> > +
> > +dp_uuid=$(ovs-vsctl get open . datapaths:$DP_TYPE)
> > +ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid
> ct_zones:"0"=@m
> > +ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid
> ct_zones:"3"=@m
>
> This is not a user-friendly inteerface.  We should have a separate
> command like we have for timeout policies.  Also, both types of the
> database update should be tested in ovs-vsctl.at.
>
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +zone=3,limit=3,count=0
> > +])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000200080000
> actions=resubmit(,0)"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000300080000
> actions=resubmit(,0)"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000400080000
> actions=resubmit(,0)"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000500080000
> actions=resubmit(,0)"])
> > +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2
> packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000600080000
> actions=resubmit(,0)"])
> > +
> > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep
> "orig=.src=10\.1\.1\.3," | sort ], [0], [dnl
> >
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=2),reply=(src=10.1.1.4,dst=10.1.1.3,sport=2,dport=1),zone=3
> >
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.1.1.3,sport=3,dport=1),zone=3
> >
> +udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
> > +])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=3,count=0
> > +zone=3,limit=3,count=3
> > +])
> > +
> > +# Overwrite the zone limit via command
>
> Period.  And 'dnl' - strange to mix different comment types within
> a single test.
>
> > +AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5])
> > +
> > +AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
> > +default limit=10
> > +zone=0,limit=5,count=0
>
> This should not happen while the database has a different value.
>
> > +zone=3,limit=3,count=3
> > +])
> > +
> >  OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> >  /could not create datapath/d
> > -/(Cannot allocate memory) on packet/d"])
> > +/(Cannot allocate memory) on packet/d
> > +/failed to .* timeout policy/d"])
>
> Please move the '"])' to a separate line.  Same idea as with trailing
> commas.
>
> >  AT_CLEANUP
> >
> >  AT_SETUP([FTP - no conntrack])
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index e9110c1d8..b62e07f98 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -157,6 +157,7 @@ struct aa_mapping {
> >  /* Internal representation of conntrack zone configuration table in
> OVSDB. */
> >  struct ct_zone {
> >      uint16_t zone_id;
> > +    uint32_t limit;             /* Limit of allowed entries. */
>
> s/Limit/Number/
>
> >      struct simap tp;            /* A map from timeout policy attribute
> to
> >                                   * timeout value. */
> >      struct hmap_node node;      /* Node in 'struct datapath' 'ct_zones'
> > @@ -756,6 +757,12 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> >              ofproto_ct_set_zone_timeout_policy(dp->type,
> ct_zone->zone_id,
> >                                                 &ct_zone->tp);
> >          }
> > +
> > +        if (ct_zone->limit != zone_cfg->limit) {
> > +            ofproto_ct_zone_limit_queue_update(dp->type, zone_id,
> > +                                               zone_cfg->limit);
> > +        }
> > +        ct_zone->limit = zone_cfg->limit;
> >          ct_zone->last_used = idl_seqno;
> >      }
> >
> > @@ -763,9 +770,12 @@ ct_zones_reconfigure(struct datapath *dp, struct
> ovsrec_datapath *dp_cfg)
> >      HMAP_FOR_EACH_SAFE (ct_zone, node, &dp->ct_zones) {
> >          if (ct_zone->last_used != idl_seqno) {
> >              ofproto_ct_del_zone_timeout_policy(dp->type,
> ct_zone->zone_id);
> > +            ofproto_ct_zone_limit_queue_update(dp->type,
> ct_zone->zone_id, 0);
> >              ct_zone_remove_and_destroy(dp, ct_zone);
> >          }
> >      }
> > +
> > +    ofproto_ct_zone_limits_commit(dp->type);
> >  }
> >
> >  static void
> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index 2d395ff95..3505ba238 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.4.1",
>
> .z is for aesthetic changes only.  New columns should increase .y.
>
> > + "cksum": "3443162273 27294",
> >   "tables": {
> >     "Open_vSwitch": {
> >       "columns": {
> > @@ -679,6 +679,10 @@
> >           "type": {"key": {"type": "uuid",
> >                            "refTable": "CT_Timeout_Policy"},
> >                    "min": 0, "max": 1}},
> > +       "limit": {
> > +          "type": { "key": {"type": "integer",
> > +                            "minInteger": 0,
> > +                            "maxInteger": 4294967295}}},
> >         "external_ids": {
> >           "type": {"key": "string", "value": "string",
> >                    "min": 0, "max": "unlimited"}}}},
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index cfcde34ff..ae6524320 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -6428,6 +6428,11 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
> type=patch options:peer=p1 \
> >    <table name="CT_Zone">
> >      Connection tracking zone configuration
> >
> > +    <column name="limit">
> > +      Connection tracking limit for this zone. If a limit is zero, it
> > +      defaults to the limit in the system.
> > +    </column>
> > +
> >      <column name="timeout_policy">
> >        Connection tracking timeout policy for this zone. If a timeout
> policy
> >        is not specified, it defaults to the timeout policy in the system.
>
>
Thanks,
Ales
Ilya Maximets Sept. 18, 2023, 5:37 p.m. UTC | #4
On 9/11/23 08:36, Ales Musil wrote:
> 
> 
> On Sat, Sep 9, 2023 at 12:33 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 8/10/23 14:48, Ales Musil wrote:
>     > Add support for setting CT zone limit via ovs-vswitchd
>     > database CT_Zone entry. The limit is propagated
>     > into corresponding datapath.
> 
> 
> Hi Ilya,
>  
> 
> 
>     Thanks for tha patch!  I didn't try it, but see some comments inline.
> 
> 
> thank you for the review.
>  
> 
> 
>     > In order to keep
>     > backward compatibility the dpctl/ct-set-limits command
>     > can overwrite the database settings.
> 
>     This doesn't seem like a good option.  If anything, the behavior
>     should be the opposite, i.e. if the value is set in the database,
>     it should overwrite whatever user have set before via appctl.
>     We can treat a zero as a 'whatever currently is set' value, and
>     all other values set in the database has to be enforced.  Or the
>     column in the database may be optional, in this case we may treat
>     an empty column as not configured and zero as unlimited.
> 
> 
> In this version 0 is default system behavior I guess for backward compatibility it would make sense to leave the setting untouched for 0.

What happens if you set the column to some value and then set it
back to zero?

>  
> 
> 
>     The dpctl/ct-set-limits may warn or fail in case of conflicting data.
>     We also need a NEWS record about the new functionality.
> 
> 
> Fail would be better IMO if we go with the database value being enforced unless 0. WDYT?

Failure should be fine.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e22ca757a..0c2818a5a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5522,6 +5522,8 @@  ct_zone_config_init(struct dpif_backer *backer)
     cmap_init(&backer->ct_zones);
     hmap_init(&backer->ct_tps);
     ovs_list_init(&backer->ct_tp_kill_list);
+    ovs_list_init(&backer->ct_zone_limits_to_add);
+    ovs_list_init(&backer->ct_zone_limits_to_del);
     clear_existing_ct_timeout_policies(backer);
 }
 
@@ -5545,6 +5547,8 @@  ct_zone_config_uninit(struct dpif_backer *backer)
     id_pool_destroy(backer->tp_ids);
     cmap_destroy(&backer->ct_zones);
     hmap_destroy(&backer->ct_tps);
+    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
+    ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
 }
 
 static void
@@ -5625,6 +5629,42 @@  ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
     }
 }
 
+static void
+ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
+                           uint32_t limit)
+{
+    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+                                                 datapath_type);
+    if (!backer) {
+        return;
+    }
+
+    struct ovs_list *queue = limit ?
+            &backer->ct_zone_limits_to_add : &backer->ct_zone_limits_to_del;
+
+    ct_dpif_push_zone_limit(queue, zone_id, limit, 0);
+}
+
+static void
+ct_zone_limits_commit(const char *datapath_type)
+{
+    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
+                                                 datapath_type);
+    if (!backer) {
+        return;
+    }
+
+    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_add)) {
+        ct_dpif_set_limits(backer->dpif, NULL, &backer->ct_zone_limits_to_add);
+        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_add);
+    }
+
+    if (!ovs_list_is_empty(&backer->ct_zone_limits_to_del)) {
+        ct_dpif_del_limits(backer->dpif, &backer->ct_zone_limits_to_del);
+        ct_dpif_free_zone_limits(&backer->ct_zone_limits_to_del);
+    }
+}
+
 static void
 get_datapath_cap(const char *datapath_type, struct smap *cap)
 {
@@ -6914,4 +6954,6 @@  const struct ofproto_class ofproto_dpif_class = {
     ct_flush,                   /* ct_flush */
     ct_set_zone_timeout_policy,
     ct_del_zone_timeout_policy,
+    ct_zone_limit_queue_update,
+    ct_zone_limits_commit
 };
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d8e0cd37a..b863dd6fc 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -284,6 +284,11 @@  struct dpif_backer {
                                                 feature than 'bt_support'. */
 
     struct atomic_count tnl_count;
+
+    struct ovs_list ct_zone_limits_to_add;  /* CT zone limits queued for
+                                             * addition into datapath. */
+    struct ovs_list ct_zone_limits_to_del;  /* CT zone limt queued for
+                                             * deletion from datapath. */
 };
 
 /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 143ded690..99e4017c3 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1921,6 +1921,14 @@  struct ofproto_class {
     /* Deletes the timeout policy associated with 'zone' in datapath type
      * 'dp_type'. */
     void (*ct_del_zone_timeout_policy)(const char *dp_type, uint16_t zone);
+
+    /* Queues the CT zone limit update. In order for this change to take
+     * effect it needs to be commited. */
+    void (*ct_zone_limit_queue_update)(const char *dp_type, uint16_t zone,
+                                       uint32_t limit);
+
+    /* Commits the queued CT zone limit updates to datapath. */
+    void (*ct_zone_limits_commit)(const char *dp_type);
 };
 
 extern const struct ofproto_class ofproto_dpif_class;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dbf4958bc..c293d97ac 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1026,6 +1026,29 @@  ofproto_ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
 
 }
 
+void
+ofproto_ct_zone_limit_queue_update(const char *datapath_type, uint16_t zone_id,
+                                   uint32_t limit)
+{
+    datapath_type = ofproto_normalize_type(datapath_type);
+    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
+
+    if (class && class->ct_zone_limit_queue_update) {
+        class->ct_zone_limit_queue_update(datapath_type, zone_id, limit);
+    }
+}
+
+void
+ofproto_ct_zone_limits_commit(const char *datapath_type)
+{
+    datapath_type = ofproto_normalize_type(datapath_type);
+    const struct ofproto_class *class = ofproto_class_find__(datapath_type);
+
+    if (class && class->ct_zone_limits_commit) {
+        class->ct_zone_limits_commit(datapath_type);
+    }
+}
+
 
 /* Spanning Tree Protocol (STP) configuration. */
 
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 8efdb20a0..740afba12 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -384,6 +384,9 @@  void ofproto_ct_set_zone_timeout_policy(const char *datapath_type,
                                         struct simap *timeout_policy);
 void ofproto_ct_del_zone_timeout_policy(const char *datapath_type,
                                         uint16_t zone);
+void ofproto_ct_zone_limit_queue_update(const char *datapath_type,
+                                        uint16_t zone_id, uint32_t limit);
+void ofproto_ct_zone_limits_commit(const char *datapath_type);
 void ofproto_get_datapath_cap(const char *datapath_type,
                               struct smap *dp_cap);
 
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 945037ec0..6bf2ca938 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -5177,9 +5177,58 @@  udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.
 udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
 ])
 
+dnl Test limit set via database
+VSCTL_ADD_DATAPATH_TABLE()
+
+AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=0])
+AT_CHECK([ovs-appctl dpctl/flush-conntrack zone=3])
+
+AT_CHECK([ovs-appctl dpctl/ct-del-limits zone=0,3])
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+])
+
+dp_uuid=$(ovs-vsctl get open . datapaths:$DP_TYPE)
+ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid ct_zones:"0"=@m
+ovs-vsctl --id=@m create ct_zone limit=3 -- set datapath $dp_uuid ct_zones:"3"=@m
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+zone=3,limit=3,count=0
+])
+
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000200080000 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000300080000 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000400080000 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000500080000 actions=resubmit(,0)"])
+AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=2 packet=50540000000a50540000000908004500001c000000000011a4c90a0101030a0101040001000600080000 actions=resubmit(,0)"])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.3," | sort ], [0], [dnl
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=2),reply=(src=10.1.1.4,dst=10.1.1.3,sport=2,dport=1),zone=3
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=3),reply=(src=10.1.1.4,dst=10.1.1.3,sport=3,dport=1),zone=3
+udp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=1,dport=4),reply=(src=10.1.1.4,dst=10.1.1.3,sport=4,dport=1),zone=3
+])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=3,count=0
+zone=3,limit=3,count=3
+])
+
+# Overwrite the zone limit via command
+AT_CHECK([ovs-appctl dpctl/ct-set-limits zone=0,limit=5])
+
+AT_CHECK([ovs-appctl dpctl/ct-get-limits], [0], [dnl
+default limit=10
+zone=0,limit=5,count=0
+zone=3,limit=3,count=3
+])
+
 OVS_TRAFFIC_VSWITCHD_STOP(["dnl
 /could not create datapath/d
-/(Cannot allocate memory) on packet/d"])
+/(Cannot allocate memory) on packet/d
+/failed to .* timeout policy/d"])
 AT_CLEANUP
 
 AT_SETUP([FTP - no conntrack])
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e9110c1d8..b62e07f98 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -157,6 +157,7 @@  struct aa_mapping {
 /* Internal representation of conntrack zone configuration table in OVSDB. */
 struct ct_zone {
     uint16_t zone_id;
+    uint32_t limit;             /* Limit of allowed entries. */
     struct simap tp;            /* A map from timeout policy attribute to
                                  * timeout value. */
     struct hmap_node node;      /* Node in 'struct datapath' 'ct_zones'
@@ -756,6 +757,12 @@  ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
             ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id,
                                                &ct_zone->tp);
         }
+
+        if (ct_zone->limit != zone_cfg->limit) {
+            ofproto_ct_zone_limit_queue_update(dp->type, zone_id,
+                                               zone_cfg->limit);
+        }
+        ct_zone->limit = zone_cfg->limit;
         ct_zone->last_used = idl_seqno;
     }
 
@@ -763,9 +770,12 @@  ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg)
     HMAP_FOR_EACH_SAFE (ct_zone, node, &dp->ct_zones) {
         if (ct_zone->last_used != idl_seqno) {
             ofproto_ct_del_zone_timeout_policy(dp->type, ct_zone->zone_id);
+            ofproto_ct_zone_limit_queue_update(dp->type, ct_zone->zone_id, 0);
             ct_zone_remove_and_destroy(dp, ct_zone);
         }
     }
+
+    ofproto_ct_zone_limits_commit(dp->type);
 }
 
 static void
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 2d395ff95..3505ba238 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.4.1",
+ "cksum": "3443162273 27294",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -679,6 +679,10 @@ 
          "type": {"key": {"type": "uuid",
                           "refTable": "CT_Timeout_Policy"},
                   "min": 0, "max": 1}},
+       "limit": {
+          "type": { "key": {"type": "integer",
+                            "minInteger": 0,
+                            "maxInteger": 4294967295}}},
        "external_ids": {
          "type": {"key": "string", "value": "string",
                   "min": 0, "max": "unlimited"}}}},
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cfcde34ff..ae6524320 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -6428,6 +6428,11 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
   <table name="CT_Zone">
     Connection tracking zone configuration
 
+    <column name="limit">
+      Connection tracking limit for this zone. If a limit is zero, it
+      defaults to the limit in the system.
+    </column>
+
     <column name="timeout_policy">
       Connection tracking timeout policy for this zone. If a timeout policy
       is not specified, it defaults to the timeout policy in the system.