[ovs-dev] [PATCH] ovn-controller: Handle physical changes correctly

Ryan Moats rmoats at us.ibm.com
Fri Jul 22 21:54:26 UTC 2016


[1] reported increased failure rates in certain tests
with incremental processing (the numbers are the number of failures
seen in 100 tests):

   2  ovn -- vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS
  10  ovn -- 2 HVs, 2 LS, 1 lport/LS, 2 peer LRs
  52  ovn -- 1 HV, 1 LS, 2 lport/LS, 1 LR
  45  ovn -- 1 HV, 2 LSs, 1 lport/LS, 1 LR
  23  ovn -- 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static routes
  53  ovn -- 2 HVs, 3 LRs connected via LS, static routes
  32  ovn -- 2 HVs, 2 LRs connected via LS, gateway router
  50  ovn -- icmp_reply: 1 HVs, 2 LSs, 1 lport/LS, 1 LR

These failures were caused by a combination of problems in
handling physical changes:

  1. When a vif was removed, the localvif_to_ofport entry was not
     removed.
  2. When a physical change was detected, ovn-controller would wait
     a poll cycle before processing the logical flow table.

This patch set addresses both of these issues while simultaneously
cleaning up the code in physical.c.  A side effect is a modification
of where OF flows are dumped in the gateway router case that allowed
the root causes of this issue to be found.

With these changes, all of the above tests had a 100/100 success rate.

[1] http://openvswitch.org/pipermail/dev/2016-July/075803.html

Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
---
 ovn/controller/physical.c | 92 +++++++++++++++++++++--------------------------
 tests/ovn.at              | 25 ++++++++-----
 2 files changed, 57 insertions(+), 60 deletions(-)

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index b8f3105..dc4ef77 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -14,9 +14,11 @@
  */
 
 #include <config.h>
+#include "binding.h"
 #include "byte-order.h"
 #include "flow.h"
 #include "lflow.h"
+#include "lib/poll-loop.h"
 #include "ofctrl.h"
 #include "openvswitch/match.h"
 #include "openvswitch/ofp-actions.h"
@@ -53,6 +55,8 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 
 static struct simap localvif_to_ofport =
     SIMAP_INITIALIZER(&localvif_to_ofport);
+static struct simap new_localvif_to_ofport =
+    SIMAP_INITIALIZER(&new_localvif_to_ofport);
 static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
 
 /* UUID to identify OF flows not associated with ovsdb rows. */
@@ -638,51 +642,15 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             bool is_patch = !strcmp(iface_rec->type, "patch");
             if (is_patch && localnet) {
                 /* localnet patch ports can be handled just like VIFs. */
-                if (simap_find(&localvif_to_ofport, localnet)) {
-                    unsigned int old_port = simap_get(&localvif_to_ofport,
-                                                      localnet);
-                    if (old_port != ofport) {
-                        simap_put(&localvif_to_ofport, localnet, ofport);
-                        full_binding_processing = true;
-                        lflow_reset_processing();
-                    }
-                } else {
-                    simap_put(&localvif_to_ofport, localnet, ofport);
-                    full_binding_processing = true;
-                    lflow_reset_processing();
-                }
+                simap_put(&new_localvif_to_ofport, localnet, ofport);
                 break;
             } else if (is_patch && l2gateway) {
                 /* L2 gateway patch ports can be handled just like VIFs. */
-                if (simap_find(&localvif_to_ofport, l2gateway)) {
-                    unsigned int old_port = simap_get(&localvif_to_ofport,
-                                                      l2gateway);
-                    if (old_port != ofport) {
-                        simap_put(&localvif_to_ofport, l2gateway, ofport);
-                        full_binding_processing = true;
-                        lflow_reset_processing();
-                    }
-                } else {
-                    simap_put(&localvif_to_ofport, l2gateway, ofport);
-                    full_binding_processing = true;
-                    lflow_reset_processing();
-                }
+                simap_put(&new_localvif_to_ofport, l2gateway, ofport);
                 break;
             } else if (is_patch && logpatch) {
                 /* Logical patch ports can be handled just like VIFs. */
-                if (simap_find(&localvif_to_ofport, logpatch)) {
-                    unsigned int old_port = simap_get(&localvif_to_ofport,
-                                                      logpatch);
-                    if (old_port != ofport) {
-                        simap_put(&localvif_to_ofport, logpatch, ofport);
-                        full_binding_processing = true;
-                        lflow_reset_processing();
-                    }
-                } else {
-                    simap_put(&localvif_to_ofport, logpatch, ofport);
-                    full_binding_processing = true;
-                    lflow_reset_processing();
-                }
+                simap_put(&new_localvif_to_ofport, logpatch, ofport);
                 break;
             } else if (chassis_id) {
                 enum chassis_tunnel_type tunnel_type;
@@ -706,29 +674,48 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 tun->ofport = u16_to_ofp(ofport);
                 tun->type = tunnel_type;
                 full_binding_processing = true;
+                lflow_reset_processing();
+                binding_reset_processing();
+                poll_immediate_wake();
                 break;
             } else {
                 const char *iface_id = smap_get(&iface_rec->external_ids,
                                                 "iface-id");
                 if (iface_id) {
-                    if (simap_find(&localvif_to_ofport, iface_id)) {
-                        unsigned int old_port = simap_get(&localvif_to_ofport,
-                                                          iface_id);
-                        if (old_port != ofport) {
-                            simap_put(&localvif_to_ofport, iface_id, ofport);
-                            full_binding_processing = true;
-                            lflow_reset_processing();
-                        }
-                    } else {
-                        simap_put(&localvif_to_ofport, iface_id, ofport);
-                        full_binding_processing = true;
-                        lflow_reset_processing();
-                    }
+                    simap_put(&new_localvif_to_ofport, iface_id, ofport);
                 }
             }
         }
     }
 
