[ovs-dev] [PATCH 03/12] ovs-vsctl: Add datapath and CT zone commands.

Darrell Ball dlu998 at gmail.com
Fri Jul 26 23:10:43 UTC 2019


added one more comment for now


On Fri, Jul 26, 2019 at 11:13 AM Darrell Ball <dlu998 at gmail.com> wrote:

> Thanks for the patch
>
> Not a full review; just some initial testing
>
>
> 1/ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2
> icmp_reply_blah=3])])
>
> The above syntax is NOT flagged as an error
>
>
> 2/ AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=2
> icmp_first=2 icmp_reply=3])])
>
> The above "--may-exist" option fails with
> +ovs-vsctl: 'add-zone-tp' command has no '--may-exist' option
>
> AT_CHECK([RUN_OVS_VSCTL([--if-exists del-zone-tp netdev zone=1])])
> is also failing
> +ovs-vsctl: 'del-zone-tp' command has no '--if-exists' option
>
> Please support both "--may-exist" and "--if-exists"
>
>
> 3/ The below should fail, but it is accepted.
>
> 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])])
>
>
> 4/ The below fails (which is good), but the error is in idl, rather than
> the 'del-zone-tp' command
>
> AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> AT_CHECK([RUN_OVS_VSCTL([del-zone-tp netdev zone=1])])
> fails with
> +2019-07-26T17:56:10Z|00002|ovsdb_idl|WARN|Trying to delete a key that
> doesn't exist in the map.
>
>
>
> 5/ Please support --may-exist for add-dp
>
> 6/ Please support --if-exists for del-dp
>
>
> 7/ Few comments below
>
>
> Thanks Darrell
>
>
> On Thu, Jul 25, 2019 at 4:26 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
>> From: William Tu <u9012063 at gmail.com>
>>
>> The patch adds the following commands
>>   $ ovs-vsctl {add,del,list}-dp
>> for creating/deleting/listing the datapath, and
>>   $ ovs-vsctl {add,del,list}-zone-tp
>> for conntrack zones and timeout policies.
>>
>> Signed-off-by: William Tu <u9012063 at gmail.com>
>> ---
>>  tests/ovs-vsctl.at       |  20 +++-
>>  utilities/ovs-vsctl.8.in |  29 ++++++
>>  utilities/ovs-vsctl.c    | 245
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 292 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
>> index 77604c58a2bc..8854138ecb1e 100644
>> --- a/tests/ovs-vsctl.at
>> +++ b/tests/ovs-vsctl.at
>> @@ -805,6 +805,22 @@ 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([add-dp netdev])])
>> +AT_CHECK([RUN_OVS_VSCTL([add-dp system])])
>> +AT_CHECK([RUN_OVS_VSCTL([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])])
>>
>
> Add all possible keys as part of positive tests so we know thye work
>
>
>
>
>> +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([list-zone-tp netdev])], [0], [Zone:2, Timeout
>> Policies: icmp_first=2 icmp_reply=3
>> +])
>> +AT_CHECK([RUN_OVS_VSCTL([del-dp netdev])])
>> +AT_CHECK([RUN_OVS_VSCTL([list-dp | sed 's/ uuid.*$//'])], [0], [system
>> +])
>>  OVS_VSCTL_CLEANUP
>>  AT_CLEANUP
>>
>> @@ -890,10 +906,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'])],
>>
>
> unrelated change
>
>
>
>>    [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])],
>>
>
> unrelated change
>
>
>
>>    [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])],
>> diff --git a/utilities/ovs-vsctl.8.in b/utilities/ovs-vsctl.8.in
>> index 7c09df79bd29..f8ec995247e7 100644
>> --- a/utilities/ovs-vsctl.8.in
>> +++ b/utilities/ovs-vsctl.8.in
>> @@ -353,6 +353,35 @@ list.
>>  Prints the name of the bridge that contains \fIiface\fR on standard
>>  output.
>>  .
>> +.SS "Datapath Commands"
>> +These commands examine and manipulate Open vSwitch datapath.
>> +.
>> +.IP "\fBadd\-dp \fIdatapath\fR"
>> +Creates a new datapath named \fIdatapath\fR.  Use "netdev" for userspace
>> +datapath and "system" for kernel datapath.  Initially the datapath will
>> +have no CT zones or other data.
>> +.IP "\fBdel\-dp \fIdatapath\fR"
>> +Deletes \fIdatapath\fR.
>> +.IP "\fBlist\-dp \fIdatapath\fR"
>> +Prints the datapath name and its uuid.
>> +.
>> +.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, separated 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 "\fBdel\-zone\-tp \fIdatapath \fBzone=\fIzone_id\fR"
>> +Delete a zone under \fIdatapath\fR by specifying its zone ID.
>> +.
>> +.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..3ec9b2b05f35 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"
>>
>> @@ -1154,6 +1156,239 @@ cmd_emer_reset(struct ctl_context *ctx)
>>  }
>>
>>  static void
>> +cmd_add_dp(struct ctl_context *ctx)
>> +{
>> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
>> +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
>> +    struct ovsrec_datapath *dp;
>> +    const char *dp_name;
>> +    int i;
>> +
>> +    dp_name = ctx->argv[1];
>> +
>> +    for (i = 0; i < ovs->n_datapaths; i++) {
>> +        if (!strcmp(dp_name, ovs->key_datapaths[i])) {
>> +            VLOG_ERR("Datapath %s alread exists", dp_name);
>> +            return;
>> +        }
>> +    }
>> +
>> +    dp = ovsrec_datapath_insert(ctx->txn);
>> +    ovsrec_open_vswitch_update_datapaths_setkey(ovs, dp_name, dp);
>> +}
>> +
>> +static void
>> +cmd_del_dp(struct ctl_context *ctx)
>> +{
>> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
>> +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
>> +
>> +    ovsrec_open_vswitch_update_datapaths_delkey(ovs, ctx->argv[1]);
>> +}
>> +
>> +static void
>> +cmd_list_dp(struct ctl_context *ctx)
>> +{
>> +    struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
>> +    const struct ovsrec_open_vswitch *ovs = vsctl_ctx->ovs;
>> +    int i;
>> +
>> +    for (i = 0; i < ovs->n_datapaths; i++) {
>> +        struct ovsrec_datapath *dp = ovs->value_datapaths[i];
>> +        char *key;
>> +
>> +        key = ovs->key_datapaths[i];
>> +        ds_put_format(&ctx->output, "%s uuid="UUID_FMT"\n",
>> +                      key, UUID_ARGS(&dp->header_.uuid));
>> +    }
>> +}
>> +
>> +static void
>> +pre_get_dp(struct ctl_context *ctx)
>> +{
>> +    ovsdb_idl_add_column(ctx->idl, &ovsrec_open_vswitch_col_datapaths);
>> +}
>> +
>> +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;
>> +    uint32_t hash_new_tp;
>> +    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]);
>> +    }
>> +done:
>> +    hash_new_tp = simap_hash(&new_tp);
>> +
>> +    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_hash(&s) == hash_new_tp) {
>>
>
In this case an existing timeouts policy is found with the same hash value.
The following logic skips creating a new policy.
However, although the hash is the same, the timeout policy may be different
Maybe you need to compare all the key and values to confirm they are the
same.



> +            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;
>> +    int n_tps;
>> +
>> +    dp_name = ctx->argv[1];
>> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
>> +
>> +    dp = find_datapath(vsctl_ctx, dp_name);
>> +    if (!dp) {
>> +        VLOG_ERR("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) {
>> +        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_datapath *dp;
>> +    const char *dp_name;
>> +    int64_t zone_id;
>> +
>> +    dp_name = ctx->argv[1];
>> +    ovs_scan(ctx->argv[2], "zone=%"SCNi64, &zone_id);
>> +
>> +    dp = find_datapath(vsctl_ctx, dp_name);
>> +    if (!dp) {
>> +        VLOG_ERR("datapath: %s record not found", dp_name);
>> +        return;
>> +    }
>> +
>> +    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) {
>> +        VLOG_ERR("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]);
>> +    }
>> +}
>> +
>> +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)
>>  {
>>      struct vsctl_context *vsctl_ctx = vsctl_context_cast(ctx);
>> @@ -2896,6 +3131,16 @@ static const struct ctl_command_syntax
>> vsctl_commands[] = {
>>      /* Switch commands. */
>>      {"emer-reset", 0, 0, "", pre_cmd_emer_reset, cmd_emer_reset, NULL,
>> "", RW},
>>
>> +    /* Datapath commands. */
>> +    {"add-dp", 1, 1, "", pre_get_dp, cmd_add_dp, NULL, "", RW},
>> +    {"del-dp", 1, 1, "", pre_get_dp, cmd_del_dp, NULL, "", RW},
>> +    {"list-dp", 0, 0, "", pre_get_dp, cmd_list_dp, NULL, "", RO},
>> +
>> +    /* Zone and CT Timeout Policy commands. */
>> +    {"add-zone-tp", 2, 19, "", pre_get_zone, cmd_add_zone, NULL, "", RW},
>> +    {"del-zone-tp", 2, 2, "", pre_get_zone, cmd_del_zone, NULL, "", 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
>>
>


More information about the dev mailing list