[ovs-dev] [gc 13/13] ovsdb: Implement garbage collection.

Ben Pfaff blp at nicira.com
Wed Mar 9 21:09:27 UTC 2011


On Mon, Mar 07, 2011 at 01:55:26PM -0800, Ethan Jackson wrote:
> Out of curiosity, why do we want to make the QoS and Queue table part
> of the root set?  Are they useful when unreferenced?

When I spoke to our controller team, they said that our controllers
sometimes construct QoS and Queue records without connecting them to the
rest of the database in the same transaction.  In turn the controllers
do this because they usually want to attach the same QoS record to
multiple physical interfaces, because we want the same queues on all of
the physical interfaces.  This sounded reasonable to me.

It is easily changed, of course.

> > +        options = {}
> > +        if table.is_root:
> > +            options['style'] = 'bold'
> > +        print "\t%s [%s];" % (
> > +            tableName,
> > +            ', '.join(['%s=%s' % (k,v) for k,v in options.items()]))
> 
> There is trailing whitespace after tableName.

Thank you.  I have fixed this now.

> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index b0c1026..a3f93a5 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> 
> There is some trailing whitespace strewn about this file.

Oops.  I have now removed trailing whitespace from lines already changed
by this patch.  (I've also changed my Emacs setup to highlight trailing
whitespace in autotest files.)

> > @@ -3402,8 +3346,20 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
> >                         "with \"-- --id=%s create ...\")",
> >                         node->name, node->name);
> >         }
> > +        if (!symbol->strong_ref) {
> > +            if (!symbol->weak_ref) {
> > +                VLOG_WARN("row id \"%s\" was created but no reference to it "
> > +                          "was inserted, so it will not actually appear in "
> > +                          "the database", node->name);
> > +            } else {
> > +                VLOG_WARN("row id \"%s\" was created but only a weak "
> > +                          "reference to it was inserted, so it will not "
> > +                          "actually appear in the database", node->name);
> > +            }
> > +        }
> >     }
> >
> > +
> Extra newline added here.

Thanks.  I have fixed this now.

> > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> > index 3537d28..739952b 100644
> > --- a/vswitchd/vswitch.ovsschema
> > +++ b/vswitchd/vswitch.ovsschema
> 
> I think it's fine how it is, but my inclination would be to make
> isRoot = false the default and force the user to specify true on root
> nodes.  Most nodes we create will not be root nodes, so it seems a bit
> less error prone.

I agree.  The reason that I adopted "isRoot": true as the default is to
avoid changing the interpretation of existing schemas.  If "isRoot":
false is the default, then we need to know whether a schema or a
database (since a database includes a schema) is written in the old form
or the new form when we read it.  Otherwise, every record in an old
database will always be discarded, since the old database has no root
set at all!

Another approach, which had not occurred to me before now, would be to
use "true" as the default if a schema does not have any tables with
"isRoot" explicitly specified (for compatibility), and "false"
otherwise.  Do you like that better?  If you do, then I will implement
it that way instead.

> >  <?xml version="1.0" encoding="utf-8"?>
> >  <database title="Open vSwitch Configuration Database">
> >   <p>A database with this schema holds the configuration for one Open
> > -    vSwitch daemon.  The root of the configuration for the daemon is
> > -    the <ref table="Open_vSwitch"/> table, which must have exactly one
> > -    record.  Records in other tables are significant only when they
> > -    can be reached directly or indirectly from the
> > -    <ref table="Open_vSwitch"/> table.</p>
> > +  vSwitch daemon.  The top-level configuration for the daemon is the
> > +  <ref table="Open_vSwitch"/> table, which must have exactly one
> > +  record.  Records in other tables are significant only when they can
> > +  be reached directly or indirectly from the <ref
> > +  table="Open_vSwitch"/> table.  Records that are not reachable from
> > +  the <ref table="Open_vSwitch"/> table are automatically deleted from
> > +  the database, except for records in a few distinguished ``root set''
> I would phrase this as "except for a few records in a distinguished"
> 
> The above paragraph should be left shifted two spaces to be consistent
> with the indentation in the rest of the file.

Fixed.  Thank you!

Here are the changes that I made:

