[ovs-dev] [PATCH v4] ovn-controller: Add datapath-type and iface-types in chassis:external_ids

Ben Pfaff blp at ovn.org
Sat Aug 13 16:20:47 UTC 2016


On Sat, Jul 30, 2016 at 03:32:01PM +0530, Numan Siddique wrote:
> This patch reads the 'Bridge.datapath_type' column value of the integration
> bridge and 'Open_vSwitch.iface_types' column value and sets these in the
> external_ids:datapath-type and external_ids:iface-types of Chassis table.
> 
> This will provide hints to the CMS or clients monitoring OVN SB DB to
> determine the datapath type (DPDK or non-DPDK) configured and take some
> actions based on it.
> 
> One usecase is, OVN neutron plugin can use this information to set the
> vif_type (ovs or vhostuser) during the port binding.
> 
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>

Thanks for the patch.

I made a few simplifications to the code in chassis.c, and fixed a
memory leak.  I clarified a little in the documentation.  I also fixed
some terminology (chassis:external-ids implies a column named chassis
and a key named external-ids; the right terminology is Chassis
external-ids for the Chassis table and external-ids column) and adjusted
the test to eliminate some races.

I folded in those changes, copied below, and applied this to master.

Thanks again!

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

diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
index 9c1be0b..97fc2bf 100644
--- a/ovn/controller/chassis.c
+++ b/ovn/controller/chassis.c
@@ -136,47 +136,23 @@ chassis_run(struct controller_ctx *ctx, const char *chassis_id,
             sbrec_chassis_set_hostname(chassis_rec, hostname);
         }
 
+        /* Determine new values for Chassis external-ids. */
         const char *chassis_bridge_mappings
             = get_bridge_mappings(&chassis_rec->external_ids);
         const char *chassis_datapath_type
-            = smap_get(&chassis_rec->external_ids, "datapath-type");
+            = smap_get_def(&chassis_rec->external_ids, "datapath-type", "");
         const char *chassis_iface_types
-            = smap_get(&chassis_rec->external_ids, "iface-types");
+            = smap_get_def(&chassis_rec->external_ids, "iface-types", "");
 
-        if (!chassis_datapath_type) {
-            chassis_datapath_type = "";
-        }
-
-        if (!chassis_iface_types) {
-            chassis_iface_types = "";
-        }
-
-        if (!strcmp(bridge_mappings, chassis_bridge_mappings)) {
-            bridge_mappings = NULL;
-        }
-        if (!strcmp(datapath_type, chassis_datapath_type)) {
-            datapath_type = NULL;
-        }
-
-        if (!strcmp(iface_types_str, chassis_iface_types)) {
-            iface_types_str = NULL;
-        }
-
-        if (bridge_mappings || datapath_type || iface_types_str) {
+        /* If any of the external-ids should change, update them. */
+        if (strcmp(bridge_mappings, chassis_bridge_mappings) ||
+            strcmp(datapath_type, chassis_datapath_type) ||
+            strcmp(iface_types_str, chassis_iface_types)) {
             struct smap new_ids;
             smap_clone(&new_ids, &chassis_rec->external_ids);
-            if (bridge_mappings) {
-                smap_replace(&new_ids, "ovn-bridge-mappings", bridge_mappings);
-            }
-
-            if (datapath_type) {
-                smap_replace(&new_ids, "datapath-type", datapath_type);
-            }
-
-            if (iface_types_str) {
-                smap_replace(&new_ids, "iface-types", iface_types_str);
-            }
-
+            smap_replace(&new_ids, "ovn-bridge-mappings", bridge_mappings);
+            smap_replace(&new_ids, "datapath-type", datapath_type);
+            smap_replace(&new_ids, "iface-types", iface_types_str);
             sbrec_chassis_verify_external_ids(chassis_rec);
             sbrec_chassis_set_external_ids(chassis_rec, &new_ids);
             smap_destroy(&new_ids);
@@ -194,6 +170,7 @@ chassis_run(struct controller_ctx *ctx, const char *chassis_id,
         if (same) {
             /* Nothing changed. */
             inited = true;
+            ds_destroy(&iface_types);
             return chassis_rec;
         } else if (!inited) {
             struct ds cur_encaps = DS_EMPTY_INITIALIZER;
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index f7acf2d..795be68 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -215,18 +215,18 @@
 
     <column name="external_ids" key="datapath-type">
       <code>ovn-controller</code> populates this key with the datapath type
-      configured in the <ref table="Bridge" column="datapath_type"/> of the
-      Open_vSwitch database's <ref table="Bridge" db="Open_vSwitch"/> table.
-      Other applications should treat this key as read-only. See
+      configured in the <ref table="Bridge" column="datapath_type"/> column of
+      the Open_vSwitch database's <ref table="Bridge" db="Open_vSwitch"/>
+      table.  Other applications should treat this key as read-only. See
       <code>ovn-controller</code>(8) for more information.
     </column>
 
     <column name="external_ids" key="iface-types">
-      <code>ovn-controller</code> populates this key with the iface-types
-      configured in the <ref table="Open_vSwitch" column="iface_types"/> of the
-      Open_vSwitch database's <ref table="Open_vSwitch" db="Open_vSwitch"/> table.
-      Other applications should treat this key as read-only. See
-      <code>ovn-controller</code>(8) for more information.
+      <code>ovn-controller</code> populates this key with the interface types
+      configured in the <ref table="Open_vSwitch" column="iface_types"/> column
+      of the Open_vSwitch database's <ref table="Open_vSwitch"
+      db="Open_vSwitch"/> table.  Other applications should treat this key as
+      read-only. See <code>ovn-controller</code>(8) for more information.
     </column>
 
     <group title="Common Columns">
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 91fb2af..af64804 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -152,7 +152,9 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 
 AT_CLEANUP
 
-AT_SETUP([ovn-controller - chassis:external_ids - datapath-type and iface-types])
+# Checks that ovn-controller populates datapath-type and iface-types
+# correctly in the Chassis external-ids column.
+AT_SETUP([ovn-controller - Chassis external_ids])
 AT_KEYWORDS([ovn])
 ovn_init_db ovn-sb
 
@@ -166,32 +168,34 @@ ovs-vsctl \
     -- add-br br-eth2
 ovn_attach n1 br-phys 192.168.0.1
 
-# Make sure that the datapath_type set in the bridge table
+sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
+
+# Make sure that the datapath_type set in the Bridge table
 # is mirrored into the Chassis record in the OVN_Southbound db.
 check_datapath_type () {
     datapath_type=$1
-    sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
-    chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} external_ids:datapath-type | sed -e 's/\"//g')
-    AT_CHECK([test "${datapath_type}" = "${chassis_datapath_type}"])
+    chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} external_ids:datapath-type | sed -e 's/"//g') #"
+    test "${datapath_type}" = "${chassis_datapath_type}"
 }
 
