On Tue, Feb 21, 2023 at 01:32:04PM +0100, Ales Musil wrote: > The ADD_INT does not work very well with userspace datapath. > To avoid any warnings that might fail the tests > use ADD_VETH instead. Also encourage this in > documentation for ADD_INT, because there should > be good reasoning behind using internal interface.
I verified that it doesn't work work well, but I I think it would be good to expand on why. > Signed-off-by: Ales Musil <amu...@redhat.com> > --- > tests/system-common-macros.at | 20 +------------------- > tests/system-ovn.at | 35 +++++++++++++++++++---------------- > 2 files changed, 20 insertions(+), 35 deletions(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index d65f359a6..2b23f89b0 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -49,6 +49,7 @@ m4_define([ADD_BR], [ovs-vsctl _ADD_BR([$1]) -- $2]) > # Add an internal port to 'ovs-br', then shift it into 'namespace' and > # configure it with 'ip_addr' (specified in CIDR notation). > # Optionally add an ipv6 address > +# The ADD_VETH should be used instead if possible. Likewise, here. I think it would be good to explain why. Or in which cases one may chose ADD_INT over ADD_VETH. > m4_define([ADD_INT], > [ AT_CHECK([ovs-vsctl add-port $3 $1 -- set int $1 type=internal]) > AT_CHECK([ip link set $1 netns $2]) > @@ -60,25 +61,6 @@ m4_define([ADD_INT], > ] > ) > > -# NS_ADD_INT([port], [namespace], [ovs-br], [ip_addr] [mac_addr] [ip6_addr] > [default_gw] [default_ipv6_gw]) > -# Create a namespace > -# Add an internal port to 'ovs-br', then shift it into 'namespace'. > -# Configure it with 'ip_addr' (specified in CIDR notation) and ip6_addr. > -# Set mac_addr > -# Add default gw for ipv4 and ipv6 > -m4_define([NS_ADD_INT], > - [ AT_CHECK([ovs-vsctl add-port $3 $1 -- set int $1 type=internal > external_ids:iface-id=$1]) > - ADD_NAMESPACES($2) > - AT_CHECK([ip link set $1 netns $2]) > - NS_CHECK_EXEC([$2], [ip link set $1 address $5]) > - NS_CHECK_EXEC([$2], [ip link set dev $1 up]) > - NS_CHECK_EXEC([$2], [ip addr add $4 dev $1]) > - NS_CHECK_EXEC([$2], [ip addr add $6 dev $1]) > - NS_CHECK_EXEC([$2], [ip route add default via $7 dev $1]) > - NS_CHECK_EXEC([$2], [ip -6 route add default via $8 dev $1]) > - ] > -) > - Perhaps I am missing something obvious, but could this macro be changed / reimplemented / replaced... > # ADD_VETH([port], [namespace], [ovs-br], [ip_addr] [mac_addr], [gateway], > # [ip_addr_flags]) > # > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 07884dfbe..01112f33e 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -8482,11 +8482,22 @@ check ovn-nbctl lsp-set-addresses ln unknown > check ovn-nbctl lr-nat-add lr1 snat 172.16.1.10 192.168.1.0/24 > check ovn-nbctl lr-nat-add lr1 snat 1711::10 2001::/64 > > -NS_ADD_INT(ls1p1, ls1p1, br-int, "192.168.1.1/24", "00:00:00:01:01:01", > "2001::1/64", "192.168.1.254", "2001::a" ) > -NS_ADD_INT(ls1p2, ls1p2, br-int, "192.168.1.2/24", "00:00:00:01:01:02", > "2001::2/64", "192.168.1.254", "2001::a" ) > +ADD_NAMESPACES(ls1p1) > +ADD_VETH(ls1p1, ls1p1, br-int, "192.168.1.1/24", "00:00:00:01:01:01", \ > + "192.168.1.254") > +NS_CHECK_EXEC([ls1p1], [ip addr add 2001::1/64 dev ls1p1], [0]) > +NS_CHECK_EXEC([ls1p1], [ip route add default via 2001::a dev ls1p1], [0]) > + > +ADD_NAMESPACES(ls1p2) > +ADD_VETH(ls1p2, ls1p2, br-int, "192.168.1.2/24", "00:00:00:01:01:02", \ > + "192.168.1.254") > +NS_CHECK_EXEC([ls1p2], [ip addr add 2001::2/64 dev ls1p2], [0]) > +NS_CHECK_EXEC([ls1p2], [ip route add default via 2001::a dev ls1p2], [0]) ... because this seems a little verbose. OTOH, maybe its better to open code something that only happens twice. > ADD_NAMESPACES(ext1) > -ADD_INT(ext1, ext1, br0, 172.16.1.1/24, 1711::1/64) > +ADD_VETH(ext1, ext1, br0, "172.16.1.1/24", "00:ee:00:01:01:01") > +NS_CHECK_EXEC([ext1], [ip addr add 1711::1/64 dev ext1], [0]) Perhaps not for this patch, but I wonder if there is a value in allowing ADD_VETH to add multiple addresses. Or at least 2, for the dual stack situation. > + > check ovn-nbctl --wait=hv sync > wait_for_ports_up > OVS_WAIT_UNTIL([test "$(ip netns exec ls1p1 ip a | grep 2001::1 | grep > tentative)" = ""]) > @@ -8548,25 +8559,17 @@ wait_igmp_flows_installed() > } > > ADD_NAMESPACES(vm1) > -ADD_INT([vm1], [vm1], [br-int], [42.42.42.1/24]) > -NS_CHECK_EXEC([vm1], [ip link set vm1 address 00:00:00:00:00:01], [0]) > -NS_CHECK_EXEC([vm1], [ip route add default via 42.42.42.5], [0]) > -check ovs-vsctl set Interface vm1 external_ids:iface-id=vm1 > +ADD_VETH(vm1, vm1, br-int, "42.42.42.1/24", "00:00:00:00:00:01", \ > + "42.42.42.5") > > ADD_NAMESPACES(vm2) > -ADD_INT([vm2], [vm2], [br-int], [42.42.42.2/24]) > -NS_CHECK_EXEC([vm2], [ip link set vm2 address 00:00:00:00:00:02], [0]) > -NS_CHECK_EXEC([vm2], [ip link set lo up], [0]) > -check ovs-vsctl set Interface vm2 external_ids:iface-id=vm2 > +ADD_VETH(vm2, vm2, br-int, "42.42.42.2/24", "00:00:00:00:00:02") > > ADD_NAMESPACES(vm3) > NETNS_DAEMONIZE([vm3], [tcpdump -n -i any -nnleX > vm3.pcap 2>/dev/null], > [tcpdump3.pid]) > > -ADD_INT([vm3], [vm3], [br-int], [42.42.42.3/24]) > -NS_CHECK_EXEC([vm3], [ip link set vm3 address 00:00:00:00:00:03], [0]) > -NS_CHECK_EXEC([vm3], [ip link set lo up], [0]) > -NS_CHECK_EXEC([vm3], [ip route add default via 42.42.42.5], [0]) > -check ovs-vsctl set Interface vm3 external_ids:iface-id=vm3 > +ADD_VETH(vm3, vm3, br-int, "42.42.42.3/24", "00:00:00:00:00:03", \ > + "42.42.42.5") > > NS_CHECK_EXEC([vm2], [sysctl -w net.ipv4.igmp_max_memberships=100], > [ignore], [ignore]) > NS_CHECK_EXEC([vm3], [sysctl -w net.ipv4.igmp_max_memberships=100], > [ignore], [ignore]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
