On Sat, Sep 9, 2023 at 12:33 AM Ilya Maximets <i.maxim...@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 <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.
>
>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
