[ovs-dev] [PATCH ovn] ovn-controller: Fix the missing ct zone entries for container ports.

Mark Michelson mmichels at redhat.com
Fri Jul 17 13:24:46 UTC 2020


On 7/17/20 5:51 AM, Dumitru Ceara wrote:
> On 7/17/20 11:21 AM, numans at ovn.org wrote:
>> From: Numan Siddique <numans at ovn.org>
>>
>> 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 <numans at ovn.org>
>> ---
>>   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 <dceara at redhat.com>
> 
> 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.

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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list