[ovs-dev] [PATCH v2 2/9] ovs-vsctl: Add conntrack zone commands.
Darrell Ball
dlu998 at gmail.com
Wed Aug 7 23:19:35 UTC 2019
On Wed, Aug 7, 2019 at 1:37 PM William Tu <u9012063 at gmail.com> wrote:
> Thanks for the review.
>
> On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote:
> > Thanks for the patch
> >
> > I noticed '--may-exist' and '--if-exists' are supported now for
> > add--zone-tp/del-zone-tp - thanks
> > The check for duplicate timeout policies now correctly checks all key and
> > values - thanks
>
> yes, thanks. Will do it in next version.
> >
> > Some more comments inline
> > I am trying to avoid duplicate comment from Justin, so I just won't
> comment
> > on some parts in this version
> > to avoid confusion.
> >
> >
> > On Thu, Aug 1, 2019 at 3:09 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> >
> > > From: William Tu <u9012063 at gmail.com>
> > >
> > > The patch adds commands creating/deleting/listing conntrack zone
> > > timeout policies:
> > > $ ovs-vsctl {add,del,list}-zone-tp zone=zone_id ...
> > >
> > > Signed-off-by: William Tu <u9012063 at gmail.com>
> > > ---
> > > tests/ovs-vsctl.at | 34 +++++++-
> > > utilities/ovs-vsctl.8.in | 25 ++++++
> > > utilities/ovs-vsctl.c | 204
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 261 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > > index 46fa3c5b1a33..f0c5975edd0e 100644
> > > --- a/tests/ovs-vsctl.at
> > > +++ b/tests/ovs-vsctl.at
> > > @@ -805,6 +805,20 @@ AT_CHECK(
> > > [RUN_OVS_VSCTL([--if-exists remove netflow x targets '"1.2.3.4:567
> > > "'])])
> > > AT_CHECK(
> > > [RUN_OVS_VSCTL([--if-exists clear netflow x targets])])
> > > +
> > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath
> datapath_version=0 --
> > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> > >
> >
> > What happens if we add a datapath type here and there are no bridges of
> > that type; meaning the datapath of that type does not even exist.
> > Seems like a contradiction.
> > Maybe we should check for that at least and raise an error.
> > Ideally, it is better if these 'datapaths' are auto-managed by bridge
> > creation/deletion with given datapath types,
> > but we can certainly defer that.
> >
> >
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > > icmp_reply=2])])
> > >
> >
> > I mentioned this in V1:
> > There is no filtering of bad timeout key; for example
> >
> > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > foo_bar=2])])
> >
> > is accepted as valid
> >
> > Even worse, a minor typo will go unnoticed - missing 'y' in 'icmp_reply'.
> >
> > AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > icmp_repl=2])])
>
> agree.
> >
> >
> > > +AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=1
> > > icmp_first=1 icmp_reply=2])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > > Policies: icmp_first=1 icmp_reply=2
> > >
> >
> > I mentioned in V1
> > We should check all possible timeout keys to make sure they work.
>
> OK.
> >
> >
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> > > icmp_reply=3])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:1, Timeout
> > > Policies: icmp_first=1 icmp_reply=2
> > > +Zone:2, Timeout Policies: icmp_first=2 icmp_reply=3
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> > > +AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > > +])
> > > OVS_VSCTL_CLEANUP
> > > AT_CLEANUP
> > >
> > > @@ -890,10 +904,10 @@ AT_CHECK([RUN_OVS_VSCTL([set bridge br0
> > > flood_vlans=-1])],
> > > AT_CHECK([RUN_OVS_VSCTL([set bridge br0 flood_vlans=4096])],
> > > [1], [], [ovs-vsctl: constraint violation: 4096 is not in the valid
> > > range 0 to 4095 (inclusive)
> > > ])
> > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 'connection-mode=xyz'])],
> > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 'connection-mode=xyz'])],
> > >
> >
> > I mentioned in V1.
> > I don't think we should make unrelated changes in a feature patch
> > especially since it seems the author wanted to convey
> > short form syntax is valid
>
> This has to be there, otherwise test fails.
>
yep, I got the obvious reason after running the negative test.
+ovs-vsctl: multiple table names match "c"
> >
> >
> > > [1], [], [[ovs-vsctl: constraint violation: xyz is not one of the
> > > allowed values ([in-band, out-of-band])
> > > ]])
> > > -AT_CHECK([RUN_OVS_VSCTL([set c br1 connection-mode:x=y])],
> > > +AT_CHECK([RUN_OVS_VSCTL([set controller br1 connection-mode:x=y])],
> > > [1], [], [ovs-vsctl: cannot specify key to set for non-map column
> > > connection_mode
> > > ])
> > > AT_CHECK([RUN_OVS_VSCTL([add bridge br1 datapath_id x y])],
> > > @@ -929,6 +943,22 @@ AT_CHECK([RUN_OVS_VSCTL([remove bridge br1
> > > flood-vlans true])],
> > > AT_CHECK([RUN_OVS_VSCTL([clear bridge br1 name])],
> > > [1], [], [ovs-vsctl: cannot modify read-only column name in table
> Bridge
> > > ])
> > > +
> > > +AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath
> datapath_version=0 --
> > > set Open_vSwitch . datapaths:"netdev"=@m])], [0], [stdout])
> > > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > > icmp_reply=2])],
> > > + [1], [], [ovs-vsctl: datapath: netdevxx record not found
> > > +])
> > > +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 alread exists
> > > +])
> > > +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 not exists.
> > > +])
> > > +AT_CHECK([RUN_OVS_VSCTL([list-zone-tp netdev])], [0], [Zone:2, Timeout
> > > Policies: icmp_first=2 icmp_reply=3
> > >
> >
> > Ideally, we should be able to list one zone, but I know I did not mention
> > that before, so lets defer that or not worry about it
> > altogether.
> >
> >
> > > +])
> > > OVS_VSCTL_CLEANUP
> > > AT_CLEANUP
> > >
> > > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > > index 7c09df79bd29..0925d4d97b39 100644
> > > --- a/utilities/ovs-vsctl.8.in
> > > +++ b/utilities/ovs-vsctl.8.in
> > > @@ -353,6 +353,31 @@ list.
> > > 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.
> > > +.
> > > +.IP "\fBadd\-zone\-tp \fIdatapath \fBzone=\fIzone_id \fIpolicies\fR"
> > > +Creates a conntrack zone with \fIzone_id\fR under the datapath
> > > \fIdatapath\fR.
> > > +Associate the conntrack timeout policies to it by a list of
> > > +\fIkey\fB=\fIvalue\fR pairs, separeted by space. For example,
> specifying
> > > +30-second timeout policy for first icmp packet, and 60-second for icmp
> > > reply packet
> > > +by doing \fBicmp_first=30 icmp_reply=60\fR. See CT_Timeout_Policy
> TABLE
> > > +at \fBovs-vswitchd.conf.db\fR(5) for all available configurations.
> > > +.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 is already created\fR.
> > > +.
> > > +.IP "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
> > > +Delete a zone under \fIdatapath\fR by specifying its zone ID.
> > > +.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.
> > > +.
> > > +.IP "\fBlist\-zone\-tp \fIdatapath\fR"
> > > +Prints the timeout policies of all zones under the \fIdatapath\fR.
> > > +.
> > > .SS "OpenFlow Controller Connectivity"
> > > .
> > > \fBovs\-vswitchd\fR can perform all configured bridging and switching
> > > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > > index 4948137efe8c..16578edfc331 100644
> > > --- a/utilities/ovs-vsctl.c
> > > +++ b/utilities/ovs-vsctl.c
> > > @@ -40,6 +40,7 @@
> > > #include "ovsdb-idl.h"
> > > #include "openvswitch/poll-loop.h"
> > > #include "process.h"
> > > +#include "simap.h"
> > > #include "stream.h"
> > > #include "stream-ssl.h"
> > > #include "smap.h"
> > > @@ -49,6 +50,7 @@
> > > #include "table.h"
> > > #include "timeval.h"
> > > #include "util.h"
> > > +#include "openvswitch/ofp-parse.h"
> > > #include "openvswitch/vconn.h"
> > > #include "openvswitch/vlog.h"
> > >
> > > @@ -1153,6 +1155,201 @@ cmd_emer_reset(struct ctl_context *ctx)
> > > vsctl_context_invalidate_cache(ctx);
> > > }
> > >
> > > +static struct ovsrec_datapath *
> > > +find_datapath(struct vsctl_context *vsctl_ctx, const char *dp_name)
> > > +{
> > > + const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
> > > + int i;
> > > +
> > > + for (i = 0; i < ovs->n_datapaths; i++) {
> > > + if (!strcmp(ovs->key_datapaths[i], dp_name)) {
> > > + return ovs->value_datapaths[i];
> > > + }
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > +static struct ovsrec_ct_zone *
> > > +find_ct_zone(struct ovsrec_datapath *dp, const int64_t zone_id)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < dp->n_ct_zones; i++) {
> > > + if (dp->key_ct_zones[i] == zone_id) {
> > > + return dp->value_ct_zones[i];
> > > + }
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > +static struct ovsrec_ct_timeout_policy *
> > > +create_timeout_policy(struct ctl_context *ctx, char **argv, int n_tps)
> > > +{
> > > + const struct ovsrec_ct_timeout_policy_table *tp_table;
> > > + const struct ovsrec_ct_timeout_policy *row;
> > > + struct ovsrec_ct_timeout_policy *tp = NULL;
> > > + const char **key_timeouts;
> > > + int64_t *value_timeouts;
> > > + struct simap new_tp, s;
> > > + int i, j;
> > > +
> > > + simap_init(&new_tp);
> > > +
> > > + key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > > + value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > > +
> > > + /* Parse timeout arguments. */
> > > + for (i = 0; i < n_tps; i++) {
> > > + char *key, *value, *pos, *copy;
> > > +
> > > + pos = copy = xstrdup(argv[i]);
> > > + if (!ofputil_parse_key_value(&pos, &key, &value)) {
> > > + goto done;
> > > + }
> > > + key_timeouts[i] = key;
> > > + value_timeouts[i] = atoi(value);
> > > + simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> > >
> >
> > Be careful how you free the string to be parsed...
> > If still not fixed in V3, we can deal with it.
> >
> >
> > > + }
> > > +done:
> > > +
> > > + tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > > + OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > > + simap_init(&s);
> > > + /* Covert to simap. */
> > > + for (j = 0; j < row->n_timeouts; j++) {
> > > + simap_put(&s, row->key_timeouts[j],
> row->value_timeouts[j]);
> > > + }
> > > +
> > > + if (simap_equal(&s, &new_tp)) {
> > > + tp = CONST_CAST(struct ovsrec_ct_timeout_policy *, row);
> > > + simap_destroy(&s);
> > > + break;
> > > + }
> > > + simap_destroy(&s);
> > > + }
> > > +
> > > + if (!tp) {
> > > + tp = ovsrec_ct_timeout_policy_insert(ctx->txn);
> > > + ovsrec_ct_timeout_policy_set_timeouts(tp, key_timeouts,
> > > + (const int64_t
> > > *)value_timeouts,
> > > + n_tps);
> > > + }
> > > +
> > > + free(key_timeouts);
> > > + free(value_timeouts);
> > > + return tp;
> > > +}
> > > +
> > > +static void
> > > +cmd_add_zone(struct ctl_context *ctx)
> > > +{
> > > + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > + struct ovsrec_ct_timeout_policy *tp;
> > > + struct ovsrec_ct_zone *zone;
> > > + struct ovsrec_datapath *dp;
> > > + const char *dp_name;
> > > + int64_t zone_id;
> > > + bool may_exist;
> > > + int n_tps;
> > > +
> > > + dp_name = ctx->argv[1];
> > > + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > + may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > > +
> > > + dp = find_datapath(vsctl_ctx, dp_name);
> > > + if (!dp) {
> > > + ctl_fatal("datapath: %s record not found", dp_name);
> > > + return;
> > > + }
> > > +
> > > + n_tps = ctx->argc - 3;
> > > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > > +
> > > + zone = find_ct_zone(dp, zone_id);
> > > + if (zone) {
> > > + if (!may_exist) {
> > > + ctl_fatal("zone id %"PRIu64" alread exists", zone_id);
> > >
> >
> > In this error case, we have already created a new timeout policy above,
> > here:
> >
> > + tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> >
> > I do not think that is a good idea.
> > If it is an error, don't create anything.
>
> OK I will fix it.
> >
> >
> >
> > > + return;
> > > + }
> > > + ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > > + } else {
> > > + zone = ovsrec_ct_zone_insert(ctx->txn);
> > > + ovsrec_ct_zone_set_timeout_policy(zone, tp);
> > > + ovsrec_datapath_update_ct_zones_setkey(dp, zone_id, zone);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +cmd_del_zone(struct ctl_context *ctx)
> > > +{
> > > + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > + struct ovsrec_ct_zone *zone;
> > > + struct ovsrec_datapath *dp;
> > > + const char *dp_name;
> > > + int64_t zone_id;
> > > + bool must_exist;
> > > +
> > > + must_exist = !shash_find(&ctx->options, "--if-exists");
> > > + dp_name = ctx->argv[1];
> > > + ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > > +
> > > + dp = find_datapath(vsctl_ctx, dp_name);
> > > + if (!dp) {
> > > + ctl_fatal("datapath: %s record not found.", dp_name);
> > > + return;
> > > + }
> > > +
> > > + zone = find_ct_zone(dp, zone_id);
> > > + if (must_exist && !zone) {
> > > + ctl_fatal("zone id %"PRIu64" not exists.", zone_id);
> > > + return;
> > > + }
> > > +
> > > + if (zone) {
> > > + ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +cmd_list_zone(struct ctl_context *ctx)
> > > +{
> > > + struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > > + struct ovsrec_ct_timeout_policy *tp;
> > > + struct ovsrec_ct_zone *zone;
> > > + struct ovsrec_datapath *dp;
> > > + int i, j;
> > > +
> > > + dp = find_datapath(vsctl_ctx, ctx->argv[1]);
> > > + if (!dp) {
> > > + ctl_fatal("datapath: %s record not found", ctx->argv[1]);
> > > + return;
> > > + }
> > > +
> > > + for (i = 0; i < dp->n_ct_zones; i++) {
> > > + zone = dp->value_ct_zones[i];
> > > + ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout
> Policies: ",
> > > + dp->key_ct_zones[i]);
> > > +
> > > + tp = zone->timeout_policy;
> > > +
> > > + for (j = 0; j < tp->n_timeouts - 1; j++) {
> > > + ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > > + tp->key_timeouts[j], tp->value_timeouts[j]);
> > > + }
> > > + ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > > + tp->key_timeouts[j], tp->value_timeouts[j]);
> > >
> >
> > What exactly are we printing above when the number of timeouts is zero ?
> >
> > Maybe something like this:
> >
> > for (j = 0; j < tp->n_timeouts ; j++) {
> > ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > tp->key_timeouts[j], tp->value_timeouts[j]);
> > }
> > ds_put_format(&ctx->output, "\n");
> >
> > will be better.
>
> OK Will fix it.
>
> Thank you for your review.
> William
> >
> >
> >
> > > + }
> > > +}
> > > +
> > > +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_timeout_policy_col_timeouts);
> > > +}
> > > +
> > > static void
> > > cmd_add_br(struct ctl_context *ctx)
> > > {
> > > @@ -2896,6 +3093,13 @@ static const struct ctl_command_syntax
> > > vsctl_commands[] = {
> > > /* Switch commands. */
> > > {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL,
> > > "", RW},
> > >
> > > + /* Zone and CT Timeout Policy commands. */
> > > + {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone, NULL,
> > > + "--may-exist", RW},
> > > + {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL,
> > > + "--if-exists", RW},
> > > + {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone, NULL, "",
> RO},
> > > +
> > > {NULL, 0, 0, NULL, NULL, NULL, NULL, NULL, RO},
> > > };
> > >
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list