diff mbox

[ovs-dev,v2] ovn-controller: Reset flow processing after (re)connection to switch

Message ID 107f6263-4c17-80fc-a7ba-cc78cc67bd3e@redhat.com
State Accepted
Headers show

Commit Message

Numan Siddique Aug. 11, 2016, 12:21 p.m. UTC
When ovn-controller reconnects to the ovs-vswitchd, it deletes all the
OF flows in the switch. It doesn't install the flows again, leaving
the datapath broken unless ovn-controller is restarted or ovn-northd
updates the SB DB.

The reason for this is
  - lflow_reset_processing() is not called after the reconnection
  - the hmap "installed_flows" is not cleared, because of which
    ofctrl_put skips adding the flows to the switch.

This patch fixes the issue and also adds a test case to test
this scenario.

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 ovn/controller/ofctrl.c |  7 ++++
 tests/ovn.at            | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

v1 -> v2
--------
 * Enhanced the test case to restart vswitchd and restore the flows

Comments

Ryan Moats Aug. 11, 2016, 2:15 p.m. UTC | #1
"dev" <dev-bounces@openvswitch.org> wrote on 08/11/2016 07:21:39 AM:

> From: Numan Siddique <nusiddiq@redhat.com>
> To: ovs dev <dev@openvswitch.org>
> Date: 08/11/2016 07:21 AM
> Subject: [ovs-dev] [PATCH v2] ovn-controller: Reset flow processing
> after (re)connection to switch
> Sent by: "dev" <dev-bounces@openvswitch.org>
>
> When ovn-controller reconnects to the ovs-vswitchd, it deletes all the
> OF flows in the switch. It doesn't install the flows again, leaving
> the datapath broken unless ovn-controller is restarted or ovn-northd
> updates the SB DB.
>
> The reason for this is
>   - lflow_reset_processing() is not called after the reconnection
>   - the hmap "installed_flows" is not cleared, because of which
>     ofctrl_put skips adding the flows to the switch.
>
> This patch fixes the issue and also adds a test case to test
> this scenario.
>
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> ---

This all looks does what it promises, so let's get the immediate
out of the way:

Acked-by: Ryan Moats <rmoats@us.ibm.com>

Having said that, I'm still concerned about what happens in the
kernel during an ovs-vswitchd restart...

I *think* everything will be fine for active connections whose
flow rules are still in the kernel, but there will be a momentary
loss of connectivity for those connections that require an upcall
to access the flows known to vswitchd.

If the above is right, then I'm willing to accept that, but it
begs two questions:

(1) is this something that is too complex for a unit test case?

(2) if it isn't too complex, how would one code such a unit
    test case?
Numan Siddique Aug. 11, 2016, 2:40 p.m. UTC | #2
On Thu, Aug 11, 2016 at 7:45 PM, Ryan Moats <rmoats@us.ibm.com> wrote:

> "dev" <dev-bounces@openvswitch.org> wrote on 08/11/2016 07:21:39 AM:
>
> >
> > Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> > ---
>
> This all looks does what it promises, so let's get the immediate
> out of the way:
>
> Acked-by: Ryan Moats <rmoats@us.ibm.com>
>

​Thanks for the Ack and review.
​


>
>
> Having said that, I'm still concerned about what happens in the
> kernel during an ovs-vswitchd restart...
>
> I *think* everything will be fine for active connections whose
> flow rules are still in the kernel, but there will be a momentary
> loss of connectivity for those connections that require an upcall
> to access the flows known to vswitchd.
>
> If the above is right, then I'm willing to accept that, but it
> begs two questions:
>
> (1) is this something that is too complex for a unit test case?
>
> (2) if it isn't too complex, how would one code such a unit
>     test case?
>
>
> ​I am not too familiar with the vswitchd internals. So I am not sure about
it.

Thanks
Numan
​
diff mbox

Patch

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 9388cb8..26f85db 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -20,6 +20,7 @@ 
 #include "flow.h"
 #include "hash.h"
 #include "hindex.h"
