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