Notification time stamped 2021-03-01 07:11:36 UTC
From af11e5bdbdeb85a58a61ef1b247c6e18cc8d076e Mon Sep 17 00:00:00 2001
From: Numan Siddique <numans(a)ovn.org>
Date: Mar 01 2021 07:11:11 +0000
Subject: Backport "ofctrl: Fix the assert seen when flood removing flows with conj
actions."
Resolves: #1929978
Signed-off-by: Numan Siddique <numans(a)ovn.org>
---
diff --git a/0001-ofctrl-Fix-assert-seen-when-flood-removing-flows-conj.patch
b/0001-ofctrl-Fix-assert-seen-when-flood-removing-flows-conj.patch
new file mode 100644
index 0000000..5b4c6bd
--- /dev/null
+++ b/0001-ofctrl-Fix-assert-seen-when-flood-removing-flows-conj.patch
@@ -0,0 +1,244 @@
+From 16476ae504d692ec0a7440c27037d155a3f76445 Mon Sep 17 00:00:00 2001
+From: Numan Siddique <numans(a)ovn.org>
+Date: Tue, 23 Feb 2021 00:18:42 +0530
+Subject: [PATCH] ofctrl: Fix the assert seen when flood removing flows with
+ conj actions.
+
+In one of the scaled deployments, ovn-controller is asserting with the
+below stack trace
+
+ ***
+ (gdb) bt
+ 0 raise () from /lib64/libc.so.6
+ 1 abort () from /lib64/libc.so.6
+ 2 ovs_abort_valist ("%s: assertion %s failed in %s()") at
lib/util.c:419
+ 3 vlog_abort_valist ("%s: assertion %s failed in %s()") at
lib/vlog.c:1249
+ 4 vlog_abort ("%s: assertion %s failed in %s()") at lib/vlog.c:1263
+ 5 ovs_assert_failure (where="controller/ofctrl.c:1216",
+ function="flood_remove_flows_for_sb_uuid",
+
condition="ovs_list_is_empty(&f->list_node)") at lib/util.c:86
+ 6 flood_remove_flows_for_sb_uuid (sb_uuid=...134,
+ flood_remove_nodes=...ef0) at controller/ofctrl.c:1216
+ 9 ofctrl_flood_remove_flows (flood_remove_nodes=...ef0) at
controller/ofctrl.c:1280
+ 10 lflow_handle_changed_ref (ref_type=REF_TYPE_PORTGROUP,
+ ref_name= "5564_pg_14...aac") at controller/lflow.c:522
+ 11 _flow_output_resource_ref_handler (ref_type=REF_TYPE_PORTGROUP)
+ at controller/ovn-controller.c:2220
+ 12 engine_compute () at lib/inc-proc-eng.c:306
+ 13 engine_run_node (recompute_allowed=true) at lib/inc-proc-eng.c:352
+ 14 engine_run (recompute_allowed=true) at lib/inc-proc-eng.c:377
+ 15 main () at controller/ovn-controller.c:2887
+ ***
+
+The reason for this assert is different from the assertion bug fixed in
+the commit 858d1dd716("ofctrl: Fix the assert seen when flood removing
+flows.")
+
+The above assertion is seen if lflow.c calls ofctrl_add_or_append_flow()
+twice for the same flow uuid 'S'. When this function is called for
+the first time, a new desired flow 'f' is created and 'f->references'
+will have [(S, f)]. When it is called for the 2nd time, the list
+'f->references' is updated as [(S, f), (S,f)]. Later when
+flood_remove_flows_for_sb_uuid() is called for 'S', the above assertion
+is seen.
+
+This scenarion can happen when ovn-controller receives an update message
+from SB ovsdb-server in which a new logical flow 'F' belonging to a
+datapath 'D' is added which would result in conjunction flows. If this
+datapath 'D' is added to 'local_datapaths' in the same loop, then
+logical flow 'F' is first processed in lflow_add_flows_for_datapath()
+and the second time in lflow_handle_changed_flows().
+
+This issue can be fixed in 2 ways
+1. Flood remove all the tracked flows in lflow_handle_changed_flows()
+ first and then process the modified or newly added logical flows.
+
+2. In the function ofctrl_add_or_append_flow(), check if there is
+ already a reference for the flow uuid 'S' in the existing desired
+ flows and only append the actions if it doesn't exist.
+
+This patch has taken the approach (1) to ensure correctness of flows.
+
+Reported-at:
https://bugzilla.redhat.com/show_bug.cgi?id=1929978
+Signed-off-by: Numan Siddique <numans(a)ovn.org>
+Acked-by: Dumitru Ceara <dceara(a)redhat.com>
+Signed-off-by: Numan Siddique <numans(a)ovn.org>
+
+(cherry-picked from master commit c6c61b4e3462fb5201a61a226c2acaf6f4caf917)
+---
+ controller/lflow.c | 34 ++++-----------
+ tests/ovn.at | 104 +++++++++++++++++++++++++++++++++++++++++++++
+ 2 files changed, 112 insertions(+), 26 deletions(-)
+
+diff --git a/controller/lflow.c b/controller/lflow.c
+index 2b7d3565e..21ff51d2b 100644
+--- a/controller/lflow.c
++++ b/controller/lflow.c
+@@ -480,22 +480,19 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
+ struct controller_event_options controller_event_opts;
+ controller_event_opts_init(&controller_event_opts);
+
+- /* Handle flow removing first (for deleted or updated lflows), and then
+- * handle reprocessing or adding flows, so that when the flows being
+- * removed and added with same match conditions can be processed in the
+- * proper order */
+-
++ /* Flood remove the flows for all the tracked lflows. Its possible that
++ * lflow_add_flows_for_datapath() may have been called before calling
++ * this function. */
+ struct hmap flood_remove_nodes = HMAP_INITIALIZER(&flood_remove_nodes);
+ struct ofctrl_flood_remove_node *ofrn, *next;
+ SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
+ l_ctx_in->logical_flow_table) {
++ VLOG_DBG("delete lflow "UUID_FMT,
UUID_ARGS(&lflow->header_.uuid));
++ ofrn = xmalloc(sizeof *ofrn);
++ ofrn->sb_uuid = lflow->header_.uuid;
++ hmap_insert(&flood_remove_nodes, &ofrn->hmap_node,
++ uuid_hash(&ofrn->sb_uuid));
+ if (!sbrec_logical_flow_is_new(lflow)) {
+- VLOG_DBG("delete lflow "UUID_FMT,
+- UUID_ARGS(&lflow->header_.uuid));
+- ofrn = xmalloc(sizeof *ofrn);
+- ofrn->sb_uuid = lflow->header_.uuid;
+- hmap_insert(&flood_remove_nodes, &ofrn->hmap_node,
+- uuid_hash(&ofrn->sb_uuid));
+ if (l_ctx_out->lflow_cache_map) {
+ lflow_cache_delete(l_ctx_out->lflow_cache_map, lflow);
+ }
+@@ -525,21 +522,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
+ }
+ hmap_destroy(&flood_remove_nodes);
+
+- /* Now handle new lflows only. */
+- SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
+- l_ctx_in->logical_flow_table) {
+- if (sbrec_logical_flow_is_new(lflow)) {
+- VLOG_DBG("add lflow "UUID_FMT,
+- UUID_ARGS(&lflow->header_.uuid));
+- if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
+- &nd_ra_opts, &controller_event_opts,
+- l_ctx_in, l_ctx_out)) {
+- ret = false;
+- l_ctx_out->conj_id_overflow = true;
+- break;
+- }
+- }
+- }
+ dhcp_opts_destroy(&dhcp_opts);
+ dhcp_opts_destroy(&dhcpv6_opts);
+ nd_ra_opts_destroy(&nd_ra_opts);
+diff --git a/tests/ovn.at b/tests/ovn.at
+index e403fd0f4..6cb4a4cad 100644
+--- a/tests/ovn.at
++++ b/tests/ovn.at
+@@ -23963,3 +23963,107 @@ done
+
+ OVN_CLEANUP([hv1])
+ AT_CLEANUP
++
++# Test case to check that ovn-controller doesn't assert when
++# handling conjunction flows. When ovn-controller claims
++# the first port of a logical switch datapath, it programs the flows
++# for this datapath incrementally (without full recompute). If
++# suppose, in the same SB update from ovsdb-server, a logical flow is added
++# which results in conjunction action, then this logical flow is also
++# handled incrementally. The newly added logical flow is processed
++# twice which results in wrong oflows and it results in an assertion
++# in ovn-controller. Test this ovn-controller handles this scenario
++# properly and doesn't assert.
++AT_SETUP([ovn -- No ovn-controller assert when generating conjunction flows])
++ovn_start
++
++net_add n1
++sim_add hv1
++as hv1
++ovs-vsctl add-br br-phys
++ovn_attach n1 br-phys 192.168.0.10
++
++as hv1
++ovs-vsctl \
++ -- add-port br-int vif1 \
++ -- set Interface vif1 external_ids:iface-id=sw0-p1 \
++ ofport-request=1
++
++check as hv1
++ovs-vsctl set open . external_ids:ovn-monitor-all=true
++
++check ovn-nbctl ls-add sw0
++check ovn-nbctl pg-add pg1
++check ovn-nbctl pg-add pg2
++check ovn-nbctl lsp-add sw0 sw0-p2
++check ovn-nbctl lsp-set-addresses sw0-p2 "00:00:00:00:00:02 192.168.47.2"
++check ovn-nbctl lsp-add sw0 sw0-p3
++check ovn-nbctl lsp-set-addresses sw0-p3 "00:00:00:00:00:03 192.168.47.3"
++
++# Pause ovn-northd. When it is resumed, all the below NB updates
++# will be sent in one transaction.
++
++check as northd ovn-appctl -t ovn-northd pause
++check as northd-backup ovn-appctl -t ovn-northd pause
++
++check ovn-nbctl lsp-add sw0 sw0-p1
++check ovn-nbctl lsp-set-addresses sw0-p1 "00:00:00:00:00:01 192.168.47.1"
++check ovn-nbctl pg-set-ports pg1 sw0-p1 sw0-p2
++check ovn-nbctl pg-set-ports pg2 sw0-p3
++check ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4
&& ip4.src == \$pg2_ip4 && udp && udp.dst >= 1 &&
udp.dst <= 65535" allow-related
++
++# resume ovn-northd now. This should result in a single update message
++# from SB ovsdb-server to ovn-controller for all the above NB updates.
++check as northd ovn-appctl -t ovn-northd resume
++
++AS_BOX([Wait for sw0-p1 to be up])
++wait_for_ports_up sw0-p1
++
++# When the port group pg1 is updated, it should not result in
++# any assert in ovn-controller.
++ovn-nbctl --wait=hv pg-set-ports pg1 sw0-p1 sw0-p2 sw0-p3
++AT_CHECK([kill -0 $(cat hv1/ovn-controller.pid)])
++check ovn-nbctl --wait=hv sync
++
++# Check OVS flows are installed properly.
++AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=45 | ofctl_strip_all | \
++ grep "priority=2002" | grep conjunction | \
++ sed 's/conjunction([[^)]]*)/conjunction()/g' | sort], [0], [dnl
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x10/0xfff0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x100/0xff00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x1000/0xf000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x2/0xfffe
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x20/0xffe0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x200/0xfe00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x2000/0xe000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x4/0xfffc
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x40/0xffc0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x400/0xfc00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x4000/0xc000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x8/0xfff8
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x80/0xff80
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x800/0xf800
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x8000/0x8000
actions=conjunction()
++ table=45, priority=2002,udp,reg0=0x100/0x100,metadata=0x1,nw_src=192.168.47.3,tp_dst=1
actions=conjunction()
++ table=45, priority=2002,udp,reg0=0x100/0x100,reg15=0x3,metadata=0x1,nw_src=192.168.47.3
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x10/0xfff0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x100/0xff00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x1000/0xf000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x2/0xfffe
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x20/0xffe0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x200/0xfe00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x2000/0xe000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x4/0xfffc
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x40/0xffc0
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x400/0xfc00
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x4000/0xc000
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x8/0xfff8
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x80/0xff80
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x800/0xf800
actions=conjunction()
++ table=45,
priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=0x8000/0x8000
actions=conjunction()
++ table=45, priority=2002,udp,reg0=0x80/0x80,metadata=0x1,nw_src=192.168.47.3,tp_dst=1
actions=conjunction()
++ table=45, priority=2002,udp,reg0=0x80/0x80,reg15=0x3,metadata=0x1,nw_src=192.168.47.3
actions=conjunction()
++])
++
++OVN_CLEANUP([hv1])
++AT_CLEANUP
+--
+2.29.2
+
diff --git a/ovn.spec b/ovn.spec
index fe7fa57..a65feb1 100644
--- a/ovn.spec
+++ b/ovn.spec
@@ -43,7 +43,7 @@ Name: ovn
Summary: Open Virtual Network support
URL:
http://www.openvswitch.org/
Version: 20.12.0
-Release: 22%{?commit0:.%{date}git%{shortcommit0}}%{?dist}
+Release: 23%{?commit0:.%{date}git%{shortcommit0}}%{?dist}
Obsoletes: openvswitch-ovn-common < %{?epoch_ovs:%{epoch_ovs}:}2.11.0-8
Provides: openvswitch-ovn-common = %{?epoch:%{epoch}:}%{version}-%{release}
@@ -175,6 +175,9 @@ Patch220:
0001-northd-Provide-the-Gateway-router-option-lb_force_sn.patch
Patch230: 0001-binding-Fix-potential-crash-when-binding_seqno_run-i.patch
Patch231: 0002-ofctrl-Do-not-link-a-desired-flow-twice.patch
+# Bug 1929978
+Patch240: 0001-ofctrl-Fix-assert-seen-when-flood-removing-flows-conj.patch
+
# OpenvSwitch backports (400-) if required.
# Address crpto policy for fedora
%if 0%{?fedora}
@@ -540,6 +543,9 @@ fi
%{_unitdir}/ovn-controller-vtep.service
%changelog
+* Mon Mar 01 2021 Numan Siddique <numans(a)ovn.org> - 20.12.0-23
+- Backport "ofctrl: Fix the assert seen when flood removing flows with conj
actions." (#1929978)
+
* Mon Feb 22 2021 Numan Siddique <numans(a)ovn.org> - 20.12.0-22
- Backport "binding: Fix potential crash when binding_seqno_run is skipped."
(#1930030)
- Backport "ofctrl: Do not link a desired flow twice."
https://src.fedoraproject.org/rpms/ovn/c/af11e5bdbdeb85a58a61ef1b247c6e18...