[ovs-dev] [PATCH] ovs-xapi-sync: Always set iface-id, not just when xs-vif-uuid changes.

Ben Pfaff blp at nicira.com
Thu Feb 2 01:19:19 UTC 2012


On Wed, Feb 01, 2012 at 04:57:35PM -0800, Justin Pettit wrote:
> On Feb 1, 2012, at 2:58 PM, Ben Pfaff wrote:
> 
> > +            xvu = row.external_ids.get("xs-vif-uuid")
> > +            if xvu:
> > +                iface_id = (new_iface_ids.get(xvu)
> > +                            or iface_ids.get(xvu)
> > +                            or get_iface_id(row.name, xvu))
> 
> Doesn't this prefer an old iface_id (in "iface_ids") over a new one
> (through get_iface_id())?  Is that intended behavior?  Is your
> concern about making a bunch of XAPI checks to get
> "nicira-iface-id"?

The intent is that the new caching behavior should be the same as the
old caching behavior.  In either case, after we translate a
xs-vif-uuid to an iface-id once, we cache the translation until the
xs-vif-uuid is no longer in use.  That's the intention, anyhow.  Do
you see a change?

(Yes, I really don't want to call XAPI on every iteration for each
iface.)

> >             # When there's a vif and a tap, the tap is active (used for
> >             # traffic).  When there's just a vif, the vif is active.
> > @@ -339,6 +344,7 @@ def main():
> > 
> >             new_interfaces[row.name] = new_xvu
> 
> I don't think "new_xvu" is defined any longer, but see next comment...

Oops.  Removed.

> >         interfaces = new_interfaces
> 
> I don't think "interfaces" or "new_interfaces" are needed anymore, are they?

Oops.  Removed.

Here's an incremental:

diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index f54d85b..8392c61 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -256,7 +256,6 @@ def main():
     signal.signal(signal.SIGHUP, handler)
 
     bridges = {}                # Map from bridge name to xs_network_uuids
-    interfaces = {}             # Map from interface name to xs-vif-uuid
     iface_ids = {}              # Map from xs-vif-uuid to iface-id
     while True:
         if not force_run and not idl.run():
@@ -268,7 +267,7 @@ def main():
         if force_run:
             vlog.info("Forced to re-run as the result of a SIGHUP")
             bridges = {}
-            interfaces = {}
+            iface_ids = {}
             force_run = False
 
         txn = ovs.db.idl.Transaction(idl)
@@ -291,7 +290,6 @@ def main():
         for row in idl.tables["Interface"].rows.itervalues():
             iface_by_name[row.name] = row
 
-        new_interfaces = {}
         new_iface_ids = {}
         for row in idl.tables["Interface"].rows.itervalues():
             # Match up paired vif and tap devices.
@@ -341,9 +339,6 @@ def main():
                 set_external_id(vif, "iface-status", "active")
             else:
                 set_external_id(row, "iface-status", None)
-
-            new_interfaces[row.name] = new_xvu
-        interfaces = new_interfaces
         iface_ids = new_iface_ids
 
         txn.commit_block()

and the full thing (still untested):

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Wed, 1 Feb 2012 17:18:52 -0800
Subject: [PATCH] ovs-xapi-sync: Always set iface-id, not just when xs-vif-uuid changes.

When XAPI moves an interface from one bridge to another, the vif script
removes the vif from one bridge and adds it to (possibly) a different
bridge in a single transaction.  The new record does not have an iface-id
initially (because the vif script never adds the iface-id initially) but
it has the same name and xs-vif-uuid as the old one, so the caching logic
in ovs-xapi-sync failed to add a new iface-id.  This commit fixes the
caching logic.

Observed on XenServer 5.6.100.  It's possible that XAPI behavior changed in
later versions so the bug cannot be triggered there, but we have not
checked.

Bug #9414.
Reported-by: Duffie Cooley <dcooley at nicira.com>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 .../usr_share_openvswitch_scripts_ovs-xapi-sync    |   31 ++++++++++---------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index 85795e0..8392c61 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -1,5 +1,5 @@
 #!/usr/bin/python
-# Copyright (c) 2009, 2010, 2011 Nicira Networks
+# Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -256,7 +256,7 @@ def main():
     signal.signal(signal.SIGHUP, handler)
 
     bridges = {}                # Map from bridge name to xs_network_uuids
-    interfaces = {}             # Map from interface name to xs-vif-uuid
+    iface_ids = {}              # Map from xs-vif-uuid to iface-id
     while True:
         if not force_run and not idl.run():
             poller = ovs.poller.Poller()
@@ -267,7 +267,7 @@ def main():
         if force_run:
             vlog.info("Forced to re-run as the result of a SIGHUP")
             bridges = {}
-            interfaces = {}
+            iface_ids = {}
             force_run = False
 
         txn = ovs.db.idl.Transaction(idl)
@@ -290,7 +290,7 @@ def main():
         for row in idl.tables["Interface"].rows.itervalues():
             iface_by_name[row.name] = row
 
-        new_interfaces = {}
+        new_iface_ids = {}
         for row in idl.tables["Interface"].rows.itervalues():
             # Match up paired vif and tap devices.
             if row.name.startswith("vif"):
@@ -311,17 +311,20 @@ def main():
                 for k in keys:
                     set_external_id(row, k, vif.external_ids.get(k))
 
-            # If it's a new interface or its xs-vif-uuid has changed, then
-            # obtain the iface-id from XAPI.
+            # Map from xs-vif-uuid to iface-id.
             #
             # (A tap's xs-vif-uuid comes from its vif.  That falls out
             # naturally from the copy loop above.)
-            new_xvu = row.external_ids.get("xs-vif-uuid", "")
-            old_xvu = interfaces.get(row.name)
-            if old_xvu != new_xvu:
-                iface_id = get_iface_id(row.name, new_xvu)
-                if iface_id and row.external_ids.get("iface-id") != iface_id:
-                    set_external_id(row, "iface-id", iface_id)
+            xvu = row.external_ids.get("xs-vif-uuid")
+            if xvu:
+                iface_id = (new_iface_ids.get(xvu)
+                            or iface_ids.get(xvu)
+                            or get_iface_id(row.name, xvu))
+                new_iface_ids[xvu] = iface_id
+            else:
+                # No xs-vif-uuid therefore no iface-id.
+                iface_id = None
+            set_external_id(row, "iface-id", iface_id)
 
             # When there's a vif and a tap, the tap is active (used for
             # traffic).  When there's just a vif, the vif is active.
@@ -336,9 +339,7 @@ def main():
                 set_external_id(vif, "iface-status", "active")
             else:
                 set_external_id(row, "iface-status", None)
-
-            new_interfaces[row.name] = new_xvu
-        interfaces = new_interfaces
+        iface_ids = new_iface_ids
 
         txn.commit_block()
 
-- 
1.7.2.5




More information about the dev mailing list