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

Darrell Ball dlu998 at gmail.com
Mon Jul 29 21:02:08 UTC 2019


added one more comment.

On Fri, Jul 26, 2019 at 4:10 PM Darrell Ball <dlu998 at gmail.com> wrote:

> 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);
>>>
>>
Memory leak; need to add:

free(copy);



> +    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