+#include "lflow.h"
 #include "ofctrl.h"
 #include "openflow/openflow.h"
 #include "openvswitch/dynamic-string.h"
@@ -368,6 +369,7 @@  run_S_CLEAR_FLOWS(void)
 
     /* Clear installed_flows, to match the state of the switch. */
     ovn_flow_table_clear();
+    lflow_reset_processing();
 
     /* Clear existing groups, to match the state of the switch. */
     if (groups) {
@@ -812,6 +814,11 @@  ovn_flow_table_clear(void)
         hindex_remove(&uuid_flow_table, &f->uuid_hindex_node);
         ovn_flow_destroy(f);
     }
+
+    HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_flows) {
+        hmap_remove(&installed_flows, &f->match_hmap_node);
+        ovn_flow_destroy(f);
+    }
 }
 
 static void
diff --git a/tests/ovn.at b/tests/ovn.at
index a95e0b2..7e3e25d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4072,3 +4072,98 @@  AT_CHECK([cat received2.packets], [0], [expout])
 OVN_CLEANUP([hv1])
 
 AT_CLEANUP
+
+AT_SETUP([ovn -- ovs-vswitchd restart])
+AT_KEYWORDS([vswitchd restart])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+ovn-nbctl ls-add ls1
+
+ovn-nbctl lsp-add ls1 ls1-lp1 \
+-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01 10.0.0.4"
+
+ovn-nbctl lsp-set-port-security ls1-lp1 "f0:00:00:00:00:01 10.0.0.4"
+
+net_add n1
+sim_add hv1
+
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+ovn_populate_arp
+sleep 2
+
+as hv1 ovs-vsctl show
+
+echo "---------------------"
+ovn-sbctl dump-flows
+echo "---------------------"
+
+echo "------ hv1 dump ----------"
+as hv1 ovs-ofctl dump-flows br-int
+total_flows=`as hv1 ovs-ofctl dump-flows br-int | wc -l`
+
+echo "Total flows before vswitchd restart = " $total_flows
+
+# Code taken from ovs-save utility
+save_flows () {
+    echo "ovs-ofctl add-flows br-int - << EOF" > restore_flows.sh
+    as hv1 ovs-ofctl dump-flows "br-int" | sed -e '/NXST_FLOW/d' \
+            -e 's/\(idle\|hard\)_age=[^,]*,//g' >> restore_flows.sh
+    echo "EOF" >> restore_flows.sh
+}
+
+restart_vswitchd () {
+    restore_flows=$1
+
+    if test $restore_flows = true; then
+        save_flows
+    fi
+
+    as hv1
+    OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
+
+    if test $restore_flows = true; then
+        as hv1
+        ovs-vsctl --no-wait set open_vswitch . other_config:flow-restore-wait="true"
+    fi
+
+    as hv1
+    start_daemon ovs-vswitchd --enable-dummy=system -vvconn -vofproto_dpif -vunixctl
+    ovs-ofctl dump-flows br-int
+
+    if test $restore_flows = true; then
+        sh ./restore_flows.sh
+        echo "Flows after restore"
+        as hv1
+        ovs-ofctl dump-flows br-int
+        ovs-vsctl --no-wait --if-exists remove open_vswitch . other_config \
+            flow-restore-wait="true"
+    fi
+}
+
+# Save the flows, restart vswitchd and restore the flows
+restart_vswitchd true
+sleep 2
+total_flows_after_restart=`as hv1 ovs-ofctl dump-flows br-int | wc -l`
+echo "Total flows after vswitchd restart = " $total_flows_after_restart
+
+AT_CHECK([test "${total_flows}" = "${total_flows_after_restart}"])
+
+# Restart vswitchd without restoring
+restart_vswitchd false
+sleep 2
+total_flows_after_restart=`as hv1 ovs-ofctl dump-flows br-int | wc -l`
+echo "Total flows after vswitchd restart = " $total_flows_after_restart
+
+AT_CHECK([test "${total_flows}" = "${total_flows_after_restart}"])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP