[ovs-dev,v2] test: add dedicated test for garp-max-timeout

Introduce a dedicated test for garp-max-timeout knob

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 tests/ovn.at | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

Hi Lorenzo,

I have some comments below

On 12/22/23 10:47, Lorenzo Bianconi wrote:
> Introduce a dedicated test for garp-max-timeout knob
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>   tests/ovn.at | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 918f97a9e..deeb2a201 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -37464,3 +37464,72 @@ wait_for_ports_up
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([gratuitous arp max timeout])
> +AT_KEYWORDS([garp-timeout])
> +TAG_UNSTABLE
> +AT_SKIP_IF([test $HAVE_TCPDUMP = no])

Since this test uses fmt_pkt(), you'll also need to skip the test if 
$HAVE_SCAPY = no .

> +ovn_start
> +
> +check ovn-nbctl ls-add ls0
> +check ovn-nbctl lr-add lr0
> +check ovn-nbctl lrp-add lr0 lr0-ls0 f0:00:00:00:00:01 192.168.0.1/24
> +check ovn-nbctl lsp-add ls0 ls0-lr0 -- set Logical_Switch_Port ls0-lr0 \
> +    type=router options:router-port=lr0-ls0 addresses='"f0:00:00:00:00:01"'
> +
> +check ovn-nbctl lsp-add ls0 ln_port
> +check ovn-nbctl lsp-set-addresses ln_port unknown
> +check ovn-nbctl lsp-set-type ln_port localnet
> +check ovn-nbctl --wait=hv lsp-set-options ln_port network_name=physnet1
> +
> +# Prepare packets
> +echo $(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='f0:00:00:00:00:01')/ \
> +                ARP(op=2, hwsrc='f0:00:00:00:00:01', hwdst='00:00:00:00:00:00', psrc='192.168.0.1', pdst='192.168.0.1')") > arp_expected

the arp_expected packet is never referenced after this point. This can 
either be removed (and you can therefore ignore my comment earlier about 
needing to check for HAVE_SCAPY), or you need to check the pcaps for 
accuracy using arp_expected.

> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +check ovs-vsctl \
> +    -- add-br br-phys \
> +    -- add-br br-eth0
> +
> +ovn_attach n1 br-phys 192.168.0.10
> +
> +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap])
> +
> +# set garp max timeout to 2s
> +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=2])
> +
> +# Wait until the patch ports are created in hv1 to connect br-int to br-eth0
> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv1"])
> +
> +# sleep for 20s to get a garp every ~ 2s
> +sleep 22

I understand why the sleeps are here, but maybe you could sleep for less 
time and check for fewer packets to arrive? This way you still are 
testing the garp-max-timeout but the test won't take as long to run.

And if you really do need a total of ~34 seconds of sleeping, then I 
think this test should have AT_KEYWORDS[slowtest].

> +
> +n_arp=$(tcpdump -ner hv1/snoopvif-tx.pcap arp | wc -l)
> +AT_CHECK([test $n_arp -ge 10 -a $n_arp -lt 15])
> +
> +# Temporarily remove lr0 chassis
> +# Wait for hv confirmation to make sure chassis is removed before we reset pcap
> +# Otherwise a garp might be sent after pcap have been reset but before chassis is removed
> +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis])
> +
> +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> +# set garp max timeout to 1s
> +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=1])
> +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1)

hv1_uuid is never used. You can probably remove this line.

> +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> +
> +# sleep for 12s to get a garp every ~ 1s
> +sleep 12

Same comment about the sleep here: you can probably sleep for less time 
and check for fewer packets.

> +
> +n_arp=$(tcpdump -ner hv1/snoopvif-tx.pcap arp | wc -l)
> +AT_CHECK([test $n_arp -ge 10 -a $n_arp -lt 15])
> +
> +OVN_CLEANUP([hv1])
> +
> +AT_CLEANUP
> +])

> Hi Lorenzo,

Hi Mark,


thx for the review

> 
> I have some comments below
> 
> On 12/22/23 10:47, Lorenzo Bianconi wrote:
> > Introduce a dedicated test for garp-max-timeout knob
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> >   tests/ovn.at | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 69 insertions(+)
> > 
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 918f97a9e..deeb2a201 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -37464,3 +37464,72 @@ wait_for_ports_up
> >   OVN_CLEANUP([hv1])
> >   AT_CLEANUP
> >   ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([gratuitous arp max timeout])
> > +AT_KEYWORDS([garp-timeout])
> > +TAG_UNSTABLE
> > +AT_SKIP_IF([test $HAVE_TCPDUMP = no])
> 
> Since this test uses fmt_pkt(), you'll also need to skip the test if
> $HAVE_SCAPY = no .

