[ovs-dev] tests: Test reset_counts in flow_mod message

commit 23c753793204df86aa6eedd00ec62911a1ef9559
Author: Tony van der Peet <tony.vanderpeet@alliedtelesis.co.nz>
Date:   Tue Nov 29 13:16:17 2016 +1300

    tests: Test reset_counts in flow_mod message

    Setting the reset_counts flag in a flow_mod message should cause the
    flow counters to be reset. Test this by:

    - creating a flow
    - sending a single packet
    - modifying the flow with reset_counts flag set
    - dumping the flow and expecting n_packets=0, n_bytes=1

    This is assertion 140.80 of the ONF conformance test specification for
    v1.3.4.

Hi Tony,

On Tue, Nov 29, 2016 at 12:47:57AM +0000, Tony van der Peet wrote:
> commit 23c753793204df86aa6eedd00ec62911a1ef9559
> Author: Tony van der Peet <tony.vanderpeet@alliedtelesis.co.nz>
> Date:   Tue Nov 29 13:16:17 2016 +1300
> 
>     tests: Test reset_counts in flow_mod message
> 
>     Setting the reset_counts flag in a flow_mod message should cause the
>     flow counters to be reset. Test this by:
> 
>     - creating a flow
>     - sending a single packet
>     - modifying the flow with reset_counts flag set
>     - dumping the flow and expecting n_packets=0, n_bytes=1
> 
>     This is assertion 140.80 of the ONF conformance test specification for
>     v1.3.4.

This email seems to be in the format emitted by "git log -p" rather
than that of "git format-patch".

In particular:
* The "commit", "Date" and "Author" lines should not be present
# The subject "tests:..." should not be duplicated in the changelog
* The changelog should not be indended by four spaces

Also, please provided a Signed-off-by: tag which matches
the From address used to submit the patch.

Please consider using "[PATCH v2]" as the subject prefix when posting a
revised version of this patch.

> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index d360b7a..1723e77 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1648,6 +1648,19 @@ OFPST_FLOW reply (OF1.2):
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> 
> +AT_SETUP([ofproto - mod flow with reset_counts flag - OF1.3])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=1,actions=2])
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no)'
> +AT_CHECK([ovs-ofctl -O openflow13 mod-flows br0 reset_counts,in_port=1,actions=3])
> +AT_CHECK([ovs-ofctl -O openflow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
> + in_port=1 actions=output:3
> +OFPST_FLOW reply (OF1.3):
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto - del flows with cookies])
>  OVS_VSWITCHD_START
>  AT_CHECK([ovs-ofctl add-flow br0 cookie=0x1,in_port=1,actions=1])
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Thanks for the patch!  One comment below.

On Tue, Nov 29, 2016 at 12:47:57AM +0000, Tony van der Peet wrote:
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index d360b7a..1723e77 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -1648,6 +1648,19 @@ OFPST_FLOW reply (OF1.2):
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> 
> +AT_SETUP([ofproto - mod flow with reset_counts flag - OF1.3])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3
> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 in_port=1,actions=2])
> +ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:07,dst=50:54:00:00:00:05),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no)'
> +AT_CHECK([ovs-ofctl -O openflow13 mod-flows br0 reset_counts,in_port=1,actions=3])

There's a race condition above because it's possible that processing the
packet could be delayed past the ovs-ofctl call, so that the reset
happens before the packet is processed.  This kind of race triggers
rarely, but occasionally we do see similar problems when tests are run
on heavily loaded systems.  It's hard to fix this completely, but I'd
recommend putting a 1-second sleep after the netdev-dummy/receive.

> +AT_CHECK([ovs-ofctl -O openflow13 dump-flows br0 | ofctl_strip | sort], [0], [dnl
> + in_port=1 actions=output:3
> +OFPST_FLOW reply (OF1.3):
> +])
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto - del flows with cookies])
>  OVS_VSWITCHD_START
>  AT_CHECK([ovs-ofctl add-flow br0 cookie=0x1,in_port=1,actions=1])
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks for feedback, it's been a useful exercise in learning the ropes of submitting patches.

Given the fine work by Jarno however, I don't think this patch is required any more and am happy enough to let it be abandoned.

Thanks
Tony