[ovs-dev] tests: Test for ovs-ofctl snoop command

Added test for snoop command to check for the initial handshake messages
when a bridge connects to a controller via 'unix' connection method.

Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com>
---
 tests/ovs-ofctl.at | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Hello. Thanks for the patch.
I'm not much familiar with that functionality thus someone
else should review the sanity of this patch, but I have few
comments about testing itself. See inline.

Best regard, Ilya Maximets.

> Added test for snoop command to check for the initial handshake messages
> when a bridge connects to a controller via 'unix' connection method.
> 
> Signed-off-by: Ashish Varma <ashishvarma.ovs at gmail.com>
> ---
>  tests/ovs-ofctl.at | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 06597d7..794277b 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -3184,3 +3184,31 @@ AT_CHECK([grep -q "ct_dpif|DBG|.*ct_flush: zone 123" ovs-vswitchd.log])
>  
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +
> +AT_SETUP([ovs-ofctl snoop-unix-connection])
> +OVS_VSWITCHD_START
> +
> +dnl setup controller for br0 before starting the controller
> +AT_CHECK([ovs-vsctl set-controller br0 unix:testcontroller])
> +
> +dnl then start listening on the '.snoop' connection
> +AT_CHECK([ovs-ofctl --detach --pidfile=ovsofctl_snoop.pid snoop br0 1> snoopbr0.txt 2>&1])
> +on_exit 'kill `cat ovsofctl_snoop.pid`'
> +on_exit 'unlink snoopbr0.txt'
> +
> +dnl finally start the controller
> +AT_CHECK([ovs-testcontroller --detach --pidfile punix:testcontroller], [0], [ignore])

Could you, please, add '-vsyslog:off' to this command?

> +on_exit 'kill `cat ovs-testcontroller.pid`'
> +OVS_WAIT_UNTIL([test -e testcontroller])
> +
> +dnl wait for 2 seconds for snoop to collect the messages from the bridge
> +sleep 2

Waiting few seconds is a bad style, IMHO. For example this could
lead to test failures in parallel testing if the thread will have
no enough time to work.

Is it possible to replace this by something like OVS_WAIT_UNTIL ?

> +
> +dnl check some of the initial openflow setup messages
> +AT_CHECK([egrep "OFPT_FEATURES_REQUEST" snoopbr0.txt 1> /dev/null 2>&1])
> +AT_CHECK([egrep "OFPT_FEATURES_REPLY" snoopbr0.txt 1> /dev/null 2>&1])
> +AT_CHECK([egrep "OFPT_SET_CONFIG" snoopbr0.txt 1> /dev/null 2>&1])
> +
> +OVS_VSWITCHD_STOP(["/connection failed (No such file or directory)/d"])
> +AT_CLEANUP
> -- 
> 2.7.4

Thanks for the review. I will try to use OVS_WAIT_UNTIL (or something
similar)

On Thu, Aug 2, 2018 at 7:33 AM, Ilya Maximets <i.maximets@samsung.com>
wrote:

> Hello. Thanks for the patch.
> I'm not much familiar with that functionality thus someone
> else should review the sanity of this patch, but I have few
> comments about testing itself. See inline.
>
> Best regard, Ilya Maximets.
>
> > Added test for snoop command to check for the initial handshake messages
> > when a bridge connects to a controller via 'unix' connection method.
> >
> > Signed-off-by: Ashish Varma <ashishvarma.ovs at gmail.com>
> > ---
> >  tests/ovs-ofctl.at | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> > index 06597d7..794277b 100644
> > --- a/tests/ovs-ofctl.at
> > +++ b/tests/ovs-ofctl.at
> > @@ -3184,3 +3184,31 @@ AT_CHECK([grep -q "ct_dpif|DBG|.*ct_flush: zone
> 123" ovs-vswitchd.log])
> >
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +
> > +AT_SETUP([ovs-ofctl snoop-unix-connection])
> > +OVS_VSWITCHD_START
> > +
> > +dnl setup controller for br0 before starting the controller
> > +AT_CHECK([ovs-vsctl set-controller br0 unix:testcontroller])
> > +
> > +dnl then start listening on the '.snoop' connection
> > +AT_CHECK([ovs-ofctl --detach --pidfile=ovsofctl_snoop.pid snoop br0 1>
> snoopbr0.txt 2>&1])
> > +on_exit 'kill `cat ovsofctl_snoop.pid`'
> > +on_exit 'unlink snoopbr0.txt'
> > +
> > +dnl finally start the controller
> > +AT_CHECK([ovs-testcontroller --detach --pidfile punix:testcontroller],
> [0], [ignore])
>
> Could you, please, add '-vsyslog:off' to this command?
>
> > +on_exit 'kill `cat ovs-testcontroller.pid`'
> > +OVS_WAIT_UNTIL([test -e testcontroller])
> > +
> > +dnl wait for 2 seconds for snoop to collect the messages from the bridge
> > +sleep 2
>
> Waiting few seconds is a bad style, IMHO. For example this could
> lead to test failures in parallel testing if the thread will have
> no enough time to work.
>
> Is it possible to replace this by something like OVS_WAIT_UNTIL ?
>
> > +
> > +dnl check some of the initial openflow setup messages
> > +AT_CHECK([egrep "OFPT_FEATURES_REQUEST" snoopbr0.txt 1> /dev/null 2>&1])
> > +AT_CHECK([egrep "OFPT_FEATURES_REPLY" snoopbr0.txt 1> /dev/null 2>&1])
> > +AT_CHECK([egrep "OFPT_SET_CONFIG" snoopbr0.txt 1> /dev/null 2>&1])
> > +
> > +OVS_VSWITCHD_STOP(["/connection failed (No such file or directory)/d"])
> > +AT_CLEANUP
> > --
> > 2.7.4
>