Hello,
Here is my unit test:
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index 57589758f..bfb0f440b 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -842,3 +842,52 @@ Datapath actions: 7
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([tunnel_push_pop - VXLAN access port])
+
+dnl Create bridge that has a MAC address
+OVS_VSWITCHD_START([set bridge br0 dnl
+ datapath_type=dummy -- set Interface dnl
+ br0 other-config:hwaddr=aa:55:aa:55:00:00])
+AT_CHECK([ovs-vsctl add-port br0 p8 -- set Interface p8 dnl
+ type=dummy ofport_request=8])
+
+dnl Create another bridge
+AT_CHECK([ovs-vsctl add-br ovs-tun0 -- set bridge ovs-tun0
datapath_type=dummy])
+
+dnl Add VXLAN port to this bridge
+AT_CHECK([ovs-vsctl add-port ovs-tun0 tun0 -- set int tun0 dnl
+ type=vxlan options:remote_ip=10.0.0.11 -- add-port ovs-tun0 p7 dnl
+ -- set interface p7 type=dummy ofport_request=7])
+
+dnl Set VLAN tags, so that br0 and its port p8 have the same tag,
+dnl but ovs-tun0's port p7 has a different tag
+AT_CHECK([ovs-vsctl set port p8 tag=42 -- set port br0 tag=42 dnl
+ -- set port p7 tag=200])
+
+dnl Set IP address and route for br0
+AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 10.0.0.2/24], [0], [OK
+])
+AT_CHECK([ovs-appctl ovs/route/add 10.0.0.11/24 br0], [0], [OK
+])
+
+dnl Send an ARP reply to port b8 on br0, so that packets will be forwarded
+dnl to learned port
+AT_CHECK([ovs-ofctl add-flow br0 action=normal])
+AT_CHECK([ovs-appctl netdev-dummy/receive p8 'in_port(8),dnl
+ eth(src=aa:55:aa:66:00:00,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),dnl
+
arp(sip=10.0.0.11,tip=10.0.0.2,op=2,sha=aa:55:aa:66:00:00,tha=00:00:00:00:00:00)'])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-tun0 in_port=p7], [0], [stdout])
+AT_CHECK([tail -2 stdout], [0], [dnl
+Megaflow: recirc_id=0,eth,in_port=7,dl_src=00:00:00:00:00:00,dnl
+dl_dst=00:00:00:00:00:00,dl_type=0x0000
+Datapath actions: push_vlan(vid=200,pcp=0),1,clone(tnl_push(tnl_port(4789),dnl
+header(size=50,type=4,eth(dst=aa:55:aa:66:00:00,src=aa:55:aa:55:00:00,dnl
+dl_type=0x0800),ipv4(src=10.0.0.2,dst=10.0.0.11,proto=17,tos=0,ttl=64,dnl
+frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x8000000,vni=0x0)),dnl
+out_port(100)),8)
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
On Tue, Apr 5, 2022 at 2:35 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> Hi. Thanks for the patch!
>
> One general comment for the subject line:
> 'netdev' doesn't seem to be a correct 'area' for the change.
> It should be 'ofproto-dpif-xlate' instead. And, please,
> add the space after the ':'. 'summary' should preferably
> start with a capital letter and end with a period.
>
> More details on how to format the patch here:
> Documentation/internals/contributing/submitting-patches.rst
>
> See some more comments inline.
>
> On 4/5/22 01:01, Thilak Raj Surendra Babu wrote:
> > when a packet is received over an access port that needs to be sent over a
> > vxlan tunnel,
> > the access port VLAN id is used in the lookup leading to a wrong packet
> > being crafted and
> > sent over the tunnel.Clear out the flow 's VLAN field as it should not be
> > used while performing
> > mac lookup for the outer tunnel and also at this point the VLAN action
> > related to inner flow is
> > already committed.
>
> Please, limit the line length for a commit message.
> 72 characters is the most common limit for it.
>
> >
>
> Please, add a 'Fixes' tag.
>
> > Signed-off-by: Thilak Raj Surendra Babu <thilakraj...@nutanix.com>
> > ---
>
> While sending new versions of a patch, please, add here
> a brief explanation of what changed between versions.
> E.g.:
>
> Version 2:
> - Fixed line length warning from checkpatch.
> - Dropped unrelated changes.
>
> > ofproto/ofproto-dpif-xlate.c | 3 ++
> > tests/system-traffic.at | 54 ++++++++++++++++++++++++++++++++++++
>
> I see you added a system test. That is OK, good to have one.
> However, the issue doesn't require any actual network to be
> reproduced. So, we should have a unit test for it.
>
> I know that Rosemarie worked on the same problem over the
> past few weeks, and she already has a working unit test. So,
> I think it would be great if you can cooperate and include
> the unit test into the patch.
>
> Rosemarie, could you share what you have?
>
> > 2 files changed, 57 insertions(+)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index bfd4960dd..34f74aade 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3541,6 +3541,9 @@ propagate_tunnel_data_to_flow__(struct flow *dst_flow,
> > {
> > dst_flow->dl_dst = dmac;
> > dst_flow->dl_src = smac;
>
> An empty line here would be nice.
>
> > + /* Clear VLAN entries which do not apply for tunnel flows */
>
> Comments should end with a period.
>
> > + memset (dst_flow->vlans, 0,
>
> No need for extra space between the function name and the '('.
>
> > + sizeof(union flow_vlan_hdr) * FLOW_MAX_VLAN_HEADERS);
>
> Please, avoid use of sizeof with the type as an argument.
> Use the expression (actual object) as an argument instead.
>
> For more details see the coding-style guide:
> Documentation/internals/contributing/coding-style.rst
>
> >
> > dst_flow->packet_type = htonl(PT_ETH);
> > dst_flow->nw_dst = src_flow->tunnel.ip_dst;
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 4a7fa49fc..b0003c2e5 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -259,6 +259,60 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3
> > -w 2 10.1.1.100 | FORMAT_PI
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +
>
> This extra space is unnecessary.
>
> > +AT_SETUP([datapath - ping vlan over vxlan tunnel])
> > +OVS_CHECK_TUNNEL_TSO()
> > +OVS_CHECK_VXLAN()
> > +
> > +OVS_TRAFFIC_VSWITCHD_START()
> > +ADD_BR([br-underlay])
> > +
> > +AT_CHECK([ovs-vsctl -- add-port br0 patch0 -- set interface patch0
> > type=patch options:peer=patch1 -- add-port br-underlay patch1 -- set
> > interface patch1 type=patch options:peer=patch0])
> > +
> > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> > +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"])
> > +
> > +ADD_NAMESPACES(at_ns0)
> > +
> > +dnl Set up underlay link from host into the namespace using veth pair.
> > +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24")
> > +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"])
> > +AT_CHECK([ip link set dev br-underlay up])
> > +
> > +
> > +dnl Set up tunnel endpoints on OVS outside the namespace and with a native
> > +dnl linux device inside the namespace.
> > +
> > +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100],
> > [10.1.1.1/24],
> > + [id 0 dstport 4789])
> > +
> > +ADD_OVS_TUNNEL([vxlan], [br-underlay], [at_vxlan0], [172.31.1.1],
> > [10.1.1.100/24])
> > +
> > +NS_EXEC([at_ns0], [ip link add link at_vxlan1 name at_vxlan1.100 type vlan
> > id 100])
> > +
> > +NS_EXEC([at_ns0], [ip addr flush dev at_vxlan1])
> > +NS_EXEC([at_ns0], [ip addr add dev at_vxlan1.100 "10.1.1.30/24"])
> > +NS_EXEC([at_ns0], [ip link set dev at_vxlan1.100 up])
> > +
> > +ADD_NAMESPACES(at_ns1)
> > +ADD_VETH(p1, at_ns1, br0, "10.1.1.10/24")
> > +
> > +AT_CHECK([ovs-vsctl set port ovs-p1 tag=100])
> > +
> > +dnl First, check the underlay
> > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 |
> > FORMAT_PING], [0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +
> > +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.30 |
> > FORMAT_PING], [0], [dnl
> > +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> > +])
> > +
> > +OVS_TRAFFIC_VSWITCHD_STOP(["/ofproto_dpif_xlate(revalidator.*)|WARN|over
> > max translation depth 64.*/d"])
>
> Looks like you have a loop in the test topology. That is not good.
> Please, remove the loop and you'll not need to filter out warnings.
>
> > +AT_CLEANUP
> > +
> > +
> > +
>
> One empty line is enough.
>
> > AT_SETUP([datapath - ping over vxlan6 tunnel])
> > OVS_CHECK_TUNNEL_TSO()
> > OVS_CHECK_VXLAN_UDP6ZEROCSUM()
>
> Best regards, Ilya Maximets.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
