[ovs-dev] [PATCH v3 05/16] ofp-actions: Add clone action.

[William Tu]

thanks. I will submit v4 patch.

On Wed, Dec 21, 2016 at 10:46 AM, Joe Stringer <joe at ovn.org> wrote:
> On 21 December 2016 at 10:28, William Tu <u9012063 at gmail.com> wrote:
>> sorry, I forgot to add priority. Below lower the priority of the NORMAL action.
>>
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index d70c5c3..321a901 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -340,6 +340,7 @@ AT_CLEANUP
>>  AT_SETUP([datapath - clone action])
>>  OVS_TRAFFIC_VSWITCHD_START()
>>
>> +AT_CHECK([ovs-ofctl add-flow br0 "priority=1 actions=normal"])
>>  ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
>>
>>  ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>> @@ -352,9 +353,9 @@ AT_CHECK([ovs-vsctl -- set interface ovs-p2
>> ofport_request=3])
>>
>>  dnl verify that the clone(...) won't affect the original packet, so
>> ping still works OK
>>  dnl without 'output' in 'clone()'
>> -AT_CHECK([ovs-ofctl add-flow br0
>> "in_port=1,ip,actions=clone(mod_dl_dst(50:54:00:00:00:0a),set_field:192.168.3.3->ip_dst),
>> output:2"])
>> +AT_CHECK([ovs-ofctl add-flow br0 "priority=99
>> in_port=1,ip,actions=clone(mod_dl_dst(50:54:00:00:00:0a),set_field:192.168.3.3->ip_dst),
>> output:2"])
>>  dnl with 'output' in 'clone()'
>> -AT_CHECK([ovs-ofctl add-flow br0
>> "in_port=2,ip,actions=clone(mod_dl_dst(50:54:00:00:00:0b),set_field:192.168.4.4->ip_dst,
>> output:3), output:1"])
>> +AT_CHECK([ovs-ofctl add-flow br0 "priority=99
>> in_port=2,ip,actions=clone(mod_dl_dst(50:54:00:00:00:0b),set_field:192.168.4.4->ip_dst,
>> output:3), output:1"])
>>
>>  NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 |
>> FORMAT_PING], [0], [dnl
>>  3 packets transmitted, 3 received, 0% packet loss, time 0ms
>
> I think it's sufficient to set the priority of the "normal" flow to 1,
> or just restrict it to ARP.
>
> A couple of other pieces of feedback on the test:
> * It's better if there is one ovs-ofctl call for all flows (look for
> flows.txt examples in the test file)
> * Similarly for the ofport_request setup, there doesn't need to be
> three separate ovs-vsctl calls, the commands can be separated by "--".
> (although since you're reliably adding the ports in order, I don't
> think it shouldn't be necessary to request the particular port numbers
> at all; OVS will allocate the numbers in the order you add them)
> * at_ns2 isn't used at all. Please come up with a way to ensure that
> the packets going to that port are modified as you would expect.
> * You might consider adding a controller action in the clone action,
> then using "ovs-ofctl monitor" to observe the traffic
>
> Please submit a patch the usual way addressing this feedback.
>
> Thanks,
> Joe