[ovs-dev] [PATCH v3 2/9] ovs-vsctl: Add conntrack zone commands.

William Tu u9012063 at gmail.com
Thu Aug 15 17:53:28 UTC 2019


Thanks for the review.

On Mon, Aug 12, 2019 at 09:54:42PM -0700, Darrell Ball wrote:
> Thanks for the patch
> 
> Thanks for the fixups; mostly minor comments inline.
> 
> On Mon, Aug 12, 2019 at 5:53 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 dp zone=zone_id ...
> >
> > Signed-off-by: William Tu <u9012063 at gmail.com>
> > ---
> >  tests/ovs-vsctl.at       |  34 ++++++++-
> >  utilities/ovs-vsctl.8.in |  26 +++++++
> >  utilities/ovs-vsctl.c    | 194
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 252 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index 46fa3c5b1a33..df15fb6901a0 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])
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=1 icmp_first=1
> > icmp_reply=2])])
> > +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
> > +])
> > +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'])],
> >    [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])
> >
> 
> If I execute
> 
> AT_CHECK([RUN_OVS_VSCTL([-- --id=@m create Datapath datapath_version=0 --
> set Open_vSwitch . datapaths:"netdevvv"=@m])], [0], [stdout])
> 
> it works, but there is no such datapath type 'netdevvv'
> 
this is using database command, I don't think we should enforce it.

