added one more comment. On Fri, Jul 26, 2019 at 4:10 PM Darrell Ball <[email protected]> wrote:
> added one more comment for now > > > On Fri, Jul 26, 2019 at 11:13 AM Darrell Ball <[email protected]> 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 <[email protected]> wrote: >> >>> From: William Tu <[email protected]> >>> >>> 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 <[email protected]> >>> --- >>> 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 >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
