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

Han Zhou zhouhan at gmail.com
Thu Jan 21 06:40:56 UTC 2016


On Wed, Jan 20, 2016 at 3:22 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> 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);
>             }
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev

Hi Ben,

Do you mean even if tag == 0, we should still strip 802.1Q header? That
make sense, and then it is an existed bug even before this patch because
the new logic of this patch is equivalent to the old logic. It will not add
STRIP_VLAN action if vlan tag is not matched.

--
Best regards,
Han



More information about the dev mailing list