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 <amu...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
