Hello, On Tue, Nov 5, 2024 at 4:37 PM Dumitru Ceara <[email protected]> wrote: > > Hi Xavier, > > On 11/5/24 13:07, Xavier Simonart wrote: > > Hi > > > > The test needs two small changes: > > - ovs-appctl vlog/set dbg should be removed (Thanks Dumitru for catching > > this) > > - the two AT_CHECK in the test should be replaced with OVS_WAIT_UNTIL to > > avoid a potential flaky test as waiting for ofports does not guarantee that > > ovn-controller handles them before the flow-dump. > > > > Ack, I changed these two checks and removed the "ovs-appctl vlog/set > dbg"; then I applied the fix to main and backported it to 24.09, 24.03 > and 23.09 (officially I think 23.09 is not supported anymore but it was > a straightforward cherry pick). > > Regards, > Dumitru > > > Thanks > > Xavier > > > > On Mon, Nov 4, 2024 at 4:52 PM Xavier Simonart <[email protected]> wrote: > > > >> This reverts commit edc064b4c589ab1bb69352523481bd6d997aa1ca. > >> > >> In setups with multiple localnet ports w/o vlans (or multiple localnet > >> ports with the same vlans), that patch was trying to create the same > >> flows with action conjunction multiple times, which caused the following > >> assert: > >> 0 0x00007ffff77cd834 in __pthread_kill_implementation () from > >> /lib64/libc.so.6 > >> 1 0x00007ffff777b8ee in raise () from /lib64/libc.so.6 > >> 2 0x00007ffff77638ff in abort () from /lib64/libc.so.6 > >> 3 0x000000000042f861 in flow_is_preferred (a=0xafe9a0, b=0xaf9380) at > >> controller/ofctrl.c:966 > >> 4 0x000000000042f340 in link_installed_to_desired (i=0xb2eaf0, > >> d=0xafe9a0) at controller/ofctrl.c:987 > >> 5 0x000000000042c17c in update_installed_flows_by_track > >> (flow_table=0x813a80, bc=0x7ffffffcc740, installed_flows=0x7894e0 > >> <installed_pflows>, msgs=0x7ffffffcc790) at controller/ofctrl.c:2583 > >> 6 0x000000000042af14 in ofctrl_put (lflow_table=0x810180, > >> pflow_table=0x813a80, pending_ct_zones=0x8129b0, > >> pending_lb_tuples=0x80e030, sbrec_meter_by_name=0x7e6840, req_cfg=0, > >> lflows_changed=true, pflows_changed=true) at controller/ofctrl.c:2826 > >> 7 0x000000000045015c in main (argc=1, argv=0x7fffffffe218) at > >> controller/ovn-controller.c:5788 > >> > >> Reverting that patch means that flows such as > >> table_id=0, priority=180, vlan_tci=0x0000/0x1000, > >> actions=conjunction(100,2/2) > >> remains after the localnet port is deleted. > >> > >> Fixes: edc064b4c589 ("controller: Properly handle localnet flows in I+P.") > >> Reported-at: https://issues.redhat.com/browse/FDP-926
Note that we also saw regression in the form flows unexpectedly not being present that was root caused to this patch (routing protocol redirect flows to be specific). Just wanted to highlight that so extra care is taken in any rerolls of the original series. I see that a "Compare I+P flows with recompute ones" [0] series is up, is that a reroll of this fix? 0: https://patchwork.ozlabs.org/project/ovn/list/?series=432794&state=* > >> Signed-off-by: Xavier Simonart <[email protected]> > >> > >> --- > >> v2: Update test based on Dumitru's feedback i.e. check some localnet flows. > >> --- > >> controller/physical.c | 9 ++-- > >> tests/ovn.at | 116 ++++++++++++++++++++++-------------------- > >> 2 files changed, 64 insertions(+), 61 deletions(-) > >> > >> diff --git a/controller/physical.c b/controller/physical.c > >> index 2aaa16cbd..c6db4f376 100644 > >> --- a/controller/physical.c > >> +++ b/controller/physical.c > >> @@ -699,7 +699,7 @@ put_replace_chassis_mac_flows(const struct shash > >> *ct_zones, > >> put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p); > >> ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, > >> rport_binding->header_.uuid.parts[0], > >> - &match, ofpacts_p, &localnet_port->header_.uuid); > >> + &match, ofpacts_p, hc_uuid); > >> > >> /* Provide second search criteria, i.e localnet port's > >> * vlan ID for conjunction flow */ > >> @@ -719,7 +719,7 @@ put_replace_chassis_mac_flows(const struct shash > >> *ct_zones, > >> conj->clause = 1; > >> ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 180, > >> rport_binding->header_.uuid.parts[0], > >> - &match, ofpacts_p, &localnet_port->header_.uuid); > >> + &match, ofpacts_p, hc_uuid); > >> } > >> } > >> > >> @@ -2393,9 +2393,8 @@ physical_handle_flows_for_lport(const struct > >> sbrec_port_binding *pb, > >> struct local_datapath *ldp = > >> get_local_datapath(p_ctx->local_datapaths, > >> pb->datapath->tunnel_key); > >> - if (!strcmp(pb->type, "external") || > >> - !strcmp(pb->type, "patch") || !strcmp(pb->type, "l3gateway")) { > >> - /* Those lports have a dependency on the localnet port. > >> + if (!strcmp(pb->type, "external")) { > >> + /* External lports have a dependency on the localnet port. > >> * We need to remove the flows of the localnet port as well > >> * and re-consider adding the flows for it. > >> */ > >> diff --git a/tests/ovn.at b/tests/ovn.at > >> index 10cd7a79b..403e1813a 100644 > >> --- a/tests/ovn.at > >> +++ b/tests/ovn.at > >> @@ -39495,62 +39495,6 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) > >> OVN_CLEANUP([hv1]) > >> AT_CLEANUP > >> ]) > >> -OVN_FOR_EACH_NORTHD([ > >> -AT_SETUP([localnet port flows after deletion]) > >> -ovn_start > >> -net_add n1 > >> - > >> -check ovn-nbctl ls-add sw0 > >> - > >> -for i in 1 2; do > >> - check ovn-nbctl lsp-add sw0 sw0-p${i} -- lsp-set-addresses sw0-p${i} > >> "00:00:10:01:02:0${i} 10.0.0.${i}" > >> - sim_add hv${i} > >> - as hv${i} > >> - ovs-vsctl add-br br-phys > >> - ovn_attach n1 br-phys 192.168.0.${i} > >> - ovs-vsctl set open . external_ids:ovn-bridge-mappings=physnet1:br-phys > >> - ovs-vsctl add-port br-int vif${i} -- \ > >> - set Interface vif${i} external-ids:iface-id=sw0-p${i} \ > >> - options:tx_pcap=hv${i}/vif${i}-tx.pcap \ > >> - options:rxq_pcap=hv${i}/vif${i}-rx.pcap > >> -done > >> - > >> -check ovn-nbctl lr-add lr0 > >> -check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.254/24 > >> -check <http://10.0.0.254/24-check> ovn-nbctl lsp-add sw0 sw0-lr0 > >> -check ovn-nbctl lsp-set-type sw0-lr0 router > >> -check ovn-nbctl lsp-set-addresses sw0-lr0 router > >> -check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > >> - > >> -check ovn-nbctl --wait=hv sync > >> -wait_for_ports_up > >> - > >> -# We should not have any flows in table OFTABLE_PHY_TO_LOG from in_port > >> different from vif1 and ovn-hv2-0 > >> -OVN_WAIT_REMOTE_INPUT_FLOWS(["hv1"],["hv2"]) > >> -of1=$(as hv1 ovs-vsctl --bare --columns ofport find Interface name=vif1) > >> -of2=$(as hv1 ovs-vsctl --bare --columns ofport find Interface > >> name=ovn-hv2-0) > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | > >> grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v > >> "in_port=$of2" | wc -l], [0], [dnl > >> -0 > >> -]) > >> - > >> -# Add localnet port to sw0 > >> -check ovn-nbctl lsp-add sw0 ln-sw0 -- lsp-set-addresses ln-sw0 unknown -- > >> lsp-set-type ln-sw0 localnet > >> -check ovn-nbctl --wait=hv lsp-set-options ln-sw0 network_name=physnet1 -- > >> set logical_switch_port ln-sw0 tag_request=100 > >> - > >> -OVN_WAIT_PATCH_PORT_FLOWS(["ln-sw0"], ["hv1"]) > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | > >> grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v > >> "in_port=$of2" | wc -l], [0], [dnl > >> -2 > >> -]) > >> - > >> -# Remove localnet port from sw0. Peer-ports flows should be deleted. > >> -check ovn-nbctl --wait=hv lsp-del ln-sw0 > >> -AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | > >> grep -v NXST_FLOW | grep "in_port=" | grep -v "in_port=$of1" | grep -v > >> "in_port=$of2" | wc -l], [0], [dnl > >> -0 > >> -]) > >> - > >> -OVN_CLEANUP([hv1],[hv2]) > >> -AT_CLEANUP > >> -]) > >> > >> AT_SETUP([Patch ports not owned by OVN]) > >> > >> @@ -39665,3 +39609,63 @@ check_patch_ports > >> > >> OVN_CLEANUP([hv1]) > >> AT_CLEANUP > >> + > >> +OVN_FOR_EACH_NORTHD([ > >> +AT_SETUP([Multiple localnet ports without vlans]) > >> +AT_KEYWORDS([localnet]) > >> + > >> +ovn_start > >> + > >> +net_add n1 > >> +sim_add hv1 > >> +ovs-vsctl add-br br-phys > >> +ovn_attach n1 br-phys 192.168.0.1 > >> +ovn-appctl vlog/set dbg > >> +ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=phys:br-phys > >> + > >> +check ovn-nbctl lr-add lr1 > >> +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:01:ff:02:03 192.168.1.254/24 > >> + > >> +check ovn-nbctl ls-add ls1 > >> +check ovn-nbctl lsp-add ls1 ls1-lr1 > >> +check ovn-nbctl lsp-set-type ls1-lr1 router > >> +check ovn-nbctl lsp-set-options ls1-lr1 router-port=lr1-ls1 > >> +check ovn-nbctl lsp-set-addresses ls1-lr1 router > >> + > >> +check ovn-nbctl ls-add pub > >> +check ovn-nbctl lsp-add pub pub-lr1 > >> +check ovn-nbctl lsp-set-type pub-lr1 router > >> +check ovn-nbctl lsp-set-options pub-lr1 router-port=lr1-pub > >> +check ovn-nbctl lsp-set-addresses pub-lr1 router > >> + > >> +check ovn-nbctl lrp-add lr1 lr1-pub 00:00:01:ff:01:03 172.16.1.254/24 > >> +check ovn-nbctl lrp-set-gateway-chassis lr1-pub hv1 > >> + > >> +check ovn-nbctl --wait=hv sync > >> +wait_for_ports_up > >> + > >> +check ovn-nbctl -- lsp-add pub pub-ln -- lsp-set-type pub-ln localnet -- > >> lsp-set-addresses pub-ln unknown -- lsp-set-options pub-ln > >> network_name=phys > >> +check ovn-nbctl -- lsp-add ls1 ls1-ln -- lsp-set-type ls1-ln localnet -- > >> lsp-set-addresses ls1-ln unknown -- lsp-set-options ls1-ln > >> network_name=phys > >> + > >> +# Wait for ofports. > >> +OVS_WAIT_UNTIL([ > >> + pub_ln_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find > >> Interface name=patch-br-int-to-pub-ln) > >> + test -n "$pub_ln_ofport" && test 1 -le $pub_ln_ofport > >> +]) > >> + > >> +OVS_WAIT_UNTIL([ > >> + ls1_ln_ofport=$(as hv1 ovs-vsctl --bare --columns ofport find > >> Interface name=patch-br-int-to-ls1-ln) > >> + test -n "$ls1_ln_ofport" && test 1 -le $ls1_ln_ofport > >> +]) > >> + > >> +pub_lr1_cookie=$(ovn-sbctl --bare --columns _uuid list port_binding > >> lr1-pub | awk -F '-' '{print $1}') > >> +ls1_lr1_cookie=$(ovn-sbctl --bare --columns _uuid list port_binding > >> lr1-ls1 | awk -F '-' '{print $1}') > >> + > >> +# Expect to receive one flow matching on conjunction id (replacing source > >> mac with router port mac) > >> +AT_CHECK([test "X1" = "X$(ovs-ofctl dump-flows br-int > >> cookie=0x${ls1_lr1_cookie}/-1 | grep "conj_id=100" | grep > >> in_port=$ls1_ln_ofport | wc -l)"]) > >> +AT_CHECK([test "X1" = "X$(ovs-ofctl dump-flows br-int > >> cookie=0x${pub_lr1_cookie}/-1 | grep "conj_id=100" | grep > >> in_port=$pub_ln_ofport | wc -l)"]) > >> + > >> +OVN_CLEANUP([hv1]) > >> + > >> +AT_CLEANUP > >> +]) > >> -- > >> 2.31.1 > >> > >> > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
