| Message ID | 20231129072334.91442-4-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | Expose CT limit via DB | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 11/29/23 08:23, 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> > --- > v7: Rebase on top of current master. > Address comments from Ilya: > - Add missing 'a'. > - Slightly change the format for limit listing. > - Add usage string for all the new comamnds. > v6: Rebase on top of current master. > Address comments from Ilya: > - Update the semantics and documentation of the set command. > v5: Rebase on top of current master. > Address comments from Ilya: > - Use only single command for setting zone and default limit. > - Correct the errors in the man page. > - Use references for the column description. > 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 | 5 ++ > tests/ovs-vsctl.at | 88 ++++++++++++++++++++++- > utilities/ovs-vsctl.8.in | 31 ++++++-- > utilities/ovs-vsctl.c | 141 +++++++++++++++++++++++++++++++++++-- > vswitchd/vswitch.ovsschema | 14 +++- > vswitchd/vswitch.xml | 14 ++++ > 6 files changed, 277 insertions(+), 16 deletions(-) > > diff --git a/NEWS b/NEWS > index 474b3a4ef..77117573c 100644 > --- a/NEWS > +++ b/NEWS > @@ -17,6 +17,11 @@ Post-v3.2.0 > Reported names adjusted accordingly. > * Added support for removal of default CT zone limit, e.g. > "ovs-appctl dpctl/ct-del-limits default". > + - ovs-vsctl: > + * New commands 'set-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 and > + '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..c913c5a2d 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([set-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([set-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([set-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([set-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-limit netdev default 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([set-zone-limit netdev default 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-limit netdev default])]) > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl > +Zone: 10, Limit: 5 > +]) > + > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-limit netdev default])]) > + > > 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 > @@ -1113,16 +1174,39 @@ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1 icmp_reply=2]) > ]) > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])]) > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])], > - [1], [], [ovs-vsctl: zone id 2 already exists > + [1], [], [ovs-vsctl: zone id 2 already has a policy > ]) > 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([del-zone-tp netdev zone=11])], > - [1], [], [ovs-vsctl: zone id 11 does not exist > + [1], [], [ovs-vsctl: zone id 11 does not have a policy > ]) > 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([set-zone-limit netdevxx zone=5 limit=1])], > + [1], [], [ovs-vsctl: datapath netdevxx does not exist > +]) > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=88888 limit=1])], > + [1], [], [ovs-vsctl: zone_id (88888) out of range > +]) > +AT_CHECK([RUN_OVS_VSCTL([set-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 a limit > +]) > + > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdevxx default limit=1])], > + [1], [], [ovs-vsctl: datapath netdevxx does not exist > +]) > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev default limit=-1])], > + [1], [], [ovs-vsctl: limit (-1) out of range > +]) > +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev default])], > + [1], [], [ovs-vsctl: datapath netdev does not have a 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..9bc95b82c 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,37 @@ 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 \fIpolicy\fR for > +\fIzone_id\fR that already has a policy is an error. > + With \fB\-\-may\-exist\fR, this command does nothing if policy for > + \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 > -does not exist is an error. With \fB\-\-if\-exists\fR, attempting to > -delete a zone that does not exist has no effect. > +Without \fB\-\-if\-exists\fR, attempting to delete a policy for zone that > +does not exist or doesn't have a policy is an error. With > +\fB\-\-if\-exists\fR, attempting to delete a a policy that does not > +exist has no effect. > . > .IP "\fBlist\-zone\-tp \fIdatapath\fR" > Prints the timeout policies of all zones in \fIdatapath\fR. > . > +.IP "\fBset\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR|\fBdefault \fBlimit=\fIzone_limit\fR" > +Sets a conntrack zone limit with \fIzone_id\fR|\fIdefault\fR in > +\fIdatapath\fR. The \fIlimit\fR with value \fB0\fR means unlimited. > +.IP > +. > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR|\fBdefault\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 or doesn't have a limit is an error. With \fB\-\-if\-exists\fR, > +attempting to delete a limit that does not exist has no effect. > +. > +.IP "\fBlist\-zone\-limit \fIdatapath\fR" > +Prints the limits of all zones in \fIdatapath\fR. > +. > .SS "Datapath Capabilities Command" > The command query datapath capabilities. > . > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index 5e549df00..cb11bdf00 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -442,6 +442,14 @@ Auto Attach commands:\n\ > Switch commands:\n\ > emer-reset reset switch to known good state\n\ > \n\ > +CT zone limit commands:\n\ It might be better to expand the CT to be more user-friendly and ditch the 'limit' part as other connection tracking related commands can go into this section later (timeout policies). So, suggesting the "Connection Tracking commands:". > + set-zone-limit BRIDGE zone_id=ZONE|default limit=LIMIT set CT LIMIT for\ > + ZONE|default on BRIDGE\n\ > + del-zone-limit BRIDGE zone_id=ZONE|default delete CT limit\ > + for ZONE|default on BRIDGE\n\ > + list-zone-limit BRIDGE list all limits\ > + configured on BRIDGE\n\ Hmm. This is just incorrect: 1. The first argument is not a bridge, but a datapath. 2. The zone argument is just 'zone', not 'zone_id'. Also, instead of having very long lines, it's better to just start description on a new line, as man pages typically do. e.g.: Connection Tracking commands: set-zone-limit BRIDGE zone_id=ZONE|default limit=LIMIT set CT LIMIT for ZONE|default on BRIDGE del-zone-limit BRIDGE zone_id=ZONE|default delete CT limit for ZONE|default on BRIDGE list-zone-limit BRIDGE list all limits configured on BRIDGE And I just noticed, but we should probably rename 'list-zone-limit' to 'list-zone-limits'. Other similar commands use plural form as well, unless they end with an abbreviation, e.g. 'list-ports' or 'list-ifaces'. I'm not sure, but one other thought is maybe to remove the 'zone_id' and 'limit' words from these commands entirely. These are positional arguments anyway (the code is looking for them in spacific places), so it doesn't have a lot of sense to have them named. And we're not trying to parse multiple pairs as dpctl does, commands can be chained if necessary. Extention of these commands also seems unlikely in the future as the names are very simple and descriptive, i.e. 'set-zone-limit' is unlikely to do anything else, but to set a limit for a zone. WDYT? > +\n\ > %s\ > %s\ > \n\ > @@ -1302,8 +1310,8 @@ cmd_add_zone_tp(struct ctl_context *ctx) > ctl_fatal("No timeout policy"); > } > > - if (zone && !may_exist) { > - ctl_fatal("zone id %"PRIu64" already exists", zone_id); > + if (zone && zone->timeout_policy && !may_exist) { > + ctl_fatal("zone id %"PRIu64" already has a policy", zone_id); > } > > tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > @@ -1332,11 +1340,20 @@ cmd_del_zone_tp(struct ctl_context *ctx) > } > > struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id); > - if (must_exist && !zone) { > - ctl_fatal("zone id %"PRIu64" does not exist", zone_id); > + if (must_exist && !(zone && zone->timeout_policy)) { > + ctl_fatal("zone id %"PRIu64" does not have a policy", 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 +1388,118 @@ cmd_list_zone_tp(struct ctl_context *ctx) > } > } > > +static void > +cmd_set_zone_limit(struct ctl_context *ctx) > +{ > + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx); > + int64_t zone_id = -1; > + int64_t limit = -1; > + > + const char *dp_name = ctx->argv[1]; > + > + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id); > + ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit); > + > + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name); > + if (!dp) { > + ctl_fatal("datapath %s does not exist", dp_name); > + } > + > + if (limit < 0 || limit > UINT32_MAX) { > + ctl_fatal("limit (%"PRIi64") out of range", limit); > + } > + > + if (!strcmp(ctx->argv[2], "default")) { > + ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1); > + return; > + } > + > + if (zone_id < 0 || zone_id > UINT16_MAX) { > + ctl_fatal("zone_id (%"PRIi64") out of range", zone_id); > + } > + > + struct ovsrec_ct_zone *zone = find_ct_zone(dp, 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); > + } > + > + if (!strcmp(ctx->argv[2], "default")) { > + if (must_exist && !dp->ct_zone_default_limit) { > + ctl_fatal("datapath %s does not have a limit", dp_name); > + } > + > + ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0); > + return; > + } > + > + 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 a 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 > 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 +3282,14 @@ 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. */ > + {"set-zone-limit", 3, 3, "", pre_get_zone, cmd_set_zone_limit, NULL, > + "", 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}, The usage strings are still empty here. They should be "ARG ARG ARG", "ARG ARG" and "ARG", respectively. They are used for bash autocompletion. Without specifying them, autocomp will suggest '--' without waiting for arguments specified. See the comment for the vsctl_commands[] array. And see the utilities/ovs-vsctl-bashcomp.bash. Note: It might be nice to add DATAPATH to the bashcomp script as a recognizeable pattern, but it's out of scope for this patch. > + > {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 68392ac41..eaccd85cf 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -6488,6 +6488,14 @@ 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 <ref table="CT_Zone" column="limit"/> > + explicitly. If the limit is unspecified the default limit > + configuration for the datapath 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. > @@ -6504,6 +6512,12 @@ 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 <ref table="Datapath" column="ct_zone_default_limit"/> will be used. > + 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.
On Wed, Nov 29, 2023 at 2:30 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 11/29/23 08:23, 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> > > --- > > v7: Rebase on top of current master. > > Address comments from Ilya: > > - Add missing 'a'. > > - Slightly change the format for limit listing. > > - Add usage string for all the new comamnds. > > v6: Rebase on top of current master. > > Address comments from Ilya: > > - Update the semantics and documentation of the set command. > > v5: Rebase on top of current master. > > Address comments from Ilya: > > - Use only single command for setting zone and default limit. > > - Correct the errors in the man page. > > - Use references for the column description. > > 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 | 5 ++ > > tests/ovs-vsctl.at | 88 ++++++++++++++++++++++- > > utilities/ovs-vsctl.8.in | 31 ++++++-- > > utilities/ovs-vsctl.c | 141 +++++++++++++++++++++++++++++++++++-- > > vswitchd/vswitch.ovsschema | 14 +++- > > vswitchd/vswitch.xml | 14 ++++ > > 6 files changed, 277 insertions(+), 16 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 474b3a4ef..77117573c 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -17,6 +17,11 @@ Post-v3.2.0 > > Reported names adjusted accordingly. > > * Added support for removal of default CT zone limit, e.g. > > "ovs-appctl dpctl/ct-del-limits default". > > + - ovs-vsctl: > > + * New commands 'set-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 and > > + '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..c913c5a2d 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([set-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([set-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([set-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([set-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-limit netdev default 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([set-zone-limit netdev default 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-limit netdev default])]) > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl > > +Zone: 10, Limit: 5 > > +]) > > + > > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-limit netdev default])]) > > + > > > > 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 > > @@ -1113,16 +1174,39 @@ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx > zone=1 icmp_first=1 icmp_reply=2]) > > ]) > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > icmp_reply=3])]) > > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > icmp_reply=3])], > > - [1], [], [ovs-vsctl: zone id 2 already exists > > + [1], [], [ovs-vsctl: zone id 2 already has a policy > > ]) > > 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([del-zone-tp netdev zone=11])], > > - [1], [], [ovs-vsctl: zone id 11 does not exist > > + [1], [], [ovs-vsctl: zone id 11 does not have a policy > > ]) > > 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([set-zone-limit netdevxx zone=5 limit=1])], > > + [1], [], [ovs-vsctl: datapath netdevxx does not exist > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=88888 limit=1])], > > + [1], [], [ovs-vsctl: zone_id (88888) out of range > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([set-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 a limit > > +]) > > + > > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdevxx default limit=1])], > > + [1], [], [ovs-vsctl: datapath netdevxx does not exist > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev default limit=-1])], > > + [1], [], [ovs-vsctl: limit (-1) out of range > > +]) > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev default])], > > + [1], [], [ovs-vsctl: datapath netdev does not have a 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..9bc95b82c 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,37 @@ 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 \fIpolicy\fR for > > +\fIzone_id\fR that already has a policy is an error. > > + With \fB\-\-may\-exist\fR, this command does nothing if policy for > > + \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 > > -does not exist is an error. With \fB\-\-if\-exists\fR, attempting to > > -delete a zone that does not exist has no effect. > > +Without \fB\-\-if\-exists\fR, attempting to delete a policy for zone > that > > +does not exist or doesn't have a policy is an error. With > > +\fB\-\-if\-exists\fR, attempting to delete a a policy that does not > > +exist has no effect. > > . > > .IP "\fBlist\-zone\-tp \fIdatapath\fR" > > Prints the timeout policies of all zones in \fIdatapath\fR. > > . > > +.IP "\fBset\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR|\fBdefault > \fBlimit=\fIzone_limit\fR" > > +Sets a conntrack zone limit with \fIzone_id\fR|\fIdefault\fR in > > +\fIdatapath\fR. The \fIlimit\fR with value \fB0\fR means unlimited. > > +.IP > > +. > > +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath > \fBzone=\fIzone_id\fR|\fBdefault\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 or doesn't have a limit is an error. With > \fB\-\-if\-exists\fR, > > +attempting to delete a limit that does not exist has no effect. > > +. > > +.IP "\fBlist\-zone\-limit \fIdatapath\fR" > > +Prints the limits of all zones in \fIdatapath\fR. > > +. > > .SS "Datapath Capabilities Command" > > The command query datapath capabilities. > > . > > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > > index 5e549df00..cb11bdf00 100644 > > --- a/utilities/ovs-vsctl.c > > +++ b/utilities/ovs-vsctl.c > > @@ -442,6 +442,14 @@ Auto Attach commands:\n\ > > Switch commands:\n\ > > emer-reset reset switch to known good state\n\ > > \n\ > > +CT zone limit commands:\n\ > > It might be better to expand the CT to be more user-friendly and ditch > the 'limit' part as other connection tracking related commands can go > into this section later (timeout policies). So, suggesting the > "Connection Tracking commands:". > Changed in v8. > > > + set-zone-limit BRIDGE zone_id=ZONE|default limit=LIMIT set CT LIMIT > for\ > > + ZONE|default on BRIDGE\n\ > > + del-zone-limit BRIDGE zone_id=ZONE|default delete CT > limit\ > > + for ZONE|default on BRIDGE\n\ > > + list-zone-limit BRIDGE list all > limits\ > > + configured on BRIDGE\n\ > > Hmm. This is just incorrect: > > 1. The first argument is not a bridge, but a datapath. > 2. The zone argument is just 'zone', not 'zone_id'. > Yeah, that's a copy/paste error. > > Also, instead of having very long lines, it's better to just > start description on a new line, as man pages typically do. > e.g.: > > Connection Tracking commands: > set-zone-limit BRIDGE zone_id=ZONE|default limit=LIMIT > set CT LIMIT for ZONE|default on BRIDGE > del-zone-limit BRIDGE zone_id=ZONE|default > delete CT limit for ZONE|default on BRIDGE > list-zone-limit BRIDGE list all limits configured on BRIDGE > > And I just noticed, but we should probably rename 'list-zone-limit' > to 'list-zone-limits'. Other similar commands use plural form as > well, unless they end with an abbreviation, e.g. 'list-ports' or > 'list-ifaces'. > > I'm not sure, but one other thought is maybe to remove the 'zone_id' > and 'limit' words from these commands entirely. These are positional > arguments anyway (the code is looking for them in spacific places), > so it doesn't have a lot of sense to have them named. And we're not > trying to parse multiple pairs as dpctl does, commands can be chained > if necessary. Extention of these commands also seems unlikely in the > future as the names are very simple and descriptive, i.e. 'set-zone-limit' > is unlikely to do anything else, but to set a limit for a zone. WDYT? > > > Sounds reasonable, I've changed it in v8. > > +\n\ > > %s\ > > %s\ > > \n\ > > @@ -1302,8 +1310,8 @@ cmd_add_zone_tp(struct ctl_context *ctx) > > ctl_fatal("No timeout policy"); > > } > > > > - if (zone && !may_exist) { > > - ctl_fatal("zone id %"PRIu64" already exists", zone_id); > > + if (zone && zone->timeout_policy && !may_exist) { > > + ctl_fatal("zone id %"PRIu64" already has a policy", zone_id); > > } > > > > tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); > > @@ -1332,11 +1340,20 @@ cmd_del_zone_tp(struct ctl_context *ctx) > > } > > > > struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id); > > - if (must_exist && !zone) { > > - ctl_fatal("zone id %"PRIu64" does not exist", zone_id); > > + if (must_exist && !(zone && zone->timeout_policy)) { > > + ctl_fatal("zone id %"PRIu64" does not have a policy", 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 +1388,118 @@ cmd_list_zone_tp(struct ctl_context *ctx) > > } > > } > > > > +static void > > +cmd_set_zone_limit(struct ctl_context *ctx) > > +{ > > + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx); > > + int64_t zone_id = -1; > > + int64_t limit = -1; > > + > > + const char *dp_name = ctx->argv[1]; > > + > > + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id); > > + ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit); > > + > > + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name); > > + if (!dp) { > > + ctl_fatal("datapath %s does not exist", dp_name); > > + } > > + > > + if (limit < 0 || limit > UINT32_MAX) { > > + ctl_fatal("limit (%"PRIi64") out of range", limit); > > + } > > + > > + if (!strcmp(ctx->argv[2], "default")) { > > + ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1); > > + return; > > + } > > + > > + if (zone_id < 0 || zone_id > UINT16_MAX) { > > + ctl_fatal("zone_id (%"PRIi64") out of range", zone_id); > > + } > > + > > + struct ovsrec_ct_zone *zone = find_ct_zone(dp, 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); > > + } > > + > > + if (!strcmp(ctx->argv[2], "default")) { > > + if (must_exist && !dp->ct_zone_default_limit) { > > + ctl_fatal("datapath %s does not have a limit", dp_name); > > + } > > + > > + ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0); > > + return; > > + } > > + > > + 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 a 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 > > 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 +3282,14 @@ 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. */ > > + {"set-zone-limit", 3, 3, "", pre_get_zone, cmd_set_zone_limit, NULL, > > + "", 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}, > > The usage strings are still empty here. They should be "ARG ARG ARG", > "ARG ARG" and "ARG", respectively. They are used for bash autocompletion. > Without specifying them, autocomp will suggest '--' without waiting for > arguments specified. See the comment for the vsctl_commands[] array. > And see the utilities/ovs-vsctl-bashcomp.bash. > I completely missed that sorry. > Note: It might be nice to add DATAPATH to the bashcomp script as a > recognizeable pattern, but it's out of scope for this patch. > > > + > > {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 68392ac41..eaccd85cf 100644 > > --- a/vswitchd/vswitch.xml > > +++ b/vswitchd/vswitch.xml > > @@ -6488,6 +6488,14 @@ 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 <ref table="CT_Zone" column="limit"/> > > + explicitly. If the limit is unspecified the default limit > > + configuration for the datapath 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. > > @@ -6504,6 +6512,12 @@ 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 <ref table="Datapath" column="ct_zone_default_limit"/> will > be used. > > + 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. > > All should be addressed in v8. Thanks, Ales
diff --git a/NEWS b/NEWS index 474b3a4ef..77117573c 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,11 @@ Post-v3.2.0 Reported names adjusted accordingly. * Added support for removal of default CT zone limit, e.g. "ovs-appctl dpctl/ct-del-limits default". + - ovs-vsctl: + * New commands 'set-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 and + '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..c913c5a2d 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([set-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([set-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([set-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([set-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-limit netdev default 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([set-zone-limit netdev default 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-limit netdev default])]) +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])], [0], [dnl +Zone: 10, Limit: 5 +]) + +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-limit netdev default])]) + 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 @@ -1113,16 +1174,39 @@ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1 icmp_reply=2]) ]) AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])]) AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply=3])], - [1], [], [ovs-vsctl: zone id 2 already exists + [1], [], [ovs-vsctl: zone id 2 already has a policy ]) 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([del-zone-tp netdev zone=11])], - [1], [], [ovs-vsctl: zone id 11 does not exist + [1], [], [ovs-vsctl: zone id 11 does not have a policy ]) 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([set-zone-limit netdevxx zone=5 limit=1])], + [1], [], [ovs-vsctl: datapath netdevxx does not exist +]) +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev zone=88888 limit=1])], + [1], [], [ovs-vsctl: zone_id (88888) out of range +]) +AT_CHECK([RUN_OVS_VSCTL([set-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 a limit +]) + +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdevxx default limit=1])], + [1], [], [ovs-vsctl: datapath netdevxx does not exist +]) +AT_CHECK([RUN_OVS_VSCTL([set-zone-limit netdev default limit=-1])], + [1], [], [ovs-vsctl: limit (-1) out of range +]) +AT_CHECK([RUN_OVS_VSCTL([del-zone-limit netdev default])], + [1], [], [ovs-vsctl: datapath netdev does not have a 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..9bc95b82c 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,37 @@ 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 \fIpolicy\fR for +\fIzone_id\fR that already has a policy is an error. + With \fB\-\-may\-exist\fR, this command does nothing if policy for + \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 -does not exist is an error. With \fB\-\-if\-exists\fR, attempting to -delete a zone that does not exist has no effect. +Without \fB\-\-if\-exists\fR, attempting to delete a policy for zone that +does not exist or doesn't have a policy is an error. With +\fB\-\-if\-exists\fR, attempting to delete a a policy that does not +exist has no effect. . .IP "\fBlist\-zone\-tp \fIdatapath\fR" Prints the timeout policies of all zones in \fIdatapath\fR. . +.IP "\fBset\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR|\fBdefault \fBlimit=\fIzone_limit\fR" +Sets a conntrack zone limit with \fIzone_id\fR|\fIdefault\fR in +\fIdatapath\fR. The \fIlimit\fR with value \fB0\fR means unlimited. +.IP +. +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-limit \fIdatapath \fBzone=\fIzone_id\fR|\fBdefault\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 or doesn't have a limit is an error. With \fB\-\-if\-exists\fR, +attempting to delete a limit that does not exist has no effect. +. +.IP "\fBlist\-zone\-limit \fIdatapath\fR" +Prints the limits of all zones in \fIdatapath\fR. +. .SS "Datapath Capabilities Command" The command query datapath capabilities. . diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 5e549df00..cb11bdf00 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -442,6 +442,14 @@ Auto Attach commands:\n\ Switch commands:\n\ emer-reset reset switch to known good state\n\ \n\ +CT zone limit commands:\n\ + set-zone-limit BRIDGE zone_id=ZONE|default limit=LIMIT set CT LIMIT for\ + ZONE|default on BRIDGE\n\ + del-zone-limit BRIDGE zone_id=ZONE|default delete CT limit\ + for ZONE|default on BRIDGE\n\ + list-zone-limit BRIDGE list all limits\ + configured on BRIDGE\n\ +\n\ %s\ %s\ \n\ @@ -1302,8 +1310,8 @@ cmd_add_zone_tp(struct ctl_context *ctx) ctl_fatal("No timeout policy"); } - if (zone && !may_exist) { - ctl_fatal("zone id %"PRIu64" already exists", zone_id); + if (zone && zone->timeout_policy && !may_exist) { + ctl_fatal("zone id %"PRIu64" already has a policy", zone_id); } tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps); @@ -1332,11 +1340,20 @@ cmd_del_zone_tp(struct ctl_context *ctx) } struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id); - if (must_exist && !zone) { - ctl_fatal("zone id %"PRIu64" does not exist", zone_id); + if (must_exist && !(zone && zone->timeout_policy)) { + ctl_fatal("zone id %"PRIu64" does not have a policy", 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 +1388,118 @@ cmd_list_zone_tp(struct ctl_context *ctx) } } +static void +cmd_set_zone_limit(struct ctl_context *ctx) +{ + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx); + int64_t zone_id = -1; + int64_t limit = -1; + + const char *dp_name = ctx->argv[1]; + + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id); + ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit); + + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name); + if (!dp) { + ctl_fatal("datapath %s does not exist", dp_name); + } + + if (limit < 0 || limit > UINT32_MAX) { + ctl_fatal("limit (%"PRIi64") out of range", limit); + } + + if (!strcmp(ctx->argv[2], "default")) { + ovsrec_datapath_set_ct_zone_default_limit(dp, &limit, 1); + return; + } + + if (zone_id < 0 || zone_id > UINT16_MAX) { + ctl_fatal("zone_id (%"PRIi64") out of range", zone_id); + } + + struct ovsrec_ct_zone *zone = find_ct_zone(dp, 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); + } + + if (!strcmp(ctx->argv[2], "default")) { + if (must_exist && !dp->ct_zone_default_limit) { + ctl_fatal("datapath %s does not have a limit", dp_name); + } + + ovsrec_datapath_set_ct_zone_default_limit(dp, NULL, 0); + return; + } + + 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 a 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 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 +3282,14 @@ 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. */ + {"set-zone-limit", 3, 3, "", pre_get_zone, cmd_set_zone_limit, NULL, + "", 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}, + {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 68392ac41..eaccd85cf 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -6488,6 +6488,14 @@ 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 <ref table="CT_Zone" column="limit"/> + explicitly. If the limit is unspecified the default limit + configuration for the datapath 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. @@ -6504,6 +6512,12 @@ 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 <ref table="Datapath" column="ct_zone_default_limit"/> will be used. + 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.
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> --- v7: Rebase on top of current master. Address comments from Ilya: - Add missing 'a'. - Slightly change the format for limit listing. - Add usage string for all the new comamnds. v6: Rebase on top of current master. Address comments from Ilya: - Update the semantics and documentation of the set command. v5: Rebase on top of current master. Address comments from Ilya: - Use only single command for setting zone and default limit. - Correct the errors in the man page. - Use references for the column description. 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 | 5 ++ tests/ovs-vsctl.at | 88 ++++++++++++++++++++++- utilities/ovs-vsctl.8.in | 31 ++++++-- utilities/ovs-vsctl.c | 141 +++++++++++++++++++++++++++++++++++-- vswitchd/vswitch.ovsschema | 14 +++- vswitchd/vswitch.xml | 14 ++++ 6 files changed, 277 insertions(+), 16 deletions(-)