[ovs-dev] [PATCH v2 3/3] ovn: Fix localnet ports on the same chassis.

Ben Pfaff blp at ovn.org
Wed Jan 20 23:22:01 UTC 2016


On Wed, Jan 20, 2016 at 11:31:08AM -0500, Russell Bryant wrote:
> Multiple logical ports on the same chassis that were connected to the
> same physical network via localnet ports were not able to send packets
> to each other.  This was because ovn-controller created a single patch
> port between br-int and the physical network access bridge and used it
> for all localnet ports.
> 
> The fix implemented here is to create a separate patch port for every
> logical port of type=localnet.  An optimization is included where these
> ports are only created if the localnet port is on a logical switch with
> another logical port with an associated local VIF.
> 
> A nice side effect of this fix is that the code in physical.c got a lot
> simpler, as localnet ports are now handled mostly like local VIFs.
> 
> Fixes: c02819293d52 ("ovn: Add "localnet" logical port type.")
> Reported-by: Han Zhou <zhouhan at gmail.com>
> Reported-at: http://openvswitch.org/pipermail/dev/2016-January/064413.html
> Signed-off-by: Russell Bryant <russell at ovn.org>
> Tested-by: Kyle Mestery <mestery at mestery.com
> Acked-By: Kyle Mestery <mestery at mestery.com>
> ---
> 
> v1->v2:
>  - updated for feedback from Han Zhou.

I guess I reviewed v1 accidentally.  Here's a copy of my feedback from
that version.

The new logic here in physical_run() looks wrong to me.  I think that it
strips a vlan if it didn't match on one.

            if (tag) {
                match_set_dl_vlan(&match, htons(tag));
            } else if (!strcmp(binding->type, "localnet")) {
                /* Match priority-tagged frames, e.g. VLAN ID 0.
                 *
                 * We'll add a second flow for frames that lack any 802.1Q
                 * header later. */
                match_set_dl_tci_masked(&match, htons(VLAN_CFI),
                                        htons(VLAN_VID_MASK | VLAN_CFI));
            }

            /* Strip vlans. */
            if (tag) {
                ofpact_put_STRIP_VLAN(&ofpacts);
            }

I believe that we can simplify it to the following, while fixing the bug
at the same time:

            /* Match a VLAN tag and strip it, including stripping priority tags
             * (e.g. VLAN ID 0).  In the latter case we'll add a second flow
             * for frames that lack any 802.1Q header later. */
            if (tag || !strcmp(binding->type, "localnet")) {
                match_set_dl_vlan(&match, htons(tag));
                ofpact_put_STRIP_VLAN(&ofpacts);
            }



More information about the dev mailing list