> I think it would be better to enforce an enum here as well thru the schema,
> as I mentioned in V2, since this handles errors better.
> This is actually the same idea as enforcing enum for timeout keys that was
> done for V3 to block bad timeout keys like "foo_bar=3"
> I commented patch 1 anyways.
> 
> 
> > +AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdevxx zone=1 icmp_first=1
> > icmp_reply=2])],
> > +  [1], [], [ovs-vsctl: datapath netdevxx does not exist
> > +])
> > +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
> > +])
> > +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
> > +])
> > +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
> >
> > diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
> > index 7c09df79bd29..5b9883ae1c3d 100644
> > --- a/utilities/ovs-vsctl.8.in
> > +++ b/utilities/ovs-vsctl.8.in
> > @@ -353,6 +353,32 @@ 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 "[\fB\-\-may\-exist\fR] \fBadd\-zone\-tp \fIdatapath
> > \fBzone=\fIzone_id \fIpolicies\fR"
> > +Creates a conntrack zone timeout policy with \fIzone_id\fR in
> > +\fIdatapath\fR.  The \fIpolicies\fR consist of \fIkey\fB=\fIvalue\fR
> > +pairs, separated by spaces.  For example, \fBicmp_first=30
> > +icmp_reply=60\fR specifies a 30-second timeout policy for the first ICMP
> > +packet and a 60-second policy for ICMP reply packet.  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 is already created\fR.
> > +.
> > +.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.
> > +.
> > +.IP "\fBlist\-zone\-tp \fIdatapath\fR"
> > +Prints the timeout policies of all zones in \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..7419f723804c 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 ordering is not right
> 
OK
> 
> >  #include "openvswitch/vconn.h"
> >  #include "openvswitch/vlog.h"
> >
> > @@ -1153,6 +1155,191 @@ 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;
> >
> 
> remove
> 
> 
> 
> > +
> > +    for (i = 0; i < ovs->n_datapaths; i++) {
> >
> 
> +    for (int 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;
> >
> 
> remove
> 
> 
> > +
> > +    for (i = 0; i < dp->n_ct_zones; i++) {
> 
> 
> +    for (int 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)
> >
> 
> recommend checking for n_tps of 0 and bailing out early
> Could also check in the caller cmd_add_zone_tp().
OK
> 
> 
> > +{
> > +    const struct ovsrec_ct_timeout_policy_table *tp_table;
> > +    const struct ovsrec_ct_timeout_policy *row;
> > +    struct ovsrec_ct_timeout_policy *tp = NULL;
> >
> 
> I think the above 3 declarations could be moved after 'done:' label.
> 
> +    struct simap new_tp = SIMAP_INITIALIZER(&new_tp);
> > +
> > +    char **policies = xzalloc(sizeof *policies * n_tps);
> > +    const char **key_timeouts = xmalloc(sizeof *key_timeouts * n_tps);
> > +    int64_t *value_timeouts = xmalloc(sizeof *value_timeouts * n_tps);
> > +
> > +    /* Parse timeout arguments. */
> > +    for (int i = 0; i < n_tps; i++) {
> > +        policies[i] = xstrdup(argv[i]);
> > +
> > +        char *key, *value;
> > +        char *policy = policies[i];
> > +        if (!ofputil_parse_key_value(&policy, &key, &value)) {
> > +            goto done;
> > +        }
> > +        key_timeouts[i] = key;
> > +        value_timeouts[i] = atoi(value);
> > +        simap_put(&new_tp, key, (unsigned int)value_timeouts[i]);
> > +    }
> > +
> > +done:
> > +    tp_table = ovsrec_ct_timeout_policy_table_get(ctx->idl);
> > +    OVSREC_CT_TIMEOUT_POLICY_TABLE_FOR_EACH (row, tp_table) {
> > +        struct simap s = SIMAP_INITIALIZER(&s);
> > +
> > +        /* Convert to simap. */
> > +        for (int i = 0; i < row->n_timeouts; i++) {
> > +            simap_put(&s, row->key_timeouts[i], row->value_timeouts[i]);
> > +        }
> > +
> > +        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);
> > +    }
> > +
> > +    for (int i = 0; i < n_tps; i++) {
> > +        free(policies[i]);
> > +    }
> > +    free(policies);
> > +    simap_destroy(&new_tp);
> > +    free(key_timeouts);
> > +    free(value_timeouts);
> > +    return tp;
> > +}
> > +
> > +static void
> > +cmd_add_zone_tp(struct ctl_context *ctx)
> > +{
> > +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
> > +    struct ovsrec_ct_timeout_policy *tp;
> > +    int64_t zone_id;
> > +
> > +    const char *dp_name = ctx->argv[1];
> > +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
> > +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> > +
> > +    struct ovsrec_datapath *dp = find_datapath(vsctl_ctx, dp_name);
> > +    if (!dp) {
> > +        ctl_fatal("datapath %s does not exist", dp_name);
> > +    }
> > +
> > +    int n_tps = ctx->argc - 3;
> > +    struct ovsrec_ct_zone *zone = find_ct_zone(dp, zone_id);
> > +
> > +    if (zone && !may_exist) {
> > +        ctl_fatal("zone id %"PRIu64" already exists", zone_id);
> > +    }
> > +
> > +    tp = create_timeout_policy(ctx, &ctx->argv[3], n_tps);
> > +    if (zone) {
> > +        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_tp(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);
> > +    }
> > +
> > +    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 (zone) {
> > +        ovsrec_datapath_update_ct_zones_delkey(dp, zone_id);
> > +    }
> > +}
> > +
> > +static void
> > +cmd_list_zone_tp(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];
> > +        ds_put_format(&ctx->output, "Zone:%"PRIu64", Timeout Policies: ",
> > +                      dp->key_ct_zones[i]);
> > +
> > +        struct ovsrec_ct_timeout_policy *tp = zone->timeout_policy;
> > +
> > +        int j;
> >
> remove
> 
> 
> 
> > +        for (j = 0; j < tp->n_timeouts; j++) {
> >
> 
> 
> move into for loop
> +        for (int j = 0; j < tp->n_timeouts; j++) {
> 
> 
> > +            if (j == tp->n_timeouts - 1) {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64"\n",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            } else {
> > +                ds_put_format(&ctx->output, "%s=%"PRIu64" ",
> > +                          tp->key_timeouts[j], tp->value_timeouts[j]);
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +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 +3083,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_tp, NULL,
> >
> 
> s/19/18/ ?

good catch, thanks
William

> 
> 
> > +     "--may-exist", RW},
> > +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone_tp, NULL,
> > +     "--if-exists", RW},
> > +    {"list-zone-tp", 1, 1, "", pre_get_zone, cmd_list_zone_tp, 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