Operating System: All
Platform: All

I think this bug (OPNFLWPLUG-229) is not a duplicate of OPNFLWJAVA-31.

The "Product" of OPNFLWJAVA-31 is "openflowjava".
Therefore, they focused on OpenFlow Java, and discussed if OpenflowJava should take care of the problem or not.
And Michal concluded that some of the components above openflowjava should take care of it.
(I agree with his conclusion.)

On the other hand, the "Product" of this bug report (OPNFLWPLUG-229) is "openflowplugin".
As I mentioned in the "Description" of this bug report, here, I'm focusing on OpenFlow plugin.

Can I revert the status of this bug report back to CONFIRMED?

Hi Hideyuki,
as far as I understood this issue we have 2 ways in md-sal models for creating action in order to change src/dst port of an outgoing packet.

One is of-1.0 way: direct action on packetOut
and the other is of-1.3: general set-field action on packetOut containing port stuff.

And you are trying to use the of-1.0 simple way while talking to of-1.3 device. I guess we need decision here. In md-sal there should not exist 2 ways to the same goal and app should not be forced to distill protocol version of particular device and choose the right way depending on that version.

Having 2 sources for the same action is definitely a bad idea. I guess we should change md-sal models so that only one way of packetOut actions is supported where possible (presuming that most of of-1.0 features are supported in higher versions). But this would break API towards app now.

Now - do you think that there is possibility for VTN to change packetOut actions adding code? We can definitely write hotfix for helium so that if only one of mentioned actions is provided then it would be propagated. But this is not the way to a good maintainable code.

(In reply to michal rehak from comment #4)
> Hi Hideyuki,
(snip)
>
> Now - do you think that there is possibility for VTN to change packetOut
> actions adding code? We can definitely write hotfix for helium so that if
> only one of mentioned actions is provided then it would be propagated. But
> this is not the way to a good maintainable code.

Hi Michal,

That makes sense to me.

Though VTN Project plans to use SET_TP_SRC/SET_TP_DST actions (or SET_FIELD action) for FLOW_MOD messages, but not for PACKET_OUT messages.

Anyway, our answer is Yes, we plan to change code to handle this problem.
Therefore, if OpenFlow Plugin project writes the hotfix, we would be grateful.

However, VTN Manager is a AD-SAL application, and it does not use the OF plugin directly.
VTN Manger indirectly uses the OF plugin via sal-compatibility bundle.
So I think after OpenFlow Plugin project writes the hotfix, sal-compatibility also needs to be fixed.

Status in the Aug 15 IRC daily meeting:

ACTION: OPNFLWPLUG-229: Timotej Kubas will write a hotfix in ActionConverter in order to ship the change port action as set-field encapsulated stuff if of-1.3 and later we will clean md-sal models and fix that in sal-compatibility - deadline Aug 22 (Friday) - but trying to finish by the Tuedsay before. (abhijitkumbhare, 14:17:26)

https://git.opendaylight.org/gerrit/#/c/10067/

Merged.

Thank you for submitting the patch for OPNFLWPLUG-229.

However, we've found out that it is failed to install flow entries which change UDP port and ICMP type/code for OF 1.3 switches.

When we try to install such flow entries, the OF plugin incorrectly creates flow entries which change TCP port even though the flow entries match UDP or ICMP packets, and consequently OF switches reject FLOW_MOD messages for such flow entries due to MATCH_INCONSISTENT.

We've reviewed the following patch.
https://git.opendaylight.org/gerrit/#/c/10067/

We've found out that this patch always converts SET_TP_SRC/SET_TP_DST actions to SET_FIELD(TCP_SRC/TCP_DST) for OF 1.3 switches, though it should convert them to SET_FIELD(UDP_SRC/UDP_DSP) or SET_FIELD(ICMP_TYPE/ICMPV4_CODE) depending on match condition.

Hideyuki,
could you please provide example inputs containing setTpDst/Src with combination of udp and icpm. Shouldn't there also be ipv6 involved?

Thank you.

(In reply to michal rehak from comment #10)
> Hideyuki,
> could you please provide example inputs containing setTpDst/Src with
> combination of udp and icpm. Shouldn't there also be ipv6 involved?
>
> Thank you.

Hi Micahl,

Test Case #1: TCP (protocol = 6)

curl --user "admin":"admin" -H "Content-type: application/json" -X PUT \
http://localhost:8080/controller/nb/v2/flowprogrammer/default/node/OF/00:00:00:00:00:00:00:01/staticFlow/flow1 \
-d '{ "installInHw":"true", "name":"flow1", "node":

,"ingressPort":"1", "protocol":"6", "priority":"500","actions":["SET_TP_DST=1", "SET_TP_SRC=5"], "etherType":"2048" }'

==> That's OK.

Test Case #2: UDP (protocol = 17)

curl --user "admin":"admin" -H "Content-type: application/json" -X PUT \
http://localhost:8080/controller/nb/v2/flowprogrammer/default/node/OF/00:00:00:00:00:00:00:01/staticFlow/flow2 \
-d '{ "installInHw":"true", "name":"flow2", "node":

,"ingressPort":"1", "protocol":"17", "priority":"500","actions":["SET_TP_DST=1", "SET_TP_SRC=5"], "etherType":"2048" }'

