[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 9/9] network: merge relevant virtualports rather than choosing one



This looks good to me, with some minor nits below.

Acked-by: Kyle Mestery <kmestery cisco com>

On Aug 14, 2012, at 2:04 AM, Laine Stump wrote:

> One of the original ideas behind allowing a <virtualport> in an
> interface definition as well as in the <network> definition *and*one
> or more <portgroup>s within the network, was that guest-specific
> parameteres (like instanceid and interfaceid) could be given in the
> interface's virtualport, and more general things (portid, managerid,
> etc) could be given in the network and/or portgroup, with all the bits
> brought together at guest startup time and combined into a single
> virtualport to be used by the guest. This was somehow overlooked in
> the implementation, though - it simply picks the "most specific"
> virtualport, and uses the entire thing, with no attempt to merge in
> details from the others.
> 
> This patch uses virNetDevVPortProfileMerge3() to combine the three
> possible virtualports into one, then uses
> virNetDevVPortProfileCheck*() to verify that the resulting virtualport
> type is appropriate for the type of network, and that all the required
> attributes for that type are present.
> 
> An example of usage is this: assuming a <network> definitions on host
> ABC of:
> 
>  <network>
>    <name>testA</name>
>    ...
>    <virtualport type='openvswitch'/>
>    ...
>    <portgroup name='engineering'>
>      <virtualport>
>        <parameters profileid='eng'/>
>      </virtualport>
>    </portgroup>
>    <portgroup name='sales'>
>      <virtualport>
>        <parameters profileid='sales'/>
>      </virtualport>
>    </portgroup>
>  </network>
> 
> and the same <network> on host DEF of:
> 
>  <network>
>    <name>testA</name>
>    ...
>    <virtualport type='802.1Qbg'>
>      <parameters typeid="1193047" typeidversion="2"/>
>    </virtualport>
>    ...
>    <portgroup name='engineering'>
>      <virtualport>
>        <parameters managerid="11"/>
>      </virtualport>
>    </portgroup>
>    <portgroup name='sales'>
>      <virtualport>
>        <parameters managerid="55"/>
>      </virtualport>
>    </portgroup>
>  </network>
> 
> and a guest <interface> definition of:
> 
>  <interface type='network'>
>    <source network='testA' portgroup='sales'/>
>    <virtualport>
>      <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"
>                  interfaceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"\>
>    </virtualport>
>    ...
>  </interface>
> 
> If the guest was started on host ABC, the <virtualport> used would be:
> 
>  <virtualport type='openvswitch'>
>    <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'
>                profileid='sales'/>
>  </virtualport>
> 
> but if that guest was started on host DEF, the <virtualport> would be:
> 
>    <virtualport type='802.1Qbg'>
>      <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"
>                  typeid="1193047" typeidversion="2"
>                  managerid="55"/>
>    </virtualport>
> 
> Additionally, if none of the involved <virtualport>s had a specified type
> (this includes cases where no virtualport is given at all),
> ---
> docs/formatdomain.html.in   | 72 ++++++++++++++++++++++++++++++++++++-------
> docs/formatnetwork.html.in  | 27 ++++++++++++-----
> src/network/bridge_driver.c | 74 ++++++++++++++++++++++++++++++++-------------
> 3 files changed, 134 insertions(+), 39 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f97c630..f3b3fa8 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2230,11 +2230,40 @@
>       the network; one network may have multiple portgroups defined,
>       with each portgroup containing slightly different configuration
>       information for different classes of network
> -      connections. <span class="since">Since 0.9.4</span>). Also,
> -      similar to <code>direct</code> network connections (described
> -      below), a connection of type <code>network</code> may specify
> -      a <code>virtportprofile</code> element, with configuration data
> -      to be forwarded to a vepa or 802.1Qbh compliant switch.
> +      connections. <span class="since">Since 0.9.4</span>.
> +    </p>
> +    <p>
> +      Also, similar to <code>direct</code> network connections
> +      (described below), a connection of type <code>network</code> may
> +      specify a <code>virtualport</code> element, with configuration
> +      data to be forwarded to a vepa (802.1Qbg) or 802.1Qbh compliant
> +      switch (<span class="since">Since 0.8.2</span>), or to an
> +      OpenvSwitch virtual switch (<span class="since">Since

If you look at http://openvswitch.org/, you can see they use "Open vSwitch"
instead of "OpenvSwitch" as the name. Can you adjust the term in this
patch to match that?

> +      0.9.11</span>).
> +    </p>
> +    <p>
> +      Since the actual type of switch may vary depending on the
> +      configuration in the <code>&lt;network&gt;</code> on the host,
> +      it is acceptable to omit the virtualport <code>type</code>
> +      attribute, and specify attributes from multiple different
> +      virtualport types (and also to leave out certain attributes); at
> +      domain startup time, a complete <code>&lt;virtualport&gt;</code>
> +      element will be constructed by merging together the type and
> +      attributes found in the which will be filled in from the network
> +      or portgroup <code>&lt;virtualport&gt;</code>)
> +      (<span class="since">Since 0.10.0</span>). For example, in order
> +      to work properly with both an 802.1Qbh switch and an OpenvSwitch

