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

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(a)uniudc.com&gt; > > > > --- > > > > 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)?