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

[Laine Stump <laine@xxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> > 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)


> +        </summary>
> +        <description>
> +          Add support for the 802.ad double-tagged modes on openvswitch


maybe "802.1ad double-tagged (tunneled) mode" ?? (I'm no expert with 
vlan terminology, but I think having "tunnel" in the description may 
help some people recognize the utility of the feature).


> +          networks.
> +        </description>
> +      </change>
>
> Please let me know if you believe that's enough wording or would like
> any modifications...
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> John
>
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index b848e53..253f329 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -6097,6 +6097,13 @@ qemu-kvm -net nic,model=? /dev/null
> >       <b>&lt;/vlan&gt;</b>
> >       ...
> >     &lt;/interface&gt;
> > +  &lt;interface type='bridge'&gt;
> > +    <b>&lt;vlan trunk='yes'&gt;</b>
> > +      <b>&lt;tag id='42'/&gt;</b>
> > +      <b>&lt;tag id='555' nativeMode='802.1ad'/&gt;</b>
> > +    <b>&lt;/vlan&gt;</b>
> > +    ...
> > +  &lt;/interface&gt;
> >   &lt;/devices&gt;
> >   ...</pre>
> >   
> > @@ -6132,16 +6139,22 @@ qemu-kvm -net nic,model=? /dev/null
> >       </p>
> >       <p>
> >         For network connections using Open vSwitch it is also possible
> > -      to configure 'native-tagged' and 'native-untagged' VLAN modes
> > -      <span class="since">Since 1.1.0.</span> This is done with the
> > -      optional <code>nativeMode</code> attribute on
> > -      the <code>&lt;tag&gt;</code> subelement: <code>nativeMode</code>
> > -      may be set to 'tagged' or 'untagged'. The <code>id</code>
> > -      attribute of the <code>&lt;tag&gt;</code> subelement
> > -      containing <code>nativeMode</code> sets which VLAN is considered
> > -      to be the "native" VLAN for this interface, and
> > -      the <code>nativeMode</code> attribute determines whether or not
> > -      traffic for that VLAN will be tagged.
> > +      to configure the following VLAN modes:
> > +    </p>
> > +      <ul>
> > +        <li>'tagged' <span class="since">Since 1.1.0.</span></li>
> > +        <li>'untagged' <span class="since">Since 1.1.0.</span></li>
> > +        <li>'802.1ad' <span class="since">Since 5.1.0.</span></li>
> > +      </ul>
> > +    <p>
> > +      This is done with the optional <code>nativeMode</code> attribute
> > +      on the <code>&lt;tag&gt;</code> subelement. The attribute may be
> > +      set to a string from the above list. The <code>id</code> attribute

  "set to a value" instead of "set to a string" ?? Not important but 
sounds better to my ear.


> > +      of the <code>&lt;tag&gt;</code> subelement containing
> > +      <code>nativeMode</code> sets which VLAN is considered to be the
> > +      "native" VLAN for this interface and the <code>nativeMode</code>
> > +      attribute determines whether or not traffic for that VLAN will be
> > +      tagged or 802.1ad double tagged.


