Re: [ovs-dev] [PATCH 10/11] dpctl: Implement dpctl commands for conntrack per zone limit
sorry, totally missed this e-mail On Thu, Aug 2, 2018 at 2:35 PM, Yi-Hung Wei wrote: > On Wed, Aug 1, 2018 at 10:37 PM, Darrell Ball wrote: > > On Wed, Aug 1, 2018 at 3:46 PM, Yi-Hung Wei > wrote: > > > > Do these commands need a ‘zone’ keyword > > > > eg) ct-set-zone-limits > > > >> > >> $ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 > zone=1,limit=3 > > I think I am fine either way. I can change ct-set-zone-limits to > ct-set-limits in later version. > > > > > I wonder if it makes sense to write > > > > ‘zone=default,limit=10’ > > > > so that ‘default’ is treated like any zone? > That makes sense, and I think it does simplify the syntax. Will make > this change in v3. > I can add a quick incremental later as well. > > >> > >> $ ovs-appctl dpct/ct-del-limits zone=0 > > > > > > > > I wonder if we set zone limit to zero (unlimited), it could be like > deleting > > a zone limit. > > > > $ ovs-appctl dpctl/ct-set-limits zone=1,limit=0 > > The default limit may change from time to time, it is not necessarily > to be zero (unlimited). > I understand; more generally: If these special values were strings at the dpctl level then it would be more obvious what I meant. i.e. ovs-appctl dpctl/ct-zone-set-limits zone=1,limit=unlimited ovs-appctl dpctl/ct-zone-set-limits zone=1,limit=default (i.e. "delete") ovs-appctl dpctl/ct-zone-set-limits zone=1,limit=4 The internal values could be unlimited: 0 default: -1 or whatever. > > On the other hand, we may set limit to 'default', such as having > commands like $ ovs-appctl dpctl/ct-set-limits zone=1,limit=default > zone=2,limit=default zone=3,limit=default > But I understand your time constraint, so don't worry about it for your patchset. > > But it seems to be easier to have syntax like $ ovs-appctl > dpctl/ct-del-lmits zone=1,2,3 > > Therefore, I prefer to keep the ct-del-limits command as is. > > > Thanks, > > -Yi-Hung > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 10/11] dpctl: Implement dpctl commands for conntrack per zone limit
On Wed, Aug 1, 2018 at 10:37 PM, Darrell Ball wrote: > On Wed, Aug 1, 2018 at 3:46 PM, Yi-Hung Wei wrote: > > Do these commands need a ‘zone’ keyword > > eg) ct-set-zone-limits > >> >> $ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 zone=1,limit=3 I think I am fine either way. I can change ct-set-zone-limits to ct-set-limits in later version. > > I wonder if it makes sense to write > > ‘zone=default,limit=10’ > > so that ‘default’ is treated like any zone? That makes sense, and I think it does simplify the syntax. Will make this change in v3. >> >> $ ovs-appctl dpct/ct-del-limits zone=0 > > > > I wonder if we set zone limit to zero (unlimited), it could be like deleting > a zone limit. > > $ ovs-appctl dpctl/ct-set-limits zone=1,limit=0 The default limit may change from time to time, it is not necessarily to be zero (unlimited). On the other hand, we may set limit to 'default', such as having commands like $ ovs-appctl dpctl/ct-set-limits zone=1,limit=default zone=2,limit=default zone=3,limit=default But it seems to be easier to have syntax like $ ovs-appctl dpctl/ct-del-lmits zone=1,2,3 Therefore, I prefer to keep the ct-del-limits command as is. Thanks, -Yi-Hung ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 10/11] dpctl: Implement dpctl commands for conntrack per zone limit
Thanks for the patch Yi-hung
On Wed, Aug 1, 2018 at 3:46 PM, Yi-Hung Wei wrote:
> This patch implments the following three commands on dpctl so that
> users can use ovs-dpctl or ovs-appctl to set, delete, and get the
> per zone limit.
>
> For example,
>
Do these commands need a ‘zone’ keyword
eg) ct-set-zone-limits
>
> $ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 zone=1,limit=3
>
I wonder if it makes sense to write
‘zone=default,limit=10’
so that ‘default’ is treated like any zone?
> $ ovs-appctl dpct/ct-del-limits zone=0
>
I wonder if we set zone limit to zero (unlimited), it could be like
deleting a zone limit.
$ ovs-appctl dpctl/ct-set-limits zone=1,limit=0
in which case, we don’t need a special command for delete, such as:
$ ovs-appctl dpct/ct-del-limits zone=1
So, we would have just ‘set’ and ‘get’ commands remaining.
> $ ovs-appctl dpct/ct-get-limits zone=1,2,3
>
> Signed-off-by: Yi-Hung Wei
> ---
> NEWS | 2 +
> lib/ct-dpif.c | 67 +++
> lib/ct-dpif.h | 4 ++
> lib/dpctl.c | 169 ++
> +++-
> lib/dpctl.man | 18 +++
> 5 files changed, 259 insertions(+), 1 deletion(-)
>
> diff --git a/NEWS b/NEWS
> index 8270ef46ea34..31c5a1e400dc 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -19,6 +19,8 @@ v2.10.0 - xx xxx
> default it always accepts names and in interactive use it displays
> them;
> use --names or --no-names to override. See ovs-ofctl(8) for
> details.
> - ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
> + - ovs-dpctl:
> + * New commands "ct-set-limits", "ct-del-limits", and "ct-get-limits".
> - OpenFlow:
> * OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
> * OpenFlow 1.5 extensible statistics (OXS) now implemented.
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index a772799fe347..bb809d9920b5 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -629,3 +629,70 @@ ct_dpif_free_zone_limits(struct ovs_list
> *zone_limits)
> free(p);
> }
> }
> +
> +/* Parses a specification of a conntrack zone limit from 's' into '*pzone'
> + * and '*plimit'. Returns true on success. Otherwise, returns false and
> + * and puts the error message in 'ds'. */
> +bool
> +ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
> + uint32_t *plimit, struct ds *ds)
> +{
> +char *pos, *key, *value, *copy, *err;
> +bool parsed_limit = false, parsed_zone = false;
> +
> +pos = copy = xstrdup(s);
> +while (ofputil_parse_key_value(, , )) {
> +if (!*value) {
> +ds_put_format(ds, "field %s missing value", key);
> +goto error;
> +}
> +
> +if (!strcmp(key, "zone")) {
> +err = str_to_u16(value, key, pzone);
> +if (err) {
> +free(err);
> +goto error_with_msg;
> +}
> +parsed_zone = true;
> +} else if (!strcmp(key, "limit")) {
> +err = str_to_u32(value, plimit);
> +if (err) {
> +free(err);
> +goto error_with_msg;
> +}
> +parsed_limit = true;
> +} else {
> +ds_put_format(ds, "invalid zone limit field: %s", key);
> +goto error;
> +}
> +}
> +
> +if (parsed_zone == false || parsed_limit == false) {
> +ds_put_format(ds, "failed to parse zone limit");
> +goto error;
> +}
> +
> +free(copy);
> +return true;
> +
> +error_with_msg:
> +ds_put_format(ds, "failed to parse field %s", key);
> +error:
> +free(copy);
> +return false;
> +}
> +
> +void
> +ct_dpif_format_zone_limits(uint32_t default_limit,
> + const struct ovs_list *zone_limits, struct ds
> *ds)
> +{
> +struct ct_dpif_zone_limit *zone_limit;
> +
> +ds_put_format(ds, "default_limit=%"PRIu32, default_limit);
> +
> +LIST_FOR_EACH (zone_limit, node, zone_limits) {
> +ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone);
> +ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit);
> +ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
> +}
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index c80e18b72b56..c9cfb258b133 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -223,5 +223,9 @@ void ct_dpif_push_zone_limit(struct ovs_list *,
> uint16_t zone, uint32_t limit,
> uint32_t count);
> struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *);
> void ct_dpif_free_zone_limits(struct ovs_list *);
> +bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
> +uint32_t *plimit, struct ds *);
> +void ct_dpif_format_zone_limits(uint32_t default_limit,
> +const struct ovs_list *, struct ds *);
>
> #endif /* CT_DPIF_H */
> diff --git
[ovs-dev] [PATCH 10/11] dpctl: Implement dpctl commands for conntrack per zone limit
This patch implments the following three commands on dpctl so that
users can use ovs-dpctl or ovs-appctl to set, delete, and get the
per zone limit.
For example,
$ ovs-appctl dpctl/ct-set-limits default=10 zone=0,limit=5 zone=1,limit=3
$ ovs-appctl dpct/ct-del-limits zone=0
$ ovs-appctl dpct/ct-get-limits zone=1,2,3
Signed-off-by: Yi-Hung Wei
---
NEWS | 2 +
lib/ct-dpif.c | 67 +++
lib/ct-dpif.h | 4 ++
lib/dpctl.c | 169 +-
lib/dpctl.man | 18 +++
5 files changed, 259 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index 8270ef46ea34..31c5a1e400dc 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,8 @@ v2.10.0 - xx xxx
default it always accepts names and in interactive use it displays them;
use --names or --no-names to override. See ovs-ofctl(8) for details.
- ovs-vsctl: New commands "add-bond-iface" and "del-bond-iface".
+ - ovs-dpctl:
+ * New commands "ct-set-limits", "ct-del-limits", and "ct-get-limits".
- OpenFlow:
* OFPT_ROLE_STATUS is now available in OpenFlow 1.3.
* OpenFlow 1.5 extensible statistics (OXS) now implemented.
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index a772799fe347..bb809d9920b5 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -629,3 +629,70 @@ ct_dpif_free_zone_limits(struct ovs_list *zone_limits)
free(p);
}
}
+
+/* Parses a specification of a conntrack zone limit from 's' into '*pzone'
+ * and '*plimit'. Returns true on success. Otherwise, returns false and
+ * and puts the error message in 'ds'. */
+bool
+ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
+ uint32_t *plimit, struct ds *ds)
+{
+char *pos, *key, *value, *copy, *err;
+bool parsed_limit = false, parsed_zone = false;
+
+pos = copy = xstrdup(s);
+while (ofputil_parse_key_value(, , )) {
+if (!*value) {
+ds_put_format(ds, "field %s missing value", key);
+goto error;
+}
+
+if (!strcmp(key, "zone")) {
+err = str_to_u16(value, key, pzone);
+if (err) {
+free(err);
+goto error_with_msg;
+}
+parsed_zone = true;
+} else if (!strcmp(key, "limit")) {
+err = str_to_u32(value, plimit);
+if (err) {
+free(err);
+goto error_with_msg;
+}
+parsed_limit = true;
+} else {
+ds_put_format(ds, "invalid zone limit field: %s", key);
+goto error;
+}
+}
+
+if (parsed_zone == false || parsed_limit == false) {
+ds_put_format(ds, "failed to parse zone limit");
+goto error;
+}
+
+free(copy);
+return true;
+
+error_with_msg:
+ds_put_format(ds, "failed to parse field %s", key);
+error:
+free(copy);
+return false;
+}
+
+void
+ct_dpif_format_zone_limits(uint32_t default_limit,
+ const struct ovs_list *zone_limits, struct ds *ds)
+{
+struct ct_dpif_zone_limit *zone_limit;
+
+ds_put_format(ds, "default_limit=%"PRIu32, default_limit);
+
+LIST_FOR_EACH (zone_limit, node, zone_limits) {
+ds_put_format(ds, " zone=%"PRIu16, zone_limit->zone);
+ds_put_format(ds, ",limit=%"PRIu32, zone_limit->limit);
+ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
+}
+}
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index c80e18b72b56..c9cfb258b133 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -223,5 +223,9 @@ void ct_dpif_push_zone_limit(struct ovs_list *, uint16_t
zone, uint32_t limit,
uint32_t count);
struct ct_dpif_zone_limit * ct_dpif_pop_zone_limit(struct ovs_list *);
void ct_dpif_free_zone_limits(struct ovs_list *);
+bool ct_dpif_parse_zone_limit_tuple(const char *s, uint16_t *pzone,
+uint32_t *plimit, struct ds *);
+void ct_dpif_format_zone_limits(uint32_t default_limit,
+const struct ovs_list *, struct ds *);
#endif /* CT_DPIF_H */
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 35733774b331..560f713cfd51 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -199,7 +199,7 @@ parsed_dpif_open(const char *arg_, bool create, struct dpif
**dpifp)
* to be parsed in '*indexp'. */
static int
opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p,
- uint8_t max_args, struct dpif **dpifp, bool multi_opt,
+ int max_args, struct dpif **dpifp, bool multi_opt,
int *indexp)
{
char *dpname;
@@ -1683,6 +1683,167 @@ dpctl_ct_get_nconns(int argc, const char *argv[],
return error;
}
+static int
+dpctl_ct_set_limits(int argc, const char *argv[],
+struct dpctl_params *dpctl_p)
+{
+struct dpif *dpif;
+struct ds ds = DS_EMPTY_INITIALIZER;
+int error, i = 1;
+uint32_t default_limit,
