diff mbox series

[ovs-dev] ovn-nb/sbctl: add inactivity probe in ovn-nb/sbctl set-connection

Message ID 20180307053016.11856-1-ligs@dtdream.com
State Changes Requested
Headers show
Series [ovs-dev] ovn-nb/sbctl: add inactivity probe in ovn-nb/sbctl set-connection | expand

Commit Message

Guoshuai Li March 7, 2018, 5:30 a.m. UTC
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>
---
 ovn/utilities/ovn-nbctl.8.xml |  2 +-
 ovn/utilities/ovn-nbctl.c     | 28 ++++++++++++++++++++++++----
 ovn/utilities/ovn-sbctl.c     | 33 ++++++++++++++++++++++++++-------
 3 files changed, 51 insertions(+), 12 deletions(-)

Comments

Ben Pfaff March 7, 2018, 8:45 p.m. UTC | #1
On Wed, Mar 07, 2018 at 01:30:16PM +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 working on making OVN better.  I think that this feature can
be useful.  I have a few additional comments.

I see a few casts to (int64_t).  I don't think that these have any value
and can be removed.

The user cannot tell how to use this feature from the documentation.
The actual syntax is inactivity_probe=<number>, but the documentation
doesn't say that, in multiple places.  For example:

> -      <dt><code>set-connection</code> <var>target</var>...</dt>
> +      <dt><code>set-connection</code> <code>inactivity_probe</code> <var>target</var>...</dt>

I suggest that the documentation should also use [] to show that the
inactivity_probe is optional.

Here's another place with misleading documentation.  I won't point out
all of them:
> -  set-connection TARGET...   set the list of connections to TARGET...\n\
> +  set-connection [INACTIVITY_PROBE] TARGET...\n\
> +                             set the list of connections to TARGET...\n\

Thanks again!

Ben
diff mbox series

Patch

diff --git a/ovn/utilities/ovn-nbctl.8.xml b/ovn/utilities/ovn-nbctl.8.xml
index 7f4b3aba8..c2373c5bf 100644
--- a/ovn/utilities/ovn-nbctl.8.xml
+++ b/ovn/utilities/ovn-nbctl.8.xml
@@ -866,7 +866,7 @@ 
       Deletes the configured connection(s).
       </dd>
 
-      <dt><code>set-connection</code> <var>target</var>...</dt>
+      <dt><code>set-connection</code> <code>inactivity_probe</code> <var>target</var>...</dt>
       <dd>
       Sets the configured manager target or targets.
       </dd>
diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 80fb97cd4..e25275bc9 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] 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 = (int64_t)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,8 @@  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] 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..a53b36561 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] [READ/WRITE] [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 = (int64_t)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,8 +1443,10 @@  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,
-     NULL, "", RW},
+    {"set-connection", 1, INT_MAX,
+        "[INACTIVITY_PROBE] [READ/WRITE] [ROLE] TARGET...",
+        pre_connection, cmd_set_connection,
+        NULL, "", RW},
 
     /* SSL commands. */
     {"get-ssl", 0, 0, "", pre_cmd_get_ssl, cmd_get_ssl, NULL, "", RO},