On 11/2/23 13:00, 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 <[email protected]>
> ---
> 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 | 133 +++++++++++++++++++++++++++++++++++--
> vswitchd/vswitch.ovsschema | 14 +++-
> vswitchd/vswitch.xml | 13 ++++
> 6 files changed, 268 insertions(+), 16 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 7bc27b687..61b48ff12 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,11 @@ Post-v3.2.0
> - ovs-appctl:
> * Added support 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..f88e986db 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 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 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 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..2cf569663 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1302,8 +1302,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 +1332,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 policy", zone_id);
'a policy'
> }
>
> - 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 +1380,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 limit", dp_name);
'a limit'
> + }
> +
> + 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 limit", zone_id);
'a limit'
> + }
> +
> + 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",
Nit: since he output is different from the dpctl anyway,
maybe it's better to print something like 'Default, Limit: N'
instead to be more inline with non-default limits? WDYT?
> + *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 +3274,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},
I just noticed that these commands do not have a usage string for some reson.
And we'll also need a usege() function update.
I see that TP commands are also not ducumented there, but that's a separate
issue.
> +
> {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 e400043ce..05af24acf 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -6465,6 +6465,13 @@ 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 datapath for default
> + limit configuration is left intact. The value 0 means unlimited.
'the datapath for default limit' ? Maybe 'default limit for the datapath'?
And maybe a comma.
> + </column>
> +
> <group title="Common Columns">
> The overall purpose of these columns is described under <code>Common
> Columns</code> at the beginning of this document.
> @@ -6481,6 +6488,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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
