Hi Stefano,
Thanks for the review!
Stefano Brivio <sbrivio@xxxxxxxxxx> writes:
> On Thu, 13 Jun 2024 14:13:32 -0400
> Aaron Conole <aconole@xxxxxxxxxx> wrote:
>
>> The current pmtu test infrastucture requires an installed copy of the
>> ovs-vswitchd userspace. This means that any automated or constrained
>> environments may not have the requisite tools to run the tests. However,
>> the pmtu tests don't require any special classifier processing. Indeed
>> they are only using the vswitchd in the most basic mode - as a NORMAL
>> switch.
>>
>> However, the ovs-dpctl kernel utility can now program all the needed basic
>> flows to allow traffic to traverse the tunnels and provide support for at
>> least testing some basic pmtu scenarios.
>
> I didn't know about that tool, that looks like a nice improvement. A
> few comments below (mostly nits):
It didn't at the time, so no worries :)
>> More complicated flow pipelines
>> can be added to the internal ovs test infrastructure, but that is work for
>> the future. For now, enable the most common cases - wide mega flows with
>> no other prerequisites.
>>
>> Signed-off-by: Aaron Conole <aconole@xxxxxxxxxx>
>> ---
>> tools/testing/selftests/net/pmtu.sh | 87 ++++++++++++++++++++++-------
>> 1 file changed, 67 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
>> index cfc84958025a..7f4f35d88dcc 100755
>> --- a/tools/testing/selftests/net/pmtu.sh
>> +++ b/tools/testing/selftests/net/pmtu.sh
>> @@ -846,22 +846,73 @@ setup_ovs_vxlan_or_geneve() {
>> type="${1}"
>> a_addr="${2}"
>> b_addr="${3}"
>> + dport="6081"
>>
>> if [ "${type}" = "vxlan" ]; then
>> + dport="4789"
>> opts="${opts} ttl 64 dstport 4789"
>> opts_b="local ${b_addr}"
>> fi
>>
>> - run_cmd ovs-vsctl add-port ovs_br0 ${type}_a -- \
>> - set interface ${type}_a type=${type} \
>> - options:remote_ip=${b_addr} options:key=1 options:csum=true || return 1
>> -
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-if ovs_br0 ${type}_a -t ${type}
>
> In some restricted environments, it might actually be more convenient
> to carry around ovs-vsctl than Python with (Python) libraries.
>
> Nowadays I typically (albeit seldom) run kselftests in throw-away VM
> images made by mbuto (https://mbuto.sh/, see demo on the right), and
> while it copies python3 and dynamic libraries from the host, adding
> Python libraries such as pyroute2 gets quite complicated.
>
> So I'm wondering, if it's not too messy: could we have two functions
> starting from approximately here (say, setup_ovs_dpctl() and
> setup_ovs_vsctl()), try with ovs-dpctl first, and, if that fails,
> fall back to ovs-vsctl?
It didn't seem to be too bad - so I went ahead and made that change. It
tested well, so I'll resubmit it with that.
>> run_cmd ${ns_b} ip link add ${type}_b type ${type} id 1
>> ${opts_b} remote ${a_addr} ${opts} || return 1
>>
>> run_cmd ${ns_b} ip addr add ${tunnel4_b_addr}/${tunnel4_mask} dev ${type}_b
>> run_cmd ${ns_b} ip addr add ${tunnel6_b_addr}/${tunnel6_mask} dev ${type}_b
>>
>> + run_cmd ip link set ${type}_a up
>> run_cmd ${ns_b} ip link set ${type}_b up
>> +
>> + ports=$(python3 ./openvswitch/ovs-dpctl.py show)
>> + br0_port=$(echo "$ports" | grep -E "\sovs_br0" | sed -e 's@port @@' | cut -d: -f1 | xargs)
>> + type_a_port=$(echo "$ports" | grep ${type}_a | sed -e 's@port @@' | cut -d: -f1 | xargs)
>> + veth_a_port=$(echo "$ports" | grep veth_A | sed -e 's@port @@' | cut -d: -f1 | xargs)
>> +
>> + v4_a_tun="${prefix4}.${a_r1}.1"
>> + v4_b_tun="${prefix4}.${b_r1}.1"
>> +
>> + v6_a_tun="${prefix6}:${a_r1}::1"
>> + v6_b_tun="${prefix6}:${b_r1}::1"
>> +
>> + if [ "${v4_a_tun}" == "${a_addr}" ]; then
>
> I see now that 05d92cb0e919 ("selftests/net: change shebang to bash to
> support "source"") turned this into a Bash script (for no real reason,
> lib.sh could have simply been sourced with '.' instead).
>
> Other than that, this happily runs with dash and possibly others, and:
>
> $ checkbashisms -f pmtu.sh
> possible bashism in pmtu.sh line 201 (should be '.', not 'source'):
> source lib.sh
> possible bashism in pmtu.sh line 202 (should be '.', not 'source'):
> source net_helper.sh
>
> Would it be possible to change this to POSIX shell:
>
> if [ "${v4_a_tun}" = "${a_addr}" ]; then
>
> even just for consistency with the rest of the file?
done.
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x0800),ipv4()" \
>> + "set(tunnel(tun_id=1,dst=${v4_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x86dd),ipv6()" \
>> + "set(tunnel(tun_id=1,dst=${v4_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,src=${v4_b_tun},dst=${v4_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x0800),ipv4()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,src=${v4_b_tun},dst=${v4_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x86dd),ipv6()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,src=${v4_b_tun},dst=${v4_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x0806),arp()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x0806),arp(sip=${veth4_c_addr},tip=${tunnel4_b_addr})" \
>> + "set(tunnel(tun_id=1,dst=${v4_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + else
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x0800),ipv4()" \
>> + "set(tunnel(tun_id=1,ipv6_dst=${v6_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x86dd),ipv6()" \
>> + "set(tunnel(tun_id=1,ipv6_dst=${v6_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,ipv6_src=${v6_b_tun},ipv6_dst=${v6_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x0800),ipv4()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,ipv6_src=${v6_b_tun},ipv6_dst=${v6_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x86dd),ipv6()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),tunnel(tun_id=1,ipv6_src=${v6_b_tun},ipv6_dst=${v6_a_tun}),in_port(${type_a_port}),eth(),eth_type(0x0806),arp()" \
>> + "${veth_a_port}"
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-flow ovs_br0 \
>> + "recirc_id(0),in_port(${veth_a_port}),eth(),eth_type(0x0806),arp(sip=${veth4_c_addr},tip=${tunnel4_b_addr})" \
>> + "set(tunnel(tun_id=1,ipv6_dst=${v6_b_tun},ttl=64,tp_dst=${dport},flags(df|csum))),${type_a_port}"
>> + fi
>> }
>>
>> setup_ovs_geneve4() {
>> @@ -881,7 +932,7 @@ setup_ovs_vxlan6() {
>> }
>>
>> setup_ovs_bridge() {
>> - run_cmd ovs-vsctl add-br ovs_br0 || return $ksft_skip
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-dp ovs_br0 || return $ksft_skip
>> run_cmd ip link set ovs_br0 up
>>
>> run_cmd ${ns_c} ip link add veth_C-A type veth peer name veth_A-C
>> @@ -891,7 +942,7 @@ setup_ovs_bridge() {
>> run_cmd ${ns_c} ip link set veth_C-A up
>> run_cmd ${ns_c} ip addr add ${veth4_c_addr}/${veth4_mask} dev veth_C-A
>> run_cmd ${ns_c} ip addr add ${veth6_c_addr}/${veth6_mask} dev veth_C-A
>> - run_cmd ovs-vsctl add-port ovs_br0 veth_A-C
>> + run_cmd python3 ./openvswitch/ovs-dpctl.py add-if ovs_br0 veth_A-C
>>
>> # Move veth_A-R1 to init
>> run_cmd ${ns_a} ip link set veth_A-R1 netns 1
>> @@ -942,8 +993,10 @@ cleanup() {
>>
>> ip link del veth_A-C 2>/dev/null
>> ip link del veth_A-R1 2>/dev/null
>> - ovs-vsctl --if-exists del-port vxlan_a 2>/dev/null
>> - ovs-vsctl --if-exists del-br ovs_br0 2>/dev/null
>> + # squelch the output of the del-if commands since it can be wordy
>> + python3 ./openvswitch/ovs-dpctl.py del-if ovs_br0 -d true vxlan_a >/dev/null 2>&1
>> + python3 ./openvswitch/ovs-dpctl.py del-if ovs_br0 -d true geneve_a >/dev/null 2>&1
>> + python3 ./openvswitch/ovs-dpctl.py del-dp ovs_br0 >/dev/null 2>&1
>
> The idea behind those tabs before 2>/dev/null was to keep them aligned
> and make those redirections a bit easier on the eyes.
>
> If you add more, you could keep those aligned as well -- or just decide
> that lines are too long and drop the tabs altogether.
Yeah, looks like I missed the /dev/null tabs but those were supposed to
be lined up. Anyway, I rewrote this part so that it looks better (I
think).
>> rm -f "$tmpoutfile"
>> }
>>
>> @@ -1407,16 +1460,10 @@ test_pmtu_ipvX_over_ovs_vxlanY_or_geneveY_exception() {
>> exp_mtu=$((${ll_mtu} - 40 - 8 - 8 - 14))
>> fi
>>
>> - if [ "${type}" = "vxlan" ]; then
>> - tun_a="vxlan_sys_4789"
>> - elif [ "${type}" = "geneve" ]; then
>> - tun_a="genev_sys_6081"
>> - fi
>> -
>> - trace "" "${tun_a}" "${ns_b}" ${type}_b \
>> - "" veth_A-R1 "${ns_r1}" veth_R1-A \
>> - "${ns_b}" veth_B-R1 "${ns_r1}" veth_R1-B \
>> - "" ovs_br0 "" veth-A-C \
>> + trace "" "${type}_a" "${ns_b}" ${type}_b \
>> + "" veth_A-R1 "${ns_r1}" veth_R1-A \
>> + "${ns_b}" veth_B-R1 "${ns_r1}" veth_R1-B \
>> + "" ovs_br0 "" veth-A_C \
>> "${ns_c}" veth_C-A
>>
>> if [ ${family} -eq 4 ]; then
>> @@ -1436,8 +1483,8 @@ test_pmtu_ipvX_over_ovs_vxlanY_or_geneveY_exception() {
>> mtu "${ns_b}" veth_B-R1 ${ll_mtu}
>> mtu "${ns_r1}" veth_R1-B ${ll_mtu}
>>
>> - mtu "" ${tun_a} $((${ll_mtu} + 1000))
>> - mtu "${ns_b}" ${type}_b $((${ll_mtu} + 1000))
>> + mtu "" ${type}_a $((${ll_mtu} + 1000))
>> + mtu "${ns_b}" ${type}_b $((${ll_mtu} + 1000))
>>
>> run_cmd ${ns_c} ${ping} -q -M want -i 0.1 -c 20 -s $((${ll_mtu} + 500)) ${dst} || return 1
>>
