vif: Stop using getattr for VIF lookups

'getattr' is really powerful and we make extensive use of it in nova.
However, the way we've used it for VIF lookups, where we use it to
retrieve functions by a key, seems to be a bit of an anti-pattern. Not
only does it completely break static code coverage analysers that we can
use to help us root out code that's not tested (or is tested but is
never used in production) but, more importantly, it makes it so much
more difficult to figure out what on earth is going on in an already
complex part of the codebase.

Be verbose and, in the absence of a true switch statement in Python, use
simple if-else blocks to do the lookups. Due to how this is done, we're
able to remove a few previously no-op functions. Funnily enough, this
actually results in fewer LOC despite being more explicit. #winning?

Change-Id: Idf08adca1e3a0d19e20bca2447c83f7372516cb7
Signed-off-by: Stephen Finucane <[email protected]>

Author: stephenfin

nova/network/os_vif_util.py

@@ -18,8 +18,6 @@
 versioned object model os_vif.objects.*
 '''
 
-import sys
-
 from os_vif import objects
 from oslo_config import cfg
 from oslo_log import log as logging
@@ -32,6 +30,13 @@
 LOG = logging.getLogger(__name__)
 CONF = cfg.CONF
 
+LEGACY_VIFS = {
+    model.VIF_TYPE_DVS, model.VIF_TYPE_IOVISOR, model.VIF_TYPE_802_QBG,
+    model.VIF_TYPE_802_QBH, model.VIF_TYPE_HW_VEB, model.VIF_TYPE_HOSTDEV,
+    model.VIF_TYPE_IB_HOSTDEV, model.VIF_TYPE_MIDONET, model.VIF_TYPE_TAP,
+    model.VIF_TYPE_MACVTAP
+}
+
 
 def _get_vif_name(vif):
     """Get a VIF device name
@@ -495,82 +500,6 @@ def _nova_to_osvif_vif_vrouter(vif):
     return obj
 
 
-# VIF_TYPE_DVS = 'dvs'
-def _nova_to_osvif_vif_dvs(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_IOVISOR = 'iovisor'
-def _nova_to_osvif_vif_iovisor(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_802_QBG = '802.1qbg'
-def _nova_to_osvif_vif_802_1qbg(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_802_QBH = '802.1qbh'
-def _nova_to_osvif_vif_802_1qbh(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_HW_VEB = 'hw_veb'
-def _nova_to_osvif_vif_hw_veb(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_IB_HOSTDEV = 'ib_hostdev'
-def _nova_to_osvif_vif_ib_hostdev(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_MIDONET = 'midonet'
-def _nova_to_osvif_vif_midonet(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_TAP = 'tap'
-def _nova_to_osvif_vif_tap(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_MACVTAP = 'macvtap'
-def _nova_to_osvif_vif_macvtap(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_HOSTDEV = 'hostdev_physical'
-def _nova_to_osvif_vif_hostdev_physical(vif):
-    raise NotImplementedError()
-
-
-# VIF_TYPE_BINDING_FAILED = 'binding_failed'
-def _nova_to_osvif_vif_binding_failed(vif):
-    """Special handler for the "binding_failed" vif type.
-
-    The "binding_failed" vif type indicates port binding to a host failed
-    and we are trying to plug the vifs again, which will fail because we
-    do not know the actual real vif type, like ovs, bridge, etc. We raise
-    NotImplementedError to indicate to the caller that we cannot handle
-    this type of vif rather than the generic "Unsupported VIF type" error
-    in nova_to_osvif_vif.
-    """
-    raise NotImplementedError()
-
-
-# VIF_TYPE_UNBOUND = 'unbound'
-def _nova_to_osvif_vif_unbound(vif):
-    """Special handler for the "unbound" vif type.
-
-    The "unbound" vif type indicates a port has not been hooked up to backend
-    network driver (OVS, linux bridge, ...). We raise NotImplementedError to
-    indicate to the caller that we cannot handle this type of vif rather than
-    the generic "Unsupported VIF type" error in nova_to_osvif_vif.
-    """
-    raise NotImplementedError()
-
-
 def nova_to_osvif_vif(vif):
     """Convert a Nova VIF model to an os-vif object
 
@@ -586,19 +515,40 @@ def nova_to_osvif_vif(vif):
 
     LOG.debug("Converting VIF %s", vif)
 
-    funcname = "_nova_to_osvif_vif_" + vif['type'].replace(".", "_")
-    func = getattr(sys.modules[__name__], funcname, None)
-
-    if not func:
-        raise exception.NovaException(
-            "Unsupported VIF type %(type)s convert '%(func)s'" %
-            {'type': vif['type'], 'func': funcname})
-
-    try:
-        vifobj = func(vif)
-        LOG.debug("Converted object %s", vifobj)
-        return vifobj
-    except NotImplementedError:
-        LOG.debug("No conversion for VIF type %s yet",
-                  vif['type'])
+    vif_type = vif['type']
+
+    if vif_type in LEGACY_VIFS:
+        # We want to explicitly fall back to the legacy path for these VIF
+        # types
+        LOG.debug('No conversion for VIF type %s yet', vif_type)
         return None
+
+    if vif_type in {model.VIF_TYPE_BINDING_FAILED, model.VIF_TYPE_UNBOUND}:
+        # These aren't real VIF types. VIF_TYPE_BINDING_FAILED indicates port
+        # binding to a host failed and we are trying to plug the VIFs again,
+        # which will fail because we do not know the actual real VIF type, like
+        # VIF_TYPE_OVS, VIF_TYPE_BRIDGE, etc. VIF_TYPE_UNBOUND, by comparison,
+        # is the default VIF type of a driver when it is not bound to any host,
+        # i.e. we have not set the host ID in the binding driver. This should
+        # also only happen in error cases.
+        # TODO(stephenfin): We probably want a more meaningful log here
+        LOG.debug('No conversion for VIF type %s yet', vif_type)
+        return None
+
+    if vif_type == model.VIF_TYPE_OVS:
+        vif_obj = _nova_to_osvif_vif_ovs(vif)
+    elif vif_type == model.VIF_TYPE_IVS:
+        vif_obj = _nova_to_osvif_vif_ivs(vif)
+    elif vif_type == model.VIF_TYPE_BRIDGE:
+        vif_obj = _nova_to_osvif_vif_bridge(vif)
+    elif vif_type == model.VIF_TYPE_AGILIO_OVS:
+        vif_obj = _nova_to_osvif_vif_agilio_ovs(vif)
+    elif vif_type == model.VIF_TYPE_VHOSTUSER:
+        vif_obj = _nova_to_osvif_vif_vhostuser(vif)
+    elif vif_type == model.VIF_TYPE_VROUTER:
+        vif_obj = _nova_to_osvif_vif_vrouter(vif)
+    else:
+        raise exception.NovaException('Unsupported VIF type %s' % vif_type)
+
+    LOG.debug('Converted object %s', vif_obj)
+    return vif_obj

nova/virt/libvirt/vif.py

@@ -23,6 +23,7 @@
 import os_vif
 from os_vif import exception as osv_exception
 from os_vif.objects import fields as osv_fields
+from os_vif.objects import vif as osv_vifs
 from oslo_concurrency import processutils
 from oslo_log import log as logging
 from oslo_utils import strutils
@@ -120,9 +121,6 @@ def set_vf_interface_vlan(pci_addr, mac_addr, vlan=0):
 class LibvirtGenericVIFDriver(object):
     """Generic VIF driver for libvirt networking."""
 
-    def _normalize_vif_type(self, vif_type):
-        return vif_type.replace('2.1q', '2q')
-
     def get_vif_devname(self, vif):
         if 'devname' in vif:
             return vif['devname']
@@ -432,6 +430,10 @@ def get_config_tap(self, instance, vif, image_meta,
 
         return conf
 
+    def get_config_ib_hostdev(self, instance, vif, image_meta,
+                              inst_type, virt_type, host):
+        return self.get_base_hostdev_pci_config(vif)
+
     def _get_virtio_queue_sizes(self, host):
         """Returns rx/tx queue sizes configured or (None, None)
 
@@ -464,10 +466,6 @@ def _get_virtio_queue_sizes(self, host):
             tx = None
         return rx, tx
 
-    def get_config_ib_hostdev(self, instance, vif, image_meta,
-                              inst_type, virt_type, host):
-        return self.get_base_hostdev_pci_config(vif)
-
     def _set_config_VIFGeneric(self, instance, vif, conf, host):
         dev = vif.vif_name
         designer.set_vif_host_backend_ethernet_config(conf, dev, host)
@@ -522,14 +520,12 @@ def _set_config_VIFPortProfileOpenVSwitch(self, profile, conf):
 
     def _set_config_VIFPortProfile(self, instance, vif, conf):
         # Set any port profile that may be required
-        profilefunc = "_set_config_" + vif.port_profile.obj_name()
-        func = getattr(self, profilefunc, None)
-        if not func:
+        profile_name = vif.port_profile.obj_name()
+        if profile_name == 'VIFPortProfileOpenVSwitch':
+            self._set_config_VIFPortProfileOpenVSwitch(vif.port_profile, conf)
+        else:
             raise exception.InternalError(
-                _("Unsupported VIF port profile type %(obj)s func %(func)s") %
-                {'obj': vif.port_profile.obj_name(), 'func': profilefunc})
-
-        func(vif.port_profile, conf)
+                _('Unsupported VIF port profile type %s') % profile_name)
 
     def _get_config_os_vif(self, instance, vif, image_meta, inst_type,
                            virt_type, host, vnic_type):
@@ -552,13 +548,19 @@ def _get_config_os_vif(self, instance, vif, image_meta, inst_type,
                                     host)
 
         # Do the VIF type specific config
-        viffunc = "_set_config_" + vif.obj_name()
-        func = getattr(self, viffunc, None)
-        if not func:
+        if isinstance(vif, osv_vifs.VIFGeneric):
+            self._set_config_VIFGeneric(instance, vif, conf, host)
+        elif isinstance(vif, osv_vifs.VIFBridge):
+            self._set_config_VIFBridge(instance, vif, conf, host)
+        elif isinstance(vif, osv_vifs.VIFOpenVSwitch):
+            self._set_config_VIFOpenVSwitch(instance, vif, conf, host)
+        elif isinstance(vif, osv_vifs.VIFVHostUser):
+            self._set_config_VIFVHostUser(instance, vif, conf, host)
+        elif isinstance(vif, osv_vifs.VIFHostDevice):
+            self._set_config_VIFHostDevice(instance, vif, conf, host)
+        else:
             raise exception.InternalError(
-                _("Unsupported VIF type %(obj)s func %(func)s") %
-                {'obj': vif.obj_name(), 'func': viffunc})
-        func(instance, vif, conf, host)
+                _("Unsupported VIF type %s") % vif.obj_name())
 
         # not all VIF types support bandwidth configuration
         # https://github.com/libvirt/libvirt/blob/568a41722/src/conf/netdev_bandwidth_conf.h#L38
@@ -596,13 +598,29 @@ def get_config(self, instance, vif, image_meta,
                                            vnic_type)
 
         # Legacy non-os-vif codepath
-        vif_slug = self._normalize_vif_type(vif_type)
-        func = getattr(self, 'get_config_%s' % vif_slug, None)
-        if not func:
-            raise exception.InternalError(
-                _("Unexpected vif_type=%s") % vif_type)
-        return func(instance, vif, image_meta,
-                    inst_type, virt_type, host)
+        args = (instance, vif, image_meta, inst_type, virt_type, host)
+        if vif_type == network_model.VIF_TYPE_IOVISOR:
+            return self.get_config_iovisor(*args)
+        elif vif_type == network_model.VIF_TYPE_BRIDGE:
+            return self.get_config_bridge(*args)
+        elif vif_type == network_model.VIF_TYPE_802_QBG:
+            return self.get_config_802qbg(*args)
+        elif vif_type == network_model.VIF_TYPE_802_QBH:
+            return self.get_config_802qbh(*args)
+        elif vif_type == network_model.VIF_TYPE_HW_VEB:
+            return self.get_config_hw_veb(*args)
+        elif vif_type == network_model.VIF_TYPE_HOSTDEV:
+            return self.get_config_hostdev_physical(*args)
+        elif vif_type == network_model.VIF_TYPE_MACVTAP:
+            return self.get_config_macvtap(*args)
+        elif vif_type == network_model.VIF_TYPE_MIDONET:
+            return self.get_config_midonet(*args)
+        elif vif_type == network_model.VIF_TYPE_TAP:
+            return self.get_config_tap(*args)
+        elif vif_type == network_model.VIF_TYPE_IB_HOSTDEV:
+            return self.get_config_ib_hostdev(*args)
+
+        raise exception.InternalError(_('Unexpected vif_type=%s') % vif_type)
 
     def plug_ib_hostdev(self, instance, vif):
         fabric = vif.get_physical_network()
@@ -621,12 +639,6 @@ def plug_ib_hostdev(self, instance, vif):
             LOG.exception(_("Failed while plugging ib hostdev vif"),
                           instance=instance)
 
-    def plug_802qbg(self, instance, vif):
-        pass
-
-    def plug_802qbh(self, instance, vif):
-        pass
-
     def plug_hw_veb(self, instance, vif):
         # TODO(vladikr): This code can be removed once the minimum version of
         # Libvirt is incleased above 1.3.5, as vlan will be set by libvirt
@@ -642,9 +654,6 @@ def plug_hw_veb(self, instance, vif):
             if trusted:
                 linux_net.set_vf_trusted(vif['profile']['pci_slot'], True)
 
-    def plug_hostdev_physical(self, instance, vif):
-        pass
-
     def plug_macvtap(self, instance, vif):
         vif_details = vif['details']
         vlan = vif_details.get(network_model.VIF_DETAILS_VLAN)
@@ -726,13 +735,27 @@ def plug(self, instance, vif):
             return
 
         # Legacy non-os-vif codepath
-        vif_slug = self._normalize_vif_type(vif_type)
-        func = getattr(self, 'plug_%s' % vif_slug, None)
-        if not func:
+        if vif_type == network_model.VIF_TYPE_IB_HOSTDEV:
+            self.plug_ib_hostdev(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_HW_VEB:
+            self.plug_hw_veb(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_MACVTAP:
+            self.plug_macvtap(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_MIDONET:
+            self.plug_midonet(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_IOVISOR:
+            self.plug_iovisor(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_TAP:
+            self.plug_tap(instance, vif)
+        elif vif_type in {network_model.VIF_TYPE_802_QBG,
+                          network_model.VIF_TYPE_802_QBH,
+                          network_model.VIF_TYPE_HOSTDEV}:
+            # These are no-ops
+            pass
+        else:
             raise exception.VirtualInterfacePlugException(
-                _("Plug vif failed because of unexpected "
+                _("Plug VIF failed because of unexpected "
                   "vif_type=%s") % vif_type)
-        func(instance, vif)
 
     def unplug_ib_hostdev(self, instance, vif):
         fabric = vif.get_physical_network()
@@ -746,12 +769,6 @@ def unplug_ib_hostdev(self, instance, vif):
         except Exception:
             LOG.exception(_("Failed while unplugging ib hostdev vif"))
 
-    def unplug_802qbg(self, instance, vif):
-        pass
-
-    def unplug_802qbh(self, instance, vif):
-        pass
-
     def unplug_hw_veb(self, instance, vif):
         # TODO(sean-k-mooney): remove in Train after backporting 0 mac
         # change as this should no longer be needed with libvirt >= 3.2.0.
@@ -770,12 +787,6 @@ def unplug_hw_veb(self, instance, vif):
             if "trusted" in vif['profile']:
                 linux_net.set_vf_trusted(vif['profile']['pci_slot'], False)
 
-    def unplug_hostdev_physical(self, instance, vif):
-        pass
-
-    def unplug_macvtap(self, instance, vif):
-        pass
-
     def unplug_midonet(self, instance, vif):
         """Unplug from MidoNet network port
 
@@ -842,9 +853,25 @@ def unplug(self, instance, vif):
             return
 
         # Legacy non-os-vif codepath
-        vif_slug = self._normalize_vif_type(vif_type)
-        func = getattr(self, 'unplug_%s' % vif_slug, None)
-        if not func:
-            msg = _("Unexpected vif_type=%s") % vif_type
-            raise exception.InternalError(msg)
-        func(instance, vif)
+        if vif_type == network_model.VIF_TYPE_IB_HOSTDEV:
+            self.unplug_ib_hostdev(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_HW_VEB:
+            self.unplug_hw_veb(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_MIDONET:
+            self.unplug_midonet(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_IOVISOR:
+            self.unplug_iovisor(instance, vif)
+        elif vif_type == network_model.VIF_TYPE_TAP:
+            self.unplug_tap(instance, vif)
+        elif vif_type in {network_model.VIF_TYPE_802_QBG,
+                          network_model.VIF_TYPE_802_QBH,
+                          network_model.VIF_TYPE_HOSTDEV,
+                          network_model.VIF_TYPE_MACVTAP}:
+            # These are no-ops
+            pass
+        else:
+            # TODO(stephenfin): This should probably raise
+            # VirtualInterfaceUnplugException
+            raise exception.InternalError(
+                _("Unplug VIF failed because of unexpected "
+                  "vif_type=%s") % vif_type)