Message ID | CALDO+SZaFw=0SnaoudtKdCOUi0PF=Us+Xb9sTstB5GOioO3CHw@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
On 21 December 2016 at 10:28, William Tu <u9012063@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
thanks. I will submit v4 patch. On Wed, Dec 21, 2016 at 10:46 AM, Joe Stringer <joe@ovn.org> wrote: > On 21 December 2016 at 10:28, William Tu <u9012063@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
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),