[ovs-dev] [PATCH] ovs-vsctl: Add commands to get/delete/set manager connections.

Andrew Evans aevans at nicira.com
Tue Feb 8 01:48:40 UTC 2011


Thanks for your review.

On 2/7/11 1:22 PM, Ben Pfaff wrote:
> In the manpage, lib/vconn-active.man lists ways to connect to OpenFlow
> switches or controllers.  Instead, we should list ways to connect to
> OSVDB clients, which are listed in ovsdb/remote-passive.man and
> ovsdb/remote-active.man.

Oops, good catch. Thanks, I've fixed it with:

@@ -376,7 +380,8 @@ Sets the configured manager target or targets.  Each
\fItarget\fR may
 use any of the following forms:
 .
 .RS
-.so lib/vconn-active.man
+.so ovsdb/remote-active.man
+.so ovsdb/remote-passive.man
 .RE
 .
 .SS "SSL Configuration"


> I'd be a little more explicit about what the managers commands do.
> Maybe something like this:
> 
>     These commands manipulate the \fBmanagers\fR and
>     \fBmanager_options\fR columns in the \fBOpen_vSwitch\fR table and
>     rows in the \fBManagers\fR table.  When \fBovsdb\-server\fR is
>     configured to use those rows and columns for OVSDB connections, as
>     described in \fBINSTALL.Linux\fR and in the startup scripts provided
>     with Open vSwitch, this allows the administrator to use
>     \fBovs\-vsctl\fR to configure database connections.

Looks great, so I've used it verbatim.

> From pre_manager(), I would not call pre_get_info().  It should not be
> necessary, because none of the manager operations call get_info().

Thanks, missed this one.

> In cmd_get_manager() I would use svec_sort_unique(), instead of
> svec_sort(), because it is valid for a manager to appear in both places
> and in such a case we only want to print it once.

Another good catch. Done.

> In delete_managers(), I would only delete the rows in the Managers table
> that the manager_options column referred to, instead of all the rows.

Hmm, ok. I don't see anything useful about keeping dangling Manager rows
around, but I can understand the principle. Changed:

@@ -1924,19 +1922,18 @@ static void
 delete_managers(const struct vsctl_context *ctx)
 {
     const struct ovsrec_open_vswitch *ovs = ctx->ovs;
-    const struct ovsdb_idl *idl = ctx->idl;
-    const struct ovsrec_manager *row, *next;
+    size_t i;

     /* Delete manager targets in deprecated 'managers' column. */
     ovsrec_open_vswitch_set_managers(ovs, NULL, 0);

+    /* Delete Manager rows pointed to by 'manager_options' column. */
+    for (i = 0; i < ovs->n_manager_options; i++) {
+        ovsrec_manager_delete(ovs->manager_options[i]);
+    }
+
     /* Delete 'Manager' row refs in 'manager_options' column. */
     ovsrec_open_vswitch_set_manager_options(ovs, NULL, 0);
-
-    /* Delete all rows in 'Manager' table. */
-    OVSREC_MANAGER_FOR_EACH_SAFE (row, next, idl) {
-        ovsrec_manager_delete(row);
-    }
 }

 static void

> In insert_managers(), I would also put each manager in the 'managers'
> column of Open_vSwitch, for compatibility with any software that expects
> the list of managers to appear there.

Sensible. Done:

@@ -1954,12 +1951,17 @@ insert_managers(struct vsctl_context *ctx, char
*targets[], size_t n)
     struct ovsrec_manager **managers;
     size_t i;

+    /* Store in deprecated 'manager' column. */
+    ovsrec_open_vswitch_set_managers(ctx->ovs, targets, n);
+
+    /* Insert each manager in a new row in Manager table. */
     managers = xmalloc(n * sizeof *managers);
     for (i = 0; i < n; i++) {
         managers[i] = ovsrec_manager_insert(ctx->txn);
         ovsrec_manager_set_target(managers[i], targets[i]);
     }

+    /* Store uuids of new Manager rows in 'manager_options' column. */
     ovsrec_open_vswitch_set_manager_options(ctx->ovs, managers, n);
     free(managers);
 }

> I suggest adding a test to tests/ovs-vsctl.at.  You can follow the form
> of the "controllers" test that is already there.

Ok, how does this look?

diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a05e805..bb49450 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -504,6 +504,32 @@ OVS_VSCTL_CLEANUP
 AT_CLEANUP

 dnl ----------------------------------------------------------------------
+AT_BANNER([ovs-vsctl unit tests -- manager commands])
+
+AT_SETUP([managers])
+AT_KEYWORDS([manager ovs-vsctl])
+OVS_VSCTL_SETUP
+AT_CHECK([RUN_OVS_VSCTL_TOGETHER(
+  [del-manager],
+  [get-manager],
+  [set-manager tcp:4.5.6.7],
+  [get-manager],
+  [set-manager tcp:8.9.10.11 tcp:5.4.3.2],
+  [get-manager],
+  [del-manager],
+  [get-manager])], [0], [
+
+
+tcp:4.5.6.7
+
+tcp:5.4.3.2\ntcp:8.9.10.11
+
+
+], [], [OVS_VSCTL_CLEANUP])
+OVS_VSCTL_CLEANUP
+AT_CLEANUP
+
+dnl ----------------------------------------------------------------------
 AT_BANNER([ovs-vsctl unit tests -- database commands])

 AT_SETUP([database commands -- positive checks])

> Thanks,
> 
> Ben.

Thank you!

-Andrew




More information about the dev mailing list