ofproto: Honor OFPFF_RESET_COUNTS flag in flow modify message.

While a flow modify must keep the original flow's flags, it must reset
counts if (and only if) the reset_counts flag is present in the flow
mod message.

Behavior prior to this patch is broken in a few ways:

- OpenFlow 1.0 and 1.1 mod-flows did reset the counts, if the flow had
  reset_counts flag set.  Only add-flow should reset counts.
- With OpenFlow 1.2 and later, if the old flow had the reset_counts
  flag set, the counts would be reset by mod-flows, even if the
  flow-mod message does not have the reset_counts flag set.
- With OpenFlow 1.2 and later, mod-flows with a reset_count did not
  reset the counts, if the old flow did not have the reset_counts flag
  set.

Even though the prevailing interpretation seems to be that the
reset_counts flag in the flow-mod message should be stored as part of
the flow state (and reported back in flow dumps with OpenFlow >= 1.3),
we should always just look at the reset_counts flag in the current
flow-mod and ignore the reset_counts flag stored in the flow when
processing a flow mod.

For OpenFlow 1.0 and 1.1 we already implicitly add the reset_counts
flag for add-flow messages (only) to maintain the expected behavior.

This patch adds a comprehensive test case to prevent future regressions.

Suggested-by: Tony van der Peet <tony.vanderpeet@alliedtelesis.co.nz>
Fixes: 748eb2f5b1 ("ofproto-dpif: Always forward 'used' from the old_rule.")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Ben Pfaff <blp@ovn.org>

