| Message ID | 20230223063526.2363478-3-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | virtual port faster failover. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Feb 22, 2023 at 10:35:25PM -0800, Han Zhou wrote: Please add a patch description here. > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > tests/atlocal.in | 3 + > tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 149 insertions(+) ... > +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 10.0.0.88]) I ran into a problem with this when exercising the tests on Ubuntu 22.10. When the arping package is installed then -s expects a MAC address whereas -S expects an ip/hostname. This causes the tests to fail. By changing -s to -S here, and for the other invocation of arping, below, I was able to run the tests successfully for both check-kernel and check-system-userspace :) When, instead, arping is supplied by the iputils-arping package, then the new tests work unmodified. I am not sure what, if anything, we wish to do about such compatibility issues. But, FWIIW, I believe noticed a similar problem involving nc not so long ago, although I do not recall specifically in which environment or which alternate packages. ...
On Wed, Mar 1, 2023 at 1:50 AM Simon Horman <simon.horman@corigine.com> wrote: > > On Wed, Feb 22, 2023 at 10:35:25PM -0800, Han Zhou wrote: > > Please add a patch description here. Thank Simon for reviewing. For this commit, I think the commit title tells everything I wanted to describe, so I omitted it here rather than repeating the title. > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > tests/atlocal.in | 3 + > > tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 149 insertions(+) > > ... > > > +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 10.0.0.88]) > > I ran into a problem with this when exercising the tests on Ubuntu 22.10. > > When the arping package is installed then -s expects a MAC address > whereas -S expects an ip/hostname. This causes the tests to fail. > > By changing -s to -S here, and for the other invocation of arping, below, I > was able to run the tests successfully for both check-kernel and > check-system-userspace :) > > When, instead, arping is supplied by the iputils-arping package, > then the new tests work unmodified. > > I am not sure what, if anything, we wish to do about such compatibility > issues. But, FWIIW, I believe noticed a similar problem involving nc not so > long ago, although I do not recall specifically in which environment or > which alternate packages. > Thanks a lot for testing this. I didn't test this in Ubuntu, but it's strange that even from this man page of Ubuntu 22.10, -s is still the right one: https://manpages.ubuntu.com/manpages/kinetic/man8/arping.8.html Regardless, I also see this: If this option is absent, source address is: ... • In Unsolicited ARP mode (with options *-U *or *-A*) set to* destination*. So, hopefully with -U already in the command, we can omit the -s (or -S). Do you mind testing the same by removing the -s option and see if it works in your environment? Thanks, Han > ...
On Wed, Mar 01, 2023 at 02:27:29PM -0800, Han Zhou wrote: > On Wed, Mar 1, 2023 at 1:50 AM Simon Horman <simon.horman@corigine.com> > wrote: > > > > On Wed, Feb 22, 2023 at 10:35:25PM -0800, Han Zhou wrote: > > > > Please add a patch description here. > > Thank Simon for reviewing. For this commit, I think the commit title tells > everything I wanted to describe, so I omitted it here rather than repeating > the title. IMHO something should go here, even if it's just repeating the subject. > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > --- > > > tests/atlocal.in | 3 + > > > tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 149 insertions(+) > > > > ... > > > > > +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 > 10.0.0.88]) > > > > I ran into a problem with this when exercising the tests on Ubuntu 22.10. > > > > When the arping package is installed then -s expects a MAC address > > whereas -S expects an ip/hostname. This causes the tests to fail. > > > > By changing -s to -S here, and for the other invocation of arping, below, > I > > was able to run the tests successfully for both check-kernel and > > check-system-userspace :) > > > > When, instead, arping is supplied by the iputils-arping package, > > then the new tests work unmodified. > > > > I am not sure what, if anything, we wish to do about such compatibility > > issues. But, FWIIW, I believe noticed a similar problem involving nc not > so > > long ago, although I do not recall specifically in which environment or > > which alternate packages. > > > Thanks a lot for testing this. I didn't test this in Ubuntu, but it's > strange that even from this man page of Ubuntu 22.10, -s is still the right > one: > https://manpages.ubuntu.com/manpages/kinetic/man8/arping.8.html > > Regardless, I also see this: > > If this option is absent, source address is: > > ... • In Unsolicited ARP mode (with options *-U *or *-A*) set to* destination*. > > So, hopefully with -U already in the command, we can omit the -s (or -S). > Do you mind testing the same by removing the -s option and see if it works > in your environment? Yes, can do. Hopefully later today.
On Thu, Mar 02, 2023 at 08:51:18AM +0100, Simon Horman wrote: > On Wed, Mar 01, 2023 at 02:27:29PM -0800, Han Zhou wrote: > > On Wed, Mar 1, 2023 at 1:50 AM Simon Horman <simon.horman@corigine.com> > > wrote: > > > > > > On Wed, Feb 22, 2023 at 10:35:25PM -0800, Han Zhou wrote: > > > > > > Please add a patch description here. > > > > Thank Simon for reviewing. For this commit, I think the commit title tells > > everything I wanted to describe, so I omitted it here rather than repeating > > the title. > > IMHO something should go here, even if it's just repeating the subject. > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > > --- > > > > tests/atlocal.in | 3 + > > > > tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 149 insertions(+) > > > > > > ... > > > > > > > +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 > > 10.0.0.88]) > > > > > > I ran into a problem with this when exercising the tests on Ubuntu 22.10. > > > > > > When the arping package is installed then -s expects a MAC address > > > whereas -S expects an ip/hostname. This causes the tests to fail. > > > > > > By changing -s to -S here, and for the other invocation of arping, below, > > I > > > was able to run the tests successfully for both check-kernel and > > > check-system-userspace :) > > > > > > When, instead, arping is supplied by the iputils-arping package, > > > then the new tests work unmodified. > > > > > > I am not sure what, if anything, we wish to do about such compatibility > > > issues. But, FWIIW, I believe noticed a similar problem involving nc not > > so > > > long ago, although I do not recall specifically in which environment or > > > which alternate packages. > > > > > Thanks a lot for testing this. I didn't test this in Ubuntu, but it's > > strange that even from this man page of Ubuntu 22.10, -s is still the right > > one: > > https://manpages.ubuntu.com/manpages/kinetic/man8/arping.8.html I think that is the manpage for the 'iputils-arping' version of the tool. And the manpage for the 'arping' version can be found here: * https://github.com/ThomasHabets/arping/blob/arping-2.x/doc/arping.8 > > > > Regardless, I also see this: > > > > If this option is absent, source address is: > > > > ... • In Unsolicited ARP mode (with options *-U *or *-A*) set to* destination*. > > > > So, hopefully with -U already in the command, we can omit the -s (or -S). > > Do you mind testing the same by removing the -s option and see if it works > > in your environment? > > Yes, can do. > Hopefully later today. I confirmed that the test is successful for both the 'iputils-arping' and 'arping' versions of the arping util if the -s option is removed. I tested both check-kernel and check-system-userspace. # TESTSUITEFLAGS="-k 'virtual port with floating IP'" make check-kernel check-system-userspace
On Thu, Mar 2, 2023 at 1:12 AM Simon Horman <simon.horman@corigine.com> wrote: > > On Thu, Mar 02, 2023 at 08:51:18AM +0100, Simon Horman wrote: > > On Wed, Mar 01, 2023 at 02:27:29PM -0800, Han Zhou wrote: > > > On Wed, Mar 1, 2023 at 1:50 AM Simon Horman <simon.horman@corigine.com > > > > wrote: > > > > > > > > On Wed, Feb 22, 2023 at 10:35:25PM -0800, Han Zhou wrote: > > > > > > > > Please add a patch description here. > > > > > > Thank Simon for reviewing. For this commit, I think the commit title tells > > > everything I wanted to describe, so I omitted it here rather than repeating > > > the title. > > > > IMHO something should go here, even if it's just repeating the subject. Just curious, why would it help to repeat the subject? If the subject is not clear enough I can definitely add more details in the commit message, but I hope we don't make this a rule without good reason. In fact we have many examples in the commit log with just a subject line, in both OVN and OVS repo. > > > > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > > > > --- > > > > > tests/atlocal.in | 3 + > > > > > tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 149 insertions(+) > > > > > > > > ... > > > > > > > > > +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 > > > 10.0.0.88]) > > > > > > > > I ran into a problem with this when exercising the tests on Ubuntu 22.10. > > > > > > > > When the arping package is installed then -s expects a MAC address > > > > whereas -S expects an ip/hostname. This causes the tests to fail. > > > > > > > > By changing -s to -S here, and for the other invocation of arping, below, > > > I > > > > was able to run the tests successfully for both check-kernel and > > > > check-system-userspace :) > > > > > > > > When, instead, arping is supplied by the iputils-arping package, > > > > then the new tests work unmodified. > > > > > > > > I am not sure what, if anything, we wish to do about such compatibility > > > > issues. But, FWIIW, I believe noticed a similar problem involving nc not > > > so > > > > long ago, although I do not recall specifically in which environment or > > > > which alternate packages. > > > > > > > Thanks a lot for testing this. I didn't test this in Ubuntu, but it's > > > strange that even from this man page of Ubuntu 22.10, -s is still the right > > > one: > > > https://manpages.ubuntu.com/manpages/kinetic/man8/arping.8.html > > I think that is the manpage for the 'iputils-arping' version of the tool. > And the manpage for the 'arping' version can be found here: > > * https://github.com/ThomasHabets/arping/blob/arping-2.x/doc/arping.8 > > > > > > > Regardless, I also see this: > > > > > > If this option is absent, source address is: > > > > > > ... • In Unsolicited ARP mode (with options *-U *or *-A*) set to* destination*. > > > > > > So, hopefully with -U already in the command, we can omit the -s (or -S). > > > Do you mind testing the same by removing the -s option and see if it works > > > in your environment? > > > > Yes, can do. > > Hopefully later today. > > I confirmed that the test is successful for both the 'iputils-arping' and > 'arping' versions of the arping util if the -s option is removed. > > I tested both check-kernel and check-system-userspace. > > # TESTSUITEFLAGS="-k 'virtual port with floating IP'" make check-kernel check-system-userspace > Thanks a lot for testing it! I will remove the -s in v2. Regards, Han
diff --git a/tests/atlocal.in b/tests/atlocal.in index 0b9a312761c9..5526adac5241 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -187,6 +187,9 @@ find_command dhcpd # Set HAVE_BFDD_BEACON find_command bfdd-beacon +# Set HAVE_ARPING +find_command arping + # Turn off proxies. unset http_proxy unset https_proxy diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 563858e70384..cccb8ec4aa95 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10660,3 +10660,149 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d /connection dropped.*/d"]) AT_CLEANUP ]) + +#################################################################### +# ls1p1 (virtual parent of VIP) +# \ +# ls1 -- lr1 (floating-ip) -- public1 (localnet) -- ext1 +# / +# ls1p2 (virtual parent of VIP) +#################################################################### +OVN_FOR_EACH_NORTHD([ +AT_SETUP([virtual port with floating IP]) +AT_SKIP_IF([test "$HAVE_ARPING" = no]) + +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) +ADD_BR([br-ex], [set Bridge br-ex fail-mode=standalone]) + +check ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=provider:br-ex + +# 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 + +# Add routers +check ovn-nbctl lr-add lr1 +check ovn-nbctl set logical_router lr1 options:always_learn_from_arp_request=false + +# Add switches +check ovn-nbctl ls-add public1 +check ovn-nbctl ls-add ls1 + +# Add ls1 ports +check ovn-nbctl lsp-add ls1 ls1p1 \ + -- lsp-set-addresses ls1p1 "00:00:00:00:01:11 10.0.0.11" + +check ovn-nbctl lsp-add ls1 ls1p2 \ + -- lsp-set-addresses ls1p2 "00:00:00:00:01:12 10.0.0.12" + +check ovn-nbctl lsp-add ls1 ls1-to-lr1 \ + -- lsp-set-type ls1-to-lr1 router \ + -- lsp-set-options ls1-to-lr1 router-port=lr1-to-ls1 \ + -- lsp-set-addresses ls1-to-lr1 router + +# Add ls1 virtual port +check ovn-nbctl lsp-add ls1 vip \ + -- lsp-set-addresses vip "00:00:00:00:01:88 10.0.0.88" \ + -- lsp-set-type vip virtual \ + -- set logical_switch_port vip options:virtual-ip=10.0.0.88 \ + -- set logical_switch_port vip options:virtual-parents=ls1p1,ls1p2 + +# Add lr1 ports +check ovn-nbctl lrp-add lr1 lr1-to-ls1 "00:00:00:0f:01:01" 10.0.0.1/24 \ + -- lrp-add lr1 lr1-to-public1 "00:00:00:0f:02:01" 172.0.0.1/24 \ + -- lrp-set-gateway-chassis lr1-to-public1 hv1 10 + +# Add floating-ip +check ovn-nbctl lr-nat-add lr1 dnat_and_snat 172.0.0.88 10.0.0.88 vip 10:54:00:00:00:88 + +# Add public1 ports +check ovn-nbctl lsp-add public1 public1-to-lr1 \ + -- lsp-set-type public1-to-lr1 router \ + -- lsp-set-options public1-to-lr1 router-port=lr1-to-public1 \ + -- lsp-set-addresses public1-to-lr1 router \ + -- lsp-add public1 ln1 \ + -- lsp-set-type ln1 localnet \ + -- lsp-set-options ln1 network_name=provider \ + -- lsp-set-addresses ln1 unknown + +check ovn-nbctl --wait=hv sync + +ADD_NAMESPACES(ns_ls1p1) +ADD_VETH(ls1p1, ns_ls1p1, br-int, "10.0.0.11/24", "00:00:00:00:01:11", "10.0.0.1") + +ADD_NAMESPACES(ns_ls1p2) +ADD_VETH(ls1p2, ns_ls1p2, br-int, "10.0.0.12/24", "00:00:00:00:01:12", "10.0.0.1") + +ADD_NAMESPACES(ns_ext1) +ADD_VETH(ln1, ns_ext1, br-ex, "172.0.0.99/24", "0a:0a:b6:fc:03:01", "172.0.0.1") + +# Claim vip at ls1p1: configure the virtual IP and send GARP. +NS_CHECK_EXEC([ns_ls1p1], [ip addr del 10.0.0.11/24 dev ls1p1; + ip addr add 10.0.0.88/24 dev ls1p1; + ip route add default via 10.0.0.1]) +NS_EXEC([ns_ls1p1], [arping -U -c 1 -w 2 -I ls1p1 -s 10.0.0.88 10.0.0.88]) +wait_for_ports_up vip +check ovn-nbctl --wait=hv sync + +# ping virtual IP from ext1 +NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.88 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +# ping floating virtual IP from ext1 +NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 172.0.0.88 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +# Move virtual IP to ls1p2 +NS_CHECK_EXEC([ns_ls1p1], [ip addr del 10.0.0.88/24 dev ls1p1]) +NS_CHECK_EXEC([ns_ls1p2], [ip addr del 10.0.0.12/24 dev ls1p2; + ip addr add 10.0.0.88/24 dev ls1p2; + ip route add default via 10.0.0.1]) +NS_EXEC([ns_ls1p2], [arping -U -c 1 -w 2 -I ls1p2 -s 10.0.0.88 10.0.0.88]) + +wait_column "ls1p2" Port_Binding virtual_parent logical_port=vip +check ovn-nbctl --wait=hv sync + +# ping virtual IP from ext1 +NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.88 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +# ping floating virtual IP from ext1 +NS_CHECK_EXEC([ns_ext1], [ping -q -c 3 -i 0.3 -w 2 172.0.0.88 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +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([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) +AT_CLEANUP +])
Signed-off-by: Han Zhou <hzhou@ovn.org> --- tests/atlocal.in | 3 + tests/system-ovn.at | 146 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+)