diff --git a/ovsdb/ovsdb-dot.in b/ovsdb/ovsdb-dot.in
index 163976b..d417286 100755
--- a/ovsdb/ovsdb-dot.in
+++ b/ovsdb/ovsdb-dot.in
@@ -35,7 +35,7 @@ def schemaToDot(schemaFile):
         if table.is_root:
             options['style'] = 'bold'
         print "\t%s [%s];" % (
-            tableName, 
+            tableName,
             ', '.join(['%s=%s' % (k,v) for k,v in options.items()]))
         for columnName, column in table.columns.iteritems():
             if column.type.value:
diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
index a3f93a5..2e8af85 100644
--- a/tests/ovs-vsctl.at
+++ b/tests/ovs-vsctl.at
@@ -624,7 +624,7 @@ AT_CLEANUP
 AT_SETUP([database commands -- negative checks])
 AT_KEYWORDS([ovs-vsctl])
 OVS_VSCTL_SETUP
-AT_CHECK([RUN_OVS_VSCTL([add-br br0])], 
+AT_CHECK([RUN_OVS_VSCTL([add-br br0])],
   [0], [ignore], [], [OVS_VSCTL_CLEANUP])
 AT_CHECK([RUN_OVS_VSCTL([add-br br1])], 
   [0], [ignore], [], [OVS_VSCTL_CLEANUP])
@@ -632,7 +632,7 @@ AT_CHECK([RUN_OVS_VSCTL([set-controller br1 tcp:127.0.0.1])],
   [0], [ignore], [], [OVS_VSCTL_CLEANUP])
 AT_CHECK([
     RUN_OVS_VSCTL_TOGETHER([--id=@n create n targets='"1.2.3.4:567"'],
-                           [set bridge br0 netflow=@n])], 
+                           [set bridge br0 netflow=@n])],
   [0], [stdout], [], [OVS_VSCTL_CLEANUP])
 cp stdout netflow-uuid
 AT_CHECK([RUN_OVS_VSCTL([list n `cat netflow-uuid`])],
@@ -852,7 +852,7 @@ dnl The bug is documented in ovs-vsctl.8.
 AT_SETUP([created row UUID is wrong in same execution])
 AT_KEYWORDS([ovs-vsctl])
 OVS_VSCTL_SETUP
-AT_CHECK([RUN_OVS_VSCTL([--id=@br0 create Bridge name=br0 -- add Open_vSwitch . bridges @br0 -- list b])], 
+AT_CHECK([RUN_OVS_VSCTL([--id=@br0 create Bridge name=br0 -- add Open_vSwitch . bridges @br0 -- list b])],
   [0], [stdout], [], [OVS_VSCTL_CLEANUP])
 AT_CHECK([perl $srcdir/uuidfilt.pl stdout], [0], 
   [[<0>
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index 35cf048..80c9048 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -3359,7 +3359,6 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
         }
     }
 
-
     status = ovsdb_idl_txn_commit_block(txn);
     if (wait_for_reload && status == TXN_SUCCESS) {
         next_cfg = ovsdb_idl_txn_get_increment_new_value(txn);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 1fe97e3..89354da 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1,14 +1,16 @@
 <?xml version="1.0" encoding="utf-8"?>
 <database title="Open vSwitch Configuration Database">
-  <p>A database with this schema holds the configuration for one Open
-  vSwitch daemon.  The top-level configuration for the daemon is the
-  <ref table="Open_vSwitch"/> table, which must have exactly one
-  record.  Records in other tables are significant only when they can
-  be reached directly or indirectly from the <ref
-  table="Open_vSwitch"/> table.  Records that are not reachable from
-  the <ref table="Open_vSwitch"/> table are automatically deleted from
-  the database, except for records in a few distinguished ``root set''
-  tables noted below.</p>
+  <p>
+    A database with this schema holds the configuration for one Open
+    vSwitch daemon.  The top-level configuration for the daemon is the
+    <ref table="Open_vSwitch"/> table, which must have exactly one
+    record.  Records in other tables are significant only when they
+    can be reached directly or indirectly from the <ref
+    table="Open_vSwitch"/> table.  Records that are not reachable from
+    the <ref table="Open_vSwitch"/> table are automatically deleted
+    from the database, except for records in a few distinguished
+    ``root set'' tables noted below.
+  </p>
 
   <table name="Open_vSwitch" title="Open vSwitch configuration.">
     Configuration for an Open vSwitch daemon.  There must be exactly




More information about the dev mailing list