1 files changed, 174 insertions, 0 deletions

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index ec7bd60d4..4f7d16d85 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -6460,6 +6460,180 @@ AT_CHECK([strip_xids < stdout | sed -n 's/duration=[[0-9]]*\.[[0-9]]*s/duration=
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([ofproto-dpif - flow stats reset_counts])
+OVS_VSWITCHD_START
+flow="ip,actions=NORMAL"
+
+ovs-appctl time/stop
+
+AT_CHECK([ovs-ofctl add-flow br0 $flow])
+
+warp_and_dump_NXM () {
+    AT_CHECK([ovs-appctl time/warp 1000], [0], [ignore])
+    AT_CHECK([ovs-appctl revalidator/purge], [0])
+
+    AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br0], [0], [stdout])
+    if [[ $5 -gt 0 ]]; then
+        expected=" cookie=0x0, duration=$1s, table=0, n_packets=$2, n_bytes=$3, idle_age=$4, hard_age=$5, ip actions=NORMAL"
+    else
+        expected=" cookie=0x0, duration=$1s, table=0, n_packets=$2, n_bytes=$3, idle_age=$4, ip actions=NORMAL"
+    fi
+    AT_CHECK_UNQUOTED([strip_xids < stdout | sed -n 's/duration=\([[0-9]]*\)\.*[[0-9]]*s/duration=\1s/p' | sort], [0], [dnl
+$expected
+])
+}
+
+warp_and_dump_OF () {
+    AT_CHECK([ovs-appctl time/warp 1000], [0], [ignore])
+    AT_CHECK([ovs-appctl revalidator/purge], [0])
+
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 dump-flows br0], [0], [stdout])
+    if [[ $1 -lt 13 -o "$5X" = "X" ]]; then
+        expected=" cookie=0x0, duration=$2s, table=0, n_packets=$3, n_bytes=$4, ip actions=NORMAL"
+    else
+        expected=" cookie=0x0, duration=$2s, table=0, n_packets=$3, n_bytes=$4, $5 ip actions=NORMAL"
+    fi
+    AT_CHECK_UNQUOTED([strip_xids < stdout | sed -n 's/duration=\([[0-9]]*\)\.*[[0-9]]*s/duration=\1s/p' | sort], [0], [dnl
+$expected
+])
+}
+
+send_packet () {
+    ovs-appctl netdev-dummy/receive br0 'in_port(0),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)'
+}
+
+# OpenFlow 1.0, implicit reset_counts
+send_packet
+warp_and_dump_NXM   1 1 54 1
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow10 add-flow br0 $flow])
+# add-flow resets duration and counts,
+# but idle age is inherited from the old flow
+warp_and_dump_NXM   1 0 0  2
+
+send_packet
+warp_and_dump_NXM   2 1 54 1
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow10 mod-flows br0 $flow])
+# mod-flows resets hard_age, but not counts
+# but duration and idle_age is inherited from the old flow
+warp_and_dump_NXM   3 1 54  2 1
+
+# OpenFlow 1.1, implicit reset_counts
+send_packet
+warp_and_dump_OF 11 4 2 108
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow11 add-flow br0 $flow])
+# add-flow resets duration and counts,
+# but idle age is inherited from the old flow
+warp_and_dump_NXM   1 0 0  2
+warp_and_dump_OF 11 2 0 0
+
+send_packet
+warp_and_dump_OF 11 3 1 54
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow11 mod-flows br0 $flow])
+# mod-flows resets hard_age, but not counts
+# but duration and idle_age is inherited from the old flow
+warp_and_dump_NXM   4 1 54  2 1
+warp_and_dump_OF 11 5 1 54
+
+# OpenFlow 1.2, explicit reset_counts
+send_packet
+warp_and_dump_OF 12 6 2 108
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 add-flow br0 $flow])
+# add-flow without flags resets duration, but not counts,
+# idle age is inherited from the old flow
+warp_and_dump_NXM   1 2 108  2
+warp_and_dump_OF 12 2 2 108
+
+send_packet
+warp_and_dump_OF 12 3 3 162
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 mod-flows br0 $flow])
+# mod-flows without flags does not reset duration nor counts,
+# idle age is inherited from the old flow
+warp_and_dump_NXM   4 3 162  2 1
+warp_and_dump_OF 12 5 3 162
+
+send_packet
+warp_and_dump_OF 12 6 4 216
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 add-flow br0 reset_counts,$flow])
+# add-flow with reset_counts resets both duration and counts,
+# idle age is inherited from the old flow
+warp_and_dump_NXM   1 0 0  2
+warp_and_dump_OF 12 2 0 0
+
+send_packet
+warp_and_dump_OF 12 3 1 54
+AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow12 mod-flows br0 reset_counts,$flow])
+# mod-flows with reset_counts resets counts, but not duration,
+# idle age is inherited from the old flow
+warp_and_dump_NXM   4 0 0  2 1
+warp_and_dump_OF 12 5 0 0
+
+# OpenFlow > 1.3, explicit reset_counts
+flow_mods_reset_counts () {
+    # Reset to a known state
+    AT_CHECK([ovs-ofctl add-flow br0 $flow])
+
+    send_packet
+    warp_and_dump_OF $1 1 1 54 reset_counts
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 $flow])
+    # add-flow without flags resets duration, but not counts,
+    # idle age is inherited from the old flow
+    warp_and_dump_NXM   1 1 54  2
+    warp_and_dump_OF $1 2 1 54
+
+    send_packet
+    warp_and_dump_OF $1 3 2 108
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 $flow])
+    # mod-flows without flags does not reset duration nor counts,
+    # idle age is inherited from the old flow
+    warp_and_dump_NXM   4 2 108  2 1
+    warp_and_dump_OF $1 5 2 108
+
+    send_packet
+    warp_and_dump_OF $1 6 3 162
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 reset_counts,$flow])
+    # add-flow with reset_counts resets both duration and counts,
+    # idle age is inherited from the old flow
+    warp_and_dump_NXM   1 0 0  2
+    warp_and_dump_OF $1 2 0 0 reset_counts
+
+    send_packet
+    warp_and_dump_OF $1 3 1 54 reset_counts
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 reset_counts,$flow])
+    # mod-flows with reset_counts resets counts, but not duration,
+    # idle age is inherited from the old flow
+    warp_and_dump_NXM   4 0 0  2 1
+    warp_and_dump_OF $1 5 0 0 reset_counts
+
+    # Modify flow having reset_counts flag without reset_counts
+    send_packet
+    warp_and_dump_OF $1 6 1 54 reset_counts
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 $flow])
+    warp_and_dump_NXM   7 1 54  2 1
+    warp_and_dump_OF $1 8 1 54 reset_counts
+
+    # Add flow having reset_counts flag without reset_counts
+    send_packet
+    warp_and_dump_OF $1 9 2 108 reset_counts
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 add-flow br0 $flow])
+    warp_and_dump_NXM   1 2 108  2
+    warp_and_dump_OF $1 2 2 108
+
+    # Modify flow w/o reset_counts flag with a flow_mod having reset_counts
+    send_packet
+    warp_and_dump_OF $1 3 3 162
+    AT_CHECK_UNQUOTED([ovs-ofctl -O OpenFlow$1 mod-flows br0 reset_counts,$flow])
+    warp_and_dump_NXM   4 0 0  2 1
+    warp_and_dump_OF $1 5 0 0
+}
+
+# OpenFlow versions >= 1.3 should behave the same way
+flow_mods_reset_counts 13
+flow_mods_reset_counts 14
+flow_mods_reset_counts 15
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - flow stats, set-n-threads])
 OVS_VSWITCHD_START
 AT_CHECK([ovs-ofctl add-flow br0 "ip,actions=NORMAL"])