On Fri, Jul 17, 2020 at 7:04 PM Dumitru Ceara <[email protected]> wrote:
> On 7/17/20 3:29 PM, Numan Siddique wrote: > > On Fri, Jul 17, 2020 at 6:55 PM Mark Michelson <[email protected]> > wrote: > >> > >> On 7/17/20 5:51 AM, Dumitru Ceara wrote: > >>> On 7/17/20 11:21 AM, [email protected] wrote: > >>>> From: Numan Siddique <[email protected]> > >>>> > >>>> After the commit in the Fixes tag, ovn-controller was not creating ct > zone > >>>> entries for the container ports in the integration bridge's > external_ids > >>>> column. Because of this, when a container port sends a traffic to > >>>> load balancer VIP, zone id is not used (because REG13 is not set). > >>>> But the reverse traffic doesn't go through the ct_lb action for > undnat, > >>>> but instead go to the conntrack via the ct_commit() OVN action and the > >>>> packet gets dropped. This happens if an ACL with allow-related action > >>>> which matches in the egress pipeline of the logical switch. > >>>> > >>>> This patch fixes this regression and the tests make sure the the ct > zone > >>>> entries are created for the container ports. > >>>> > >>>> Fixes: 6c8b9a132532("ovn-controller: Store the local port bindings in > the runtime data I-P state.") > >>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1857865 > >>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1858191 > >>>> Signed-off-by: Numan Siddique <[email protected]> > >>>> --- > >>>> controller/binding.c | 11 ++++ > >>>> tests/ovn.at | 30 +++++++++++ > >>>> tests/system-ovn.at | 118 > +++++++++++++++++++++++++++++++++++++++++++ > >>>> 3 files changed, 159 insertions(+) > >>>> > >>>> diff --git a/controller/binding.c b/controller/binding.c > >>>> index e630b60801..b73abb982c 100644 > >>>> --- a/controller/binding.c > >>>> +++ b/controller/binding.c > >>>> @@ -1011,6 +1011,7 @@ consider_vif_lport_(const struct > sbrec_port_binding *pb, > >>>> b_ctx_out->local_datapaths, > >>>> b_ctx_out->tracked_dp_bindings); > >>>> update_local_lport_ids(pb, b_ctx_out); > >>>> + update_local_lports(pb->logical_port, b_ctx_out); > >>>> if (lbinding->iface && qos_map && > b_ctx_in->ovs_idl_txn) { > >>>> get_qos_params(pb, qos_map); > >>>> } > >>>> @@ -1981,6 +1982,16 @@ handle_deleted_vif_lport(const struct > sbrec_port_binding *pb, > >>>> } > >>>> } > >>>> > >>>> + /* If its a container lport, then delete its entry from > local_lports > >>>> + * if present. > >>>> + * Note: If a normal lport is deleted, we don't want to remove > >>>> + * it from local_lports if there is a VIF entry. > >>>> + * consider_iface_release() takes care of removing from the > local_lports > >>>> + * when the interface change happens. */ > >>>> + if (is_lport_container(pb)) { > >>>> + remove_local_lports(pb->logical_port, b_ctx_out); > >>>> + } > >>>> + > >>>> handle_deleted_lport(pb, b_ctx_in, b_ctx_out); > >>>> return true; > >>>> } > >>>> diff --git a/tests/ovn.at b/tests/ovn.at > >>>> index ba1a534e92..e19efafbe2 100644 > >>>> --- a/tests/ovn.at > >>>> +++ b/tests/ovn.at > >>>> @@ -8785,6 +8785,36 @@ ip_to_hex() { > >>>> printf "%02x%02x%02x%02x" "$@" > >>>> } > >>>> > >>>> +# Test that ovn-controllers create ct-zone entry for container ports. > >>>> +foo1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-foo1) > >>>> +AT_CHECK([test ! -z $foo1_zoneid]) > >>>> + > >>>> +bar1_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar1) > >>>> +AT_CHECK([test ! -z $bar1_zoneid]) > >>>> + > >>>> +bar3_zoneid=$(as hv1 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar3) > >>>> +AT_CHECK([test ! -z $bar3_zoneid]) > >>>> + > >>>> +foo2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-foo2) > >>>> +AT_CHECK([test ! -z $foo2_zoneid]) > >>>> + > >>>> +bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar2) > >>>> +AT_CHECK([test ! -z $bar2_zoneid]) > >>>> + > >>>> +ovn-nbctl lsp-del bar2 > >>>> +ovn-nbctl --wait=hv sync > >>>> + > >>>> +bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar2) > >>>> +AT_CHECK([test -z $bar2_zoneid]) > >>>> + > >>>> +# Add back bar2 > >>>> +ovn-nbctl lsp-add bar bar2 vm2 1 \ > >>>> +-- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3" > >>>> +ovn-nbctl --wait=hv sync > >>>> + > >>>> +bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int > external_ids:ct-zone-bar2) > >>>> +AT_CHECK([test ! -z $bar2_zoneid]) > >>>> + > >>>> # Send ip packets between foo1 and foo2 (same switch, different HVs > and > >>>> # different VLAN tags). > >>>> src_mac="f00000010205" > >>>> diff --git a/tests/system-ovn.at b/tests/system-ovn.at > >>>> index 2999e52fde..77c92e86ec 100644 > >>>> --- a/tests/system-ovn.at > >>>> +++ b/tests/system-ovn.at > >>>> @@ -4365,3 +4365,121 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > >>>> /Service monitor not found.*/d"]) > >>>> > >>>> AT_CLEANUP > >>>> + > >>>> +AT_SETUP([ovn -- Load balancer for container ports]) > >>>> +AT_SKIP_IF([test $HAVE_NC = no]) > >>>> +AT_KEYWORDS([lb]) > >>>> + > >>>> +ovn_start > >>>> + > >>>> +OVS_TRAFFIC_VSWITCHD_START() > >>>> +ADD_BR([br-int]) > >>>> + > >>>> +# Set external-ids in br-int needed for ovn-controller > >>>> +ovs-vsctl \ > >>>> + -- set Open_vSwitch . external-ids:system-id=hv1 \ > >>>> + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > >>>> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > >>>> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > >>>> + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > >>>> + > >>>> +# Start ovn-controller > >>>> +start_daemon ovn-controller > >>>> + > >>>> +ovn-nbctl ls-add sw0 > >>>> +ovn-nbctl lsp-add sw0 sw0-p1-lbc > >>>> +ovn-nbctl lsp-set-addresses sw0-port1 "10:54:00:00:00:03 10.0.0.3" > >>>> + > >>>> +ovn-nbctl lsp-add sw0 sw0-p2-lbc > >>>> +ovn-nbctl lsp-set-addresses sw0-port2 "10:54:00:00:00:04 10.0.0.4" > >>>> + > >>>> +ovn-nbctl ls-add sw1 > >>>> +ovn-nbctl lsp-add sw1 sw1-port1 sw0-p1-lbc 10 > >>>> +ovn-nbctl lsp-set-addresses sw1-port1 "40:54:00:00:00:03 20.0.0.3" > >>>> + > >>>> +ovn-nbctl lsp-add sw1 sw1-port2 sw0-p2-lbc 20 > >>>> +ovn-nbctl lsp-set-addresses sw1-port2 "40:54:00:00:00:04 20.0.0.4" > >>>> + > >>>> + > >>>> +ovn-nbctl lr-add lr0 > >>>> +ovn-nbctl lrp-add lr0 lr0-sw1 00:00:00:00:ff:02 20.0.0.1/24 > >>>> +ovn-nbctl lsp-add sw1 sw1-lr0 > >>>> +ovn-nbctl lsp-set-type sw1-lr0 router > >>>> +ovn-nbctl lsp-set-addresses sw1-lr0 router > >>>> +ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1 > >>>> + > >>>> + > >>>> +ovn-nbctl ls-add sw2 > >>>> +ovn-nbctl lsp-add sw2 sw2-port1 > >>>> +ovn-nbctl lsp-set-addresses sw2-port1 "50:54:00:00:00:03 30.0.0.3" > >>>> + > >>>> +ovn-nbctl lrp-add lr0 lr0-sw2 00:00:00:00:ff:03 30.0.0.1/24 > >>>> +ovn-nbctl lsp-add sw2 sw2-lr0 > >>>> +ovn-nbctl lsp-set-type sw2-lr0 router > >>>> +ovn-nbctl lsp-set-addresses sw2-lr0 router > >>>> +ovn-nbctl lsp-set-options sw2-lr0 router-port=lr0-sw2 > >>>> + > >>>> + > >>>> +ovn-nbctl lb-add lb0 "30.0.0.10:80" "20.0.0.4:80" > >>>> + > >>>> +ovn-nbctl ls-lb-add sw1 lb0 > >>>> +ovn-nbctl ls-lb-add sw2 lb0 > >>>> +ovn-nbctl lr-lb-add lr0 lb0 > >>>> + > >>>> +ADD_NAMESPACES(sw0-p1-lbc) > >>>> +ADD_VETH(sw0-p1-lbc, sw0-p1-lbc, br-int, "10.0.0.3/24", > "10:54:00:00:00:03", \ > >>>> + "10.0.0.1") > >>>> + > >>>> +ADD_NAMESPACES(sw0-p2-lbc) > >>>> +ADD_VETH(sw0-p2-lbc, sw0-p2-lbc, br-int, "10.0.0.4/24", > "10:54:00:00:00:04", \ > >>>> + "10.0.0.1") > >>>> + > >>>> +# Create the interface for lport sw1-port1 > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip link add link sw0-p1-lbc name sw1p1 > type vlan id 10], [0]) > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip link set sw1p1 address > 40:54:00:00:00:03], [0]) > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip link set sw1p1 up], [0]) > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip addr add 20.0.0.3/24 dev sw1p1], > [0]) > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip route delete default via 10.0.0.1 > dev sw0-p1-lbc], [0]) > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [ip route add default via 20.0.0.1 dev > sw1p1], [0]) > >>>> + > >>>> +# Create the interface for lport sw1-port2 > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip link add link sw0-p2-lbc name sw1p2 > type vlan id 20], [0]) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip link set sw1p2 address > 40:54:00:00:00:04], [0]) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip link set sw1p2 up], [0]) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip addr add 20.0.0.4/24 dev sw1p2], > [0]) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip route delete default via 10.0.0.1 > dev sw0-p2-lbc], [0]) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [ip route add default via 20.0.0.1 dev > sw1p2], [0]) > >>>> + > >>>> +# Start nc server on sw1p2 (sw0-p2-lbc is the parent) > >>>> +NS_CHECK_EXEC([sw0-p2-lbc], [nc -l 20.0.0.4 80 -k &], [0]) > >>> > >>> This will leave nc running after the test has ended. I think we need > >>> something like: > >>> > >>> NS_CHECK_EXEC([sw0-p2-lbc], [timeout 2s nc -l 20.0.0.4 80 -k &], [0]) > >>> > >>> With this addressed, the rest looks good to me, thanks! > >>> > >>> Acked-by: Dumitru Ceara <[email protected]> > Thanks Dumitru. I applied this patch to master and branch-20.06. I used the NETNS_DAEMONIZE as suggested by Mark to address your comment. Thanks Numan >>> > >>> Regards, > >>> Dumitru > >> > >> An alternate plan would be to use the NETNS_DAEMONIZE method to start > >> the nc listener. This way, it will automatically be cleaned up when the > >> test concludes. > >> > > > > I like this one. Thanks. I'll test it out locally. > > > > Thanks > > Numan > > > > > > Nice, thanks Mark for pointing it out! I should probably change the > Hairpin test to use it too. > > Regards, > Dumitru > > > > >> An alternate alternate plan would be to use the on_exit() function to > >> add a cleanup command yourself. > >> > >> An alternate alternate alternate plan would be to use OVS_START_L7 to > >> start an HTTP server, and then use `wget -q` instead of `nc -z` . This > >> would save you the overhead of pidfile management, but would make the > >> test more heavyweight. > >> > >>> > >>>> + > >>>> +# Send the packet to backend > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0]) > >>>> + > >>>> +# Send the packet to VIP. > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0]) > >>>> + > >>>> +# Now add an ACL in sw1. > >>>> +ovn-nbctl --wait=hv acl-add sw1 to-lport 2002 "ip" allow-related > >>>> +# Send the packet to backend > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 20.0.0.4 80], [0]) > >>>> + > >>>> +# Send the packet to VIP. > >>>> +NS_CHECK_EXEC([sw0-p1-lbc], [nc -z 30.0.0.10 80], [0]) > >>>> + > >>>> +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > >>>> + > >>>> +as ovn-sb > >>>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >>>> + > >>>> +as ovn-nb > >>>> +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > >>>> + > >>>> +as northd > >>>> +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > >>>> + > >>>> +as > >>>> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > >>>> +/connection dropped.*/d"]) > >>>> + > >>>> +AT_CLEANUP > >>>> > >>> > >>> _______________________________________________ > >>> 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 > >> > > > > _______________________________________________ > 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
