ovn-controller.c: Fix assertion failure during address set update.

When an address set is deleted and a new one is created immediately with
the same name, the OVSDB deletion and creation notifications can come to
ovn-controller within the same message, and the order of the deletion
and addition in the IDL is undefined. In this case, if the deletion
happens to be processed after the addition, it will result in problems.

If the content of the new address set is the same as the old one, the
addition will be handled as no change to an existing address set and
ignored, but later when handling the deletion, the related flows will be
deleted, while we actually expect no change to the flows.

If the content of the new address set is different from the old one, the
addition will be handled as an update to the old address set, and the
delta will be tracked in the I-P engine node data's "updated" member.
Later when handling the deletion, the address set will be deleted from
the engine node data. So at this point the tracked change in "updated"
doesn't have a related address set object in the data, so later when the
lflow node handles the addr_sets changes, the assertion fails as shown
in below example backtrace:

0  0x00007ffbe5fcabc5 in raise () from /lib64/libc.so.6
1  0x00007ffbe5fb38a4 in abort () from /lib64/libc.so.6
2  0x00000000004f579e in ovs_abort_valist (err_no=<optimized out>, format=<optimized out>, args=<optimized out>) at ../lib/util.c:444
3  0x00000000004fd001 in vlog_abort_valist (module_=<optimized out>, message=0x5e6fd0 "%s: assertion %s failed in %s()", args=args@entry=0x7ffed304caa8) at ../lib/vlog.c:1249
4  0x00000000004fd097 in vlog_abort (module=<optimized out>, message=<optimized out>) at ../lib/vlog.c:1263
5  0x00000000004f54e1 in ovs_assert_failure (where=<optimized out>, function=<optimized out>, condition=<optimized out>) at ../lib/util.c:86
6  0x000000000041941d in as_update_can_be_handled (l_ctx_in=0x7ffed304cda0, l_ctx_in=0x7ffed304cda0, as_diff=0x25520f0, as_name=0x2550a00 "as1") at ../controller/lflow.c:766
7  lflow_handle_addr_set_update (as_name=0x2550a00 "as1", as_diff=0x25520f0, l_ctx_in=0x7ffed304cda0, l_ctx_out=0x7ffed304cd50, changed=0x7ffed304cd4f) at ../controller/lflow.c:869
8  0x0000000000438376 in lflow_output_addr_sets_handler (node=0x7ffed3054420, data=<optimized out>) at ../controller/ovn-controller.c:2648
9  0x000000000045708e in engine_compute (recompute_allowed=<optimized out>, node=<optimized out>) at ../lib/inc-proc-eng.c:417
10 engine_run_node (recompute_allowed=true, node=0x7ffed3054420) at ../lib/inc-proc-eng.c:479
11 engine_run (recompute_allowed=true) at ../lib/inc-proc-eng.c:504
12 0x000000000040939b in main (argc=<optimized out>, argv=<optimized out>) at ../controller/ovn-controller.c:3757

This patch fixes it by handling deletions before additions/updates,
similar to how we handle other changes such as lflow and port-groups.

Fixes: c0fe8dc ("ovn-controller: Tracking SB address set updates.")
Signed-off-by: Han Zhou <hzhou@ovn.org>
Acked-by: Ales Musil <amusil@redhat.com>

controller/ovn-controller.c

@@ -2015,7 +2015,11 @@ addr_sets_update(const struct sbrec_address_set_table *address_set_table,
         if (sbrec_address_set_is_deleted(as)) {
             expr_const_sets_remove(addr_sets, as->name);
             sset_add(deleted, as->name);
-        } else {
+        }
+    }
+
+    SBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (as, address_set_table) {
+        if (!sbrec_address_set_is_deleted(as)) {
             struct expr_constant_set *cs_old = shash_find_data(addr_sets,
                                                                as->name);
             if (!cs_old) {

tests/ovn-controller.at

@@ -2065,6 +2065,57 @@ AT_CHECK([echo $(($reprocess_count_new - $reprocess_count_old))], [0], [2
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 
+AT_SETUP([ovn-controller - address set del-and-add])
+
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+check ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=ls1-lp1
+
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl lsp-add ls1 ls1-lp1 \
+-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01"
+
+wait_for_ports_up
+ovn-appctl -t ovn-controller vlog/set file:dbg
+
+ovn-nbctl create address_set name=as1 addresses=8.8.8.8
+check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1' drop
+check ovn-nbctl --wait=hv sync
+AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100"], [0], [1
+])
+
+# pause ovn-northd
+check as northd ovn-appctl -t ovn-northd pause
+check as northd-backup ovn-appctl -t ovn-northd pause
+
+# Simulate a SB address set "del and add" notification to ovn-controller in the
+# same IDL iteration. The flows programmed by ovn-controller should reflect the
+# newly added address set. In reality it can happen when CMS deletes an
+# address-set and immediately creates a new address-set with the same name
+# (with same or different content). The notification of the changes can come to
+# ovn-controller in one shot and the order of the "del" and "add" in the IDL is
+# undefined. This test runs the scenario ten times to make sure different
+# orders are covered and handled properly.
+
+flow_count=$(ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100")
+for i in $(seq 10); do
+    # Delete and recreate the SB address set with same name and an extra IP.
+    addrs_=$(fetch_column address_set addresses name=as1)
+    addrs=${addrs_// /,}
+    AT_CHECK([ovn-sbctl destroy address_set as1 -- create address_set name=as1 addresses=$addrs,1.1.1.$i], [0], [ignore])
+    OVS_WAIT_UNTIL([test $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep -c "priority=1100") = "$(($i + 1))"])
+done
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+
 AT_SETUP([ovn-controller - I-P handle lb_hairpin_use_ct_mark change])
 
 ovn_start --backup-northd=none