On Thu, May 25, 2023 at 6:01 PM Han Zhou <[email protected]> wrote: > 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: c0fe8dca767f ("ovn-controller: Tracking SB address set updates.") > Signed-off-by: Han Zhou <[email protected]> > --- > controller/ovn-controller.c | 6 ++++- > tests/ovn-controller.at | 51 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 1151d3664478..2706dc628d6d 100644 > --- a/controller/ovn-controller.c > +++ b/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) { > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index 64d6a9336613..a95834e69eca 100644 > --- a/tests/ovn-controller.at > +++ b/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 > -- > 2.30.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Looks good to me, thanks. Acked-by: Ales Musil <[email protected]> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