Same "Open vSwitch" adjustment here.

> +      switch, you may choose to specify no type, but both
> +      an <code>instanceid</code> (in case the switch is 802.1Qbh) and
> +      an <code>interfaceid</code> (in case the switch is OpenvSwitch)

Same "Open vSwitch" adjustment here.

> +      (you may also omit the other attributes, such as managerid,
> +      typeid, or profileid, to be filled in from the
> +      network's <code>&lt;virtualport&gt;</code>). If you want to
> +      limit a guest to connecting only to certain types of switches,
> +      you can specify the virtualport type, but still omit some/all of
> +      the parameters - in this case if the host's network has a
> +      different type of virtualport, connection of the interface will
> +      fail.
>     </p>
> 
> <pre>
> @@ -2248,8 +2277,8 @@
>       &lt;source network='default' portgroup='engineering'/&gt;
>       &lt;target dev='vnet7'/&gt;
>       &lt;mac address="00:11:22:33:44:55"/&gt;
> -      &lt;virtualport type='802.1Qbg'&gt;
> -        &lt;parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt;
> +      &lt;virtualport&gt;
> +        &lt;parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt;
>       &lt;/virtualport&gt;
> 
>     &lt;/interface&gt;
> @@ -2261,7 +2290,7 @@
>     <p>
>       <strong><em>
>       This is the recommended config for general guest connectivity on
> -      hosts with static wired networking configs
> +      hosts with static wired networking configs.
>       </em></strong>
>     </p>
> 
> @@ -2276,19 +2305,40 @@
>       configuration is whatever is used on the LAN. This provides the guest VM
>       full incoming &amp; outgoing net access just like a physical machine.
>     </p>
> -
> +    <p>
> +      On Linux systems, the bridge device is normally a standard Linux
> +      host bridge. On hosts that support OpenvSwitch, it is also

Same "Open vSwitch" adjustment here.

> +      possible to connect to an openvswitch bridge device by adding
> +      a <code>&lt;virtualport type='openvswitch'/&gt;</code> to the
> +      interface definition.  (<span class="since">Since
> +      0.9.11</span>). The OpenvSwitch type virtualport accepts two
> +      parameters in its <code>&lt;parameters&gt;</code> element -
> +      an <code>interfaceid</code> which is a standard uuid used to
> +      uniquely identify this particular interface to OpenvSwitch (if

One more "Open vSwitch".

> +      you do no specify one, a random interfaceid will be generated
> +      for you when you first define the interface), and an
> +      optional <code>profileid</code> which is sent to OpenvSwitch as

Same "Open vSwitch" adjustment here.

> +      the interfaces "port-profile".
> +    </p>
> <pre>
>   ...
>   &lt;devices&gt;
> +    ...
>     &lt;interface type='bridge'&gt;
>       &lt;source bridge='br0'/&gt;
>     &lt;/interface&gt;
> -    ...
>     &lt;interface type='bridge'&gt;
> -      &lt;source bridge='br0'/&gt;
> +      &lt;source bridge='br1'/&gt;
>       &lt;target dev='vnet7'/&gt;
>       &lt;mac address="00:11:22:33:44:55"/&gt;
>     &lt;/interface&gt;
> +    &lt;interface type='bridge'&gt;
> +      &lt;source bridge='ovsbr'/&gt;
> +      &lt;virtualport type='openvswitch'/&gt;
> +        &lt;parameters profileid='menial' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/&gt;
> +      &lt;/virtualport&gt;
> +    &lt;/interface&gt;
> +    ...
>   &lt;/devices&gt;
>   ...</pre>
> 
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 7e8e991..0ba4d2d 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -147,10 +147,17 @@
>             This network describes either 1) an existing host bridge
>             that was configured outside of libvirt (if
>             a <code>&lt;bridge name='xyz'/&gt;</code> element has been
> -            specified), or 2) an interface or group of interfaces to
> -            be used for a "direct" connection via macvtap using
> -            macvtap's "bridge" mode (if the forward element has one or
> -            more <code>&lt;interface&gt;</code> subelements)
> +            specified, <span class="since">Since 0.9.4</span>), 2) an
> +            existing OpenvSwitch bridge that was configured outside of

