On 10/2/23 12:33, 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]>
> Acked-by: Simon Horman <[email protected]>
> ---
> v3: Rebase on top of current master.
> Add ack from Simon and fix the missing '.'.
Hi, Ales. Thanks for the patch!
See some comemnts inline.
> ---
> NEWS | 2 +
> tests/ovs-vsctl.at | 58 ++++++++++++++++++++
> utilities/ovs-vsctl.8.in | 20 ++++++-
> utilities/ovs-vsctl.c | 108 ++++++++++++++++++++++++++++++++++++-
> vswitchd/vswitch.ovsschema | 9 +++-
> vswitchd/vswitch.xml | 5 ++
> 6 files changed, 198 insertions(+), 4 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 6b45492f1..e86e7f364 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,8 @@ Post-v3.2.0
> from older version is supported but it may trigger more leader
> elections
> during the process, and error logs complaining unrecognized fields may
> be observed on old nodes.
> + - CT:
> + * Add support for setting CT zone limit via DB.
The indentation here is off. However, more importantly, this line is not
user-friendly and it doesn't tell much about what changed and how to use
the functionality. First, please use 'connection tracking' or 'conntrack'
whenever possible instead of obscure 'CT'. Secondly, we should talk about
new commands being added to ovs-vsctl and a new column name in the database,
because these are interfaces users will care about.
Maybe smething like this:
- ovs-vsctl:
* New commands 'add-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.
>
>
> v3.2.0 - 17 Aug 2023
> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> index a368bff6e..b033eaf1f 100644
> --- a/tests/ovs-vsctl.at
> +++ b/tests/ovs-vsctl.at
> @@ -975,6 +975,47 @@ 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([del-zone-tp netdev zone=10])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([list-zone-limit netdev])])
> +
> +AT_CHECK([RUN_OVS_VSCTL([add-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([--may-exist add-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([--may-exist add-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([--may-exist 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([--may-exist add-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([-- --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
> @@ -1123,6 +1164,23 @@ AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=11])],
> 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([add-zone-limit netdevxx zone=5 limit=1])],
> + [1], [], [ovs-vsctl: datapath netdevxx does not exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=88888 limit=1])],
> + [1], [], [ovs-vsctl: zone_id (88888) out of range
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-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 exist
> +])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=1])])
> +AT_CHECK([RUN_OVS_VSCTL([add-zone-limit netdev zone=5 limit=2])],
> + [1], [], [ovs-vsctl: zone_id 5 already exists
> +])
> +
> 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..e6c0d6b2c 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
> @@ -379,6 +379,24 @@ delete a zone that does not exist has no effect.
> .IP "\fBlist\-zone\-tp \fIdatapath\fR"
> Prints the timeout policies of all zones in \fIdatapath\fR.
> .
> +.IP "[\fB\-\-may\-exist\fR] \fBadd\-zone\-limit \fIdatapath
> \fBzone=\fIzone_id \fBlimit=\fIzone_limit"
> +Sets a conntrack zone limit with \fIzone_id\fR in
> +\fIdatapath\fR. The \fIlimit\fR with value \fB0\fR means unlimited.
> +.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 updates the \fIlimit\fR if \fIzone_id\fR already exists.
> +.
> +.IP "[\fB\-\-if\-exists\fR] \fBdel\-zone\-imit \fIdatapath
> \fBzone=\fIzone_id\fR"
> +Delete the limit 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.
These restrictions look strange to me.
I would expect following two commands to both work and regardless
of their order:
ovs-vsctl add-zone-tp netdev zone=10 icmp_first=1 icmp_reply=2
ovs-vsctl add-zone-limit netdev zone=10 limit=1
>From the user-facing API point of view these commands are not
related to each other, but the second one will always fail without
--may-exist in the current implementation. add-zone-limit should
fail only if the limit exists. add-zone-tp should fail only if
timeout policy already exists. But they should not fail if only
the other type of resource exists. Both values in the database
are nullable, so it should be possible to track that without much
trouble.
BTW, the following should work as well:
ovs-vsctl add-zone-tp netdev zone=10 icmp_first=1 icmp_reply=2 \
-- add-zone-limit netdev zone=10 limit=1
> +.
> +.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..e6b51459f 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -1336,7 +1336,16 @@ cmd_del_zone_tp(struct ctl_context *ctx)
> ctl_fatal("zone id %"PRIu64" does not exist", 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 +1380,101 @@ cmd_list_zone_tp(struct ctl_context *ctx)
> }
> }
>
> +static void
> +cmd_add_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];
> + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
An empty line would be nice. Also, reverse x-mass tree.
> + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> + ovs_scan(ctx->argv[3], "limit=%"SCNi64, &limit);
> +
> + if (zone_id < 0 || zone_id > UINT16_MAX) {
> + ctl_fatal("zone_id (%"PRIi64") out of range", zone_id);
> + }
> +
> + if (limit < 0 || limit > UINT32_MAX) {
> + ctl_fatal("limit (%"PRIi64") out of range", limit);
> + }
> +
> + struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> + if (!dp) {
> + ctl_fatal("datapath %s does not exist", dp_name);
> + }
> +
> + struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> + if (zone && !may_exist) {
> + ctl_fatal("zone_id %"PRIi64" already exists", zone_id);
This is strange.
> + }
> +
> + 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];
Empty line.
> + 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);
> + }
> +
> + struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> + if (must_exist && !zone) {
> + ctl_fatal("zone_id %"PRIi64" does not exist", 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]);
> + }
> +
> + 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_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 +3257,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. */
> + {"add-zone-limit", 3, 3, "", pre_get_zone, cmd_add_zone_limit, NULL,
> + "--may-exist", 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..ed90c57d0 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": "2723382443 27334",
> "tables": {
> "Open_vSwitch": {
> "columns": {
> @@ -679,6 +679,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 cfcde34ff..d1674170a 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 the limit is unspecified
A period is missing?
> + the datapath configuration is left intact. The value 0 means unlimited.
> + </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.
Also, might make sense to list columns in the same order they are
in the schema.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
