| Message ID | 20180308112447.8200-1-ligs@dtdream.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ovn-nb/sbctl: add inactivity probe in ovn-nb/sbctl set-connection | expand |
On Thu, Mar 08, 2018 at 07:24:47PM +0800, Guoshuai Li wrote: > From: Dong Jun <dongj@dtdream.com> > > This patch can set inactivity probe for connection by command > ovn-nbctl set-connection inactivity_probe=30000 ptcp:6641:0.0.0.0 > ovn-sbctl set-connection inactivity_probe=30000 ptcp:6642:0.0.0.0 > > Signed-off-by: Guoshuai Li <ligs@dtdream.com> Thanks for v2. Here are some suggestions for folding in on the next version. I have some additional thoughts here. I am probably thinking about it all too hard. First, ovn-nbctl (etc.) has a syntax for options that would ordinarily be used, e.g.: ovn-nbctl --inactivity-probe=MSECS set-connection TARGET... The possible disadvantage of that is that it could not be used to set a different inactivity probe value for different targets. However, I don't know whether that is actually a valuable use case. If it is valuable, though, there is the problem that cmd_get_connection() currently won't show the inactivity probes in an order that could be fed directly back to set-connection, since it might put the ones with the defaults after the ones that are non-default. However, I'm not sure that get-connection showing the inactivity probes is actually valuable at all! Anyway, I'd be inclined to use --inactivity-probe as an option, have it apply to all connections, and then make get-connection not print the inactivity probes at all (which matches what "ovs-vsctl get-controller" does, and I don't recall anyone ever complaining about that before). What do you think? Thanks, Ben. --8<--------------------------cut here-------------------------->8-- diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml index 521d6c3b4520..cbf20f047028 100644 --- a/ovn/utilities/ovn-nbctl.8.xml +++ b/ovn/utilities/ovn-nbctl.8.xml @@ -866,11 +866,11 @@ Deletes the configured connection(s). </dd> - <dt><code>set-connection</code> [<code>inactivity_probe=time</code>] <var>target</var>...</dt> + <dt><code>set-connection</code> [<code>inactivity_probe=</code><var>msecs</var>] <var>target</var>...</dt> <dd> - Sets the configured manager target or targets. Specified Maximum number - of milliseconds of idle connection inactivity probe time by - <code>inactivity_probe=time</code>, A value of 0 disables inactivity + Sets the configured manager target or targets. Use + <code>inactivity_probe=</code><var>msecs</var> to override the default + idle connection inactivity probe time. Use 0 to disable inactivity probes. </dd> </dl> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 75ab1375bced..9d7b8591d4be 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -446,7 +446,7 @@ DHCP Options commands:\n\ Connection commands:\n\ get-connection print the connections\n\ del-connection delete the connections\n\ - set-connection [inactivity_probe=TIME] TARGET...\n\ + set-connection [inactivity_probe=MSECS] TARGET...\n\ set the list of connections to TARGET...\n\ \n\ SSL commands:\n\ @@ -3509,12 +3509,10 @@ cmd_get_connection(struct ctl_context *ctx) NBREC_CONNECTION_FOR_EACH(conn, ctx->idl) { if (conn->n_inactivity_probe) { - char *s; - s = xasprintf("inactivity_probe=%"PRId64" %s", - conn->inactivity_probe[0], - conn->target); - svec_add(&targets, s); - free(s); + svec_add_nocopy(&targets, + xasprintf("inactivity_probe=%"PRId64" %s", + conn->inactivity_probe[0], + conn->target)); } else { svec_add(&targets, conn->target); } @@ -4085,7 +4083,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, NULL, "", RO}, {"del-connection", 0, 0, "", pre_connection, cmd_del_connection, NULL, "", RW}, {"set-connection", 1, INT_MAX, - "[inactivity_probe=TIME] TARGET...", + "[inactivity_probe=MSECS] TARGET...", pre_connection, cmd_set_connection, NULL, "", RW},
> On Thu, Mar 08, 2018 at 07:24:47PM +0800, Guoshuai Li wrote: >> From: Dong Jun <dongj@dtdream.com> >> >> This patch can set inactivity probe for connection by command >> ovn-nbctl set-connection inactivity_probe=30000 ptcp:6641:0.0.0.0 >> ovn-sbctl set-connection inactivity_probe=30000 ptcp:6642:0.0.0.0 >> >> Signed-off-by: Guoshuai Li <ligs@dtdream.com> > Thanks for v2. Here are some suggestions for folding in on the next > version. > > I have some additional thoughts here. I am probably thinking about it > all too hard. > > First, ovn-nbctl (etc.) has a syntax for options that would ordinarily > be used, e.g.: > ovn-nbctl --inactivity-probe=MSECS set-connection TARGET... > > The possible disadvantage of that is that it could not be used to set a > different inactivity probe value for different targets. However, I > don't know whether that is actually a valuable use case. If it is > valuable, though, there is the problem that cmd_get_connection() > currently won't show the inactivity probes in an order that could be fed > directly back to set-connection, since it might put the ones with the > defaults after the ones that are non-default. However, I'm not sure > that get-connection showing the inactivity probes is actually valuable > at all! > > Anyway, I'd be inclined to use --inactivity-probe as an option, have it > apply to all connections, and then make get-connection not print the > inactivity probes at all (which matches what "ovs-vsctl get-controller" > does, and I don't recall anyone ever complaining about that before). > > What do you think? > > Thanks, > > Ben. Hello Ben, I agree with you, it looks like there are four commands needed --inactivity-probe： ovn-nbctl set-connection ovn-sbctl set-connection vtep-ctl set-manager ovs-vsctl set-manager I am ready to try it. There I have a little doubt： For example --time --db applies to all commands, but --inactivity-probe only applies to set-connection(set-manager). Can other commands ignore it when it is carried by the user? This may be easier to implement.
diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml index 7f4b3aba8..521d6c3b4 100644 --- a/ovn/utilities/ovn-nbctl.8.xml +++ b/ovn/utilities/ovn-nbctl.8.xml @@ -866,9 +866,12 @@ Deletes the configured connection(s). </dd> - <dt><code>set-connection</code> <var>target</var>...</dt> + <dt><code>set-connection</code> [<code>inactivity_probe=time</code>] <var>target</var>...</dt> <dd> - Sets the configured manager target or targets. + Sets the configured manager target or targets. Specified Maximum number + of milliseconds of idle connection inactivity probe time by + <code>inactivity_probe=time</code>, A value of 0 disables inactivity + probes. </dd> </dl> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 80fb97cd4..75ab1375b 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -446,7 +446,8 @@ DHCP Options commands:\n\ Connection commands:\n\ get-connection print the connections\n\ del-connection delete the connections\n\ - set-connection TARGET... set the list of connections to TARGET...\n\ + set-connection [inactivity_probe=TIME] TARGET...\n\ + set the list of connections to TARGET...\n\ \n\ SSL commands:\n\ get-ssl print the SSL configuration\n\ @@ -3491,6 +3492,7 @@ pre_connection(struct ctl_context *ctx) { ovsdb_idl_add_column(ctx->idl, &nbrec_nb_global_col_connections); ovsdb_idl_add_column(ctx->idl, &nbrec_connection_col_target); + ovsdb_idl_add_column(ctx->idl, &nbrec_connection_col_inactivity_probe); } static void @@ -3506,7 +3508,16 @@ cmd_get_connection(struct ctl_context *ctx) svec_init(&targets); NBREC_CONNECTION_FOR_EACH(conn, ctx->idl) { - svec_add(&targets, conn->target); + if (conn->n_inactivity_probe) { + char *s; + s = xasprintf("inactivity_probe=%"PRId64" %s", + conn->inactivity_probe[0], + conn->target); + svec_add(&targets, s); + free(s); + } else { + svec_add(&targets, conn->target); + } } svec_sort_unique(&targets); @@ -3544,17 +3555,25 @@ insert_connections(struct ctl_context *ctx, char *targets[], size_t n) const struct nbrec_nb_global *nb_global = nbrec_nb_global_first(ctx->idl); struct nbrec_connection **connections; size_t i, conns=0; + int64_t inactivity_probe = 0; /* Insert each connection in a new row in Connection table. */ connections = xmalloc(n * sizeof *connections); for (i = 0; i < n; i++) { - if (stream_verify_name(targets[i]) && + if (!strncmp(targets[i], "inactivity_probe=", 17)) { + inactivity_probe = atoll(targets[i] + 17); + continue; + } else if (stream_verify_name(targets[i]) && pstream_verify_name(targets[i])) { VLOG_WARN("target type \"%s\" is possibly erroneous", targets[i]); } connections[conns] = nbrec_connection_insert(ctx->txn); nbrec_connection_set_target(connections[conns], targets[i]); + if (inactivity_probe) { + nbrec_connection_set_inactivity_probe(connections[conns], + &inactivity_probe, 1); + } conns++; } @@ -4065,7 +4084,9 @@ static const struct ctl_command_syntax nbctl_commands[] = { /* Connection commands. */ {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, NULL, "", RO}, {"del-connection", 0, 0, "", pre_connection, cmd_del_connection, NULL, "", RW}, - {"set-connection", 1, INT_MAX, "TARGET...", pre_connection, cmd_set_connection, + {"set-connection", 1, INT_MAX, + "[inactivity_probe=TIME] TARGET...", + pre_connection, cmd_set_connection, NULL, "", RW}, /* SSL commands. */ diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c index c2fd18338..c1210beb9 100644 --- a/ovn/utilities/ovn-sbctl.c +++ b/ovn/utilities/ovn-sbctl.c @@ -317,7 +317,8 @@ Logical flow commands:\n\ Connection commands:\n\ get-connection print the connections\n\ del-connection delete the connections\n\ - set-connection TARGET... set the list of connections to TARGET...\n\ + set-connection [inactivity_probe=TIME] [READ-ONLY] [role=ROLE] TARGET...\n\ + set the list of connections to TARGET...\n\ \n\ SSL commands:\n\ get-ssl print the SSL configuration\n\ @@ -954,6 +955,7 @@ pre_connection(struct ctl_context *ctx) ovsdb_idl_add_column(ctx->idl, &sbrec_connection_col_target); ovsdb_idl_add_column(ctx->idl, &sbrec_connection_col_read_only); ovsdb_idl_add_column(ctx->idl, &sbrec_connection_col_role); + ovsdb_idl_add_column(ctx->idl, &sbrec_connection_col_inactivity_probe); } static void @@ -971,10 +973,17 @@ cmd_get_connection(struct ctl_context *ctx) SBREC_CONNECTION_FOR_EACH(conn, ctx->idl) { char *s; - s = xasprintf("%s role=\"%s\" %s", - conn->read_only ? "read-only" : "read-write", - conn->role, - conn->target); + if (conn->n_inactivity_probe) { + s = xasprintf("%s role=\"%s\" inactivity_probe=%"PRId64" %s", + conn->read_only ? "read-only" : "read-write", + conn->role, conn->inactivity_probe[0], + conn->target); + } else { + s = xasprintf("%s role=\"%s\" %s", + conn->read_only ? "read-only" : "read-write", + conn->role, + conn->target); + } svec_add(&targets, s); free(s); } @@ -1016,6 +1025,7 @@ insert_connections(struct ctl_context *ctx, char *targets[], size_t n) size_t i, conns=0; bool read_only = false; char *role = ""; + int64_t inactivity_probe = 0; /* Insert each connection in a new row in Connection table. */ connections = xmalloc(n * sizeof *connections); @@ -1029,6 +1039,9 @@ insert_connections(struct ctl_context *ctx, char *targets[], size_t n) } else if (!strncmp(targets[i], "role=", 5)) { role = targets[i] + 5; continue; + } else if (!strncmp(targets[i], "inactivity_probe=", 17)) { + inactivity_probe = atoll(targets[i] + 17); + continue; } else if (stream_verify_name(targets[i]) && pstream_verify_name(targets[i])) { VLOG_WARN("target type \"%s\" is possibly erroneous", targets[i]); @@ -1038,6 +1051,10 @@ insert_connections(struct ctl_context *ctx, char *targets[], size_t n) sbrec_connection_set_target(connections[conns], targets[i]); sbrec_connection_set_read_only(connections[conns], read_only); sbrec_connection_set_role(connections[conns], role); + if (inactivity_probe) { + sbrec_connection_set_inactivity_probe(connections[conns], + &inactivity_probe, 1); + } conns++; } @@ -1426,7 +1443,9 @@ static const struct ctl_command_syntax sbctl_commands[] = { /* Connection commands. */ {"get-connection", 0, 0, "", pre_connection, cmd_get_connection, NULL, "", RO}, {"del-connection", 0, 0, "", pre_connection, cmd_del_connection, NULL, "", RW}, - {"set-connection", 1, INT_MAX, "TARGET...", pre_connection, cmd_set_connection, + {"set-connection", 1, INT_MAX, + "[inactivity_probe=TIME] [READ-ONLY] [role=ROLE] TARGET...", + pre_connection, cmd_set_connection, NULL, "", RW}, /* SSL commands. */