ack, I will add it.

> 
> > +ovn_start
> > +
> > +check ovn-nbctl ls-add ls0
> > +check ovn-nbctl lr-add lr0
> > +check ovn-nbctl lrp-add lr0 lr0-ls0 f0:00:00:00:00:01 192.168.0.1/24
> > +check ovn-nbctl lsp-add ls0 ls0-lr0 -- set Logical_Switch_Port ls0-lr0 \
> > +    type=router options:router-port=lr0-ls0 addresses='"f0:00:00:00:00:01"'
> > +
> > +check ovn-nbctl lsp-add ls0 ln_port
> > +check ovn-nbctl lsp-set-addresses ln_port unknown
> > +check ovn-nbctl lsp-set-type ln_port localnet
> > +check ovn-nbctl --wait=hv lsp-set-options ln_port network_name=physnet1
> > +
> > +# Prepare packets
> > +echo $(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='f0:00:00:00:00:01')/ \
> > +                ARP(op=2, hwsrc='f0:00:00:00:00:01', hwdst='00:00:00:00:00:00', psrc='192.168.0.1', pdst='192.168.0.1')") > arp_expected
> 
> the arp_expected packet is never referenced after this point. This can
> either be removed (and you can therefore ignore my comment earlier about
> needing to check for HAVE_SCAPY), or you need to check the pcaps for
> accuracy using arp_expected.

ack, I will get rid of it.

> 
> > +
> > +net_add n1
> > +sim_add hv1
> > +as hv1
> > +check ovs-vsctl \
> > +    -- add-br br-phys \
> > +    -- add-br br-eth0
> > +
> > +ovn_attach n1 br-phys 192.168.0.10
> > +
> > +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-mappings=physnet1:br-eth0])
> > +AT_CHECK([ovs-vsctl add-port br-eth0 snoopvif -- set Interface snoopvif options:tx_pcap=hv1/snoopvif-tx.pcap options:rxq_pcap=hv1/snoopvif-rx.pcap])
> > +
> > +# set garp max timeout to 2s
> > +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=2])
> > +
> > +# Wait until the patch ports are created in hv1 to connect br-int to br-eth0
> > +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> > +OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv1"])
> > +
> > +# sleep for 20s to get a garp every ~ 2s
> > +sleep 22
> 
> I understand why the sleeps are here, but maybe you could sleep for less
> time and check for fewer packets to arrive? This way you still are testing
> the garp-max-timeout but the test won't take as long to run.
> 
> And if you really do need a total of ~34 seconds of sleeping, then I think
> this test should have AT_KEYWORDS[slowtest].

ok, I will reduce the timeout and count less packets, but I will add the
keyword as well since it is still not fast ;)

> 
> > +
> > +n_arp=$(tcpdump -ner hv1/snoopvif-tx.pcap arp | wc -l)
> > +AT_CHECK([test $n_arp -ge 10 -a $n_arp -lt 15])
> > +
> > +# Temporarily remove lr0 chassis
> > +# Wait for hv confirmation to make sure chassis is removed before we reset pcap
> > +# Otherwise a garp might be sent after pcap have been reset but before chassis is removed
> > +AT_CHECK([ovn-nbctl --wait=hv remove logical_router lr0 options chassis])
> > +
> > +as hv1 reset_pcap_file snoopvif hv1/snoopvif
> > +# set garp max timeout to 1s
> > +AT_CHECK([as hv1 ovs-vsctl set Open_vSwitch . external-ids:garp-max-timeout-sec=1])
> > +hv1_uuid=$(ovn-sbctl --bare --columns _uuid list chassis hv1)
> 
> hv1_uuid is never used. You can probably remove this line.

I will get rid of it

> 
> > +AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
> > +
> > +# sleep for 12s to get a garp every ~ 1s
> > +sleep 12
> 
> Same comment about the sleep here: you can probably sleep for less time and
> check for fewer packets.

ditto.

Regards,
Lorenzo

> 
> > +
> > +n_arp=$(tcpdump -ner hv1/snoopvif-tx.pcap arp | wc -l)
> > +AT_CHECK([test $n_arp -ge 10 -a $n_arp -lt 15])
> > +
> > +OVN_CLEANUP([hv1])
> > +
> > +AT_CLEANUP
> > +])
>