[libvirt] [PATCH v3] esx: Support VLAN tags in virtual network port groups

[Laine Stump]

On 09/09/2012 04:44 AM, Matthias Bolte wrote:
> ---
>
> v2: Use network level VLAN config if there is no portgroup specific VLAN
>     config given.
>
> v3: Add ESX_VLAN_TRUNK define for magic number 4095
>
>  src/esx/esx_network_driver.c |   70 +++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
> index 09d46d3..21eabbe 100644
> --- a/src/esx/esx_network_driver.c
> +++ b/src/esx/esx_network_driver.c
> @@ -45,6 +45,9 @@
>   */
>  verify(MD5_DIGEST_SIZE == VIR_UUID_BUFLEN);
>  
> +/* ESX utilizes the VLAN ID of 4095 to mean that this port is in trunk mode */
> +#define ESX_VLAN_TRUNK 4095
> +
>  
>  
>  static virDrvOpenStatus
> @@ -489,7 +492,42 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml)
>              goto cleanup;
>          }
>  
> -        hostPortGroupSpec->vlanId->value = 0;
> +        if (def->portGroups[i].vlan.trunk) {
> +            /* FIXME: Change this once tag-less trunk-mode is supported */

I've lost the context (if there was any) of this comment. Is this
something that libvirt needs to support? Or ESX?

> +            if (def->portGroups[i].vlan.nTags != 1 ||
> +                *def->portGroups[i].vlan.tag != ESX_VLAN_TRUNK) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("VLAN tag has to be %d in trunk mode"),
> +                               ESX_VLAN_TRUNK);

This seems a bit odd. If ESX requires  setting the vlan tag to 4095 to
indicate trunk mode, why not just have trunk='yes' imply a tag id of
4095? While not exactly the same as trunk mode for Open vSwitch, it
seems closer at least. (hmm, of course I think currently the parser
requires at least one tag id, even for trunk='yes'. Maybe that should be
relaxed in the parser, and enforced in each driver as necessary? Is that
what the comment above is referring to? If so, I would say "go for it" :-))

> +                goto cleanup;
> +            }
> +
> +            hostPortGroupSpec->vlanId->value = ESX_VLAN_TRUNK;
> +        } else if (def->portGroups[i].vlan.nTags == 1) {
> +            hostPortGroupSpec->vlanId->value = *def->portGroups[i].vlan.tag;
> +        } else if (def->portGroups[i].vlan.nTags > 1) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Can apply one VLAN tag per port group only"));
> +            goto cleanup;
> +        } else if (def->vlan.trunk) {
> +            /* FIXME: Change this once tag-less trunk-mode is supported */



> +            if (def->vlan.nTags != 1 || *def->vlan.tag != ESX_VLAN_TRUNK) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("VLAN tag has to be %d in trunk mode"),
> +                               ESX_VLAN_TRUNK);
> +                goto cleanup;
> +            }
> +
> +            hostPortGroupSpec->vlanId->value = ESX_VLAN_TRUNK;
> +        } else if (def->vlan.nTags == 1) {
> +            hostPortGroupSpec->vlanId->value = *def->vlan.tag;
> +        } else if (def->vlan.nTags > 1) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Can apply one VLAN tag per port group only"));
> +            goto cleanup;
> +        } else {
> +            hostPortGroupSpec->vlanId->value = 0;
> +        }
>  
>          if (def->portGroups[i].bandwidth != NULL) {
>              if (esxBandwidthToShapingPolicy
> @@ -519,6 +557,8 @@ esxNetworkDefineXML(virConnectPtr conn, const char *xml)
>      network = virGetNetwork(conn, hostVirtualSwitch->name, md5);
>  
>    cleanup:
> +    /* FIXME: need to remove virtual switch if adding port groups failed */
> +
>      virNetworkDefFree(def);
>      esxVI_HostVirtualSwitch_Free(&hostVirtualSwitch);
>      esxVI_HostPortGroup_Free(&hostPortGroupList);
> @@ -695,6 +735,7 @@ esxNetworkGetXMLDesc(virNetworkPtr network_, unsigned int flags)
>      esxVI_String *hostPortGroupKey = NULL;
>      esxVI_String *networkName = NULL;
>      virNetworkDefPtr def;
> +    virPortGroupDefPtr portGroup;
>  
>      if (esxVI_EnsureSession(priv->primary) < 0) {
>          return NULL;
> @@ -824,9 +865,12 @@ esxNetworkGetXMLDesc(virNetworkPtr network_, unsigned int flags)
>                      for (networkName = networkNameList; networkName != NULL;
>                           networkName = networkName->_next) {
>                          if (STREQ(networkName->value, hostPortGroup->spec->name)) {
> -                            def->portGroups[def->nPortGroups].name = strdup(networkName->value);
> +                            portGroup = &def->portGroups[def->nPortGroups];
> +                            ++def->nPortGroups;
> +
> +                            portGroup->name = strdup(networkName->value);
>  
> -                            if (def->portGroups[def->nPortGroups].name == NULL) {
> +                            if (portGroup->name == NULL) {
>                                  virReportOOMError();
>                                  goto cleanup;
>                              }
> @@ -834,13 +878,29 @@ esxNetworkGetXMLDesc(virNetworkPtr network_, unsigned int flags)
>                              if (hostPortGroup->spec->policy != NULL) {
>                                  if (esxShapingPolicyToBandwidth
>                                        (hostPortGroup->spec->policy->shapingPolicy,
> -                                       &def->portGroups[def->nPortGroups].bandwidth) < 0) {
> +                                       &portGroup->bandwidth) < 0) {
>                                      ++def->nPortGroups;
>                                      goto cleanup;
>                                  }
>                              }
>  
> -                            ++def->nPortGroups;
> +                            if (hostPortGroup->spec->vlanId->value > 0) {
> +                                if (hostPortGroup->spec->vlanId->value == ESX_VLAN_TRUNK) {
> +                                    portGroup->vlan.trunk = true;
> +                                }
> +
> +                                /* FIXME: Remove this once tag-less trunk-mode is supported */
> +                                portGroup->vlan.nTags = 1;
> +
> +                                if (VIR_ALLOC_N(portGroup->vlan.tag, 1) < 0) {
> +                                    virReportOOMError();
> +                                    goto cleanup;
> +                                }
> +
> +                                *portGroup->vlan.tag =
> +                                  hostPortGroup->spec->vlanId->value;
> +                            }
> +
>                              break;
>                          }
>                      }

It all looks fine to me (although I can't check it myself), but I think
if there's a simple solution to automatically setting the tag id sent to
ESX to "ESX_VLAN_TRUNK" just by setting "trunk='yes'", better to do that
now than later, saving the backward compatibility headaches (and it's
okay with me if the parser is changed to allow 0 tags when trunk='yes',
as long as openvswitch's (and hostdev's) use of vlan are updated to
disallow 0 tags even for trunk='yes' (if they don't already - I haven't
checked).

If I'm misunderstanding the issue and this can't be easily done, then
ACK as-is.