On Wed, Mar 1, 2023 at 4:30 PM Simon Horman <simon.hor...@corigine.com> wrote:
> 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. > I'll update the commit message in v3 to reflect that. > > > 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. > Same here. > > > 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... > Probably not needed at all if we extend the ADD_VETH. > > > # 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. > Yeah it might be a good idea to add this. I'll update it in v3. > > > + > > 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]) > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