"Open vSwitch"

> +            libvirt (if both a <code>&lt;bridge name='xyz'/&gt;</code>
> +            element <b>and</b> a <code>&lt;virtualport
> +            type='openvswitch'/&gt;</code> have been
> +            specified <span class="since">Since 0.10.0</span>) 3) an
> +            interface or group of interfaces to be used for a "direct"
> +            connection via macvtap using macvtap's "bridge" mode (if
> +            the forward element has one or
> +            more <code>&lt;interface&gt;</code>
> +            subelements, <span class="since">Since 0.9.4</span>)
>             (see <a href="formatdomain.html#elementsNICSDirect">Direct
>             attachment to physical interface</a> for descriptions of
>             the various macvtap modes). libvirt doesn't attempt to
> @@ -337,9 +344,15 @@
>       default portgroup will be used. If no portgroup is given in the
>       interface definition, and there is no default portgroup, then
>       none will be used. Any <code>&lt;bandwidth&gt;</code>
> -      or <code>&lt;virtualport&gt;</code> specified directly in the
> -      domain XML will take precedence over any setting in the chosen
> -      portgroup.
> +
> +      specified directly in the domain XML will take precedence over
> +      any setting in the chosen portgroup. if
> +      a <code>&lt;virtualport&gt;</code> is specified in the portgroup
> +      (and/or directly in the network definition), the multiple
> +      virtualports will be merged, and any parameter that is specified
> +      in more than one virtualport, and is not identical, will be
> +      considered an error, and will prevent the interface from
> +      starting.
>     </p>
> 
>     <h3><a name="elementsAddress">Addressing</a></h3>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index d9646fb..ec99e4d 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -61,6 +61,7 @@
> #include "virnetdev.h"
> #include "virnetdevbridge.h"
> #include "virnetdevtap.h"
> +#include "virnetdevvportprofile.h"
> 
> #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
> #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
> @@ -2746,6 +2747,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>     virNetworkObjPtr network;
>     virNetworkDefPtr netdef;
>     virPortGroupDefPtr portgroup;
> +    virNetDevVPortProfilePtr virtport = NULL;
> +    virNetworkForwardIfDefPtr dev = NULL;
>     unsigned int num_virt_fns = 0;
>     char **vfname = NULL;
>     int ii;
> @@ -2820,11 +2823,33 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>             goto cleanup;
>         }
> 
> +        /* merge virtualports from interface, network, and portgroup to
> +         * arrive at actual virtualport to use
> +         */
> +        if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile,
> +                                        iface->virtPortProfile,
> +                                        netdef->virtPortProfile,
> +                                        portgroup
> +                                        ? portgroup->virtPortProfile : NULL) < 0) {
> +            goto cleanup;
> +        }
> +        virtport = iface->data.network.actual->virtPortProfile;
> +        if (virtport) {
> +            /* only type='openvswitch' is allowed for bridges */
> +            if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("<virtualport type='%s'> not supported for network "
> +                                 "'%s' which uses a bridge device"),
> +                               virNetDevVPortTypeToString(virtport->virtPortType),
> +                               netdef->name);
> +                goto cleanup;
> +            }
> +        }
> +
>     } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) ||
>                (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) ||
>                (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) ||
>                (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) {
> -        virNetDevVPortProfilePtr virtport = NULL;
> 
>         /* <forward type='bridge|private|vepa|passthrough'> are all
>          * VIR_DOMAIN_NET_TYPE_DIRECT.
> @@ -2853,24 +2878,28 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>             break;
>         }
> 
> -        /* Find the most specific virtportprofile and copy it */
> -        if (iface->virtPortProfile) {
> -            virtport = iface->virtPortProfile;
> -        } else {
> -            if (portgroup)
> -                virtport = portgroup->virtPortProfile;
> -            else
> -                virtport = netdef->virtPortProfile;
> +        /* merge virtualports from interface, network, and portgroup to
> +         * arrive at actual virtualport to use
> +         */
> +        if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile,
> +                                        iface->virtPortProfile,
> +                                        netdef->virtPortProfile,
> +                                        portgroup
> +                                        ? portgroup->virtPortProfile : NULL) < 0) {
> +            goto cleanup;
>         }
> +        virtport = iface->data.network.actual->virtPortProfile;
>         if (virtport) {
> -            if (VIR_ALLOC(iface->data.network.actual->virtPortProfile) < 0) {
> -                virReportOOMError();
> +            /* make sure type is supported for macvtap connections */
> +            if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBG &&
> +                virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBH) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("<virtualport type='%s'> not supported for network "
> +                                 "'%s' which uses a macvtap device"),
> +                               virNetDevVPortTypeToString(virtport->virtPortType),
> +                               netdef->name);
>                 goto cleanup;
>             }
> -            /* There are no pointers in a virtualPortProfile, so a shallow copy
> -             * is sufficient
> -             */
> -            *iface->data.network.actual->virtPortProfile = *virtport;
>         }
> 
>         /* If there is only a single device, just return it (caller will detect
> @@ -2883,8 +2912,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>                            netdef->name);
>             goto cleanup;
>         } else {
> -            virNetworkForwardIfDefPtr dev = NULL;
> -
>             /* pick an interface from the pool */
> 
>             /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
> @@ -2967,13 +2994,18 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>                 virReportOOMError();
>                 goto cleanup;
>             }
> -            /* we are now assured of success, so mark the allocation */
> -            dev->usageCount++;
> -            VIR_DEBUG("Using physical device %s, usageCount %d",
> -                      dev->dev, dev->usageCount);
>         }
>     }
> 
> +    if (virNetDevVPortProfileCheckComplete(virtport, true) < 0)
> +        goto cleanup;
> +
> +    if (dev) {
> +        /* we are now assured of success, so mark the allocation */
> +        dev->usageCount++;
> +        VIR_DEBUG("Using physical device %s, usageCount %d",
> +                  dev->dev, dev->usageCount);
> +    }
>     ret = 0;
> cleanup:
>     for (ii = 0; ii < num_virt_fns; ii++)
> -- 
> 1.7.11.2
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]