Hi Frode On Fri, Nov 15, 2024 at 10:45 AM Frode Nordahl <[email protected]> wrote:
> 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). Do you mean that the regression you saw is due to the initial patch [0] or the revert [1] ? > Just wanted to highlight that so extra > care is taken in any rerolls of the original series. > Do you have any test you could share highlighting the regression you saw ? > > I see that a "Compare I+P flows with recompute ones" [0] series is up, > is that a reroll of this fix? > The main goal of this series is to compare flows from I+P with flows after recompute. It highlighted some issues from which most got fixed by earlier patches. A few issues remained to be fixed. The series contains [2], which can be seen as v2 of [0]. 0: http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ 1: http://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ 2: https://patchwork.ozlabs.org/project/ovn/patch/[email protected] Thanks Xavier > > 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