Just to help me understand, can you explain what happens to packets in 
both directions when nativeMode="802.1ad" on a port? For example, for a 
packet coming *from* a guest and going onto the switch, is the packet 
tagged with the given id, which makes it singly tagged if it had no tag 
when coming from the guest, and double-tagged if it was previously 
tagged? And when a packet is going from the switch *to* the guest, is 
the configured tag removed? (and what if there is a packet that isn't 
tagged, or has a different tag?) Or do I have it backwards? Can you 
possibly make a simple text-picture showing what happens to packets 
(both those that were previously tagged and those that weren't) in each 
direction ?


(This isn't for adding to the documentation, but just for me to make 
sure I understand what's happening and that it's following the 
functional form of, for example, SRIOV interfaces' use of the <vlan> 
element).


Other than making sure that it fits functionally with other uses of 
<vlan>, I think it all looks fine.



> >       </p>
> >   
> >       <h5><a id="elementLink">Modifying virtual link state</a></h5>
> > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> > index 509cca9..eea86f4 100644
> > --- a/docs/formatnetwork.html.in
> > +++ b/docs/formatnetwork.html.in
> > @@ -705,16 +705,22 @@
> >       </p>
> >       <p>
> >         For network connections using Open vSwitch it is also possible
> > -      to configure 'native-tagged' and 'native-untagged' VLAN modes
> > -      <span class="since">Since 1.1.0.</span> This is done with the
> > -      optional <code>nativeMode</code> attribute on
> > -      the <code>&lt;tag&gt;</code> subelement: <code>nativeMode</code>
> > -      may be set to 'tagged' or 'untagged'. The <code>id</code>
> > -      attribute of the <code>&lt;tag&gt;</code> subelement
> > -      containing <code>nativeMode</code> sets which VLAN is considered
> > -      to be the "native" VLAN for this interface, and
> > -      the <code>nativeMode</code> attribute determines whether or not
> > -      traffic for that VLAN will be tagged.
> > +      to configure the following VLAN modes:
> > +    </p>
> > +      <ul>
> > +        <li>'tagged' <span class="since">Since 1.1.0.</span></li>
> > +        <li>'untagged' <span class="since">Since 1.1.0.</span></li>
> > +        <li>'802.1ad' <span class="since">Since 5.1.0.</span></li>
> > +      </ul>
> > +    <p>
> > +      This is done with the optional <code>nativeMode</code> attribute
> > +      on the <code>&lt;tag&gt;</code> subelement. The attribute may be
> > +      set to a string from the above list. The <code>id</code> attribute
> > +      of the <code>&lt;tag&gt;</code> subelement containing
> > +      <code>nativeMode</code> sets which VLAN is considered to be the
> > +      "native" VLAN for this interface and the <code>nativeMode</code>
> > +      attribute determines whether or not traffic for that VLAN will be
> > +      tagged or 802.1ad double tagged.
> >       </p>
> >       <p>
> >         <code>&lt;vlan&gt;</code> elements can also be specified in
> > diff --git a/docs/schemas/networkcommon.rng b/docs/schemas/networkcommon.rng
> > index 2699555..7262783 100644
> > --- a/docs/schemas/networkcommon.rng
> > +++ b/docs/schemas/networkcommon.rng
> > @@ -223,6 +223,7 @@
> >                 <choice>
> >                   <value>tagged</value>
> >                   <value>untagged</value>
> > +                <value>802.1ad</value>
> >                 </choice>
> >               </attribute>
> >             </optional>
> > diff --git a/src/conf/netdev_vlan_conf.c b/src/conf/netdev_vlan_conf.c
> > index 57d73ed..54e2b02 100644
> > --- a/src/conf/netdev_vlan_conf.c
> > +++ b/src/conf/netdev_vlan_conf.c
> > @@ -25,7 +25,7 @@
> >   #define VIR_FROM_THIS VIR_FROM_NONE
> >   
> >   VIR_ENUM_IMPL(virNativeVlanMode, VIR_NATIVE_VLAN_MODE_LAST,
> > -              "default", "tagged", "untagged",
> > +              "default", "tagged", "untagged", "802.1ad",
> >   );
> >   
> >   int
> > diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
> > index 4fa3a57..d8268bc 100644
> > --- a/src/util/virnetdevopenvswitch.c
> > +++ b/src/util/virnetdevopenvswitch.c
> > @@ -85,6 +85,11 @@ virNetDevOpenvswitchConstructVlans(virCommandPtr cmd, virNetDevVlanPtr virtVlan)
> >           virCommandAddArg(cmd, "vlan_mode=native-untagged");
> >           virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> >           break;
> > +    case VIR_NATIVE_VLAN_MODE_8021AD:
> > +        virCommandAddArg(cmd, "vlan_mode=dot1q-tunnel");
> > +        virCommandAddArg(cmd, "other_config:qinq-ethtype=802.1q");
> > +        virCommandAddArgFormat(cmd, "tag=%d", virtVlan->nativeTag);
> > +        break;
> >       case VIR_NATIVE_VLAN_MODE_DEFAULT:
> >       default:
> >           break;
> > @@ -498,6 +503,8 @@ int virNetDevOpenvswitchUpdateVlan(const char *ifname,
> >                            "--", "--if-exists", "clear", "Port", ifname, "tag",
> >                            "--", "--if-exists", "clear", "Port", ifname, "trunk",
> >                            "--", "--if-exists", "clear", "Port", ifname, "vlan_mode",
> > +                         "--", "--if-exists", "remove", "Port", ifname,
> > +                         "other_config", "qinq-ethtype",
> >                            "--", "--if-exists", "set", "Port", ifname, NULL);
> >   
> >       if (virNetDevOpenvswitchConstructVlans(cmd, virtVlan) < 0)
> > diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h
> > index 2a13759..54109c6 100644
> > --- a/src/util/virnetdevvlan.h
> > +++ b/src/util/virnetdevvlan.h
> > @@ -27,6 +27,7 @@ typedef enum {
> >       VIR_NATIVE_VLAN_MODE_DEFAULT = 0,
> >       VIR_NATIVE_VLAN_MODE_TAGGED,
> >       VIR_NATIVE_VLAN_MODE_UNTAGGED,
> > +    VIR_NATIVE_VLAN_MODE_8021AD,
> >   
> >       VIR_NATIVE_VLAN_MODE_LAST
> >   } virNativeVlanMode;
> > diff --git a/tests/networkxml2xmlin/openvswitch-net.xml b/tests/networkxml2xmlin/openvswitch-net.xml
> > index 2f6084d..5734d36 100644
> > --- a/tests/networkxml2xmlin/openvswitch-net.xml
> > +++ b/tests/networkxml2xmlin/openvswitch-net.xml
> > @@ -30,4 +30,13 @@
> >         <parameters profileid='native-profile'/>
> >       </virtualport>
> >     </portgroup>
> > +  <portgroup name='8021ad'>
> > +    <vlan trunk='yes'>
> > +      <tag id='555' nativeMode='802.1ad'/>
> > +      <tag id='666'/>
> > +    </vlan>
> > +    <virtualport>
> > +      <parameters profileid='8021ad-profile'/>
> > +    </virtualport>
> > +  </portgroup>
> >   </network>
> > diff --git a/tests/networkxml2xmlout/openvswitch-net.xml b/tests/networkxml2xmlout/openvswitch-net.xml
> > index 2f6084d..5734d36 100644
> > --- a/tests/networkxml2xmlout/openvswitch-net.xml
> > +++ b/tests/networkxml2xmlout/openvswitch-net.xml
> > @@ -30,4 +30,13 @@
> >         <parameters profileid='native-profile'/>
> >       </virtualport>
> >     </portgroup>
> > +  <portgroup name='8021ad'>
> > +    <vlan trunk='yes'>
> > +      <tag id='555' nativeMode='802.1ad'/>
> > +      <tag id='666'/>
> > +    </vlan>
> > +    <virtualport>
> > +      <parameters profileid='8021ad-profile'/>
> > +    </virtualport>
> > +  </portgroup>
> >   </network>
> > diff --git a/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml b/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml
> > index cc0c344..ed605bf 100644
> > --- a/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml
> > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-modified.xml
> > @@ -30,4 +30,13 @@
> >         <parameters profileid='native-profile'/>
> >       </virtualport>
> >     </portgroup>
> > +  <portgroup name='8021ad'>
> > +    <vlan trunk='yes'>
> > +      <tag id='555' nativeMode='802.1ad'/>
> > +      <tag id='666'/>
> > +    </vlan>
> > +    <virtualport>
> > +      <parameters profileid='8021ad-profile'/>
> > +    </virtualport>
> > +  </portgroup>
> >   </network>
> > diff --git a/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml b/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml
> > index 7c19ad9..0237e42 100644
> > --- a/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml
> > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-more-portgroups.xml
> > @@ -41,4 +41,13 @@
> >         <parameters profileid='native-profile'/>
> >       </virtualport>
> >     </portgroup>
> > +  <portgroup name='8021ad'>
> > +    <vlan trunk='yes'>
> > +      <tag id='555' nativeMode='802.1ad'/>
> > +      <tag id='666'/>
> > +    </vlan>
> > +    <virtualport>
> > +      <parameters profileid='8021ad-profile'/>
> > +    </virtualport>
> > +  </portgroup>
> >   </network>
> > diff --git a/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml b/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml
> > index 4104424..cb9d12d 100644
> > --- a/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml
> > +++ b/tests/networkxml2xmlupdateout/openvswitch-net-without-alice.xml
> > @@ -20,4 +20,13 @@
> >         <parameters profileid='native-profile'/>
> >       </virtualport>
> >     </portgroup>
> > +  <portgroup name='8021ad'>
> > +    <vlan trunk='yes'>
> > +      <tag id='555' nativeMode='802.1ad'/>
> > +      <tag id='666'/>
> > +    </vlan>
> > +    <virtualport>
> > +      <parameters profileid='8021ad-profile'/>
> > +    </virtualport>
> > +  </portgroup>
> >   </network>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list