I'm going to backport it to branch-1.9. I had to make the following change for the unit test:
-=-=-=-=-=-=-=-=-=- diff --git a/tests/learn.at b/tests/learn.at index 305d2f8..868a4b5 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -120,9 +120,9 @@ dnl it gets re-learned again the next time a packet appears, dnl sometimes the expiration can cause temporary flooding etc.) AT_SETUP([learning action - learn refreshes hard_age]) OVS_VSWITCHD_START( - [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \ - add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2 -- \ - add-port br0 p3 -- set Interface p3 type=dummy ofport_request=3]) + [add-port br0 p1 -- set Interface p1 type=dummy -- \ + add-port br0 p2 -- set Interface p2 type=dummy -- \ + add-port br0 p3 -- set Interface p3 type=dummy]) ovs-appctl time/stop @@ -138,7 +138,7 @@ flow="in_port(3),eth(src=50:54:00:00:00:07,dst=50:54:00:00:0 AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout]) actual=`tail -1 stdout | sed 's/Datapath actions: //'` -expected="1,2,100" +expected="0,1,2" AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout]) mv stdout expout AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout]) -=-=-=-=-=-=-=-=-=- Does that look reasonable to you? If so, I'll push it. --Justin On Jan 25, 2013, at 5:24 PM, Ben Pfaff <[email protected]> wrote: > Thanks, I applied this to master. I know we talked about applying it > to earlier branches too, but I didn't do that yet. > > On Fri, Jan 25, 2013 at 04:51:51PM -0800, Justin Pettit wrote: >> Looks good. >> >> --Justin >> >> >> On Jan 25, 2013, at 3:00 PM, Ben Pfaff <[email protected]> wrote: >> >>> In Open vSwitch, a "modify" or "modify_strict" flow_mod is supposed to >>> refresh the flow's last-modified time even if nothing else changes, because >>> this interpretation makes the "learn" action more useful. As commit >>> 308881afb (ofproto: Reinterpret meaning of OpenFlow hard timeouts with >>> OFPFC_MODIFY.) notes: >>> >>> I finally found a good use for hard timeouts in OpenFlow, but they >>> require a slight reinterpretation of the meaning of hard timeouts. >>> Until now, a hard timeout meant that a flow would be removed the >>> specified number of seconds after a flow was created. Intervening >>> modifications with OFPFC_MODIFY(_STRICT) had no effect on the hard >>> timeout; the flow would still be deleted the specified number of >>> seconds after its original creation. >>> >>> This commit changes the effect of OFPFC_MODIFY(_STRICT). Now, >>> modifying a flow resets its hard timeout counter. A flow will time out >>> the specified number of seconds after creation or after the last time >>> it is modified, whichever comes later. >>> >>> However, commit 080437614b (ofproto: Represent flow cookie changes as >>> operations too.) broke this behavior because it incorrectly optimized out >>> "modify" operations that didn't change the flow's actions or flow cookie. >>> This commit fixes the problem, and adds a test to prevent future >>> regression. >>> >>> Thanks to Amar Padmanabhan <[email protected]> for helping to track this >>> down. >>> >>> Bug #14538. >>> Reported-by: Hiroshi Tanaka <[email protected]> >>> CC: Amar Padmanabhan <[email protected]> >>> Signed-off-by: Ben Pfaff <[email protected]> >>> --- >>> ofproto/ofproto.c | 18 +++++++++--- >>> tests/learn.at | 75 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 88 insertions(+), 5 deletions(-) >>> >>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >>> index bc27079..249f2d5 100644 >>> --- a/ofproto/ofproto.c >>> +++ b/ofproto/ofproto.c >>> @@ -3261,10 +3261,6 @@ modify_flows__(struct ofproto *ofproto, struct >>> ofconn *ofconn, >>> new_cookie = (fm->new_cookie != htonll(UINT64_MAX) >>> ? fm->new_cookie >>> : rule->flow_cookie); >>> - if (!actions_changed && new_cookie == rule->flow_cookie) { >>> - /* No change at all. */ >>> - continue; >>> - } >>> >>> op = ofoperation_create(group, rule, OFOPERATION_MODIFY, 0); >>> rule->flow_cookie = new_cookie; >>> @@ -4247,7 +4243,19 @@ ofopgroup_complete(struct ofopgroup *group) >>> LIST_FOR_EACH_SAFE (op, next_op, group_node, &group->ops) { >>> struct rule *rule = op->rule; >>> >>> - if (!op->error && !ofproto_rule_is_hidden(rule)) { >>> + /* We generally want to report the change to active OpenFlow flow >>> + monitors (e.g. NXST_FLOW_MONITOR). There are three exceptions: >>> + >>> + - The operation failed. >>> + >>> + - The affected rule is not visible to controllers. >>> + >>> + - The operation's only effect was to update rule->modified. >>> */ >>> + if (!(op->error >>> + || ofproto_rule_is_hidden(rule) >>> + || (op->type == OFOPERATION_MODIFY >>> + && op->ofpacts >>> + && rule->flow_cookie == op->flow_cookie))) { >>> /* Check that we can just cast from ofoperation_type to >>> * nx_flow_update_event. */ >>> BUILD_ASSERT_DECL((enum nx_flow_update_event) OFOPERATION_ADD >>> diff --git a/tests/learn.at b/tests/learn.at >>> index 47c1d32..800dc14 100644 >>> --- a/tests/learn.at >>> +++ b/tests/learn.at >>> @@ -122,6 +122,81 @@ NXST_FLOW reply: >>> OVS_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> +dnl This test checks that repeated uses of a "learn" action cause the >>> +dnl modified time of the learned flow to advance. Otherwise, the >>> +dnl learned flow will expire after its hard timeout even though it's >>> +dnl supposed to be refreshed. (The expiration can be hard to see since >>> +dnl it gets re-learned again the next time a packet appears, but >>> +dnl sometimes the expiration can cause temporary flooding etc.) >>> +AT_SETUP([learning action - learn refreshes hard_age]) >>> +OVS_VSWITCHD_START( >>> + [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1 -- \ >>> + add-port br0 p2 -- set Interface p2 type=dummy ofport_request=2 -- \ >>> + add-port br0 p3 -- set Interface p3 type=dummy ofport_request=3]) >>> + >>> +ovs-appctl time/stop >>> + >>> +# Set up flow table for MAC learning. >>> +AT_DATA([flows.txt], [[ >>> +table=0 actions=learn(table=1, hard_timeout=10, >>> NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], output:NXM_OF_IN_PORT[]), resubmit(,1) >>> +table=1 priority=0 actions=flood >>> +]]) >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) >>> + >>> +# Trace an ICMP packet arriving on port 3, to create a MAC learning entry. >>> +flow="in_port(3),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=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)" >>> +AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], [stdout]) >>> +actual=`tail -1 stdout | sed 's/Datapath actions: //'` >>> + >>> +expected="1,2,100" >>> +AT_CHECK([ovs-dpctl normalize-actions "$flow" "$expected"], [0], [stdout]) >>> +mv stdout expout >>> +AT_CHECK([ovs-dpctl normalize-actions "$flow" "$actual"], [0], [expout]) >>> + >>> +# Check that the MAC learning entry appeared. >>> +AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], [dnl >>> + table=1, hard_timeout=10, dl_dst=50:54:00:00:00:07 actions=output:3 >>> + table=1, priority=0 actions=FLOOD >>> +NXST_FLOW reply: >>> +]) >>> + >>> +# For 25 seconds, make sure that the MAC learning entry doesn't >>> +# disappear as long as we refresh it every second. >>> +for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 >>> 25; do >>> + ovs-appctl time/warp 1000 >>> + AT_CHECK([ovs-appctl ofproto/trace br0 "$flow" -generate], [0], >>> [stdout]) >>> + >>> + # Check that the entry is there. >>> + AT_CHECK([ovs-ofctl dump-flows br0 table=1], [0], [stdout]) >>> + AT_CHECK([ofctl_strip < stdout | sort], [0], [dnl >>> + table=1, hard_timeout=10, dl_dst=50:54:00:00:00:07 actions=output:3 >>> + table=1, priority=0 actions=FLOOD >>> +NXST_FLOW reply: >>> +]) >>> + >>> + if test $i != 1; then >>> + # Check that hard_age has appeared. We need to do this separately >>> + # from the above check because ofctl_strip removes it. dump-flows >>> + # only prints hard_age when it is different from the flow's >>> duration >>> + # (that is, the number of seconds from the time it was created), >>> + # so we only check for it after we've refreshed the flow once. >>> + AT_CHECK([grep dl_dst=50:54:00:00:00:07 stdout | grep -c hard_age], >>> + [0], [1 >>> +]) >>> + fi >>> +done >>> + >>> +# Make sure that 15 seconds without refreshing makes the flow time out. >>> +ovs-appctl time/warp 5000 >>> +ovs-appctl time/warp 5000 >>> +ovs-appctl time/warp 5000 >>> + AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip | sort], [0], >>> [dnl >>> + table=1, priority=0 actions=FLOOD >>> +NXST_FLOW reply: >>> +]) >>> +OVS_VSWITCHD_STOP >>> +AT_CLEANUP >>> + >>> AT_SETUP([learning action - TCPv4 port learning]) >>> OVS_VSWITCHD_START( >>> [add-port br0 p1 -- set Interface p1 type=dummy -- \ >>> -- >>> 1.7.2.5 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> http://openvswitch.org/mailman/listinfo/dev >> _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
