| Message ID | 20241031171005.215778-1-xsimonar@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] Revert "controller: Properly handle localnet flows in I+P.". | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Thanks for the fix Xavier. Acked-by: Mark Michelson <mmichels@redhat.com> On 10/31/24 13:10, Xavier Simonart 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 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/physical.c | 9 ++-- > tests/ovn.at | 98 +++++++++++++++++++------------------------ > 2 files changed, 46 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..dfb08fd1e 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 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,45 @@ 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 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 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 > + > +OVN_CLEANUP([hv1]) > + > +AT_CLEANUP > +])
Hi Xavier, On 11/1/24 15:03, Mark Michelson wrote: > Thanks for the fix Xavier. > +1 > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 10/31/24 13:10, Xavier Simonart 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 >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> --- >> controller/physical.c | 9 ++-- >> tests/ovn.at | 98 +++++++++++++++++++------------------------ >> 2 files changed, 46 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..dfb08fd1e 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 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,45 @@ 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 check ovn-nbctl -- lsp-add pub pub-ln -- lsp-set-type pub-ln Nit: check check >> localnet -- lsp-set-addresses pub-ln unknown -- lsp-set-options pub-ln >> network_name=phys >> +check check ovn-nbctl -- lsp-add ls1 ls1-ln -- lsp-set-type ls1-ln Nit: check check >> localnet -- lsp-set-addresses ls1-ln unknown -- lsp-set-options ls1-ln >> network_name=phys >> + There's a potential for a race here. We don't actually wait for ovn-controller to get the SB updates that correspond to this NB change. In general the test seems odd (out of context) because it's not obvious that it's testing the ovn-controller flow processing part. Should we at least check that some of the relevant OpenFlow rules are installed here? Thanks, Dumitru >> +OVN_CLEANUP([hv1]) >> + >> +AT_CLEANUP >> +]) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Hi Dumitru Thanks for the review and the comments. On Mon, Nov 4, 2024 at 12:34 PM Dumitru Ceara <dceara@redhat.com> wrote: > Hi Xavier, > > On 11/1/24 15:03, Mark Michelson wrote: > > Thanks for the fix Xavier. > > > > +1 > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > > On 10/31/24 13:10, Xavier Simonart 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 > >> > >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > >> --- > >> controller/physical.c | 9 ++-- > >> tests/ovn.at | 98 > +++++++++++++++++++------------------------ > >> 2 files changed, 46 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..dfb08fd1e 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 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,45 @@ 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 check ovn-nbctl -- lsp-add pub pub-ln -- lsp-set-type pub-ln > > Nit: check check > > >> localnet -- lsp-set-addresses pub-ln unknown -- lsp-set-options pub-ln > >> network_name=phys > >> +check check ovn-nbctl -- lsp-add ls1 ls1-ln -- lsp-set-type ls1-ln > > Nit: check check > > >> localnet -- lsp-set-addresses ls1-ln unknown -- lsp-set-options ls1-ln > >> network_name=phys > >> + > > There's a potential for a race here. We don't actually wait for > ovn-controller to get the SB updates that correspond to this NB change. > > In general the test seems odd (out of context) because it's not obvious > that it's testing the ovn-controller flow processing part. Should we at > least check that some of the relevant OpenFlow rules are installed here? > > I agree that the test should be better documented. However, I do not think that there is a race: OVN_CLEANUP is doing (2x) ovn-nbctl --wait=hv sync and then some recompute. So, the real check of the test is within OVN_CLEANUP: before this revert patch, the test would fail the ovn-nbctl --wait=hv sync part of OVN_CLEANUP. I'll anyhow send a v2 adding some flow check (and remove the double check), so the test would be less obscure. > Thanks, > Dumitru > > Thanks Xavier > >> +OVN_CLEANUP([hv1]) > >> + > >> +AT_CLEANUP > >> +]) > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >
On 11/4/24 16:29, Xavier Simonart wrote: > Hi Dumitru > > Thanks for the review and the comments. > > On Mon, Nov 4, 2024 at 12:34 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> Hi Xavier, >> >> On 11/1/24 15:03, Mark Michelson wrote: >>> Thanks for the fix Xavier. >>> >> >> +1 >> >>> Acked-by: Mark Michelson <mmichels@redhat.com> >>> >>> On 10/31/24 13:10, Xavier Simonart 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 >>>> >>>> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >>>> --- >>>> controller/physical.c | 9 ++-- >>>> tests/ovn.at | 98 >> +++++++++++++++++++------------------------ >>>> 2 files changed, 46 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..dfb08fd1e 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 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,45 @@ 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 check ovn-nbctl -- lsp-add pub pub-ln -- lsp-set-type pub-ln >> >> Nit: check check >> >>>> localnet -- lsp-set-addresses pub-ln unknown -- lsp-set-options pub-ln >>>> network_name=phys >>>> +check check ovn-nbctl -- lsp-add ls1 ls1-ln -- lsp-set-type ls1-ln >> >> Nit: check check >> >>>> localnet -- lsp-set-addresses ls1-ln unknown -- lsp-set-options ls1-ln >>>> network_name=phys >>>> + >> >> There's a potential for a race here. We don't actually wait for >> ovn-controller to get the SB updates that correspond to this NB change. >> >> In general the test seems odd (out of context) because it's not obvious >> that it's testing the ovn-controller flow processing part. Should we at >> least check that some of the relevant OpenFlow rules are installed here? >> >> I agree that the test should be better documented. > However, I do not think that there is a race: OVN_CLEANUP is doing (2x) > ovn-nbctl --wait=hv sync and then some recompute. Ah, good point. > So, the real check of the test is within OVN_CLEANUP: before this revert > patch, the test would fail the ovn-nbctl --wait=hv sync part of OVN_CLEANUP. > I'll anyhow send a v2 adding some flow check (and remove the double check), > so the test would be less obscure. > OK, sounds good, thanks! Regards, Dumitru
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..dfb08fd1e 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 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,45 @@ 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 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 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 + +OVN_CLEANUP([hv1]) + +AT_CLEANUP +])
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 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/physical.c | 9 ++-- tests/ovn.at | 98 +++++++++++++++++++------------------------ 2 files changed, 46 insertions(+), 61 deletions(-)