==> That's NG. OpenFlow switches return a ERROR message (CODE: OFPBAC_MATCH_INCONSISTENT)

Test CONTROLLER-1: ICMP (protocol = 1)

curl --user "admin":"admin" -H "Content-type: application/json" -X PUT \
http://localhost:8080/controller/nb/v2/flowprogrammer/default/node/OF/00:00:00:00:00:00:00:01/staticFlow/flow3 \
-d '{ "installInHw":"true", "name":"flow3", "node":

,"ingressPort":"1", "protocol":"1", "priority":"500","actions":["SET_TP_DST=1", "SET_TP_SRC=5"], "etherType":"2048" }'

==> That's NG. OpenFlow switches return a ERROR message (CODE: OFPBAC_MATCH_INCONSISTENT)

We made hotfix for this and now we can handle UDP and TCP protocols in right way. But we are still dealing with ICMP protocol, where after submitting the ICMP flow we get this error on controller:

2014-09-21 19:40:14.828 CEST [md-sal-binding-notification-313] ERROR o.o.o.t.NodeErrorListenerLoggingImpl - Error notification ----[Type=BadAction, Code=15, Xid =51]

Message caught by wireshark looks proper.
We are going to investigate this problem further.

We still get the error notification "BadAction" but it occurs on virtual device, not in controller.
Patch: https://git.opendaylight.org/gerrit/#/c/11371/
Please retest.

(In reply to Timotej Kubas from comment #13)
> We still get the error notification "BadAction" but it occurs on virtual
> device, not in controller.
> Patch: https://git.opendaylight.org/gerrit/#/c/11371/
> Please retest.

I've tested the patch, and confirmed that the patch fixes the bug partially.

With this patch, I've successfully installed a flow entry to change UDP port.
But it failed to install a flow entry to change ICMP type/code for OF 1.3 switches.

I tested icmpv4 against cpqd virtual switch and it accepted the flow.
This points to device internal data validation issue/feature which causes rejection of flow with icpmv4.type and icmpv4.code.

(In reply to Hideyuki Tai from comment #14)
> (In reply to Timotej Kubas from comment #13)
> > We still get the error notification "BadAction" but it occurs on virtual
> > device, not in controller.
> > Patch: https://git.opendaylight.org/gerrit/#/c/11371/
> > Please retest.
>
> I've tested the patch, and confirmed that the patch fixes the bug partially.
>
>
> With this patch, I've successfully installed a flow entry to change UDP port.
> But it failed to install a flow entry to change ICMP type/code for OF 1.3
> switches.

It seems that the current Open vSwitch does not support ICMP type/code for SET_FIELD actions.

http://osrg.github.io/ryu-certification/switch/ovs

merged

Thank you for creating and merging the patch (Gerrit 11371).

Since the current Open vSwitch does not support SET_FIELD(ICMP type/code) actions, I could not confirm if the patch correctly works on SET_FIELD(ICMP type/code) actions on OF 1.3 switches.

But, I captured a FLOW_MOD message which had SET_FIELD(ICMP type) and SET_FIELD(ICMP code) sent by a controller on which the patch (Gerrit 11371) was merged, and I thought that the values in the action fields was correct.

It seems that Lagopus switch support SET_FIELD(ICMP type/code) actions, I'll test on that switch at some time.

Finally, I've confirmed that the bug is fixed.

I've installed Lagopus switch, and successfully installed a flow entry which has SET_FIELD(ICMP type/code)actions into the Lagopus switch.

Could you someone cherry pick the patch to the stable/helium branch?
https://git.opendaylight.org/gerrit/#/c/11371/

done

https://git.opendaylight.org/gerrit/#/c/12409/