[libvirt] [PATCH v5] openvswitch: Add new port VLAN mode "802.1ad"

[Eric Garver]

On Fri, Mar 15, 2019 at 01:46:37PM -0400, Laine Stump wrote:
> On 3/12/19 7:48 AM, Pavel Hrdina wrote:
> > On Fri, Mar 08, 2019 at 11:21:37AM -0500, Laine Stump wrote:
> > > On 3/8/19 8:34 AM, John Ferlan wrote:
> > > > On 2/24/19 9:15 AM, ZhiPeng LU wrote:
> > > > > This patch adds functionality to allow libvirt to configure the '802.1ad'
> > > > > modes(802.1ad double-tagged) on openvswitch networks.
> > > > > For example:
> > > > >     <interface type='bridge'>
> > > > >       <mac address='2c:da:41:1d:05:42'/>
> > > > >       <source bridge='ovs0'/>
> > > > >       <vlan>
> > > > >         <tag id='41' nativeMode='dot1q-tunnel'/>
> > > > >       </vlan>
> > > > >       <virtualport type='openvswitch'>
> > > > >         <parameters interfaceid='6401a152-0b99-40b5-92be-858810aa6d37'/>
> > > > >       </virtualport>
> > > > >       <model type='virtio'/>
> > > > >       <driver name='vhost'/>
> > > > >       <alias name='net0'/>
> > > > >     </interface>
> > > > > 
> > > > > Signed-off-by: ZhiPeng Lu <luzhipeng at uniudc.com>
> > > > > ---
> > > > > v1->v2:
> > > > >     1. Fix "make syntax-check" failure
> > > > > v2->v3:
> > > > >     1. remove other_config when updating vlan
> > > > > v3->v4:
> > > > >     1. add commit message that has a brief description of the new
> > > > >         feature
> > > > >     2. add tests for 'dot1q-tunnel' vlan mode
> > > > > v4->v5:
> > > > >     1. modify some description and format
> > > > > 
> > > > > v4-resend:
> > > > >     https://www.redhat.com/archives/libvir-list/2019-February/msg00988.html
> > > > > 
> > > > >    docs/formatdomain.html.in                          | 33 +++++++++++++++-------
> > > > >    docs/formatnetwork.html.in                         | 26 ++++++++++-------
> > > > >    docs/schemas/networkcommon.rng                     |  1 +
> > > > >    src/conf/netdev_vlan_conf.c                        |  2 +-
> > > > >    src/util/virnetdevopenvswitch.c                    |  7 +++++
> > > > >    src/util/virnetdevvlan.h                           |  1 +
> > > > >    tests/networkxml2xmlin/openvswitch-net.xml         |  9 ++++++
> > > > >    tests/networkxml2xmlout/openvswitch-net.xml        |  9 ++++++
> > > > >    .../openvswitch-net-modified.xml                   |  9 ++++++
> > > > >    .../openvswitch-net-more-portgroups.xml            |  9 ++++++
> > > > >    .../openvswitch-net-without-alice.xml              |  9 ++++++
> > > > >    11 files changed, 94 insertions(+), 21 deletions(-)
> > > > > 
> > > > Apart from now needing to indicate support in 5.2.0 for the
> > > > format*.html.in files and the need for a docs/news.xml note this seems
> > > > fine to me and covers what Laine had originally reviewed. I can modify
> > > > those two before pushing.
> > > > 
> > > > Also, I've CC'd Laine in hopes he can also take a look for sanity's sake
> > > > to ensure I didn't misinterpret something he requested previously!
> > > 
> > > Yeah, sorry I haven't responded to the last couple revisions of this patch.
> > > When I saw them I tagged them in red in my mail, but don't have an effective
> > > queuing mechanism and ended up getting lost in some other distraction and
> > > not getting back until the red message was scrolled way up out of sight :-/
> > > Anyway, thanks to John for keeping track of it and reviewing it, and ZhiPeng
> > > Lu for being patient.
> > > 
> > > 
> > > 
> > > > I'll also add a followup patch to update docs/news.xml with the
> > > > following text:
> > > > 
> > > > +      <change>
> > > > +        <summary>
> > > > +          Add support for "802.1ad" VLAN mode
> > > 
> > > (You know, I expected someone to counter-propose use of a non-official term
> > > for this, since some places libvirt uses official names from the standards
> > > documents and other places it uses informal terms. Since there was no
> > > counter-proposal, I'm now unsure if that happened because 1) everyone agrees
> > > with using "802.1ad" (which is unambiguous but its function may be less
> > > obvious to a casual user), or 2) nobody even noticed :-P (but I still think
> > > using the official name is better, especially because it assures we won't
> > > end up with confusion if we later need to add some other sort of
> > > tunneled/nested tagging)
> > I would say 2) is the case :).  Is there a standard for the two existing
> > modes?  To not make it even more confusing how about we uses the
> > non-official term as the libvirt accepted value but in the docs we also
> > mention the specific standard for each mode to make it absolutely clear
> > what it refers to?
> 
> 
> This question caused me to look at the "nativeMode" attribute more closely,
> and I now think that the nativeMode attribute may be the wrong place to
> configure 802.1ad tunneled mode.
> 
> 
> It is true that all three of these modes are configured in openvswitch via
> the "vlan_mode" commandline option to ovs-vsctl, so they are mutually
> exclusive. However, I'm not sure that 802.1ad tunneling is only used for
> packets that are on the native vlan when the port is in trunk mode (which is
> what the nativeMode attribute is supposed to be for), so in libvirt's config
> maybe it shouldn't be set with an attribute called "nativeMode".
> 
> 
> Fortunately, while looking all this up, I realized that I know the person
> who added support for 802.1ad "QinQ" tunneling to Open vSwitch (Hi Eric!),
> so I'm Cc'ing him to this message in hopes that his better knowledge of the
> subject matter can help to clear up my confusion.
> 
> 
> So is it okay to shoehorn this setting into the nativeMode attribute of the
> tag id for the native VLAN? Or is this something that should apply to *all*
> traffic going through the port (and thus deserves a different config
> attribute, maybe a direct attribute of the <vlan> element)?

I'm not familiar with the libvirt configuration or use cases, so please
take my input with a cup of salt.

I don't think "nativeMode" is appropriate here. If I understand it
correctly, nativeMode currently has values "tagged" and "untagged". This
implies it's used for trunk ports.

In OVS, "dot1q-tunnel" does _not_ behave like a trunk port. It more
closely resembles an access port. Frames that egress a dot1q-tunnel port
_always_ have their outer VLAN stripped.

Furthermore, dot1q-tunnel has other features that you may have interest
in supporting:

    - configurable outer TPID: 802.1ad (0x88a8) or 802.1q (0x8100, i.e.
      QinQ)
    - "tag" specifies the outer VLAN ID
        - maybe this is covered by the tag element inside the vlan
          element in the example above.
    - customer VLAN filtering (cvlans). Default accepts all C-VLANs.

Does libvirt have any tunnel-like configuration? If so that may be a
better fit.