[libvirt] [PATCHv2 9/9] network: merge relevant virtualports rather than choosing one
Kyle Mestery (kmestery)
kmestery at cisco.com
Tue Aug 14 13:37:46 UTC 2012
This looks good to me, with some minor nits below.
Acked-by: Kyle Mestery <kmestery at 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><network></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><virtualport></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><virtualport></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><virtualport></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 @@
> <source network='default' portgroup='engineering'/>
> <target dev='vnet7'/>
> <mac address="00:11:22:33:44:55"/>
> - <virtualport type='802.1Qbg'>
> - <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
> + <virtualport>
> + <parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
> </virtualport>
>
> </interface>
> @@ -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 & 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><virtualport type='openvswitch'/></code> to the
> + interface definition. (<span class="since">Since
> + 0.9.11</span>). The OpenvSwitch type virtualport accepts two
> + parameters in its <code><parameters></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>
> ...
> <devices>
> + ...
> <interface type='bridge'>
> <source bridge='br0'/>
> </interface>
> - ...
> <interface type='bridge'>
> - <source bridge='br0'/>
> + <source bridge='br1'/>
> <target dev='vnet7'/>
> <mac address="00:11:22:33:44:55"/>
> </interface>
> + <interface type='bridge'>
> + <source bridge='ovsbr'/>
> + <virtualport type='openvswitch'/>
> + <parameters profileid='menial' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/>
> + </virtualport>
> + </interface>
> + ...
> </devices>
> ...</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><bridge name='xyz'/></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><interface></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><bridge name='xyz'/></code>
> + element <b>and</b> a <code><virtualport
> + type='openvswitch'/></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><interface></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><bandwidth></code>
> - or <code><virtualport></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><virtualport></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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list