+    /* Capture changed or removed openflow ports. */
+    bool localvif_map_changed = false;
+    struct simap_node *vif_name, *vif_name_next;
+    SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localvif_to_ofport) {
+        int newport;
+        if ((newport = simap_get(&new_localvif_to_ofport, vif_name->name))) {
+            if (newport != simap_get(&localvif_to_ofport, vif_name->name)) {
+                simap_put(&localvif_to_ofport, vif_name->name, newport);
+                localvif_map_changed = true;
+            }
+        } else {
+            simap_find_and_delete(&localvif_to_ofport, vif_name->name);
+            localvif_map_changed = true;
+        }
+    }
+    SIMAP_FOR_EACH (vif_name, &new_localvif_to_ofport) {
+        if (!simap_get(&localvif_to_ofport, vif_name->name)) {
+            simap_put(&localvif_to_ofport, vif_name->name,
+                      simap_get(&new_localvif_to_ofport, vif_name->name));
+            localvif_map_changed = true;
+        }
+    }
+    if (localvif_map_changed) {
+        full_binding_processing = true;
+        lflow_reset_processing();
+        poll_immediate_wake();
+    }
+
     struct ofpbuf ofpacts;
     ofpbuf_init(&ofpacts, 0);
 
@@ -866,6 +853,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     ofctrl_add_flow(OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts, hc_uuid);
 
     ofpbuf_uninit(&ofpacts);
+    simap_clear(&new_localvif_to_ofport);
     HMAP_FOR_EACH_POP (tun, hmap_node, &tunnels) {
         free(tun);
     }
diff --git a/tests/ovn.at b/tests/ovn.at
index 95aa822..614a4bb 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3192,14 +3192,6 @@ ovn-sbctl list chassis
 ovn-sbctl list encap
 echo "---------------------"
 
-echo "------ hv1 dump ----------"
-as hv1 ovs-ofctl show br-int
-as hv1 ovs-ofctl dump-flows br-int
-echo "------ hv2 dump ----------"
-as hv2 ovs-ofctl show br-int
-as hv2 ovs-ofctl dump-flows br-int
-echo "----------------------------"
-
 # Packet to Expect at alice1
 src_mac="000002010203"
 dst_mac="f00000010204"
@@ -3211,6 +3203,14 @@ expected=${dst_mac}${src_mac}08004500001c000000003e110200${src_ip}${dst_ip}00351
 as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
 as hv1 ovs-appctl ofproto/trace br-int in_port=1 $packet
 
+echo "------ hv1 dump after packet 1 ----------"
+as hv1 ovs-ofctl show br-int
+as hv1 ovs-ofctl dump-flows br-int
+echo "------ hv2 dump after packet 1 ----------"
+as hv2 ovs-ofctl show br-int
+as hv2 ovs-ofctl dump-flows br-int
+echo "----------------------------"
+
 $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/vif1-tx.pcap | trim_zeros > received1.packets
 echo $expected | trim_zeros > expout
 AT_CHECK([cat received1.packets], [0], [expout])
@@ -3232,6 +3232,15 @@ sleep 1
 
 # Send the packet again.
 as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
+
+echo "------ hv1 dump after packet 2 ----------"
+as hv1 ovs-ofctl show br-int
+as hv1 ovs-ofctl dump-flows br-int
+echo "------ hv2 dump after packet 2 ----------"
+as hv2 ovs-ofctl show br-int
+as hv2 ovs-ofctl dump-flows br-int
+echo "----------------------------"
+
 $PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/vif1-tx.pcap | trim_zeros > received1.packets
 echo $expected | trim_zeros >> expout
 AT_CHECK([cat received1.packets], [0], [expout])
-- 
1.9.1




More information about the dev mailing list