-check_datapath_type ""
+OVS_WAIT_UNTIL([check_datapath_type ""])
 
 ovs-vsctl set Bridge br-int datapath-type=foo
-check_datapath_type foo
+OVS_WAIT_UNTIL([check_datapath_type foo])
 
 # Change "ovn-bridge-mappings" value. It should not change the "datapath-type".
 ovs-vsctl set Open_vSwitch . external_ids:ovn-bridge-mappings=foo-mapping
 check_datapath_type foo
 
 ovs-vsctl set Bridge br-int datapath-type=bar
-check_datapath_type bar
+OVS_WAIT_UNTIL([check_datapath_type bar])
 
 ovs-vsctl set Bridge br-int datapath-type=\"\"
-check_datapath_type ""
+OVS_WAIT_UNTIL([check_datapath_type ""])
 
+# The following will need to be updated as OVS starts to support more
+# interface types.
 expected_iface_types="dummy,dummy-pmd,geneve,gre,internal,ipsec_gre,lisp,patch,stt,system,tap,vxlan"
-sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
 chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} external_ids:iface-types | sed -e 's/\"//g')
 echo "chassis_iface_types = ${chassis_iface_types}"
 AT_CHECK([test "${expected_iface_types}" = "${chassis_iface_types}"])
@@ -199,10 +203,11 @@ AT_CHECK([test "${expected_iface_types}" = "${chassis_iface_types}"])
 # Change the value of external_ids:iface-types using ovn-sbctl.
 # ovn-controller should again set it back to proper one.
 ovn-sbctl set Chassis ${sysid} external_ids:iface-types="foo"
-sleep 1
-chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} external_ids:iface-types | sed -e 's/\"//g')
-echo "chassis_iface_types = ${chassis_iface_types}"
-AT_CHECK([test "${expected_iface_types}" = "${chassis_iface_types}"])
+OVS_WAIT_UNTIL([
+    chassis_iface_types=$(ovn-sbctl get Chassis ${sysid} external_ids:iface-types | sed -e 's/\"//g')
+    echo "chassis_iface_types = ${chassis_iface_types}"
+    test "${expected_iface_types}" = "${chassis_iface_types}"
+])
 
 # Gracefully terminate daemons
 OVN_CLEANUP_SBOX([hv])



More information about the dev mailing list