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

[Joe Stringer]

On 21 December 2016 at 10:28, William Tu  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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

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

Regards,
William

On Wed, Dec 21, 2016 at 10:21 AM, Joe Stringer  wrote:
> On 21 December 2016 at 07:45, William Tu  wrote:
>> Hi Joe,
>>
>> I think we're missing the normal action for handling the non-IP
>> traffic. Can you try this patch on top of the clone?
>>
>> --- 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 "actions=normal"])
>>  ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
>>
>>  ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
>
> If you force all traffic to be handled with this flow, then the test
> will pass but it will not be testing anything.
>
> Looking at the test, I don't really follow how it is supposed to be
> guaranteeing that the expected actions are executed on both copies of
> the packet. Could you explain it?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Joe Stringer]

On 21 December 2016 at 07:45, William Tu  wrote:
> Hi Joe,
>
> I think we're missing the normal action for handling the non-IP
> traffic. Can you try this patch on top of the clone?
>
> --- 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 "actions=normal"])
>  ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)
>
>  ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")

If you force all traffic to be handled with this flow, then the test
will pass but it will not be testing anything.

Looking at the test, I don't really follow how it is supposed to be
guaranteeing that the expected actions are executed on both copies of
the packet. Could you explain it?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

Hi Joe,

I think we're missing the normal action for handling the non-IP
traffic. Can you try this patch on top of the clone?

--- 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 "actions=normal"])
 ADD_NAMESPACES(at_ns0, at_ns1, at_ns2)

 ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")

Thanks
William

On Tue, Dec 20, 2016 at 4:39 PM, William Tu  wrote:
> Hi Joe,
> Thanks I will take a look.
> William
>
> On Tue, Dec 20, 2016 at 10:25 AM, Joe Stringer  wrote:
>> On 18 December 2016 at 00:18, Ben Pfaff  wrote:
>>> From: William Tu 
>>>
>>> This patch adds OpenFlow clone action with syntax as below:
>>> "clone([action][,action...])".  The clone() action makes a copy of the
>>> current packet and executes the list of actions against the packet,
>>> without affecting the packet after the "clone(...)" action.  In other
>>> word, the packet before the clone() and after the clone() is the same,
>>> no matter what actions executed inside the clone().
>>>
>>> Use case 1:
>>> Set different fields and output to different ports without unset
>>> actions=
>>>   clone(mod_dl_src:, output:1), clone(mod_dl_dst:, output:2), 
>>> output:3
>>> Since each clone() has independent packet, output:1 has only dl_src 
>>> modified,
>>> output:2 has only dl_dst modified, output:3 has original packet.
>>>
>>> Similar to case1
>>> actions=
>>>   push_vlan(...), output:2, pop_vlan, push_vlan(...), output:3
>>> can be changed to
>>> actions=
>>>   clone(push_vlan(...), output:2),clone(push_vlan(...), output:3)
>>> without having to add pop_vlan.
>>>
>>> case 2: resubmit to another table without worrying packet being modified
>>>   actions=clone(resubmit(1,2)), ...
>>>
>>> Signed-off-by: William Tu 
>>> [b...@ovn.org revised this to omit the "sample" action]
>>> Signed-off-by: Ben Pfaff 
>>
>> It looks like the newly added system-traffic tests are failing on all
>> platforms with this patch applied.. William, will you take a look?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[William Tu]

Hi Joe,
Thanks I will take a look.
William

On Tue, Dec 20, 2016 at 10:25 AM, Joe Stringer  wrote:
> On 18 December 2016 at 00:18, Ben Pfaff  wrote:
>> From: William Tu 
>>
>> This patch adds OpenFlow clone action with syntax as below:
>> "clone([action][,action...])".  The clone() action makes a copy of the
>> current packet and executes the list of actions against the packet,
>> without affecting the packet after the "clone(...)" action.  In other
>> word, the packet before the clone() and after the clone() is the same,
>> no matter what actions executed inside the clone().
>>
>> Use case 1:
>> Set different fields and output to different ports without unset
>> actions=
>>   clone(mod_dl_src:, output:1), clone(mod_dl_dst:, output:2), 
>> output:3
>> Since each clone() has independent packet, output:1 has only dl_src modified,
>> output:2 has only dl_dst modified, output:3 has original packet.
>>
>> Similar to case1
>> actions=
>>   push_vlan(...), output:2, pop_vlan, push_vlan(...), output:3
>> can be changed to
>> actions=
>>   clone(push_vlan(...), output:2),clone(push_vlan(...), output:3)
>> without having to add pop_vlan.
>>
>> case 2: resubmit to another table without worrying packet being modified
>>   actions=clone(resubmit(1,2)), ...
>>
>> Signed-off-by: William Tu 
>> [b...@ovn.org revised this to omit the "sample" action]
>> Signed-off-by: Ben Pfaff 
>
> It looks like the newly added system-traffic tests are failing on all
> platforms with this patch applied.. William, will you take a look?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev