[ovs-dev] [PATCH ovn] ofctrl: Don't assert while merging tracked flows.

[numans at ovn.org]

From: Numan Siddique <numans at ovn.org>

Presently we do ovs_assert(del_f->installed_flows) in
merged_tracked_flows() of ofctrl.c if 'del_f' and flow 'f' are equal
and if 'del_f' is not installed.  But there are a couple of scenarios
this can happen:

1. A flow 'F1' was added to the desired flows, but ofctrl_put() couldn't
install the flow (because of the rconn queue is full) and an SB update
caused 'F1' to be removed and re-added as 'F2'.

2. In a single engine run, a flow 'F1' was added to the desired flow,
removed from the desired flow and re-added as 'F2'.  This could
happen after the commit [1].

The likelihood of (2) seems to be higher with datapath groups enabled.

The assertion - ovs_assert(del_f->installed_flows) is observed in
few deployments after the commit [1].  This patch addressed this issue
by removing that assertion.  Removing this assertion should not have
any side effects as the flow 'F2' will be installed.

This patch is proposed based on the code inspection and the possibility
that the above mentioned scenarios can happen.  The patch doesn't have
any test cases to reproduce these scenarios.

[1] - c6c61b4e3462("ofctrl: Fix the assert seen when flood removing flows with conj actions.")

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1951502
Signed-off-by: Numan Siddique <numans at ovn.org>
---
 controller/ofctrl.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index c29c3d180..9e960b034 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -1931,11 +1931,29 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table)
                 continue;
             }
 
-            /* del_f must have been installed, otherwise it should have been
-             * removed during track_flow_add_or_modify. */
-            ovs_assert(del_f->installed_flow);
+            if (!del_f->installed_flow) {
+                /* del_f must have been installed, otherwise it should have
+                 * been removed during track_flow_add_or_modify.
+                 *
+                 * But however there are a couple of scenarios this may not
+                 * happen.
+                 *
+                 * Scenario 1:  A flow was added to the desired flows, but
+                 *              ofctrl_put() couldn't install the flow and
+                 *              an SB update caused the 'del_f' to be removed
+                 *              and re-added as 'f'.
+                 *
+                 * Scenario 2:  In a  single engine run, a flow 'del_f' was
+                 *              added to the desired flow, removed from the
+                 *              desired flow and re-added as 'f'.  This could
+                 *              happen after the commit c6c61b4e3462.
+                 *
+                 * Treat both the scenarios as valid scenarios and just remove
+                 * 'del_f' from the hmap - deleted_flows.
+                 * update_installed_flows_by_track() will install 'f'.
+                 */
 
-            if (!f->installed_flow) {
+            } else if (!f->installed_flow) {
                 /* f is not installed yet. */
                 replace_installed_to_desired(del_f->installed_flow, del_f, f);
             } else {
-- 
2.29.2